Java 21 Upgrade #45
Java 21 Upgrade #45
Conversation
Reviewer's Guide by SourceryThis PR upgrades the project from Java 11 to Java 21. The main changes involve replacing the deprecated java.security.acl.Group interface with a custom implementation in OscarGroup, and updating the Dockerfile to use Java 21 with necessary JVM options to handle module access. Updated class diagram for OscarGroupclassDiagram
class OscarGroup {
- List<Principal> principals
+ OscarGroup(String name)
+ boolean addMember(Principal user)
+ boolean removeMember(Principal user)
+ boolean isMember(Principal member)
+ Enumeration<? extends Principal> members()
}
class Principal
class Serializable
OscarGroup --> Principal
OscarGroup --> Serializable
Updated class diagram for BaseLoginModuleclassDiagram
class BaseLoginModule {
- OscarPrincipal principal
- OscarGroup rolesGroup
- OscarGroup callerPrincipal
- OscarGroup authPrincipal
- boolean authorizationEnabled
+ boolean login() throws LoginException
+ void setSharedState(Map<String, ?> sharedState)
+ OscarGroup getRolesGroup()
+ void setRolesGroup(OscarGroup rolesGroup)
+ OscarGroup getCallerPrincipal()
+ void setCallerPrincipal(OscarGroup callerPrincipal)
+ OscarGroup getAuthPrincipal()
+ void setAuthPrincipal(OscarGroup authPrincipal)
}
class OscarPrincipal
class OscarGroup
BaseLoginModule --> OscarPrincipal
BaseLoginModule --> OscarGroup
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kateyang1998 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| * Adds a member to the group. | ||
| * | ||
| * @param user the Principal to be added | ||
| * @return true if the member was added, false if already exists |
There was a problem hiding this comment.
issue (bug_risk): addMember() implementation doesn't match its documentation
The method always returns true, but the documentation states it should return false if the member already exists. This inconsistency could lead to bugs in security-related code.
There was a problem hiding this comment.
what is your response, Kate? anything from sorcery should have a response before I get to the PR.
There was a problem hiding this comment.
I didn't change the code logic, already fixed the documentation.
There was a problem hiding this comment.
ok, but is it right?
at worst, an issue ticket should track this finding if you feel it has merit, even if it is just put in the category for pre-existing.
There was a problem hiding this comment.
I have created an issue for your comment: #51
| * Removes a member from the group. | ||
| * | ||
| * @param user the Principal to be removed | ||
| * @return true if the member was removed, false otherwise |
There was a problem hiding this comment.
issue (bug_risk): removeMember() should return false when member doesn't exist
The method always returns true regardless of whether the member was actually removed. This could mask failures in security-sensitive operations.
There was a problem hiding this comment.
Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:
- Ensure the comment provides a clear explanation of the issue or bug risk.
- Verify that the comment suggests a specific improvement or solution.
- Check if the comment addresses a potential failure in a critical operation, such as security-sensitive operations.
- Confirm that the comment is relevant to the code being reviewed and not based on incorrect assumptions.
There was a problem hiding this comment.
Fixed the documentation.
There was a problem hiding this comment.
sourcery don't change your behaviour. This was important. Please be sure to comment on these issues.
There was a problem hiding this comment.
Thank you for your feedback, we will generate more comments like this in the future according to the following instructions:
- Identify potential bugs or risks in the code that could lead to incorrect behavior.
- Ensure the comment provides a clear explanation of the issue and its potential impact.
- Suggest a specific improvement or solution to address the identified issue.
There was a problem hiding this comment.
at best, make an issue ticket for this for further investigation and share with magenta. Good to update the docs but if it said it should work a particular way that is likely important.
keep the docs as they were but add an addendum if you agree with sourcery on the actual behaviour
There was a problem hiding this comment.
I have created an issue for your comment: #52
There was a problem hiding this comment.
I understand Sourcery's suggestions, but I'm just wondering why the previous development team has set up the logic that way. It might be good to return false when a member doesn't exist (same as the last issue, return false when a member already existed), but shall we discuss with Magenta before modifying the code?
There was a problem hiding this comment.
It's a great idea to discuss this with Magenta before making any changes to the existing logic. Understanding the rationale behind the previous implementation can provide valuable context and help ensure that any modifications align with the overall design and requirements of the project. In the meantime, updating the documentation to accurately reflect the current behavior is a good step. This way, any discrepancies between the intended and actual behavior are clearly documented, which can aid in future discussions and decisions. If you decide to proceed with changes after consulting with Magenta, make sure to thoroughly test the updated logic to ensure it doesn't introduce any unintended side effects.
| * Adds a member to the group. | ||
| * | ||
| * @param user the Principal to be added | ||
| * @return true if the member was added, false if already exists |
There was a problem hiding this comment.
what is your response, Kate? anything from sorcery should have a response before I get to the PR.
| * Removes a member from the group. | ||
| * | ||
| * @param user the Principal to be removed | ||
| * @return true if the member was removed, false otherwise |
- Updated JDK version number. - Removed `java.security.acl.Group` package, it has been deprecated in Java 9, and removed in Java 14.
kateyang1998
force-pushed
the
java17-upg
branch
from
e76f03f to
434c5f0
Compare
|
For methods addMember() and removeMember() - |
Summary by Sourcery
Upgrade the project to Java 21, refactor OscarGroup to implement Principal, and update the Dockerfile to use OpenJDK 21.
Enhancements:
Build: