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.
|
HI @MohamedSabthar made the appropriate changes, can you do a quick review and let me know if any other changes are to made. |
| @@ -71,7 +71,7 @@ public static Object doHttpRequest(BString url, BMap<BString, Object> clientConf | |||
| } | |||
|
|
|||
| BMap<BString, ?> customHeaders = getBMapValueIfPresent(clientConfig, OAuth2Constants.CUSTOM_HEADERS); | |||
| if (customHeaders != null) { | |||
| if(customHeaders != null){ | |||
There was a problem hiding this comment.
Why are these spaces removed?
| @@ -104,20 +103,18 @@ public static Object doHttpRequest(BString url, BMap<BString, Object> clientConf | |||
| } catch (Exception e) { | |||
| return createError("Failed to init SSL context. " + e.getMessage()); | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Remove additional spaces. You can configure your IDE to remove the trailing spaces when saving a file.
| } | |
| } |
native/src/main/java/io/ballerina/stdlib/oauth2/OAuth2Client.java
Outdated
Show resolved
Hide resolved
| } | ||
| if (cert instanceof BMap) { | ||
| BMap<BString, BString> trustStore = (BMap<BString, BString>) cert; | ||
| if (key != null) { | ||
| if(key != null){ |
There was a problem hiding this comment.
Keep these spaces.
I guess if you ran a build these should be identified by the checkstyle plugin. If it doesn't we might have to check what happened there @MohamedSabthar.
Use this styling guide for your IDE.
| if(key != null){ | |
| if (key != null) { |
There was a problem hiding this comment.
The build is failing. @ritikk112 Let's fix it.
There was a problem hiding this comment.
Where can I find the report for the failed build? I want to pinpoint the error but can't find the report, do I have to build and run on my local machine for it?
There was a problem hiding this comment.
Yes, for a detailed report on Checkstyle errors and SpotBugs reports, you should run them locally. Anyway, you should also build and test the project locally before raising the PR. 🙂
|
|
Hi @ritikk112, were you able to build the code locally and apply the fix? If you're facing any issues while building the project, could you please mention them here? |
|
Closing this PR due to a lack of response from the contributor. Please feel free to reopen or submit a new PR if you wish to continue with this contribution. |
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