-
Notifications
You must be signed in to change notification settings - Fork 149
API Key YAML Example #2535
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
API Key YAML Example #2535
Conversation
…key support - Extract `X-Api-Key` into a constant for reusability in tests. - Improve router lifecycle handling with `DefaultRouter` adjustments. - Enhance YAML configuration for API key handling, including support for registry-based ApiKeyStores. - Minor refactoring and cleanup in `Apis.yaml` and related test files.
📝 WalkthroughWalkthroughRouterCLI uses Java pattern matching to inline router retrieval and type-checking. ApiKeysInterceptor now loads ApiKeyStore instances from the router registry in addition to the XML beanFactory. Added an API Key OpenAPI example YAML and small test cleanups (header constant and method visibility). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
🧹 Nitpick comments (2)
distribution/examples/security/api-key/apikey-openapi/apis.yaml (1)
23-24: Remove or document commented-out code.The commented
expressionExtractorconfiguration should either be removed if it's not needed, or documented with a comment explaining when to use it as an alternative toheaderExtractor.🔎 Suggested improvement
If this is meant to show an alternative approach, add a comment:
- headerExtractor: name: "X-Api-Key" + # Alternative: Extract from expression # - expressionExtractor: # expression: "header['X-Api-Key']"Or remove it entirely if it's not needed for the example.
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/APIKeyWithOpenAPIExampleTest.java (1)
29-29: Good refactoring to extract the header name as a constant.Extracting
"X-Api-Key"into a constant eliminates duplication and makes the tests more maintainable.Optionally, consider renaming to
API_KEY_HEADER_NAMEto make it clearer that this is the header name rather than the key value itself, though the current name is acceptable in context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptor.javadistribution/examples/security/api-key/apikey-openapi/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/APIKeyWithOpenAPIExampleTest.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). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/APIKeyWithOpenAPIExampleTest.java (1)
37-37: LGTM: Method visibility changes are appropriate.Reducing test method visibility from
publicto package-private is fine with JUnit 5, which doesn't require public test methods. This aligns with modern Java testing conventions.Also applies to: 46-46, 58-58, 83-83
core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
78-79: The pattern matching correctly handles all router types.All initialization paths (
oas,yaml,xml) explicitly returnDefaultRouterinstances.RouterXmlBootstrap.initByXML()explicitly retrieves and returns aDefaultRouterbean from the Spring context (line 52 in RouterXmlBootstrap.java), making theinstanceof DefaultRoutercheck at line 78 always true. ThewaitFor()method is only defined onDefaultRouter, and since that's the only router type returned bygetRouter(), the code correctly ensureswaitFor()is always called.Likely an incorrect or invalid review comment.
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptor.java
Show resolved
Hide resolved
…d router logic - Renamed `API_KEY` to `API_KEY_HEADER` for better clarity and consistency. - Updated tests and flow configurations to reflect the rename. - Removed unused imports and streamlined logic in `ApiKeysInterceptor`. - Enhanced YAML setup for OpenAPI validation and API key extraction ordering.
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
♻️ Duplicate comments (1)
distribution/examples/security/api-key/apikey-openapi/apis.yaml (1)
18-18: Clarify the rationale forrequired: falsein a security example.Setting
required: falseis counterintuitive for an API key security demonstration. While the comment on line 17 mentions OpenAPI validation, it doesn't clearly explain why API key validation is optional here. Consider either settingrequired: trueor enhancing the comment to explicitly state the demonstration goal (e.g., "Set to false to demonstrate dual validation paths—OpenAPI validator enforces security").
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptor.java (1)
28-28: Consider reverting to specific import.While wildcard imports work, importing only the methods actually used (
joining) makes dependencies more explicit and aids IDE optimization.🔎 Proposed fix
-import static java.util.stream.Collectors.*; +import static java.util.stream.Collectors.joining;distribution/examples/security/api-key/apikey-openapi/apis.yaml (1)
25-25: Add newline at end of file.POSIX standards and many tools expect text files to end with a newline character. This improves compatibility with line-based tools like
diffandwc.
📜 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/ApiKeysInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.javadistribution/examples/security/api-key/apikey-openapi/apis.yamldistribution/src/test/java/com/predic8/membrane/examples/withinternet/test/APIKeyWithOpenAPIExampleTest.java
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.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). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/ApiKeysInterceptor.java (1)
86-96: LGTM! Registry support for YAML configuration added correctly.The dual-source loading (XML beanFactory and YAML registry) is well-structured with clear comments explaining the configuration sources. The null-checks prevent NPEs, and the aggregation into
this.storesensures both XML and YAML stores are available.distribution/examples/security/api-key/apikey-openapi/apis.yaml (1)
15-25: LGTM! Flow ordering and validation configuration are well-designed.The explicit ordering—extracting the API key before invoking
openapiValidator—is correctly implemented and clearly documented. The comments effectively explain why the validator is positioned in the flow rather than relying on default pre-flow validation.distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/APIKeyWithOpenAPIExampleTest.java (1)
29-93: LGTM! Test refactoring improves maintainability.The extraction of
API_KEY_HEADERas a constant eliminates magic strings and improves consistency across test methods. Reducing test method visibility to package-private is appropriate and follows best practices. All usages are correctly updated.
|
This pull request needs "/ok-to-test" from an authorized committer. |
…key support
X-Api-Keyinto a constant for reusability in tests.DefaultRouteradjustments.Apis.yamland related test files.Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.