diff --git a/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java b/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java index 6573af5215..fe8cd7d58e 100644 --- a/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java +++ b/spring-core/src/main/java/org/springframework/util/AntPathMatcher.java @@ -80,7 +80,7 @@ public class AntPathMatcher implements PathMatcher { private static final Pattern VARIABLE_PATTERN = Pattern.compile("\\{[^/]+?}"); - private static final char[] WILDCARD_CHARS = { '*', '?', '{' }; + private static final char[] WILDCARD_CHARS = {'*', '?', '{'}; private String pathSeparator; @@ -646,20 +646,15 @@ public class AntPathMatcher implements PathMatcher { private static final String DEFAULT_VARIABLE_PATTERN = "(.*)"; + private final String rawPattern; + + private final boolean caseSensitive; + + private final boolean exactMatch; + + @Nullable private final Pattern pattern; - private final String originalPattern; - - /** - * True if this matcher accepts any value (e.g. regex .*) - */ - private final boolean acceptAny; - - /** - * True if given pattern is a literal pattern, thus candidate string may be tested by strings comparison for better perf. - */ - private final boolean allowStringComparison; - private final List variableNames = new LinkedList<>(); public AntPathStringMatcher(String pattern) { @@ -667,6 +662,8 @@ public class AntPathMatcher implements PathMatcher { } public AntPathStringMatcher(String pattern, boolean caseSensitive) { + this.rawPattern = pattern; + this.caseSensitive = caseSensitive; StringBuilder patternBuilder = new StringBuilder(); Matcher matcher = GLOB_PATTERN.matcher(pattern); int end = 0; @@ -696,13 +693,17 @@ public class AntPathMatcher implements PathMatcher { } end = matcher.end(); } - patternBuilder.append(quote(pattern, end, pattern.length())); - this.pattern = (caseSensitive ? Pattern.compile(patternBuilder.toString()) : - Pattern.compile(patternBuilder.toString(), Pattern.CASE_INSENSITIVE)); - - this.originalPattern = pattern; - this.acceptAny = this.pattern.pattern().equals(DEFAULT_VARIABLE_PATTERN); - this.allowStringComparison = end == 0; + // No glob pattern was found, this is an exact String match + if (end == 0) { + this.exactMatch = true; + this.pattern = null; + } + else { + this.exactMatch = false; + patternBuilder.append(quote(pattern, end, pattern.length())); + this.pattern = (this.caseSensitive ? Pattern.compile(patternBuilder.toString()) : + Pattern.compile(patternBuilder.toString(), Pattern.CASE_INSENSITIVE)); + } } private String quote(String s, int start, int end) { @@ -717,43 +718,29 @@ public class AntPathMatcher implements PathMatcher { * @return {@code true} if the string matches against the pattern, or {@code false} otherwise. */ public boolean matchStrings(String str, @Nullable Map uriTemplateVariables) { - // Check whether current pattern accepts any value or given string is exact match - if (this.acceptAny) { - if (uriTemplateVariables != null) { - assertUrlVariablesCount(1); - uriTemplateVariables.put(this.variableNames.get(0), str); - } - return true; + if (this.exactMatch) { + return this.caseSensitive ? this.rawPattern.equals(str) : this.rawPattern.equalsIgnoreCase(str); } - else if (this.allowStringComparison && str.equals(this.originalPattern)) { - return true; - } - - Matcher matcher = this.pattern.matcher(str); - if (matcher.matches()) { - if (uriTemplateVariables != null) { - assertUrlVariablesCount(matcher.groupCount()); - for (int i = 1; i <= matcher.groupCount(); i++) { - String name = this.variableNames.get(i - 1); - String value = matcher.group(i); - uriTemplateVariables.put(name, value); + else if (this.pattern != null) { + Matcher matcher = this.pattern.matcher(str); + if (matcher.matches()) { + if (uriTemplateVariables != null) { + if (this.variableNames.size() != matcher.groupCount()) { + throw new IllegalArgumentException("The number of capturing groups in the pattern segment " + + this.pattern + " does not match the number of URI template variables it defines, " + + "which can occur if capturing groups are used in a URI template regex. " + + "Use non-capturing groups instead."); + } + for (int i = 1; i <= matcher.groupCount(); i++) { + String name = this.variableNames.get(i - 1); + String value = matcher.group(i); + uriTemplateVariables.put(name, value); + } } + return true; } - return true; - } - else { - return false; - } - } - - // SPR-8455 - private void assertUrlVariablesCount(int expected) { - if (this.variableNames.size() != expected) { - throw new IllegalArgumentException("The number of capturing groups in the pattern segment " + - this.pattern + " does not match the number of URI template variables it defines, " + - "which can occur if capturing groups are used in a URI template regex. " + - "Use non-capturing groups instead."); } + return false; } } diff --git a/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java b/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java index 3d81cd0fa4..a67ebf3796 100644 --- a/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java +++ b/spring-core/src/test/java/org/springframework/util/AntPathMatcherTests.java @@ -130,9 +130,6 @@ class AntPathMatcherTests { assertThat(pathMatcher.match("", "")).isTrue(); assertThat(pathMatcher.match("/{bla}.*", "/testing.html")).isTrue(); - - // Test that sending the same pattern will not match (gh #24887) - assertThat(pathMatcher.match("/bla/{foo:[0-9]}", "/bla/{foo:[0-9]}")).isFalse(); } @Test diff --git a/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java b/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java new file mode 100644 index 0000000000..499f0a5513 --- /dev/null +++ b/spring-web/src/jmh/java/org/springframework/web/util/pattern/PathMatchingBenchmark.java @@ -0,0 +1,278 @@ +/* + * Copyright 2002-2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.util.pattern; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.infra.Blackhole; + +import org.springframework.http.server.PathContainer; +import org.springframework.util.AntPathMatcher; + +/** + * Benchmarks for matching requests paths against path patterns in a web context. + * We're considering here the {@link org.springframework.util.AntPathMatcher} and + * {@link PathPatternParser} implementations with typical sets of patterns. + * @author Brian Clozel + */ +@BenchmarkMode(Mode.Throughput) +public class PathMatchingBenchmark { + + @State(Scope.Benchmark) + public static class AllRoutesPatternParser extends PatternParserData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.allRoutes()); + } + } + + @Benchmark + public void matchAllRoutesWithPathPatternParser(AllRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + for (PathPattern pattern : data.patterns) { + bh.consume(pattern.matches(path)); + } + } + } + + @Benchmark + public void matchAndSortAllRoutesWithPathPatternParser(AllRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + List matches = new ArrayList<>(); + for (PathPattern pattern : data.patterns) { + if (pattern.matches(path)) { + matches.add(pattern); + } + } + Collections.sort(matches); + bh.consume(matches); + } + } + + @State(Scope.Benchmark) + public static class StaticRoutesPatternParser extends PatternParserData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.staticRoutes()); + } + } + + @Benchmark + public void matchStaticRoutesWithPathPatternParser(StaticRoutesPatternParser data, Blackhole bh) { + for (PathContainer path : data.requestPaths) { + for (PathPattern pattern : data.patterns) { + bh.consume(pattern.matches(path)); + } + } + } + + @State(Scope.Benchmark) + public static class AllRoutesAntPathMatcher extends AntPathMatcherData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.allRoutes()); + } + } + + @Benchmark + public void matchAllRoutesWithAntPathMatcher(AllRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + for (String pattern : data.patterns) { + bh.consume(data.matcher.match(pattern, path)); + } + } + } + + @Benchmark + public void matchAndSortAllRoutesWithAntPathMatcher(AllRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + List matches = new ArrayList<>(); + for (String pattern : data.patterns) { + if (data.matcher.match(pattern, path)) { + matches.add(pattern); + } + } + matches.sort(data.matcher.getPatternComparator(path)); + bh.consume(matches); + } + } + + @State(Scope.Benchmark) + public static class StaticRoutesAntPathMatcher extends AntPathMatcherData { + + @Setup(Level.Trial) + public void registerPatterns() { + parseRoutes(RouteGenerator.staticRoutes()); + } + } + + @Benchmark + public void matchStaticRoutesWithAntPathMatcher(StaticRoutesAntPathMatcher data, Blackhole bh) { + for (String path : data.requestPaths) { + for (String pattern : data.patterns) { + bh.consume(data.matcher.match(pattern, path)); + } + } + } + + + static class PatternParserData { + + List patterns = new ArrayList<>(); + + List requestPaths = new ArrayList<>(); + + void parseRoutes(List routes) { + PathPatternParser parser = new PathPatternParser(); + routes.forEach(route -> { + this.patterns.add(parser.parse(route.pattern)); + route.matchingPaths.forEach(path -> this.requestPaths.add(PathContainer.parsePath(path))); + }); + } + + } + + static class AntPathMatcherData { + + AntPathMatcher matcher = new AntPathMatcher(); + + List patterns = new ArrayList<>(); + + List requestPaths = new ArrayList<>(); + + void parseRoutes(List routes) { + routes.forEach(route -> { + this.patterns.add(route.pattern); + this.requestPaths.addAll(route.matchingPaths); + }); + } + + } + + /** + * Route in the web application. + * Each route has a path pattern and can generate sets of matching request paths for that pattern. + */ + static class Route { + + private final String pattern; + + private final List matchingPaths; + + public Route(String pattern, String... matchingPaths) { + this.pattern = pattern; + if (matchingPaths.length > 0) { + this.matchingPaths = Arrays.asList(matchingPaths); + } + else { + this.matchingPaths = Collections.singletonList(pattern); + } + } + + public String pattern() { + return this.pattern; + } + + public Iterable matchingPaths() { + return this.matchingPaths; + } + } + + static class RouteGenerator { + + static List staticRoutes() { + return Arrays.asList( + new Route("/"), + new Route("/why-spring"), + new Route("/microservices"), + new Route("/reactive"), + new Route("/event-driven"), + new Route("/cloud"), + new Route("/web-applications"), + new Route("/serverless"), + new Route("/batch"), + new Route("/community/overview"), + new Route("/community/team"), + new Route("/community/events"), + new Route("/community/support"), + new Route("/some/other/section"), + new Route("/blog.atom") + ); + } + + static List captureRoutes() { + return Arrays.asList( + new Route("/guides"), + new Route("/guides/gs/{repositoryName}", + "/guides/gs/rest-service", "/guides/gs/scheduling-tasks", + "/guides/gs/consuming-rest", "/guides/gs/relational-data-access"), + new Route("/projects"), + new Route("/projects/{name}", + "/projects/spring-boot", "/projects/spring-framework", + "/projects/spring-data", "/projects/spring-security", "/projects/spring-cloud"), + new Route("/blog/category/{category}.atom", + "/blog/category/releases.atom", "/blog/category/engineering.atom", + "/blog/category/news.atom"), + new Route("/tools/{name}", "/tools/eclipse", "/tools/vscode"), + new Route("/team/{username}", + "/team/jhoeller", "/team/bclozel", "/team/snicoll", "/team/sdeleuze", "/team/rstoyanchev"), + new Route("/api/projects/{projectId}", + "/api/projects/spring-boot", "/api/projects/spring-framework", + "/api/projects/reactor", "/api/projects/spring-data", + "/api/projects/spring-restdocs", "/api/projects/spring-batch"), + new Route("/api/projects/{projectId}/releases/{version}", + "/api/projects/spring-boot/releases/2.3.0", "/api/projects/spring-framework/releases/5.3.0", + "/api/projects/spring-boot/releases/2.2.0", "/api/projects/spring-framework/releases/5.2.0") + ); + } + + static List regexRoute() { + return Arrays.asList( + new Route("/blog/{year:\\\\d+}/{month:\\\\d+}/{day:\\\\d+}/{slug}", + "/blog/2020/01/01/spring-boot-released", "/blog/2020/02/10/this-week-in-spring", + "/blog/2020/03/12/spring-one-conference-2020", "/blog/2020/05/17/spring-io-barcelona-2020", + "/blog/2020/05/17/spring-io-barcelona-2020", "/blog/2020/06/06/spring-cloud-release"), + new Route("/user/{name:[a-z]+}", + "/user/emily", "/user/example", "/user/spring") + ); + } + + static List allRoutes() { + List routes = new ArrayList<>(); + routes.addAll(staticRoutes()); + routes.addAll(captureRoutes()); + routes.addAll(regexRoute()); + routes.add(new Route("/static/**", "/static/image.png", "/static/style.css")); + routes.add(new Route("/**", "/notfound", "/favicon.ico")); + return routes; + } + + } +} diff --git a/src/checkstyle/checkstyle-suppressions.xml b/src/checkstyle/checkstyle-suppressions.xml index bb91954fce..1d7e1aa0cb 100644 --- a/src/checkstyle/checkstyle-suppressions.xml +++ b/src/checkstyle/checkstyle-suppressions.xml @@ -8,7 +8,7 @@ - +