Bugfix/ticket 26 #28
Bugfix/ticket 26 #28
Conversation
…de-oss#26 Local testing works ok, will need further clean environmental testing to confirm all is in order
WalkthroughThe changes involve modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
============================================
+ Coverage 42.44% 42.63% +0.19%
- Complexity 175 176 +1
============================================
Files 53 53
Lines 959 964 +5
Branches 77 77
============================================
+ Hits 407 411 +4
Misses 515 515
- Partials 37 38 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
kinde-core/src/main/java/com/kinde/KindeClientBuilder.java (2)
Line range hint
193-199: Suggest improvements tosetParameterFromEnvironmentalmethodWhile the method works as intended, consider the following improvements:
Enhance logging:
- Include the actual value being set (if not sensitive) in the info log.
- Use string formatting instead of concatenation for better performance.
Simplify the method:
- Consider using Optional to handle null checks more elegantly.
Here's a suggested refactor:
private void setParameterFromEnvironmental(KindeParameters parameter, Dotenv dotenv) { Optional.ofNullable(dotenv.get(parameter.getValue())) .ifPresent(value -> { Object mappedValue = parameter.getMapper().apply(value); this.parameters.put(parameter.getValue(), mappedValue); log.info("Set parameter: {} = {}", parameter.getValue(), parameter.getValue().toLowerCase().contains("secret") ? "****" : mappedValue); }); }This refactored version:
- Uses Optional to handle null checks.
- Improves logging by including the set value (masking if it's a secret).
- Uses string formatting in logging for better performance.
Line range hint
176-186: Approve overall changes with a minor suggestionThe changes to the constructor don't negatively impact the rest of the class, which is good. The
build()method still properly checks for required parameters.As a minor improvement, consider refactoring the
build()method to use Java 8+ features for a more concise check of required parameters:public KindeClient build() { Set<String> requiredParams = Set.of( KindeParameters.CLIENT_ID.getValue(), KindeParameters.CLIENT_SECRET.getValue(), KindeParameters.DOMAIN.getValue() ); if (!this.parameters.keySet().containsAll(requiredParams)) { throw new InvalidParameterException("The required parameters were not set"); } return injector.getInstance(KindeClient.class); }This refactored version:
- Uses a Set literal for required parameters, improving readability.
- Simplifies the containsAll check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- kinde-core/src/main/java/com/kinde/KindeClientBuilder.java (1 hunks)
- kinde-core/src/main/java/com/kinde/authorization/AuthorizationType.java (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
kinde-core/src/main/java/com/kinde/authorization/AuthorizationType.java
[warning] 44-44: kinde-core/src/main/java/com/kinde/authorization/AuthorizationType.java#L44
Added line #L44 was not covered by tests
🔇 Additional comments (7)
kinde-core/src/main/java/com/kinde/authorization/AuthorizationType.java (5)
5-7: LGTM: Import statements are appropriate.The added imports for
ArrayListandListare necessary for the new implementation of thevalueslist in the enum. These changes align well with the modifications made to theAuthorizationTypeenum.
11-11: LGTM: Enum constant initialization updated correctly.The
CODEenum constant now includes"authorization_code"as an additional value, which aligns with the PR objectives. This change enhances the flexibility of the SDK by supporting bothKINDE_GRANT_TYPE=authorization_codeandKINDE_GRANT_TYPE=CODE.
17-17: LGTM: New field declaration is appropriate.The new
valuesfield of typeList<String>is correctly declared as final and initialized with an ArrayList. This implementation allows for efficient storage and retrieval of multiple string values for each enum constant.
24-28: LGTM: New constructor implementation is correct.The new constructor correctly initializes both the
valuefield and thevalueslist. The use ofaddAll()ensures that all provided values are added to the list, allowing for multiple string representations of each enum constant.
43-45: 🛠️ Refactor suggestionOptimize new logic and add test coverage.
The new logic correctly implements the requirement to match against additional values in the
valueslist. However, there are a few points to consider:
The stream operation can be simplified for better readability:
if (constant.values.contains(value)) { return constant; }This new logic is not covered by tests according to the static analysis hint. It's crucial to add test cases that cover this new functionality to ensure its correctness and prevent future regressions.
Could you please add test cases to cover this new logic? Here's a script to verify the current test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-44: kinde-core/src/main/java/com/kinde/authorization/AuthorizationType.java#L44
Added line #L44 was not covered by testskinde-core/src/main/java/com/kinde/KindeClientBuilder.java (2)
Line range hint
1-199: Summary: Changes align well with PR objectivesThe modifications to
KindeClientBuilder.javasuccessfully address the PR objectives:
- The new Dotenv configuration (
ignoreIfMissing()) allows for the absence of .env files without causing issues, as required.- The overall functionality of the class is maintained, ensuring that it still works as expected.
- The code has been simplified, potentially improving its robustness and maintainability.
While the changes don't directly address the grant type configuration mentioned in the PR objectives, they lay the groundwork for more flexible environment variable handling, which could facilitate such changes in the future.
To fully meet the PR objectives, ensure that the
AuthorizationTypeenum (not shown in this file) has been updated to support bothKINDE_GRANT_TYPE=authorization_codeandKINDE_GRANT_TYPE=CODE.
24-40: Approve changes with a request for verificationThe simplification of the environment variable loading process is a good improvement. The use of
ignoreIfMissing()aligns well with the PR objective of allowing the absence of .env files without causing issues.However, to ensure the changes meet the PR objectives fully:
- Please verify that the application behaves correctly when the .env file is missing.
- Consider adding a log statement to inform when the .env file is missing, for debugging purposes.
To verify the behavior when .env file is missing, you can run the following script:
This script will temporarily rename the .env file, run the application, and check for any errors in the output.
Explain your changes
Bug: KindeClient ignores environment variables #26
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.