Reject "/path/**/other" patterns in PathPatternParser

[kaladhar-mummadi]

Issue:

Created 2 mappings as following in the controller, code is in Controller .javaof the attached project.

@GetMapping("/foo/**")
public ResponseEntity getFoo() {}

@GetMapping("/foo/**/bar")
public ResponseEntity getFooBar() {}

• In webmvc whenever request comes as /foo/1/2/3/bar (as far as last path segment
  is bar) it maps to getFooBar.
• If bar is not specified ast last path segment then it maps to getFoo. i.e all foo/1, foo/1/2, foo/ab/cd.... maps to getFoo.
• The issue is in webflux, when I switch to webflux all mappings map to
  getFoo. No matter if path consists of bar in the last path segment.
• For eg. /foo/ab/cd/bar maps to getFoo instead of getFooBar.

Uploaded the project here , to switch between webflux and webmvc please comment out
appropriate starter dependency in pom.xml.

Repro project here : https://github.com/kaladhar-mummadi/demo-issue

Notes:

• Webmvc AntPathMatcherTests has tests like /bla/**/bla, which are not covered in
  PathPatternTests of WebFlux.

[bclozel]

Thanks for raising this, it seems we need to improve that case in our implementation and in the reference docs.

First, I believe this is by design (see the first comment in #19112); if I remember correctly, not supporting "**" segments in the middle of patterns was done on purpose, for several reasons:

• this does not perform well, since it requires backtracking
• it's been often the source of misunderstanding and complex matches which lead to strange matches or even security issues in particular applications

PathPattern does support ** at the end of the pattern only (see PathPattern Javadoc) - the same way it supports the new "/path/{*captureTheRest}" syntax (but with additional capturing).

I'll turn this issue into an enhancement with two goals:

1. Ensure that "/path/**/other" patterns are rejected with PatternParseException
2. Fix the WebFlux reference documentation on URI matching which does not reflect this limitation nor additional capabilities of this implementation

[bclozel]

Note: this issue can be reproduced with a simple test:

PathPatternParser parser = new PathPatternParser();
PathPattern pattern = parser.parse("/test/**/spring");
PathContainer pathContainer = PathContainer.parsePath("/test/project/other/spring");
boolean matches = pattern.matches(pathContainer);
assertThat(matches).isTrue();

[ranarula]

@bclozel - I see a recent ticket #24945 to align MVC towards using PathPatternParser. Won't this be a breaking change ?

I wonder why there are different strategies used for WebFlux and MVC. Would have been better if the change remained consistent across both.

[bclozel]

I see a recent ticket #24945 to align MVC towards using PathPatternParser. Won't this be a breaking change ?

This won't be a breaking change since we'll support both AntPathMatcher and PathPatternParser infrastructures for Spring MVC, leaving the current one as the default. This just gives an extra choice for developers interested in optimal path matching performance (we're getting quite a few requests about that).

I wonder why there are different strategies used for WebFlux and MVC. Would have been better if the change remained consistent across both.

WebFlux was a completely new effort and we took that opportunity to revisit parts we wanted to improve. Aligning both MVC and WebFlux right away in Spring Framework 5.0 wouldn't have been a wise choice since at that point we didn't have much experience/feedback on running WebFlux applications in production.

As for adding that feature to PathPatternParser, I don't think we should do that for the reasons listed above. Even if WebFlux applications are less popular than MVC apps, I believe it's the first time we're getting an issue about this since 5.0 is out (in 2017).

[ranarula]

Thanks @bclozel for the explanation. Make sense.

If both AntPathMatcher and PathPatternParser options are available for MVC I was wondering if there will be an option available to the developer to choose one or the other infrastructure ?

[bclozel]

@ranarula Yes, we’ll provide a configuration option in the usual places in Spring Framework and we’ll probably make it a configuration property in Spring Boot.

[rstoyanchev]

Ensure that "/path/**/other" patterns are rejected with PatternParseException

This is a good idea. The error could suggest re-writing as "/path/*/other" and that should be an easy correction.

Fix the WebFlux reference documentation on URI matching..

Also the PathPattern Javadoc could be made more explicit. Currently it says only:

** matches zero or more path segments until the end of the path