From dccedd5ad59422ce2b1369784a748e831f2e39a9 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 2 Aug 2017 15:05:28 +0200 Subject: [PATCH] Minor refactoring in PathPattern Rename getPathRemaining to matchStartOfPath since the method does match and to be more clear about what the method and the return value intuitively follows. Remove matchStart which matches the start of the pattern (rather than the start of the path). It is a use case that does not come up in request mapping. --- .../web/util/pattern/PathPattern.java | 70 +++----- .../web/util/pattern/PathPatternTests.java | 154 ++---------------- .../function/server/RequestPredicates.java | 2 +- .../RequestMappingInfoHandlerMapping.java | 3 + .../WebFluxConfigurationSupportTests.java | 4 +- 5 files changed, 49 insertions(+), 184 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java index d6541d7fc49..2af53b1cb85 100644 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java +++ b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPattern.java @@ -177,14 +177,35 @@ public class PathPattern implements Comparable { } /** - * Match the beginning of the given path and return the remaining portion of - * the path not covered by this pattern. This is useful for matching through - * nested routes where the path is matched incrementally at each level. + * Match this pattern to the given URI path and return extracted URI template + * variables as well as path parameters (matrix variables). + * @param pathContainer the candidate path to attempt to match against + * @return info object with the extracted variables + * @throws IllegalStateException if the path does not match the pattern + */ + public PathMatchInfo matchAndExtract(PathContainer pathContainer) { + MatchingContext matchingContext = new MatchingContext(pathContainer, true); + if (this.head != null && this.head.matches(0, matchingContext)) { + return matchingContext.getPathMatchResult(); + } + else if (!hasLength(pathContainer)) { + return PathMatchInfo.EMPTY; + } + else { + throw new IllegalStateException( + "Pattern \"" + this + "\" is not a match for \"" + pathContainer.value() + "\""); + } + } + + /** + * Match the beginning of the given path and return the remaining portion + * not covered by this pattern. This is useful for matching nested routes + * where the path is matched incrementally at each level. * @param pathContainer the candidate path to attempt to match against * @return info object with the match result or {@code null} for no match */ @Nullable - public PathRemainingMatchInfo getPathRemaining(PathContainer pathContainer) { + public PathRemainingMatchInfo matchStartOfPath(PathContainer pathContainer) { if (this.head == null) { return new PathRemainingMatchInfo(pathContainer); } @@ -205,49 +226,12 @@ public class PathPattern implements Comparable { } else { info = new PathRemainingMatchInfo(pathContainer.subPath(matchingContext.remainingPathIndex), - matchingContext.getPathMatchResult()); + matchingContext.getPathMatchResult()); } return info; } } - /** - * @param pathContainer the path to check against the pattern - * @return true if the pattern matches as much of the path as is supplied - */ - public boolean matchStart(PathContainer pathContainer) { - if (this.head == null) { - return !hasLength(pathContainer); - } - else if (!hasLength(pathContainer)) { - return true; - } - MatchingContext matchingContext = new MatchingContext(pathContainer, false); - matchingContext.setMatchStartMatching(true); - return this.head.matches(0, matchingContext); - } - - /** - * Match this pattern to the given URI path and extract URI template - * variables as well as path parameters (matrix variables). - * @param pathContainer the candidate path to attempt to match against - * @return info object with the extracted variables - * @throws IllegalStateException if the path does not match the pattern - */ - public PathMatchInfo matchAndExtract(PathContainer pathContainer) { - MatchingContext matchingContext = new MatchingContext(pathContainer, true); - if (this.head != null && this.head.matches(0, matchingContext)) { - return matchingContext.getPathMatchResult(); - } - else if (!hasLength(pathContainer)) { - return PathMatchInfo.EMPTY; - } - else { - throw new IllegalStateException( - "Pattern \"" + this + "\" is not a match for \"" + pathContainer.value() + "\""); - } - } - /** * Determine the pattern-mapped part for the given path. *

For example:

    @@ -266,7 +250,7 @@ public class PathPattern implements Comparable { // TODO: implement extractPathWithinPattern for PathContainer - // TODO: also see this from PathPattern javadoc + align behavior with getPathRemaining instead: + // TODO: align behavior with matchStartOfPath with regards to this: // As per the contract on {@link PathMatcher}, this method will trim leading/trailing // separators. It will also remove duplicate separators in the returned path. diff --git a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java index c6dcfc4e997..181bea804b7 100644 --- a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java @@ -320,40 +320,40 @@ public class PathPatternTests { @Test public void pathRemainingCornerCases_spr15336() { // No match when the literal path element is a longer form of the segment in the pattern - assertNull(parse("/foo").getPathRemaining(toPathContainer("/footastic/bar"))); - assertNull(parse("/f?o").getPathRemaining(toPathContainer("/footastic/bar"))); - assertNull(parse("/f*o*p").getPathRemaining(toPathContainer("/flooptastic/bar"))); - assertNull(parse("/{abc}abc").getPathRemaining(toPathContainer("/xyzabcbar/bar"))); + assertNull(parse("/foo").matchStartOfPath(toPathContainer("/footastic/bar"))); + assertNull(parse("/f?o").matchStartOfPath(toPathContainer("/footastic/bar"))); + assertNull(parse("/f*o*p").matchStartOfPath(toPathContainer("/flooptastic/bar"))); + assertNull(parse("/{abc}abc").matchStartOfPath(toPathContainer("/xyzabcbar/bar"))); // With a /** on the end have to check if there is any more data post // 'the match' it starts with a separator - assertNull(parse("/resource/**").getPathRemaining(toPathContainer("/resourceX"))); - assertEquals("",parse("/resource/**").getPathRemaining(toPathContainer("/resource")).getPathRemaining().value()); + assertNull(parse("/resource/**").matchStartOfPath(toPathContainer("/resourceX"))); + assertEquals("",parse("/resource/**").matchStartOfPath(toPathContainer("/resource")).getPathRemaining().value()); // Similar to above for the capture-the-rest variant - assertNull(parse("/resource/{*foo}").getPathRemaining(toPathContainer("/resourceX"))); - assertEquals("",parse("/resource/{*foo}").getPathRemaining(toPathContainer("/resource")).getPathRemaining().value()); + assertNull(parse("/resource/{*foo}").matchStartOfPath(toPathContainer("/resourceX"))); + assertEquals("",parse("/resource/{*foo}").matchStartOfPath(toPathContainer("/resource")).getPathRemaining().value()); - PathPattern.PathRemainingMatchInfo pri = parse("/aaa/{bbb}/c?d/e*f/*/g").getPathRemaining(toPathContainer("/aaa/b/ccd/ef/x/g/i")); + PathPattern.PathRemainingMatchInfo pri = parse("/aaa/{bbb}/c?d/e*f/*/g").matchStartOfPath(toPathContainer("/aaa/b/ccd/ef/x/g/i")); assertNotNull(pri); assertEquals("/i",pri.getPathRemaining().value()); assertEquals("b",pri.getUriVariables().get("bbb")); - pri = parse("/aaa/{bbb}/c?d/e*f/*/g/").getPathRemaining(toPathContainer("/aaa/b/ccd/ef/x/g/i")); + pri = parse("/aaa/{bbb}/c?d/e*f/*/g/").matchStartOfPath(toPathContainer("/aaa/b/ccd/ef/x/g/i")); assertNotNull(pri); assertEquals("i",pri.getPathRemaining().value()); assertEquals("b",pri.getUriVariables().get("bbb")); - pri = parse("/{aaa}_{bbb}/e*f/{x}/g").getPathRemaining(toPathContainer("/aa_bb/ef/x/g/i")); + pri = parse("/{aaa}_{bbb}/e*f/{x}/g").matchStartOfPath(toPathContainer("/aa_bb/ef/x/g/i")); assertNotNull(pri); assertEquals("/i",pri.getPathRemaining().value()); assertEquals("aa",pri.getUriVariables().get("aaa")); assertEquals("bb",pri.getUriVariables().get("bbb")); assertEquals("x",pri.getUriVariables().get("x")); - assertNull(parse("/a/b").getPathRemaining(toPathContainer(""))); - assertEquals("/a/b",parse("").getPathRemaining(toPathContainer("/a/b")).getPathRemaining().value()); - assertEquals("",parse("").getPathRemaining(toPathContainer("")).getPathRemaining().value()); + assertNull(parse("/a/b").matchStartOfPath(toPathContainer(""))); + assertEquals("/a/b",parse("").matchStartOfPath(toPathContainer("/a/b")).getPathRemaining().value()); + assertEquals("",parse("").matchStartOfPath(toPathContainer("")).getPathRemaining().value()); } @Test @@ -576,114 +576,6 @@ public class PathPatternTests { assertEquals("def",pri.getUriVariables().get("foo")); } - @Test - public void matchStart() { - PathPatternParser ppp = new PathPatternParser(); - ppp.setMatchOptionalTrailingSeparator(false); - PathPattern pp = ppp.parse("test"); - assertFalse(pp.matchStart(PathContainer.parsePath("test/"))); - - checkStartNoMatch("test/*/","test//"); - checkStartMatches("test/*","test/abc"); - checkStartMatches("test/*/def","test/abc/def"); - checkStartNoMatch("test/*/def","test//"); - checkStartNoMatch("test/*/def","test//def"); - - checkStartMatches("test/{a}_{b}/foo", "test/a_b"); - checkStartMatches("test/?/abc", "test/a"); - checkStartMatches("test/{*foobar}", "test/"); - checkStartMatches("test/*/bar", "test/a"); - checkStartMatches("test/{foo}/bar", "test/abc"); - checkStartMatches("test//foo", "test//"); - checkStartMatches("test/foo", "test/"); - checkStartMatches("test/*", "test/"); - checkStartMatches("test", "test"); - checkStartNoMatch("test", "tes"); - checkStartMatches("test/", "test"); - - // test exact matching - checkStartMatches("test", "test"); - checkStartMatches("/test", "/test"); - checkStartNoMatch("/test.jpg", "test.jpg"); - checkStartNoMatch("test", "/test"); - checkStartNoMatch("/test", "test"); - - // test matching with ?'s - checkStartMatches("t?st", "test"); - checkStartMatches("??st", "test"); - checkStartMatches("tes?", "test"); - checkStartMatches("te??", "test"); - checkStartMatches("?es?", "test"); - checkStartNoMatch("tes?", "tes"); - checkStartNoMatch("tes?", "testt"); - checkStartNoMatch("tes?", "tsst"); - - // test matching with *'s - checkStartMatches("*", "test"); - checkStartMatches("test*", "test"); - checkStartMatches("test*", "testTest"); - checkStartMatches("test/*", "test/Test"); - checkStartMatches("test/*", "test/t"); - checkStartMatches("test/*", "test/"); - checkStartMatches("*test*", "AnothertestTest"); - checkStartMatches("*test", "Anothertest"); - checkStartMatches("*.*", "test."); - checkStartMatches("*.*", "test.test"); - checkStartMatches("*.*", "test.test.test"); - checkStartMatches("test*aaa", "testblaaaa"); - checkStartNoMatch("test*", "tst"); - checkStartMatches("test*", "test/"); // trailing slash is optional - checkStartMatches("test*", "test"); - checkStartNoMatch("test*", "tsttest"); - checkStartNoMatch("test*", "test/t"); - checkStartMatches("test/*", "test"); - checkStartMatches("test/t*.txt", "test"); - checkStartNoMatch("*test*", "tsttst"); - checkStartNoMatch("*test", "tsttst"); - checkStartNoMatch("*.*", "tsttst"); - checkStartNoMatch("test*aaa", "test"); - checkStartNoMatch("test*aaa", "testblaaab"); - - // test matching with ?'s and /'s - checkStartMatches("/?", "/a"); - checkStartMatches("/?/a", "/a/a"); - checkStartMatches("/a/?", "/a/b"); - checkStartMatches("/??/a", "/aa/a"); - checkStartMatches("/a/??", "/a/bb"); - checkStartMatches("/?", "/a"); - - checkStartMatches("/**", "/testing/testing"); - checkStartMatches("/*/**", "/testing/testing"); - checkStartMatches("test*/**", "test/"); - checkStartMatches("test*/**", "test/t"); - checkStartMatches("/bla*bla/test", "/blaXXXbla/test"); - checkStartMatches("/*bla/test", "/XXXbla/test"); - checkStartNoMatch("/bla*bla/test", "/blaXXXbl/test"); - checkStartNoMatch("/*bla/test", "XXXblab/test"); - checkStartNoMatch("/*bla/test", "XXXbl/test"); - - checkStartNoMatch("/????", "/bala/bla"); - - checkStartMatches("/*bla*/*/bla/**", - "/XXXblaXXXX/testing/bla/testing/testing/"); - checkStartMatches("/*bla*/*/bla/*", - "/XXXblaXXXX/testing/bla/testing"); - checkStartMatches("/*bla*/*/bla/**", - "/XXXblaXXXX/testing/bla/testing/testing"); - checkStartMatches("/*bla*/*/bla/**", - "/XXXblaXXXX/testing/bla/testing/testing.jpg"); - - checkStartMatches("/abc/{foo}", "/abc/def"); - checkStartMatches("/abc/{foo}", "/abc/def/"); // trailing slash is optional - checkStartMatches("/abc/{foo}/", "/abc/def/"); - checkStartNoMatch("/abc/{foo}/", "/abc/def/ghi"); - checkStartMatches("/abc/{foo}/", "/abc/def"); - - checkStartMatches("", ""); - checkStartMatches("", null); - checkStartMatches("/abc", null); - } - @Test public void caseSensitivity() { PathPatternParser pp = new PathPatternParser(); @@ -1258,20 +1150,6 @@ public class PathPatternTests { assertTrue(p.matches(pc)); } - private void checkStartNoMatch(String uriTemplate, String path) { - PathPatternParser p = new PathPatternParser(); - p.setMatchOptionalTrailingSeparator(true); - PathPattern pattern = p.parse(uriTemplate); - assertFalse(pattern.matchStart(toPathContainer(path))); - } - - private void checkStartMatches(String uriTemplate, String path) { - PathPatternParser p = new PathPatternParser(); - p.setMatchOptionalTrailingSeparator(true); - PathPattern pattern = p.parse(uriTemplate); - assertTrue(pattern.matchStart(toPathContainer(path))); - } - private void checkNoMatch(String uriTemplate, String path) { PathPatternParser p = new PathPatternParser(); PathPattern pattern = p.parse(uriTemplate); @@ -1309,11 +1187,11 @@ public class PathPatternTests { } private PathRemainingMatchInfo getPathRemaining(String pattern, String path) { - return parse(pattern).getPathRemaining(toPathContainer(path)); + return parse(pattern).matchStartOfPath(toPathContainer(path)); } private PathRemainingMatchInfo getPathRemaining(PathPattern pattern, String path) { - return pattern.getPathRemaining(toPathContainer(path)); + return pattern.matchStartOfPath(toPathContainer(path)); } private PathPattern.PathMatchInfo matchAndExtract(PathPattern p, String path) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java index 28ba17ed701..dbf210ad242 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/RequestPredicates.java @@ -356,7 +356,7 @@ public abstract class RequestPredicates { @Override public Optional nest(ServerRequest request) { - return Optional.ofNullable(this.pattern.getPathRemaining(request.pathContainer())) + return Optional.ofNullable(this.pattern.matchStartOfPath(request.pathContainer())) .map(info -> { mergeTemplateVariables(request, info.getUriVariables()); return new SubPathServerRequestWrapper(request, info); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java index 369fef0e9a6..0452c1a8d6a 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java @@ -33,6 +33,7 @@ import org.springframework.http.InvalidMediaTypeException; import org.springframework.http.MediaType; import org.springframework.http.server.PathContainer; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; import org.springframework.web.method.HandlerMethod; import org.springframework.web.reactive.HandlerMapping; @@ -113,6 +114,8 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe else { bestPattern = patterns.iterator().next(); PathPattern.PathMatchInfo result = bestPattern.matchAndExtract(lookupPath); + Assert.notNull(result, () -> + "Expected bestPattern: " + bestPattern + " to match lookupPath " + lookupPath); uriVariables = result.getUriVariables(); matrixVariables = result.getMatrixVariables(); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java index bc244cc2ef1..e2c9899dcfc 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/config/WebFluxConfigurationSupportTests.java @@ -86,7 +86,7 @@ public class WebFluxConfigurationSupportTests { @Test public void requestMappingHandlerMapping() throws Exception { ApplicationContext context = loadConfig(WebFluxConfig.class); - final Field trailingSlashField = ReflectionUtils.findField(PathPatternParser.class, "matchOptionalTrailingSlash"); + final Field trailingSlashField = ReflectionUtils.findField(PathPatternParser.class, "matchOptionalTrailingSeparator"); ReflectionUtils.makeAccessible(trailingSlashField); String name = "requestMappingHandlerMapping"; @@ -111,7 +111,7 @@ public class WebFluxConfigurationSupportTests { @Test public void customPathMatchConfig() throws Exception { ApplicationContext context = loadConfig(CustomPatchMatchConfig.class); - final Field trailingSlashField = ReflectionUtils.findField(PathPatternParser.class, "matchOptionalTrailingSlash"); + final Field trailingSlashField = ReflectionUtils.findField(PathPatternParser.class, "matchOptionalTrailingSeparator"); ReflectionUtils.makeAccessible(trailingSlashField); String name = "requestMappingHandlerMapping";