Dedicated specificity comparator in PathPattern

rstoyanchev · rstoyanchev · commit 08dfce2cb551 · 2017-08-02T18:11:36.000+02:00
The PathPattern compareTo method is now consistent with equals when
two patterns are of the same specificity but otherwise different.

Separately PathPattern now exposes a Comparator by specificity that
offers the current functionality of compareTo. This can be used for
actual sorting where we only care about specificity.
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
@@ -17,6 +17,7 @@
 package org.springframework.web.util.pattern;
 
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -64,6 +65,7 @@
  * </ul>
  *
  * @author Andy Clement
+ * @author Rossen Stoyanchev
  * @since 5.0
  * @see PathContainer
  */
@@ -342,38 +344,9 @@ private String extractPathWithinPattern(String path) {
 	 */
 	@Override
 	public int compareTo(@Nullable PathPattern otherPattern) {
-		// 1) null is sorted last
-		if (otherPattern == null) {
-			return -1;
-		}
-
-		// 2) catchall patterns are sorted last. If both catchall then the
-		// length is considered
-		if (isCatchAll()) {
-			if (otherPattern.isCatchAll()) {
-				int lenDifference = getNormalizedLength() - otherPattern.getNormalizedLength();
-				if (lenDifference != 0) {
-					return (lenDifference < 0) ? +1 : -1;
-				}
-			}
-			else {
-				return +1;
-			}
-		}
-		else if (otherPattern.isCatchAll()) {
-			return -1;
-		}
-
-		// 3) This will sort such that if they differ in terms of wildcards or
-		// captured variable counts, the one with the most will be sorted last
-		int score = getScore() - otherPattern.getScore();
-		if (score != 0) {
-			return (score < 0) ? -1 : +1;
-		}
-
-		// 4) longer is better
-		int lenDifference = getNormalizedLength() - otherPattern.getNormalizedLength();
-		return (lenDifference < 0) ? +1 : (lenDifference == 0 ? 0 : -1);
+		int result = SPECIFICITY_COMPARATOR.compare(this, otherPattern);
+		return (result == 0 && otherPattern != null ?
+				this.patternString.compareTo(otherPattern.patternString) : result);
 	}
 
 	/**
@@ -718,4 +691,40 @@ private boolean hasLength(@Nullable PathContainer container) {
 		return container != null && container.elements().size() > 0;
 	}
 
+
+	public static final Comparator<PathPattern> SPECIFICITY_COMPARATOR = (p1, p2) -> {
+		// 1) null is sorted last
+		if (p2 == null) {
+			return -1;
+		}
+
+		// 2) catchall patterns are sorted last. If both catchall then the
+		// length is considered
+		if (p1.isCatchAll()) {
+			if (p2.isCatchAll()) {
+				int lenDifference = p1.getNormalizedLength() - p2.getNormalizedLength();
+				if (lenDifference != 0) {
+					return (lenDifference < 0) ? +1 : -1;
+				}
+			}
+			else {
+				return +1;
+			}
+		}
+		else if (p2.isCatchAll()) {
+			return -1;
+		}
+
+		// 3) This will sort such that if they differ in terms of wildcards or
+		// captured variable counts, the one with the most will be sorted last
+		int score = p1.getScore() - p2.getScore();
+		if (score != 0) {
+			return (score < 0) ? -1 : +1;
+		}
+
+		// 4) longer is better
+		int lenDifference = p1.getNormalizedLength() - p2.getNormalizedLength();
+		return Integer.compare(0, lenDifference);
+	};
+
 }
diff --git a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternParserTests.java b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternParserTests.java
@@ -387,7 +387,7 @@ public void compareTests() {
 		// Based purely on catchAll
 		p1 = parse("{*foobar}");
 		p2 = parse("{*goo}");
-		assertEquals(0, p1.compareTo(p2));
+		assertTrue(p1.compareTo(p2) != 0);
 
 		p1 = parse("/{*foobar}");
 		p2 = parse("/abc/{*ww}");
diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/handler/AbstractUrlHandlerMapping.java
@@ -118,7 +118,8 @@ protected Object lookupHandler(PathContainer lookupPath, ServerWebExchange excha
 
 		return this.handlerMap.entrySet().stream()
 				.filter(entry -> entry.getKey().matches(lookupPath))
-				.sorted(Comparator.comparing(Map.Entry::getKey))
+				.sorted((entry1, entry2) ->
+						PathPattern.SPECIFICITY_COMPARATOR.compare(entry1.getKey(), entry2.getKey()))
 				.findFirst()
 				.map(entry -> {
 					PathPattern pattern = entry.getKey();
diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceUrlProvider.java b/spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceUrlProvider.java
@@ -166,7 +166,8 @@ private int getQueryIndex(String path) {
 	private Mono<String> resolveResourceUrl(PathContainer lookupPath) {
 		return this.handlerMap.entrySet().stream()
 				.filter(entry -> entry.getKey().matches(lookupPath))
-				.sorted(Comparator.comparing(Map.Entry::getKey))
+				.sorted((entry1, entry2) ->
+						PathPattern.SPECIFICITY_COMPARATOR.compare(entry1.getKey(), entry2.getKey()))
 				.findFirst()
 				.map(entry -> {
 					PathContainer path = entry.getKey().extractPathWithinPattern(lookupPath);
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
@@ -19,7 +19,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
@@ -61,7 +60,7 @@ public PatternsRequestCondition(List<PathPattern> patterns) {
 	}
 
 	private static SortedSet<PathPattern> toSortedSet(Collection<PathPattern> patterns) {
-		TreeSet<PathPattern> sorted = new TreeSet<>(getPatternComparator());
+		TreeSet<PathPattern> sorted = new TreeSet<>();
 		sorted.addAll(patterns);
 		return sorted;
 	}
@@ -70,13 +69,6 @@ private PatternsRequestCondition(SortedSet<PathPattern> patterns) {
 		this.patterns = patterns;
 	}
 
-	private static Comparator<PathPattern> getPatternComparator() {
-		return (p1, p2) -> {
-			int index = p1.compareTo(p2);
-			return (index != 0 ? index : p1.getPatternString().compareTo(p2.getPatternString()));
-		};
-	}
-
 	public Set<PathPattern> getPatterns() {
 		return this.patterns;
 	}
@@ -170,7 +162,7 @@ public int compareTo(PatternsRequestCondition other, ServerWebExchange exchange)
 		Iterator<PathPattern> iterator = this.patterns.iterator();
 		Iterator<PathPattern> iteratorOther = other.getPatterns().iterator();
 		while (iterator.hasNext() && iteratorOther.hasNext()) {
-			int result = iterator.next().compareTo(iteratorOther.next());
+			int result = PathPattern.SPECIFICITY_COMPARATOR.compare(iterator.next(), iteratorOther.next());
 			if (result != 0) {
 				return result;
 			}
diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/PatternsRequestConditionTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/PatternsRequestConditionTests.java
@@ -137,7 +137,7 @@ public void matchPatternContainsExtension() throws Exception {
 	}
 
 	@Test
-	public void compareEqualPatterns() throws Exception {
+	public void compareToConsistentWithEquals() throws Exception {
 		PatternsRequestCondition c1 = createPatternsCondition("/foo*");
 		PatternsRequestCondition c2 = createPatternsCondition("/foo*");
 
@@ -155,10 +155,17 @@ public void equallyMatchingPatternsAreBothPresent() throws Exception {
 
 	@Test
 	public void comparePatternSpecificity() throws Exception {
+		ServerWebExchange exchange = get("/foo").toExchange();
+
 		PatternsRequestCondition c1 = createPatternsCondition("/fo*");
 		PatternsRequestCondition c2 = createPatternsCondition("/foo");
 
-		assertEquals(1, c1.compareTo(c2, get("/foo").toExchange()));
+		assertEquals(1, c1.compareTo(c2, exchange));
+
+		c1 = createPatternsCondition("/fo*");
+		c2 = createPatternsCondition("/*oo");
+
+		assertEquals("Patterns are equally specific even if not the same", 0, c1.compareTo(c2, exchange));
 	}
 
 	@Test
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
@@ -160,7 +160,7 @@ protected String getMatchingMapping(String pattern, ServerWebExchange exchange)
 
 		@Override
 		protected Comparator<String> getMappingComparator(ServerWebExchange exchange) {
-			return Comparator.comparing(o -> parser.parse(o));
+			return (o1, o2) -> PathPattern.SPECIFICITY_COMPARATOR.compare(parser.parse(o1), parser.parse(o2));
 		}
 
 	}

Original file line number	Diff line number	Diff line change
`@@ -160,7 +160,7 @@ protected String getMatchingMapping(String pattern, ServerWebExchange exchange)`
`160`	`160`
`161`	`161`	`@Override`
`162`	`162`	`protected Comparator<String> getMappingComparator(ServerWebExchange exchange) {`
`163`		`- return Comparator.comparing(o -> parser.parse(o));`
	`163`	`+ return (o1, o2) -> PathPattern.SPECIFICITY_COMPARATOR.compare(parser.parse(o1), parser.parse(o2));`
`164`	`164`	`}`
`165`	`165`
`166`	`166`	`}`