Refacftored OAuth2 #1324
Refacftored OAuth2 #1324
Conversation
ritikk112
requested review from
ThisaruGuruge,
MohamedSabthar,
DimuthuMadushan and
shafreenAnfar
as code owners
|
|
|
|
Hi @ritikk112, Are you working on this issue: ballerina-platform/ballerina-library#7237? If so, could you refactor the Ballerina codebase? The current Java code appears to follow best practices, so changes to it are not necessary. |
| @@ -70,16 +71,16 @@ public static Object doHttpRequest(BString url, BMap<BString, Object> clientConf | |||
| headersList.add(entry.getValue().getValue()); | |||
| } | |||
|
|
|||
| BMap<BString, ?> customHeaders = getBMapValueIfPresent(clientConfig, OAuth2Constants.CUSTOM_HEADERS); | |||
| if (customHeaders != null) { | |||
| Optional<BMap<BString, ?>> customHeaders = getBMapValueIfPresent(clientConfig, OAuth2Constants.CUSTOM_HEADERS); | |||
There was a problem hiding this comment.
Replacing null checks with Optional types doesn't add any value here, as we are following an imperative style of coding. The previous code block looks fine, so let's keep the original code.
There was a problem hiding this comment.
I introduced Optional checks to improve code readability as there were a lot of if else statements prior to changes and made the code less streamlined, should I revert back to if checks for null values?
|
|
||
| String httpVersion = getBStringValueIfPresent(clientConfig, OAuth2Constants.HTTP_VERSION).getValue(); | ||
| BMap<BString, ?> secureSocket = getBMapValueIfPresent(clientConfig, OAuth2Constants.SECURE_SOCKET); | ||
| Optional<BMap<BString, ?>> secureSocket = getBMapValueIfPresent(clientConfig, OAuth2Constants.SECURE_SOCKET); |
| if (e instanceof InterruptedException) { | ||
| Thread.currentThread().interrupt(); // Restore interrupted status | ||
| } |
There was a problem hiding this comment.
Here we have already handled the InterruptedException by throwing an error to the Ballerina side via the createError method. It's not necessary to restore the status here. Is there a any particular reason to introduce this change ?
There was a problem hiding this comment.
I added this just for additional security measures and to handle errors more gracefully
There was a problem hiding this comment.
Let's remove this as there's no need to propagate this interruption.
Purpose
Fixes ballerina-platform/ballerina-library#7237
Refactored SSL context initialization: Improved logic for handling optional SSL keys and certificates.
Enhanced exception handling: Streamlined error catching for IOException and InterruptedException.
Optimized code structure: Removed redundant checks and simplified nested if-else statements.
Improved readability: Cleaned up and made the code more readable across methods.
Examples
Checklist