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 index ef608e503f5..77946ee7bcb 100644 --- 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 @@ import org.springframework.util.StringUtils; * * * @author Andy Clement + * @author Rossen Stoyanchev * @since 5.0 * @see PathContainer */ @@ -342,38 +344,9 @@ public class PathPattern implements Comparable { */ @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 @@ public class PathPattern implements Comparable { return container != null && container.elements().size() > 0; } + + public static final Comparator 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 index eb40d30ff45..30345049e4d 100644 --- 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 class PathPatternParserTests { // 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 index abd9b462a77..cb7e36c3e02 100644 --- 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 @@ public abstract class AbstractUrlHandlerMapping extends AbstractHandlerMapping { 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 index 5498e555994..b4c3c88d0a3 100644 --- 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 @@ public class ResourceUrlProvider implements ApplicationListener 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 index edbadc4c66f..3c132d7cb2f 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 @@ -19,7 +19,6 @@ package org.springframework.web.reactive.result.condition; 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 final class PatternsRequestCondition extends AbstractRequestCondition toSortedSet(Collection patterns) { - TreeSet sorted = new TreeSet<>(getPatternComparator()); + TreeSet sorted = new TreeSet<>(); sorted.addAll(patterns); return sorted; } @@ -70,13 +69,6 @@ public final class PatternsRequestCondition extends AbstractRequestCondition getPatternComparator() { - return (p1, p2) -> { - int index = p1.compareTo(p2); - return (index != 0 ? index : p1.getPatternString().compareTo(p2.getPatternString())); - }; - } - public Set getPatterns() { return this.patterns; } @@ -170,7 +162,7 @@ public final class PatternsRequestCondition extends AbstractRequestCondition iterator = this.patterns.iterator(); Iterator 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 index 08e0bade875..1fa25d2d51b 100644 --- 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 class PatternsRequestConditionTests { } @Test - public void compareEqualPatterns() throws Exception { + public void compareToConsistentWithEquals() throws Exception { PatternsRequestCondition c1 = createPatternsCondition("/foo*"); PatternsRequestCondition c2 = createPatternsCondition("/foo*"); @@ -155,10 +155,17 @@ public class PatternsRequestConditionTests { @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 index d10f0d0e01d..ae311f1230e 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 @@ -160,7 +160,7 @@ public class HandlerMethodMappingTests { @Override protected Comparator getMappingComparator(ServerWebExchange exchange) { - return Comparator.comparing(o -> parser.parse(o)); + return (o1, o2) -> PathPattern.SPECIFICITY_COMPARATOR.compare(parser.parse(o1), parser.parse(o2)); } }