Proper null handling in PathPattern comparator

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

@@ -693,12 +693,17 @@ public class PathPattern implements Comparable<PathPattern> {
 
 
 	public static final Comparator<PathPattern> SPECIFICITY_COMPARATOR = (p1, p2) -> {
-		// 1) null is sorted last
+		// Same object or null == null?
+		if (p1 == p2) {
+			return 0;
+		}
+
+		// null is sorted last
 		if (p2 == null) {
 			return -1;
 		}
 
-		// 2) catchall patterns are sorted last. If both catchall then the
+		// catchall patterns are sorted last. If both catchall then the
 		// length is considered
 		if (p1.isCatchAll()) {
 			if (p2.isCatchAll()) {
@@ -715,14 +720,14 @@ public class PathPattern implements Comparable<PathPattern> {
 			return -1;
 		}
 
-		// 3) This will sort such that if they differ in terms of wildcards or
+		// 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
+		// longer is better
 		int lenDifference = p1.getNormalizedLength() - p2.getNormalizedLength();
 		return Integer.compare(0, lenDifference);
 	};

spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java

@@ -886,10 +886,7 @@ public class PathPatternTests {
 
 	@Test
 	public void patternComparator() {
-		Comparator<PathPattern> comparator = (p1, p2) -> {
-			int index = p1.compareTo(p2);
-			return (index != 0 ? index : p1.getPatternString().compareTo(p2.getPatternString()));
-		};
+		Comparator<PathPattern> comparator = PathPattern.SPECIFICITY_COMPARATOR;
 
 		assertEquals(0, comparator.compare(parse("/hotels/new"), parse("/hotels/new")));
 
@@ -970,7 +967,7 @@ public class PathPatternTests {
 	}
 
 	@Test
-	public void patternCompareTo() {
+	public void patternCompareToNull() {
 		PathPatternParser p = new PathPatternParser();
 		PathPattern pp = p.parse("/abc");
 		assertEquals(-1, pp.compareTo(null));
@@ -978,58 +975,62 @@ public class PathPatternTests {
 
 	@Test
 	public void patternComparatorSort() {
-		Comparator<PathPattern> comparator = (p1, p2) -> {
-			int index = p1.compareTo(p2);
-			return (index != 0 ? index : p1.getPatternString().compareTo(p2.getPatternString()));
-		};
+		Comparator<PathPattern> comparator = PathPattern.SPECIFICITY_COMPARATOR;
 
 		List<PathPattern> paths = new ArrayList<>(3);
 		PathPatternParser pp = new PathPatternParser();
+		paths.add(null);
+		paths.add(null);
+		paths.sort(comparator);
+		assertNull(paths.get(0));
+		assertNull(paths.get(1));
+		paths.clear();
+
 		paths.add(null);
 		paths.add(pp.parse("/hotels/new"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertNull(paths.get(1));
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/*"));
 		paths.add(pp.parse("/hotels/new"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertEquals("/hotels/*", paths.get(1).getPatternString());
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/new"));
 		paths.add(pp.parse("/hotels/*"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertEquals("/hotels/*", paths.get(1).getPatternString());
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/**"));
 		paths.add(pp.parse("/hotels/*"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/*", paths.get(0).getPatternString());
 		assertEquals("/hotels/**", paths.get(1).getPatternString());
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/*"));
 		paths.add(pp.parse("/hotels/**"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/*", paths.get(0).getPatternString());
 		assertEquals("/hotels/**", paths.get(1).getPatternString());
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/{hotel}"));
 		paths.add(pp.parse("/hotels/new"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertEquals("/hotels/{hotel}", paths.get(1).getPatternString());
 		paths.clear();
 
 		paths.add(pp.parse("/hotels/new"));
 		paths.add(pp.parse("/hotels/{hotel}"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertEquals("/hotels/{hotel}", paths.get(1).getPatternString());
 		paths.clear();
@@ -1037,7 +1038,7 @@ public class PathPatternTests {
 		paths.add(pp.parse("/hotels/*"));
 		paths.add(pp.parse("/hotels/{hotel}"));
 		paths.add(pp.parse("/hotels/new"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/new", paths.get(0).getPatternString());
 		assertEquals("/hotels/{hotel}", paths.get(1).getPatternString());
 		assertEquals("/hotels/*", paths.get(2).getPatternString());
@@ -1046,7 +1047,7 @@ public class PathPatternTests {
 		paths.add(pp.parse("/hotels/ne*"));
 		paths.add(pp.parse("/hotels/n*"));
 		Collections.shuffle(paths);
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/hotels/ne*", paths.get(0).getPatternString());
 		assertEquals("/hotels/n*", paths.get(1).getPatternString());
 		paths.clear();
@@ -1066,7 +1067,7 @@ public class PathPatternTests {
 		};
 		paths.add(pp.parse("/*/login.*"));
 		paths.add(pp.parse("/*/endUser/action/login.*"));
-		Collections.sort(paths, comparator);
+		paths.sort(comparator);
 		assertEquals("/*/endUser/action/login.*", paths.get(0).getPatternString());
 		assertEquals("/*/login.*", paths.get(1).getPatternString());
 		paths.clear();