Disable openapi response path validation#2901
Disable openapi response path validation#2901christiangoerdes wants to merge 3 commits intomasterfrom
Conversation
|
/ok-to-test |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChanged OpenAPI response validation so path mismatches no longer produce 404 errors when validating responses (only when validating requests). Introduced static imports and updated tests to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.java (1)
98-99: Add a debug breadcrumb when skipping response path mismatch validation.This path now succeeds silently; a debug log would help diagnose spec/routing drift in production.
Suggested minimal change
// Do not validate the path for response + log.debug("Skipping response path validation because no OpenAPI path matched request path '{}'.", req.getPath()); return new ValidationErrors();🤖 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/OpenAPIValidator.java` around lines 98 - 99, Add a debug breadcrumb in OpenAPIValidator immediately before the early return that skips response path mismatch validation: log a concise debug message (e.g., using the class logger instance such as logger.debug or LOG.debug) indicating that response path validation was skipped and include available context like the request path, operation identifier or response status (use the local variables available in the method surrounding the return) to help diagnose spec/routing drift, then perform the existing return new ValidationErrors().core/src/test/java/com/predic8/membrane/core/openapi/validators/ResponseTest.java (1)
117-121: Consider adding the inverse regression test for request validation.A paired test asserting request validation still returns a PATH/404 on unmatched paths would lock in the asymmetric behavior.
Companion test sketch
+ `@Test` + public void requestPathMismatchStillFailsValidation() { + ValidationErrors errors = validator.validate(Request.get().path("/does-not-exist")); + assertEquals(1, errors.size()); + assertEquals(PATH, errors.get(0).getContext().getValidatedEntityType()); + assertEquals(404, errors.get(0).getContext().getStatusCode()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/openapi/validators/ResponseTest.java` around lines 117 - 121, Add a companion test in ResponseTest that verifies request validation fails for unmatched paths: add a method (e.g., requestPathMismatchReturnsPath404) which calls validator.validateRequest(Request.get().path("/does-not-exist"), /* suitable request object */) and assert that the returned ValidationErrors indicate a path-mismatch (non-zero size and/or contains the PATH/404 error code/message). Use the same Request.get().path(...) and validator.validateRequest(...) symbols to locate where to add the test so the asymmetric behavior is locked in.
🤖 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/OpenAPIValidator.java`:
- Around line 98-99: Add a debug breadcrumb in OpenAPIValidator immediately
before the early return that skips response path mismatch validation: log a
concise debug message (e.g., using the class logger instance such as
logger.debug or LOG.debug) indicating that response path validation was skipped
and include available context like the request path, operation identifier or
response status (use the local variables available in the method surrounding the
return) to help diagnose spec/routing drift, then perform the existing return
new ValidationErrors().
In
`@core/src/test/java/com/predic8/membrane/core/openapi/validators/ResponseTest.java`:
- Around line 117-121: Add a companion test in ResponseTest that verifies
request validation fails for unmatched paths: add a method (e.g.,
requestPathMismatchReturnsPath404) which calls
validator.validateRequest(Request.get().path("/does-not-exist"), /* suitable
request object */) and assert that the returned ValidationErrors indicate a
path-mismatch (non-zero size and/or contains the PATH/404 error code/message).
Use the same Request.get().path(...) and validator.validateRequest(...) symbols
to locate where to add the test so the asymmetric behavior is locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06ff8471-8469-4f77-aa9c-f8cdb224e87d
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/ResponseTest.java
|
This pull request needs "/ok-to-test" from an authorized committer. |
core/src/test/java/com/predic8/membrane/core/openapi/validators/ResponseTest.java
Outdated
Show resolved
Hide resolved
…d inline validation.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor