Remove PathMatchResultComparator

Follow-up change to the previous commit.

spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java

@@ -18,7 +18,6 @@ package org.springframework.web.reactive.handler;
 
 import java.util.Collections;
 import java.util.Map;
-import java.util.Optional;
 
 import reactor.core.publisher.Mono;
 
@@ -113,12 +112,11 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping {
 	@Nullable
 	protected Object lookupHandler(String lookupPath, ServerWebExchange exchange) throws Exception {
 		if (this.patternRegistry != null) {
-			Optional<PathMatchResult<Object>> matches = this.patternRegistry.findFirstMatch(lookupPath);
-			if (matches.isPresent()) {
+			PathMatchResult<Object> bestMatch = this.patternRegistry.findFirstMatch(lookupPath);
+			if (bestMatch != null) {
 				if (logger.isDebugEnabled()) {
-					logger.debug("Matching patterns for request [" + lookupPath + "] are " + matches);
+					logger.debug("Matching patterns for request [" + lookupPath + "] are " + bestMatch);
 				}
-				PathMatchResult<Object> bestMatch = matches.get();
 				String pathWithinMapping = bestMatch.getPattern().extractPathWithinPattern(lookupPath);
 				Object handler = bestMatch.getHandler();
 				return handleMatch(handler, bestMatch.getPattern(), pathWithinMapping, exchange);

spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathMatchResult.java

@@ -16,20 +16,20 @@
 
 package org.springframework.web.reactive.handler;
 
+import org.jetbrains.annotations.NotNull;
+
 import org.springframework.util.Assert;
 import org.springframework.web.util.pattern.PathPattern;
 
 /**
- * Result of matching an request lookup path against {@link PathPattern} instances.
- *
- * <p>Each result optionally associates the matching {@link PathPattern}
- * with a request handler of type {@code T}.
+ * Result of path match performed through a {@link PathPatternRegistry}.
+ * Each result contains the matched pattern and handler of type {@code T}.
  *
  * @author Brian Clozel
  * @since 5.0
  * @see PathPatternRegistry
  */
-public class PathMatchResult<T> {
+public class PathMatchResult<T> implements Comparable<PathMatchResult<T>> {
 
 	private final PathPattern pattern;
 
@@ -58,4 +58,20 @@ public class PathMatchResult<T> {
 		return this.handler;
 	}
 
+
+	@Override
+	public int compareTo(@NotNull PathMatchResult<T> other) {
+		int index = this.pattern.compareTo(other.pattern);
+		return (index != 0 ? index : this.getPatternString().compareTo(other.getPatternString()));
+	}
+
+	private String getPatternString() {
+		return this.pattern.getPatternString();
+	}
+
+	@Override
+	public String toString() {
+		return "PathMatchResult{pattern=" + this.pattern + ", handler=" + this.handler + "]";
+	}
+
 }

spring-webflux/src/main/java/org/springframework/web/reactive/handler/PathPatternRegistry.java

@@ -20,7 +20,6 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Optional;
 import java.util.SortedSet;
 import java.util.TreeSet;
 import java.util.stream.Collectors;
@@ -81,38 +80,6 @@ public class PathPatternRegistry<T> {
 		return Collections.unmodifiableMap(this.patternsMap);
 	}
 
-	/**
-	 * Return a {@code SortedSet} of {@code PathPattern}s matching the given {@code lookupPath}.
-	 * <p>The returned set sorted with the most specific
-	 * patterns first, according to the given {@code lookupPath}.
-	 * @param lookupPath the URL lookup path to be matched against
-	 */
-	public SortedSet<PathMatchResult<T>> findMatches(String lookupPath) {
-		return this.patternsMap.entrySet().stream()
-				.filter(entry -> entry.getKey().matches(lookupPath))
-				.map(entry -> new PathMatchResult<>(entry.getKey(), entry.getValue()))
-				.collect(Collectors.toCollection(() ->
-						new TreeSet<>(new PathMatchResultComparator<T>(lookupPath))));
-	}
-
-	/**
-	 * Return, if any, the most specific {@code PathPattern} matching the given {@code lookupPath}.
-	 * @param lookupPath the URL lookup path to be matched against
-	 */
-	public Optional<PathMatchResult<T>> findFirstMatch(String lookupPath) {
-		return this.patternsMap.entrySet().stream()
-				.filter(entry -> entry.getKey().matches(lookupPath))
-				.sorted(Comparator.comparing(Map.Entry::getKey))
-				.findFirst()
-				.map(entry -> new PathMatchResult<>(entry.getKey(), entry.getValue()));
-	}
-
-	/**
-	 * Remove all {@link PathPattern}s from this registry
-	 */
-	public void clear() {
-		this.patternsMap.clear();
-	}
 
 	/**
 	 * Parse the given {@code rawPattern} and adds it to this registry.
@@ -125,7 +92,7 @@ public class PathPatternRegistry<T> {
 		this.patternsMap.put(newPattern, handler);
 	}
 
-	private String prependLeadingSlash(String pattern) {
+	private static String prependLeadingSlash(String pattern) {
 		if (StringUtils.hasLength(pattern) && !pattern.startsWith("/")) {
 			return "/" + pattern;
 		}
@@ -135,40 +102,39 @@ public class PathPatternRegistry<T> {
 	}
 
 
-	private class PathMatchResultComparator<T> implements Comparator<PathMatchResult<T>> {
+	/**
+	 * Return patterns matching the given {@code lookupPath}.
+	 * <p>The returned set sorted with the most specific
+	 * patterns first, according to the given {@code lookupPath}.
+	 * @param lookupPath the URL lookup path to be matched against
+	 */
+	public SortedSet<PathMatchResult<T>> findMatches(String lookupPath) {
+		return this.patternsMap.entrySet().stream()
+				.filter(entry -> entry.getKey().matches(lookupPath))
+				.sorted(Comparator.comparing(Map.Entry::getKey))
+				.map(entry -> new PathMatchResult<>(entry.getKey(), entry.getValue()))
+				.collect(Collectors.toCollection(TreeSet::new));
+	}
 
-		private final String path;
+	/**
+	 * Return, if any, the most specific {@code PathPattern} matching the given {@code lookupPath}.
+	 * @param lookupPath the URL lookup path to be matched against
+	 */
+	@Nullable
+	public PathMatchResult<T> findFirstMatch(String lookupPath) {
+		return this.patternsMap.entrySet().stream()
+				.filter(entry -> entry.getKey().matches(lookupPath))
+				.sorted(Comparator.comparing(Map.Entry::getKey))
+				.findFirst()
+				.map(entry -> new PathMatchResult<>(entry.getKey(), entry.getValue()))
+				.orElse(null);
+	}
 
-		public PathMatchResultComparator(String path) {
-			this.path = path;
-		}
-
-		@Override
-		public int compare(@Nullable PathMatchResult<T> o1, @Nullable PathMatchResult<T> o2) {
-			// Nulls get sorted to the end
-			if (o1 == null) {
-				return (o2 == null ? 0 : +1);
-			}
-			else if (o2 == null) {
-				return -1;
-			}
-			PathPattern p1 = o1.getPattern();
-			PathPattern p2 = o2.getPattern();
-			// exact matches get sorted first
-			if (p1.getPatternString().equals(path)) {
-				return (p2.getPatternString().equals(path)) ? 0 : -1;
-			}
-			else if (p2.getPatternString().equals(path)) {
-				return +1;
-			}
-			// compare pattern specificity
-			int result = p1.compareTo(p2);
-			// if equal specificity, sort using pattern string
-			if (result == 0) {
-				return p1.getPatternString().compareTo(p2.getPatternString());
-			}
-			return result;
-		}
+	/**
+	 * Remove all {@link PathPattern}s from this registry
+	 */
+	public void clear() {
+		this.patternsMap.clear();
 	}
 
 }

spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/PatternsRequestCondition.java

@@ -20,6 +20,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
@@ -85,8 +86,15 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 	}
 
 	private static Set<PathPattern> toSortedSet(Stream<PathPattern> stream) {
-		Set<PathPattern> result = stream.sorted().collect(Collectors.toCollection(TreeSet::new));
-		return Collections.unmodifiableSet(result);
+		return Collections.unmodifiableSet(stream.sorted()
+				.collect(Collectors.toCollection(() -> new TreeSet<>(getPatternComparator()))));
+	}
+
+	private static Comparator<PathPattern> getPatternComparator() {
+		return (p1, p2) -> {
+			int index = p1.compareTo(p2);
+			return (index != 0 ? index : p1.getPatternString().compareTo(p2.getPatternString()));
+		};
 	}
 
 	/**

spring-webflux/src/test/java/org/springframework/web/reactive/handler/PathPatternRegistryTests.java

@@ -24,7 +24,6 @@ import java.util.stream.Collectors;
 import org.hamcrest.BaseMatcher;
 import org.hamcrest.Description;
 import org.hamcrest.Matchers;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -45,17 +44,13 @@ import static org.junit.Assert.assertThat;
  */
 public class PathPatternRegistryTests {
 
-	private PathPatternRegistry<Object> registry;
+	private final PathPatternRegistry<Object> registry = new PathPatternRegistry();
 
 	private final PathPatternParser parser = new PathPatternParser();
 
 	@Rule
 	public ExpectedException thrown = ExpectedException.none();
 
-	@Before
-	public void setUp() throws Exception {
-		this.registry = new PathPatternRegistry();
-	}
 
 	@Test
 	public void shouldPrependPatternsWithSlash() {
@@ -79,7 +74,7 @@ public class PathPatternRegistryTests {
 		this.registry.register("/fo?", new Object());
 		this.registry.register("/f?o", new Object());
 		Set<PathMatchResult<Object>> matches = this.registry.findMatches("/foo");
-		assertThat(getPatternList(matches), contains(pattern("/f?o"), pattern("/fo?")));
+		assertThat(toPatterns(matches), contains(pattern("/f?o"), pattern("/fo?")));
 	}
 
 	@Test
@@ -94,15 +89,13 @@ public class PathPatternRegistryTests {
 		this.registry.register("/foo/bar/baz", new Object());
 		this.registry.register("/foo/bar/{baz}", new Object());
 		Set<PathMatchResult<Object>> matches = this.registry.findMatches("/foo/bar/baz");
-		assertThat(getPatternList(matches), contains(pattern("/foo/bar/baz"), pattern("/foo/bar/{baz}"),
+		assertThat(toPatterns(matches), contains(pattern("/foo/bar/baz"), pattern("/foo/bar/{baz}"),
 				pattern("/foo/{*baz}")));
 	}
 
 
-	private List<PathPattern> getPatternList(Collection<PathMatchResult<Object>> results) {
-		return results.stream()
-				.map(result -> result.getPattern()).collect(Collectors.toList());
-
+	private List<PathPattern> toPatterns(Collection<PathMatchResult<Object>> results) {
+		return results.stream().map(PathMatchResult::getPattern).collect(Collectors.toList());
 	}
 
 	private static PathPatternMatcher pattern(String pattern) {

spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/PatternsRequestConditionTests.java

@@ -16,10 +16,14 @@
 
 package org.springframework.web.reactive.result.condition;
 
+import java.util.Arrays;
+import java.util.Iterator;
+
 import org.junit.Test;
 
 import org.springframework.mock.http.server.reactive.test.MockServerWebExchange;
 import org.springframework.web.server.ServerWebExchange;
+import org.springframework.web.util.pattern.PathPattern;
 import org.springframework.web.util.pattern.PathPatternParser;
 
 import static org.junit.Assert.assertEquals;
@@ -143,6 +147,15 @@ public class PatternsRequestConditionTests {
 		assertEquals(0, c1.compareTo(c2, get("/foo").toExchange()));
 	}
 
+	@Test
+	public void equallyMatchingPatternsAreBothPresent() throws Exception {
+		PatternsRequestCondition c = new PatternsRequestCondition("/a", "/b");
+		assertEquals(2, c.getPatterns().size());
+		Iterator<PathPattern> itr = c.getPatterns().iterator();
+		assertEquals("/a", itr.next().getPatternString());
+		assertEquals("/b", itr.next().getPatternString());
+	}
+
 	@Test
 	public void comparePatternSpecificity() throws Exception {
 		PatternsRequestCondition c1 = new PatternsRequestCondition("/fo*");