OpenAPI-Validator: Split request and response validation#2903
Open
OpenAPI-Validator: Split request and response validation#2903
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughValidation was split into two phases: Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java (1)
49-49: Move inline split TODO to a tracked issue.Line 49 leaves implementation intent inside core validation flow. Prefer referencing a tracked issue ID (or removing the TODO after implementation) to keep this path clean.
I can draft a small follow-up patch that replaces this inline TODO with an issue-linked comment and scaffolds the split points.
🤖 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/openapi/validators/OperationValidator.java` at line 49, Replace the inline "// `@TODO` Split till here." comment in OperationValidator (class OperationValidator in the shown diff) with a tracked-issue reference or remove it: create or reference a ticket (e.g., ISSUE-1234 or a GitHub issue URL) describing the intended split and then update the comment to something like "See ISSUE-1234: planned split of validation flow" and optionally add brief scaffold markers (e.g., "split-point A/B") near the current location to guide the future refactor; ensure the updated comment mentions OperationValidator so it's discoverable.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java (1)
203-224: Complete split by reusing a singleOpenAPIValidatorper exchange.Right now Line 217 and Line 224 create separate validator instances. Given the TODO plan, storing/reusing one validator in
Exchangewill better support split request/response validation and avoid duplicate initialization work.Proposed refactor sketch
+ private static final String OPENAPI_VALIDATOR = "membrane.openapi.validator"; + + private OpenAPIValidator getOrCreateValidator(OpenAPIRecord rec, Exchange exc) { + OpenAPIValidator validator = exc.getProperty(OPENAPI_VALIDATOR, OpenAPIValidator.class); + if (validator == null) { + validator = new OpenAPIValidator(router.getConfiguration().getUriFactory(), rec); + exc.setProperty(OPENAPI_VALIDATOR, validator); + } + return validator; + } ... - return new OpenAPIValidator(router.getConfiguration().getUriFactory(), rec).validate(getOpenapiValidatorRequest(exc)); + return getOrCreateValidator(rec, exc).validate(getOpenapiValidatorRequest(exc)); ... - return new OpenAPIValidator(router.getConfiguration().getUriFactory(), rec).validateResponse(getOpenapiValidatorRequest(exc), getOpenapiValidatorResponse(exc)); + return getOrCreateValidator(rec, exc).validateResponse(getOpenapiValidatorRequest(exc), getOpenapiValidatorResponse(exc));🤖 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/openapi/serviceproxy/OpenAPIInterceptor.java` around lines 203 - 224, Both validateRequest and validateResponse currently instantiate separate OpenAPIValidator instances; change them to create a single OpenAPIValidator per Exchange and store it under the key "membrane.openapi.validator" so the same validator is reused for request and response validation. Implement logic in validateRequest: if shouldValidate(..., REQUESTS) create and store new OpenAPIValidator(router.getConfiguration().getUriFactory(), rec) on the Exchange and call validate(getOpenapiValidatorRequest(exc)); in validateResponse: retrieve the validator from the Exchange (and if validateResponse is true but no validator exists, create and store one using the same constructor), then call validateResponse(getOpenapiValidatorRequest(exc), getOpenapiValidatorResponse(exc)); use the existing methods OpenAPIValidator, getOpenapiValidatorRequest, getOpenapiValidatorResponse, and shouldValidate, and ensure the Exchange property key is "membrane.openapi.validator".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java`:
- Around line 203-224: Both validateRequest and validateResponse currently
instantiate separate OpenAPIValidator instances; change them to create a single
OpenAPIValidator per Exchange and store it under the key
"membrane.openapi.validator" so the same validator is reused for request and
response validation. Implement logic in validateRequest: if shouldValidate(...,
REQUESTS) create and store new
OpenAPIValidator(router.getConfiguration().getUriFactory(), rec) on the Exchange
and call validate(getOpenapiValidatorRequest(exc)); in validateResponse:
retrieve the validator from the Exchange (and if validateResponse is true but no
validator exists, create and store one using the same constructor), then call
validateResponse(getOpenapiValidatorRequest(exc),
getOpenapiValidatorResponse(exc)); use the existing methods OpenAPIValidator,
getOpenapiValidatorRequest, getOpenapiValidatorResponse, and shouldValidate, and
ensure the Exchange property key is "membrane.openapi.validator".
In
`@core/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java`:
- Line 49: Replace the inline "// `@TODO` Split till here." comment in
OperationValidator (class OperationValidator in the shown diff) with a
tracked-issue reference or remove it: create or reference a ticket (e.g.,
ISSUE-1234 or a GitHub issue URL) describing the intended split and then update
the comment to something like "See ISSUE-1234: planned split of validation flow"
and optionally add brief scaffold markers (e.g., "split-point A/B") near the
current location to guide the future refactor; ensure the updated comment
mentions OperationValidator so it's discoverable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8288c48-2b86-454c-a677-aeb0eac98ec4
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.java
…es. Improve handling of validation logic by introducing ValidationPlan class to streamline request and response validations. Update related tests to reflect changes.
Collaborator
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
Contributor
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/openapi/validators/OperationValidator.java (1)
71-79:⚠️ Potential issue | 🟠 MajorSupport for valid OpenAPI HTTP methods is incomplete.
The
getOperation(...)method currently omitsHEAD,OPTIONS, andTRACE—all of which are explicitly supported in the OpenAPI 3.0.2 specification and available in thePathItemclass viagetHead(),getOptions(), andgetTrace()methods. Specs declaring these operations will be rejected as method-not-allowed even when valid.Proposed fix
private static Operation getOperation(String method, PathItem pi) throws MethodNotAllowException { return switch (method.toUpperCase()) { case "GET" -> pi.getGet(); case "POST" -> pi.getPost(); case "PUT" -> pi.getPut(); case "DELETE" -> pi.getDelete(); case "PATCH" -> pi.getPatch(); + case "HEAD" -> pi.getHead(); + case "OPTIONS" -> pi.getOptions(); + case "TRACE" -> pi.getTrace(); default -> throw new MethodNotAllowException(); }; }🤖 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/openapi/validators/OperationValidator.java` around lines 71 - 79, The getOperation method rejects valid OpenAPI methods because it only handles GET/POST/PUT/DELETE/PATCH; update the getOperation(String method, PathItem pi) function to also handle "HEAD", "OPTIONS", and "TRACE" by mapping them to pi.getHead(), pi.getOptions(), and pi.getTrace() respectively, keeping the existing behavior of throwing MethodNotAllowException for unknown methods; locate the getOperation method and add those cases to the switch (using method.toUpperCase()) so specs with head/options/trace operations are accepted.
🤖 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/openapi/validators/OperationValidator.java`:
- Around line 71-79: The getOperation method rejects valid OpenAPI methods
because it only handles GET/POST/PUT/DELETE/PATCH; update the
getOperation(String method, PathItem pi) function to also handle "HEAD",
"OPTIONS", and "TRACE" by mapping them to pi.getHead(), pi.getOptions(), and
pi.getTrace() respectively, keeping the existing behavior of throwing
MethodNotAllowException for unknown methods; locate the getOperation method and
add those cases to the switch (using method.toUpperCase()) so specs with
head/options/trace operations are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d882bfd9-9217-49a3-b7c8-08432ac55bbb
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/OperationValidator.javacore/src/test/java/com/predic8/membrane/core/openapi/OpenAPIValidatorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.java
✅ Files skipped from review due to trivial changes (1)
- core/src/test/java/com/predic8/membrane/core/openapi/OpenAPIValidatorTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java
- core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.java
…on' into openapi-validator-split-validation
Collaborator
Member
Author
|
@christiangoerdes looks good. Could you take care about the Is there a test for:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Tests
Chores