-
Notifications
You must be signed in to change notification settings - Fork 149
Remove Scope.java and use String instead. #2541
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
📝 WalkthroughWalkthroughThe pull request refactors the API key store's scope handling by removing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/Key.java (1)
34-34: JavaDoc could be more descriptive.The updated description is functional but less informative than it could be. Consider adding a note about the expected format or purpose of the scopes (e.g., "List of scope identifiers that define access permissions for this key").
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/Key.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/Scope.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/SimpleKeyStore.javacore/src/test/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/SimpleKeyStoreTest.java
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/Scope.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/test/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/SimpleKeyStoreTest.java (1)
35-35: LGTM! Clean migration to string-based scopes.The test correctly uses string literals instead of Scope objects, matching the simplified API.
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/SimpleKeyStore.java (1)
48-48: LGTM! Cleaner implementation with direct HashSet construction.The refactoring eliminates unnecessary stream mapping since scopes are now strings, making the code more straightforward.
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/stores/inConfig/Key.java (1)
29-29: Migration from Scope to String is complete.The verification confirms no remaining references to the removed Scope class from the apikey module. All identified Scope usages are from unrelated sources (OpenTelemetry library and other modules), confirming the breaking API change has been fully propagated across the codebase.
|
This pull request needs "/ok-to-test" from an authorized committer. |
|
/ok-to-test |
Yaml with Scope class:
Yaml with String replacement:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.