From a5e54788ccedbe6eef7e7d87586364fdfd67c601 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 30 Jun 2017 18:14:32 -0400 Subject: [PATCH] Remove PathPatternComparator Direct comparison of a pattern (as a String) to the path does not make much sense now that we deal with URL encoding through PathContainer which exposes (safely) decoded path segments. Removing the PathPatternComparator also means we can keep patterns pre-sorted instead of sorting them all the time. That probably offsets any benefits from comparing to the lookup path for direct matches and patterns are still sorted according to specificity. --- .../util/pattern/PathPatternComparator.java | 72 ---------------- .../reactive/handler/PathPatternRegistry.java | 5 +- .../condition/PatternsRequestCondition.java | 82 ++++++++----------- .../method/AbstractHandlerMethodMapping.java | 7 +- .../RequestMappingInfoHandlerMapping.java | 8 -- .../method/HandlerMethodMappingTests.java | 12 +-- ...RequestMappingInfoHandlerMappingTests.java | 10 --- .../RequestMappingHandlerMappingTests.java | 5 +- 8 files changed, 45 insertions(+), 156 deletions(-) delete mode 100644 spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternComparator.java diff --git a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternComparator.java b/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternComparator.java deleted file mode 100644 index b589bd0163..0000000000 --- a/spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternComparator.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright 2002-2017 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 - * - * http://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.Comparator; - -import org.springframework.lang.Nullable; - -/** - * {@link PathPattern} comparator that takes account of a specified - * path and sorts anything that exactly matches it to be first. - * - *

