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.

spring-web/src/main/java/org/springframework/web/util/pattern/PathPatternComparator.java

@@ -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.
- *
- * <p>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<PathPattern> {
-
-	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;
-	}
-
-}

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<T> {
 	 * @param lookupPath the URL lookup path to be matched against
 	 */
 	public Optional<PathMatchResult<T>> 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()));
 	}
 

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<PatternsRequestCondition> {
 
-	private final List<PathPattern> patterns;
+	private final Set<PathPattern> 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<Pat
 	 * @param patternParser for parsing string patterns
 	 */
 	public PatternsRequestCondition(String[] patterns, PathPatternParser patternParser) {
-
-		this(asList(patterns), patternParser);
+		this(Arrays.asList(patterns), patternParser);
 	}
 
 	/**
 	 * Private constructor accepting a collection of raw patterns.
 	 */
-	private PatternsRequestCondition(Collection<String> 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<String> 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<PathPattern> toSortedSet(Stream<PathPattern> stream) {
+		Set<PathPattern> result = stream.sorted().collect(Collectors.toCollection(TreeSet::new));
+		return Collections.unmodifiableSet(result);
 	}
 
 	/**
 	 * Private constructor accepting a list of path patterns.
 	 */
 	private PatternsRequestCondition(List<PathPattern> patterns, PathPatternParser patternParser) {
-		this.patterns = patterns;
+		this.patterns = toSortedSet(patterns.stream());
 		this.parser = patternParser;
 	}
 
-
-	private static List<String> asList(String... patterns) {
-		return (patterns != null ? Arrays.asList(patterns) : Collections.emptyList());
-	}
-
 	public Set<PathPattern> getPatterns() {
-		return new TreeSet<>(this.patterns);
-	}
-
-	public Set<String> 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<Pat
 		else {
 			combined.add(this.parser.parse(""));
 		}
-
 		return new PatternsRequestCondition(combined, this.parser);
 	}
 
 	/**
 	 * Checks if any of the patterns match the given request and returns an instance
-	 * that is guaranteed to contain matching patterns, sorted with a
-	 * {@link PathPatternComparator}.
+	 * that is guaranteed to contain matching patterns, sorted.
 	 * @param exchange the current exchange
 	 * @return the same instance if the condition contains no patterns;
 	 * or a new condition with sorted matching patterns;
@@ -162,11 +157,9 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 		if (this.patterns.isEmpty()) {
 			return this;
 		}
-
-		String lookupPath = exchange.getRequest().getPath().pathWithinApplication().value();
-		SortedSet<PathPattern> matches = getMatchingPatterns(lookupPath);
-		return matches.isEmpty() ? null : new PatternsRequestCondition(
-				new ArrayList<PathPattern>(matches), this.parser);
+		SortedSet<PathPattern> matches = getMatchingPatterns(exchange);
+		return matches.isEmpty() ? null :
+				new PatternsRequestCondition(new ArrayList<PathPattern>(matches), this.parser);
 	}
 
 	/**
@@ -175,20 +168,19 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 	 * {@link #getMatchingCondition(ServerWebExchange)}.
 	 * This method is provided as an alternative to be used if no request is available
 	 * (e.g. introspection, tooling, etc).
-	 * @param lookupPath the lookup path to match to existing patterns
+	 * @param exchange the current exchange
 	 * @return a sorted set of matching patterns sorted with the closest match first
 	 */
-	public SortedSet<PathPattern> getMatchingPatterns(String lookupPath) {
+	private SortedSet<PathPattern> 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.
 	 * <p>It is assumed that both instances have been obtained via
@@ -198,14 +190,10 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 	 */
 	@Override
 	public int compareTo(PatternsRequestCondition other, ServerWebExchange exchange) {
-		String lookupPath = exchange.getRequest().getPath().pathWithinApplication().value();
-		PathPatternComparator comparator = new PathPatternComparator(lookupPath);
-		Iterator<PathPattern> iterator = this.patterns.stream()
-				.sorted(comparator).collect(Collectors.toList()).iterator();
-		Iterator<PathPattern> iteratorOther = other.getPatterns().stream()
-				.sorted(comparator).collect(Collectors.toList()).iterator();
+		Iterator<PathPattern> iterator = this.patterns.iterator();
+		Iterator<PathPattern> 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;
 			}

spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java

@@ -292,7 +292,7 @@ public abstract class AbstractHandlerMethodMapping<T> 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<T> extends AbstractHandlerMap
 	@Nullable
 	protected abstract T getMappingForMethod(Method method, Class<?> handlerType);
 
-	/**
-	 * Extract and return the URL paths contained in a mapping.
-	 */
-	protected abstract Set<String> getMappingPathPatterns(T mapping);
-
 	/**
 	 * Check if a mapping matches the current request and return a (potentially
 	 * new) mapping with conditions relevant to the current request.

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<String> 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

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<String> 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();

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<String> 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();

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<String> paths = info.getPatternsCondition().getPatternStrings();
+		Set<PathPattern> paths = info.getPatternsCondition().getPatterns();
 		assertEquals(1, paths.size());
-		assertEquals(path, paths.iterator().next());
+		assertEquals(path, paths.iterator().next().getPatternString());
 
 		Set<RequestMethod> methods = info.getMethodsCondition().getMethods();
 		assertEquals(1, methods.size());