-
Notifications
You must be signed in to change notification settings - Fork 41
Add AuthAdmin.getRole(roleName) for RBAC
#3238
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
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 enhances 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
|
|
/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 introduces a new getRole(roleName) method to the AuthAdmin interface, providing a more efficient way to retrieve a single role by its name instead of fetching all roles. The change is well-motivated and the implementation in DecoratedDistributedTransactionAdmin correctly delegates the call. My only feedback is to improve the Javadoc for the new interface method to make it more complete and accurate for future users of the API.
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 introduces a getRole(roleName) method to the AuthAdmin interface, providing a more efficient way to retrieve a specific role compared to fetching all roles and filtering. The change is well-implemented in DecoratedDistributedTransactionAdmin by delegating the call. My feedback focuses on improving the Javadoc for the new method to ensure it is complete and accurate.
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 adds a new getRole(roleName) method to the AuthAdmin interface and its decorator implementation to enable more efficient role retrieval in RBAC operations. Previously, clients had to fetch all roles via getRoles() and filter locally, which was inefficient.
Key Changes
- Added
AuthAdmin.getRole(String roleName)interface method with default implementation that throwsUnsupportedOperationException - Implemented
getRole(String roleName)inDecoratedDistributedTransactionAdminto delegate to the underlying admin instance
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/api/AuthAdmin.java | Adds new getRole(roleName) interface method with default implementation for retrieving a single role |
| core/src/main/java/com/scalar/db/common/DecoratedDistributedTransactionAdmin.java | Implements the new getRole(roleName) method by delegating to the wrapped admin instance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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!
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!
Co-authored-by: Hiroyuki Yamada <[email protected]>
Co-authored-by: Hiroyuki Yamada <[email protected]>
Description
In the current implementation,
AuthAdminhasgetRoles(), but doesn't havegetRole(roleName). Therefore, dependent libraries need to retrieve all the roles by usinggetRoles()and then filter them by the target role name. This PR introducesgetRole(roleName)method for address the inefficient execution.Related issues and/or PRs
Changes made
AuthAdmin.getRole(roleName)DecoratedDistributedTransactionAdmin.getRole(roleName)Checklist
Additional notes (optional)
The added interface method has a default implementation, so it shouldn't cause any compile error in the dependent libraries.
Release notes
Added
AuthAdmin.getRole(roleName).