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 due to trivial changes (1)
📝 WalkthroughWalkthroughNormalized OpenAPI server/base paths to always include a trailing slash for duplicate-path detection and base-path storage; added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/serviceproxy/APIProxy.java (1)
203-209:⚠️ Potential issue | 🟡 MinorVerify that requests to exact base path without trailing slash are intentionally rejected.
The test suite explicitly covers requests to
/foo/matching base path/foo/(OpenAPIInterceptorTest.getMatchingBasePathExactServerPath), but provides no test case for requests to/foo(without trailing slash) against the same base path. SinceOpenAPIInterceptor.getMatchingBasePath()usesstartsWith()without normalizing the incoming request URI, a request to/foowill not match/foo/and will return 404.This is likely intentional for correct path segment boundary detection, but confirm this doesn't break existing clients that access the base path without a trailing slash and whether a redirect or fallback behavior is needed.
🤖 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/APIProxy.java` around lines 203 - 209, The code currently enforces a trailing slash for server base paths via getServerPathPrefix (ensureTrailingSlash(new URIFactory(true).create(server.getUrl()).getPath())), which causes OpenAPIInterceptor.getMatchingBasePath() (which uses startsWith() on the raw request URI) to reject requests to the exact base without a trailing slash; update behavior by either normalizing incoming request paths before matching or by treating the base prefix both with and without the trailing slash as valid matches and/or emitting a redirect for the slash-missing case; add a unit test covering a request to "/foo" against a base path "/foo/" and modify getMatchingBasePath to compare normalized paths (or implement a 301 redirect) so clients hitting the base without trailing slash are handled consistently.
🧹 Nitpick comments (1)
core/src/test/java/com/predic8/membrane/core/openapi/util/UriUtilTest.java (1)
60-65: Consider adding test cases fornullinput and paths already ending with/.The
ensureTrailingSlashimplementation handlesnullinput (returning"/") and paths that already end with/(returning unchanged), but these scenarios aren't tested.🧪 Suggested additional assertions
`@Test` public void rootTrailingSlash() { assertEquals("/foo/bar/", ensureTrailingSlash("/foo/bar")); assertEquals("/", ensureTrailingSlash("/")); assertEquals("/", ensureTrailingSlash("")); + assertEquals("/", ensureTrailingSlash(null)); + assertEquals("/foo/bar/", ensureTrailingSlash("/foo/bar/")); // already has trailing slash }🤖 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/util/UriUtilTest.java` around lines 60 - 65, Add test assertions in UriUtilTest for the ensureTrailingSlash method to cover null input and inputs already ending with a slash: call ensureTrailingSlash(null) and assert it returns "/" and call ensureTrailingSlash("/foo/") (and maybe "/" again) and assert it returns the same string unchanged; place these new assertions inside the existing rootTrailingSlash test or a new test method to ensure null and already-trailing-slash cases are verified.
🤖 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/serviceproxy/APIProxy.java`:
- Around line 203-209: The code currently enforces a trailing slash for server
base paths via getServerPathPrefix (ensureTrailingSlash(new
URIFactory(true).create(server.getUrl()).getPath())), which causes
OpenAPIInterceptor.getMatchingBasePath() (which uses startsWith() on the raw
request URI) to reject requests to the exact base without a trailing slash;
update behavior by either normalizing incoming request paths before matching or
by treating the base prefix both with and without the trailing slash as valid
matches and/or emitting a redirect for the slash-missing case; add a unit test
covering a request to "/foo" against a base path "/foo/" and modify
getMatchingBasePath to compare normalized paths (or implement a 301 redirect) so
clients hitting the base without trailing slash are handled consistently.
---
Nitpick comments:
In `@core/src/test/java/com/predic8/membrane/core/openapi/util/UriUtilTest.java`:
- Around line 60-65: Add test assertions in UriUtilTest for the
ensureTrailingSlash method to cover null input and inputs already ending with a
slash: call ensureTrailingSlash(null) and assert it returns "/" and call
ensureTrailingSlash("/foo/") (and maybe "/" again) and assert it returns the
same string unchanged; place these new assertions inside the existing
rootTrailingSlash test or a new test method to ensure null and
already-trailing-slash cases are verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cd7478d-3312-4aaa-939d-1b1042bf557d
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.javacore/src/main/java/com/predic8/membrane/core/openapi/util/UriUtil.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxyKeyComplexMatchTest.javacore/src/test/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/openapi/util/UriUtilTest.java
2 participants
Summary by CodeRabbit
Bug Fixes
Tests