Simplify API Key YAML configuration and JavaDoc; add collapsed attr…#2808
Conversation
…ibute to MCElement annotations.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR changes ApiKey extractor YAML from nested mappings to inline scalar form (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/MIGRATION-GUIDE.md (2)
207-211:⚠️ Potential issue | 🟡 MinorNewly-collapsed
headerandqueryextractor elements are missing from the supported-elements listThis PR adds
collapsed = trueto theApiKeyHeaderExtractorandApiKeyQueryParamExtractorMCElement annotations, giving them the same inline single-attribute syntax as the elements listed here. However, the enumeration at line 210 still omits them. Because the sentence reads "The supported elements are: …", users looking for authoritative coverage won't find these two entries.📝 Suggested update
-The supported elements are: `api.description`, `publicURL`, `headerFilter.include`, `headerFilter.exclude`, `keyTable`, `scopeTable`, `accessControl.allow`, `accessControl.deny`, `counter`, `mutation` +The supported elements are: `api.description`, `publicURL`, `headerFilter.include`, `headerFilter.exclude`, `keyTable`, `scopeTable`, `accessControl.allow`, `accessControl.deny`, `counter`, `mutation`, `header` (ApiKey extractor), `query` (ApiKey extractor)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/MIGRATION-GUIDE.md` around lines 207 - 211, Update the supported-elements enumeration to include the newly-collapsed extractor elements by adding the ApiKeyHeaderExtractor and ApiKeyQueryParamExtractor to the list (these correspond to the MCElement annotations with collapsed = true); edit the sentence listing supported elements so it explicitly includes `header` and `query` (or the names representing ApiKeyHeaderExtractor and ApiKeyQueryParamExtractor) alongside the existing entries like `headerFilter.include` and `headerFilter.exclude` to ensure the documentation reflects the new inline single-attribute syntax.
30-31:⚠️ Potential issue | 🟡 MinorMigration bullets describe a rename-only change but the syntax also restructures the value
The bullet points instruct users to "rename" the extractors, but the actual change is more than a rename — the nested
name:attribute is also collapsed into an inline value. A user following only the bullets would produce:# result of rename-only migration apiKey: extractors: - header: name: X-API-KEY - query: name: api-key…which diverges from the documented "now" form. Adding a note about the inline value change (or replacing "Rename" with "Replace") prevents this confusion:
📝 Suggested wording
-## ApiKey extractors: -- Rename `headerExtractor` to `header` -- Rename `queryParamExtractor` to `query` +## ApiKey extractors: +- Replace `headerExtractor: \n name: <value>` with `header: <value>` +- Replace `queryParamExtractor: \n name: <value>` with `query: <value>`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/MIGRATION-GUIDE.md` around lines 30 - 31, Update the migration bullets for headerExtractor -> header and queryParamExtractor -> query to indicate that this is more than a simple rename: the nested "name:" attribute is collapsed into an inline value (i.e., replace the nested object form with an inline scalar), so change the wording from "Rename" to "Replace" or add a note explaining the value restructuring and include a before/after example showing headerExtractor/header and queryParamExtractor/query with the nested `name:` form transforming into the inline form; reference the extractor keys headerExtractor/queryParamExtractor and their new forms header/query in the guidance.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.java (1)
30-34: YAML doc only shows collapsed/custom-header form — consider adding a note on the default and the expanded syntax.The previous documentation showed both the default
X-Api-Keybehavior and a custom header. The new example exclusively shows the custom inline form. Users who relied on the explicit expanded attribute form (e.g.,- header:\n name: Authorization) won't find it documented here, and the default-header case (no YAML configuration needed at all due to theApiKeysInterceptorfallback) is no longer illustrated.Consider adding a brief note or a second example snippet like:
* `@yaml` <pre><code> * apiKey: * extractors: * - header: x-key # custom header name + * # or use default (X-Api-Key) — no extractors block needed * </code></pre>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.java` around lines 30 - 34, Update the documentation in ApiKeyHeaderExtractor.java to show both the collapsed custom-header form and the default/expanded syntaxes: add a short note that the interceptor falls back to the default header X-Api-Key when no extractor is configured (mention ApiKeysInterceptor), and include a second YAML example demonstrating the expanded form (e.g., - header: name: Authorization) so users see both custom inline and explicit attribute ways to configure the extractor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.java`:
- Line 37: The `@MCElement` annotation on ApiKeyHeaderExtractor (the
ApiKeyHeaderExtractor class) currently sets collapsed=true which breaks existing
expanded-form YAML configs; revert this by removing the collapsed attribute or
explicitly setting collapsed=false on the `@MCElement` annotation for
ApiKeyHeaderExtractor so the parser continues to accept expanded object form
(alternatively update the example YAMLs to the inline scalar form if the
collapsed behavior is intended).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyQueryParamExtractor.java`:
- Line 40: The `@MCElement` annotation on ApiKeyQueryParamExtractor was changed to
include collapsed = true which forces inline scalar YAML and will break existing
expanded-form configs; revert the change or make it optional: remove collapsed =
true from the `@MCElement` on class ApiKeyQueryParamExtractor (or add a
compatibility flag) so the GenericYamlParser.handleCollapsed logic is not
triggered, and run config parsing tests to ensure expanded YAML like "- query:\n
name: api-key" still parses correctly; if you must keep collapsed behavior, add
migration docs and a compatibility check around parsing in
GenericYamlParser.handleCollapsed to accept mapping form for this element name.
---
Outside diff comments:
In `@docs/MIGRATION-GUIDE.md`:
- Around line 207-211: Update the supported-elements enumeration to include the
newly-collapsed extractor elements by adding the ApiKeyHeaderExtractor and
ApiKeyQueryParamExtractor to the list (these correspond to the MCElement
annotations with collapsed = true); edit the sentence listing supported elements
so it explicitly includes `header` and `query` (or the names representing
ApiKeyHeaderExtractor and ApiKeyQueryParamExtractor) alongside the existing
entries like `headerFilter.include` and `headerFilter.exclude` to ensure the
documentation reflects the new inline single-attribute syntax.
- Around line 30-31: Update the migration bullets for headerExtractor -> header
and queryParamExtractor -> query to indicate that this is more than a simple
rename: the nested "name:" attribute is collapsed into an inline value (i.e.,
replace the nested object form with an inline scalar), so change the wording
from "Rename" to "Replace" or add a note explaining the value restructuring and
include a before/after example showing headerExtractor/header and
queryParamExtractor/query with the nested `name:` form transforming into the
inline form; reference the extractor keys headerExtractor/queryParamExtractor
and their new forms header/query in the guidance.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyHeaderExtractor.java`:
- Around line 30-34: Update the documentation in ApiKeyHeaderExtractor.java to
show both the collapsed custom-header form and the default/expanded syntaxes:
add a short note that the interceptor falls back to the default header X-Api-Key
when no extractor is configured (mention ApiKeysInterceptor), and include a
second YAML example demonstrating the expanded form (e.g., - header: name:
Authorization) so users see both custom inline and explicit attribute ways to
configure the extractor.
…on guide - Replaced `headerExtractor` and `queryParamExtractor` with simplified `header` and `query` formats in YAML. - Updated Migration Guide and examples to reflect the new configuration styles for ApiKey extractors. - Added new configuration formats to the list of supported elements.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
distribution/examples/security/api-key/apikey-openapi/apis.yaml (1)
1-1:⚠️ Potential issue | 🟡 MinorThe referenced schema v7.1.0 does not support the new scalar
header: X-Api-Keyformat used in this file.The schema defines
HeaderExtractorParseras an object expecting anameproperty, but line 19 uses a scalar string syntax. This will cause yaml-language-server validation errors. Either use the older object format (header: { name: X-Api-Key }) or update the schema reference to a version that supports the new scalar extractor syntax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@distribution/examples/security/api-key/apikey-openapi/apis.yaml` at line 1, The YAML uses the new scalar extractor syntax "header: X-Api-Key" which conflicts with the referenced schema (HeaderExtractorParser expects an object with a name property); fix by replacing the scalar with the object form (e.g., header: { name: X-Api-Key }) wherever the scalar header is used (or alternatively update the schema reference to a version that explicitly supports the scalar extractor syntax), ensuring entries that map to HeaderExtractorParser match the object shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@distribution/examples/security/api-key/apikey-openapi/apis.yaml`:
- Line 1: The YAML uses the new scalar extractor syntax "header: X-Api-Key"
which conflicts with the referenced schema (HeaderExtractorParser expects an
object with a name property); fix by replacing the scalar with the object form
(e.g., header: { name: X-Api-Key }) wherever the scalar header is used (or
alternatively update the schema reference to a version that explicitly supports
the scalar extractor syntax), ensuring entries that map to HeaderExtractorParser
match the object shape.
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyQueryParamExtractor.java (1)
29-32:⚠️ Potential issue | 🟠 Major
@Requiredbreaks existing configs that relied on the default"api-key"value — update Javadoc to reflect mandatory requirement.Adding
@RequiredonsetParamNameenforces the attribute as mandatory during YAML parsing. Before this PR, users who wrote- query(omitting the attribute entirely) silently got the default"api-key". After this change, the YAML parser throwsConfigurationParsingExceptionbefore the field default is ever used, breaking those existing configurations.The class-level Javadoc on lines 30–31 still reads "By default, the parameter name is
api-key", which directly contradicts@Requiredby implying the attribute is optional. Update the Javadoc to clarify that thenameattribute is now mandatory.The same issue exists in
ApiKeyHeaderExtractor.java(line 57 has@Required; lines 25–26 have conflicting Javadoc).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyQueryParamExtractor.java` around lines 29 - 32, Update the class Javadoc for ApiKeyQueryParamExtractor to remove/replace the sentence claiming a default "api-key" and instead state that the parameter name is mandatory (the name attribute must be provided) to match the presence of `@Required` on setParamName; reference the setParamName method and the `@Required` annotation so maintainers know the enforced contract. Do the same change in ApiKeyHeaderExtractor: update its class Javadoc to reflect that the header name is required (matching the `@Required` on the setter, e.g., setName) rather than implying a default. Ensure both Javadocs clearly state the attribute is mandatory and will be validated during YAML parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyQueryParamExtractor.java`:
- Around line 29-32: Update the class Javadoc for ApiKeyQueryParamExtractor to
remove/replace the sentence claiming a default "api-key" and instead state that
the parameter name is mandatory (the name attribute must be provided) to match
the presence of `@Required` on setParamName; reference the setParamName method and
the `@Required` annotation so maintainers know the enforced contract. Do the same
change in ApiKeyHeaderExtractor: update its class Javadoc to reflect that the
header name is required (matching the `@Required` on the setter, e.g., setName)
rather than implying a default. Ensure both Javadocs clearly state the attribute
is mandatory and will be validated during YAML parsing.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyQueryParamExtractor.java`:
- Line 40: The MCElement annotation on ApiKeyQueryParamExtractor currently sets
collapsed = true which will reject expanded YAML mappings and break existing
configs; update the annotation on the ApiKeyQueryParamExtractor class (the
`@MCElement`(...)) to remove collapsed = true or set collapsed = false so the
GenericYamlParser will accept both collapsed and expanded mapping forms,
preserving backward compatibility.
2 participants
…ibute to MCElement annotations.
Summary by CodeRabbit
Documentation
Refactor