Patterns that have the same specificity are then compared - * using their String representation, in order to avoid - * considering them as "duplicates" when sorting them - * in {@link java.util.Set} collections. - * - * @author Brian Clozel - * @since 5.0 - */ -public class PathPatternComparator implements Comparator { - - private final String path; - - - public PathPatternComparator(String path) { - this.path = path; - } - - - @Override - public int compare(@Nullable PathPattern o1, @Nullable PathPattern o2) { - // Nulls get sorted to the end - if (o1 == null) { - return (o2 == null ? 0 : +1); - } - else if (o2 == null) { - return -1; - } - - // exact matches get sorted first - if (o1.getPatternString().equals(path)) { - return (o2.getPatternString().equals(path)) ? 0 : -1; - } - else if (o2.getPatternString().equals(path)) { - return +1; - } - - // compare pattern specificity - int result = o1.compareTo(o2); - // if equal specificity, sort using pattern string - if (result == 0) { - return o1.getPatternString().compareTo(o2.getPatternString()); - } - return result; - } - -} diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathPatternRegistry.java b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathPatternRegistry.java index 99be68ea19..7cbfd5f197 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathPatternRegistry.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathPatternRegistry.java @@ -28,7 +28,6 @@ import java.util.stream.Collectors; import org.springframework.lang.Nullable; import org.springframework.util.StringUtils; import org.springframework.web.util.pattern.PathPattern; -import org.springframework.web.util.pattern.PathPatternComparator; import org.springframework.web.util.pattern.PathPatternParser; /** @@ -101,10 +100,10 @@ public class PathPatternRegistry { * @param lookupPath the URL lookup path to be matched against */ public Optional> findFirstMatch(String lookupPath) { - PathPatternComparator comparator = new PathPatternComparator(lookupPath); return this.patternsMap.entrySet().stream() .filter(entry -> entry.getKey().matches(lookupPath)) - .reduce((e1, e2) -> comparator.compare(e1.getKey(), e2.getKey()) < 0 ? e1 : e2) + .sorted(Comparator.comparing(Map.Entry::getKey)) + .findFirst() .map(entry -> new PathMatchResult<>(entry.getKey(), entry.getValue())); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/PatternsRequestCondition.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/PatternsRequestCondition.java index aa0a87208d..2e630dca99 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/PatternsRequestCondition.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/PatternsRequestCondition.java @@ -26,12 +26,13 @@ import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.springframework.http.server.reactive.PathContainer; import org.springframework.lang.Nullable; import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.pattern.PathPattern; -import org.springframework.web.util.pattern.PathPatternComparator; import org.springframework.web.util.pattern.PathPatternParser; /** @@ -44,17 +45,18 @@ import org.springframework.web.util.pattern.PathPatternParser; */ public final class PatternsRequestCondition extends AbstractRequestCondition { - private final List patterns; + private final Set patterns; private final PathPatternParser parser; + /** * Creates a new instance with the given URL patterns. * Each pattern is prepended with "/" if not already. * @param patterns 0 or more URL patterns; if 0 the condition will match to every request. */ public PatternsRequestCondition(String... patterns) { - this(asList(patterns), null); + this(patterns, null); } /** @@ -64,44 +66,39 @@ public final class PatternsRequestCondition extends AbstractRequestCondition patterns, PathPatternParser patternParser) { - this.parser = patternParser != null ? patternParser : new PathPatternParser(); - this.patterns = new ArrayList<>(); - patterns.forEach(pattern -> { - if (StringUtils.hasText(pattern) && !pattern.startsWith("/")) { - pattern = "/" + pattern; - } - this.patterns.add(this.parser.parse(pattern)); - }); + private PatternsRequestCondition(Collection patterns, PathPatternParser parser) { + this.parser = (parser != null ? parser : new PathPatternParser()); + this.patterns = toSortedSet(patterns.stream().map(pattern -> parse(pattern, this.parser))); + } + + private static PathPattern parse(String pattern, PathPatternParser parser) { + if (StringUtils.hasText(pattern) && !pattern.startsWith("/")) { + pattern = "/" + pattern; + } + return parser.parse(pattern); + } + + private static Set toSortedSet(Stream stream) { + Set result = stream.sorted().collect(Collectors.toCollection(TreeSet::new)); + return Collections.unmodifiableSet(result); } /** * Private constructor accepting a list of path patterns. */ private PatternsRequestCondition(List patterns, PathPatternParser patternParser) { - this.patterns = patterns; + this.patterns = toSortedSet(patterns.stream()); this.parser = patternParser; } - - private static List asList(String... patterns) { - return (patterns != null ? Arrays.asList(patterns) : Collections.emptyList()); - } - public Set getPatterns() { - return new TreeSet<>(this.patterns); - } - - public Set getPatternStrings() { - return this.patterns.stream() - .map(PathPattern::toString).collect(Collectors.toSet()); + return this.patterns; } @Override @@ -143,14 +140,12 @@ public final class PatternsRequestCondition extends AbstractRequestCondition matches = getMatchingPatterns(lookupPath); - return matches.isEmpty() ? null : new PatternsRequestCondition( - new ArrayList(matches), this.parser); + SortedSet matches = getMatchingPatterns(exchange); + return matches.isEmpty() ? null : + new PatternsRequestCondition(new ArrayList(matches), this.parser); } /** @@ -175,20 +168,19 @@ public final class PatternsRequestCondition extends AbstractRequestCondition getMatchingPatterns(String lookupPath) { + private SortedSet getMatchingPatterns(ServerWebExchange exchange) { + PathContainer lookupPath = exchange.getRequest().getPath().pathWithinApplication(); return patterns.stream() .filter(pattern -> pattern.matches(lookupPath)) - .collect(Collectors.toCollection(() -> - new TreeSet<>(new PathPatternComparator(lookupPath)))); + .collect(Collectors.toCollection(TreeSet::new)); } /** * Compare the two conditions based on the URL patterns they contain. - * Patterns are compared one at a time, from top to bottom via - * {@link PathPatternComparator}. If all compared + * Patterns are compared one at a time, from top to bottom. If all compared * patterns match equally, but one instance has more patterns, it is * considered a closer match. *

It is assumed that both instances have been obtained via @@ -198,14 +190,10 @@ public final class PatternsRequestCondition extends AbstractRequestCondition iterator = this.patterns.stream() - .sorted(comparator).collect(Collectors.toList()).iterator(); - Iterator iteratorOther = other.getPatterns().stream() - .sorted(comparator).collect(Collectors.toList()).iterator(); + Iterator iterator = this.patterns.iterator(); + Iterator iteratorOther = other.getPatterns().iterator(); while (iterator.hasNext() && iteratorOther.hasNext()) { - int result = comparator.compare(iterator.next(), iteratorOther.next()); + int result = iterator.next().compareTo(iteratorOther.next()); if (result != 0) { return result; } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java index b075544191..aeccd29802 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java @@ -292,7 +292,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap * @param lookupPath the lookup path within the current mapping * @param exchange the current exchange * @return the best-matching handler method, or {@code null} if no match - * @see #handleMatch(Object, String, ServerWebExchange) + * @see #handleMatch(Object, HandlerMethod, String, ServerWebExchange) * @see #handleNoMatch(Set, String, ServerWebExchange) */ @Nullable @@ -400,11 +400,6 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap @Nullable protected abstract T getMappingForMethod(Method method, Class handlerType); - /** - * Extract and return the URL paths contained in a mapping. - */ - protected abstract Set getMappingPathPatterns(T mapping); - /** * Check if a mapping matches the current request and return a (potentially * new) mapping with conditions relevant to the current request. 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 32b962d77f..00b3d7d7ae 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 @@ -66,14 +66,6 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe } - /** - * Get the URL path patterns associated with this {@link RequestMappingInfo}. - */ - @Override - protected Set getMappingPathPatterns(RequestMappingInfo info) { - return info.getPatternsCondition().getPatternStrings(); - } - /** * Check if the given RequestMappingInfo matches the current request and * return a (potentially new) instance with conditions that match the diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java index 53df01de07..ef429c90e1 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java @@ -17,9 +17,7 @@ package org.springframework.web.reactive.result.method; import java.lang.reflect.Method; -import java.util.Collections; import java.util.Comparator; -import java.util.Set; import org.hamcrest.Matchers; import org.junit.Before; @@ -35,7 +33,10 @@ import org.springframework.web.method.HandlerMethod; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.pattern.ParsingPathMatcher; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; /** * Unit tests for {@link AbstractHandlerMethodMapping}. @@ -149,11 +150,6 @@ public class HandlerMethodMappingTests { return methodName.startsWith("handler") ? methodName : null; } - @Override - protected Set getMappingPathPatterns(String key) { - return (this.pathMatcher.isPattern(key) ? Collections.emptySet() : Collections.singleton(key)); - } - @Override protected String getMatchingMapping(String pattern, ServerWebExchange exchange) { String lookupPath = exchange.getRequest().getURI().getPath(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java index a8c7ea8280..4a81a1cf78 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java @@ -21,7 +21,6 @@ import java.net.URI; import java.util.Arrays; import java.util.Collections; import java.util.EnumSet; -import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.function.Consumer; @@ -94,15 +93,6 @@ public class RequestMappingInfoHandlerMappingTests { } - @Test - public void getMappingPathPatterns() throws Exception { - String[] patterns = {"/foo/*", "/foo", "/bar/*", "/bar"}; - RequestMappingInfo info = paths(patterns).build(); - Set actual = this.handlerMapping.getMappingPathPatterns(info); - - assertEquals(new HashSet<>(Arrays.asList(patterns)), actual); - } - @Test public void getHandlerDirectMatch() throws Exception { Method expected = on(TestController.class).annot(getMapping("/foo").params()).resolveMethod(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java index e0dd6e6b62..eb1c6ca7d8 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java @@ -39,6 +39,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.context.support.StaticWebApplicationContext; import org.springframework.web.reactive.result.method.RequestMappingInfo; +import org.springframework.web.util.pattern.PathPattern; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; @@ -133,9 +134,9 @@ public class RequestMappingHandlerMappingTests { assertNotNull(info); - Set paths = info.getPatternsCondition().getPatternStrings(); + Set paths = info.getPatternsCondition().getPatterns(); assertEquals(1, paths.size()); - assertEquals(path, paths.iterator().next()); + assertEquals(path, paths.iterator().next().getPatternString()); Set methods = info.getMethodsCondition().getMethods(); assertEquals(1, methods.size());