Fix StripPrefix and RewritePath filters including servlet context-path#4089
Fix StripPrefix and RewritePath filters including servlet context-path#4089garvit-joshi wants to merge 3 commits intospring-cloud:mainfrom
Conversation
60ae2f3 to
c37295f
Compare
c37295f to
c13ff9c
Compare
|
The failing test is in the webflux module and seems unrelated to my changes — I only modified the webmvc module. |
a78302f to
b8aeb12
Compare
Fixes spring-cloudgh-4088 Co-authored-by: Garvit Joshi <garvitjoshi9@gmail.com> Signed-off-by: Garvit Joshi <garvitjoshi9@gmail.com>
b8aeb12 to
500a596
Compare
Signed-off-by: Garvit Joshi <garvitjoshi9@gmail.com>
9e465e3 to
155d4b2
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a mismatch in the WebMVC gateway between how Path predicates match (path within the servlet context) and how path-manipulating filters (StripPrefix, RewritePath, PrefixPath) rewrite (previously using the full URI path including the servlet context-path). It introduces an opt-in mechanism to strip the servlet context-path before applying route filters.
Changes:
- Add
MvcUtils.stripContextPath(...)andBeforeFilterFunctions.stripContextPath()to remove the servlet context-path from the request URI before downstream path manipulation. - Add
spring.cloud.gateway.server.webmvc.strip-context-pathproperty and wire it into route construction via a per-routebefore(...)processor. - Add unit tests covering context-path stripping and expected interactions with
StripPrefix,RewritePath, andPrefixPath.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
spring-cloud-gateway-server-webmvc/src/main/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtils.java |
Adds a utility to strip servlet context-path from a provided path string. |
spring-cloud-gateway-server-webmvc/src/main/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctions.java |
Introduces a new before-filter function that rewrites the request URI by removing the context-path. |
spring-cloud-gateway-server-webmvc/src/main/java/org/springframework/cloud/gateway/server/mvc/config/RouterFunctionHolderFactory.java |
Optionally applies the new before-filter to routes when the new property is enabled. |
spring-cloud-gateway-server-webmvc/src/main/java/org/springframework/cloud/gateway/server/mvc/config/GatewayMvcProperties.java |
Adds the stripContextPath configuration property (default false). |
spring-cloud-gateway-server-webmvc/src/test/java/org/springframework/cloud/gateway/server/mvc/filter/BeforeFilterFunctionsTests.java |
Adds unit tests for stripping context-path and behavior with other path filters. |
spring-cloud-gateway-server-webmvc/src/test/java/org/springframework/cloud/gateway/server/mvc/common/MvcUtilsTests.java |
Adds unit tests for MvcUtils.stripContextPath(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (stripContextPath) { | ||
| builder.before(BeforeFilterFunctions.stripContextPath()); | ||
| } |
There was a problem hiding this comment.
The new stripContextPath behavior is wired into route construction here, but there’s no test coverage ensuring the property actually results in the BeforeFilterFunctions.stripContextPath() processor being applied (and applied before other route filters). Since there are already tests verifying route construction in GatewayMvcPropertiesBeanDefinitionRegistrarTests, consider adding a focused test that sets spring.cloud.gateway.server.webmvc.strip-context-path=true and asserts the resulting RouterFunction/filter chain (or an integration test using server.servlet.context-path) behaves as intended.
There was a problem hiding this comment.
I'm not sure I like adding filters here. One of my rules building the WebMVC gateway server was no implicit or magic filters. This goes against this rule.
| // Simulate global filter stripping context path first | ||
| ServerRequest stripped = BeforeFilterFunctions.stripContextPath().apply(request); | ||
| ServerRequest result = BeforeFilterFunctions.stripPrefix(2).apply(stripped); |
There was a problem hiding this comment.
These tests describe the setup as a “global filter stripping context path first”, but the production wiring adds stripContextPath() as a per-route before(...) processor (controlled by spring.cloud.gateway.server.webmvc.strip-context-path). Consider rewording the comment to match the actual behavior so the tests don’t suggest an implementation detail that doesn’t exist.
.../src/main/java/org/springframework/cloud/gateway/server/mvc/config/GatewayMvcProperties.java
Show resolved
Hide resolved
|
Please add some documentation for the new functionality |
There was a problem hiding this comment.
I'm beginning to think we need to support this like we do like adaptCachedBody. If someone uses a predicate like readBody they are required to use adaptCachedBody later. If someone is using a servlet context path, they should use a strip context path filter.
|
Got it, Should I add something like: |
StripPrefix and RewritePath filters in the WebMVC variant operated on the full URI path including the servlet context-path, while the Path route predicate matches against the path without it. This caused incorrect segment counting and path rewriting when server.servlet.context-path is configured.
Add MvcUtils.stripContextPath() to remove the context-path prefix before path manipulation, aligning filter behavior with predicate matching.
Closes #4088