-
Notifications
You must be signed in to change notification settings - Fork 40
[RBAC] Refactor AuthAdmin RBAC interfaces to eliminate confusion #3142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
to eliminate confusion
Summary of ChangesHello @komamitsu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Role-Based Access Control (RBAC) interfaces within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request provides an excellent refactoring of the RBAC interfaces in AuthAdmin, significantly improving their clarity and maintainability. The new interface names like GranteeUser and RoleForUser are much more intuitive, and flattening the Role/RoleDetail hierarchy simplifies the model. The changes are well-explained and consistently applied across the affected files. However, I noticed that the checklist for tests is unchecked, and no test files were modified in this PR. Since this is a breaking change to public interfaces, adding or updating unit tests is crucial to verify the contracts of the new interfaces and prevent future regressions. Please consider adding tests for the AuthAdmin RBAC interfaces.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the RBAC-related interfaces in AuthAdmin to improve clarity and reduce confusion, which is a welcome improvement. The renamings like RoleHierarchy to MemberRole and UserRole to GranteeUser make the API more intuitive. The changes are consistently applied to DecoratedDistributedTransactionAdmin as well. I've added a few suggestions to further improve the Javadoc clarity. More importantly, I've raised a concern about the lack of unit test updates for DecoratedDistributedTransactionAdmin, especially since this PR introduces breaking changes to a public API. Adding tests would increase confidence in the correctness of these changes.
core/src/main/java/com/scalar/db/common/DecoratedDistributedTransactionAdmin.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the RBAC (Role-Based Access Control) interfaces in AuthAdmin to improve clarity and eliminate confusion in their naming and structure. The refactoring merges Role and RoleDetail, renames several interfaces to better reflect their purpose, and updates method names for better semantic clarity.
Key Changes:
- Merged
RoleandRoleDetailinto a singleRoleinterface with flattened fields - Renamed interfaces:
RoleHierarchy→MemberRole,UserRoleDetail→RoleForUser,UserRole→GranteeUser - Renamed method:
getUsersForRole()→getGranteeUsersForRole()andgetRoleHierarchies()→getMemberRoles()
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/api/AuthAdmin.java | Updates RBAC interface definitions and method signatures to use new naming conventions |
| core/src/main/java/com/scalar/db/common/DecoratedDistributedTransactionAdmin.java | Updates delegating methods to use new return types and method names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Torch3333
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
feeblefakie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
brfrn169
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
We added RBAC related interfaces to AuthAdmin. We also noticed the current interface names and structures were confusing in terms of the use cases.
Per discussions, this PR updates AuthAdmin's RBAC interfaces as follows:
RoleandRoleDetailRoleis not used separatelyRole's fields asRoleinterfacegetRoleHierarchies()togetMemberRoles()(see below for detail)RoleHierarchySHOW ROLES/SHOW ROLES FOR <role_name>(memberRolescolumn)getRoleName()that returns the same name asRole.getName()MemberRole(previous option:MemberRoleInfo) which containsgetName()(this is the granted member role name) andhasAdminOption()RoleHierarchy.getRoleName()is removedUserRoleDetailRoleDetail, but the name seems to be related toUserRoleand is confusingRoleForUsersince it containshasAdminOptionOnUser()in addition toRole(previous name isRoleDetail)UserRoleSHOW ROLE USERS FOR <role_name>GranteeUser(previous option:UserRoleInfo) which containsgetName()(this is the grantee username who received a role) andhasAdminOption()UserRole.getRoleName()is removedRelated issues and/or PRs
#3083
https://github.com/scalar-labs/scalardb-sql/pull/895
Changes made
com.scalar.db.api.AuthAdmincom.scalar.db.common.DecoratedDistributedTransactionAdminChecklist
Additional notes (optional)
First, I planed to add new interfaces and methods keeping the old ones as-is so that the dependent projects can be successfully compiled. But
Roleinterface conflicts while it's structure is changed. So, I gave up with the plan and replaced the old interfaces and methods with the new ones.Release notes
N/A