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 c87a538cbe..906b33de17 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 @@ -374,19 +374,49 @@ public class PathPattern implements Comparable { } /** - * Combine this pattern with another. + * Combine this pattern with the one given as parameter, returning a new + * {@code PathPattern} instance that concatenates or merges both. + * This operation is not commutative, meaning {@code pattern1.combine(pattern2)} + * is not equal to {@code pattern2.combine(pattern1)}. + * + *

Combining two "fixed" patterns effectively concatenates them: + *

+ * Combining a "fixed" pattern with a "matching" pattern concatenates them: + * + * Combining a "matching" pattern with a "fixed" pattern merges them: + * + * Combining two "matching" patterns merges them: + * + * Note, if a pattern does not end with a "matching" segment, it is considered as a "fixed" one: + * + * @param otherPattern the pattern to be combined with the current one + * @return the new {@code PathPattern} that combines both patterns + * @throws IllegalArgumentException if the combination is not allowed */ - public PathPattern combine(PathPattern pattern2string) { - // If one of them is empty the result is the other. If both empty the result is "" + public PathPattern combine(PathPattern otherPattern) { + // If one of them is empty, the result is the other. If both are empty, the result is "" if (!StringUtils.hasLength(this.patternString)) { - if (!StringUtils.hasLength(pattern2string.patternString)) { + if (!StringUtils.hasLength(otherPattern.patternString)) { return this.parser.parse(""); } else { - return pattern2string; + return otherPattern; } } - else if (!StringUtils.hasLength(pattern2string.patternString)) { + else if (!StringUtils.hasLength(otherPattern.patternString)) { return this; } @@ -395,40 +425,55 @@ public class PathPattern implements Comparable { // However: // /usr + /user => /usr/user // /{foo} + /bar => /{foo}/bar - if (!this.patternString.equals(pattern2string.patternString) && this.capturedVariableCount == 0 && - matches(PathContainer.parsePath(pattern2string.patternString))) { - return pattern2string; + if (!this.patternString.equals(otherPattern.patternString) && this.capturedVariableCount == 0 && + matches(PathContainer.parsePath(otherPattern.patternString))) { + return otherPattern; } // /hotels/* + /booking => /hotels/booking // /hotels/* + booking => /hotels/booking if (this.endsWithSeparatorWildcard) { - return this.parser.parse(concat( - this.patternString.substring(0, this.patternString.length() - 2), - pattern2string.patternString)); + String prefix = this.patternString.length() > 2 ? + this.patternString.substring(0, this.patternString.length() - 2) : + String.valueOf(this.getSeparator()); + return this.parser.parse(concat(prefix, otherPattern.patternString)); + } + + // /hotels/** + "/booking/rooms => /hotels/booking/rooms + if (this.catchAll) { + return this.parser.parse(concat(this.patternString.substring(0, this.patternString.length() - 3), + otherPattern.patternString)); } // /hotels + /booking => /hotels/booking // /hotels + booking => /hotels/booking - int starDotPos1 = this.patternString.indexOf("*."); // Are there any file prefix/suffix things to consider? - if (this.capturedVariableCount != 0 || starDotPos1 == -1 || getSeparator() == '.') { - return this.parser.parse(concat(this.patternString, pattern2string.patternString)); + int firstStarDotPos = this.patternString.indexOf("*."); // Are there any file prefix/suffix things to consider? + if (this.capturedVariableCount != 0 || firstStarDotPos == -1 || getSeparator() == '.') { + return this.parser.parse(concat(this.patternString, otherPattern.patternString)); } // /*.html + /hotel => /hotel.html // /*.html + /hotel.* => /hotel.html - String firstExtension = this.patternString.substring(starDotPos1 + 1); // looking for the first extension - String p2string = pattern2string.patternString; - int dotPos2 = p2string.indexOf('.'); - String file2 = (dotPos2 == -1 ? p2string : p2string.substring(0, dotPos2)); - String secondExtension = (dotPos2 == -1 ? "" : p2string.substring(dotPos2)); - boolean firstExtensionWild = (firstExtension.equals(".*") || firstExtension.isEmpty()); - boolean secondExtensionWild = (secondExtension.equals(".*") || secondExtension.isEmpty()); - if (!firstExtensionWild && !secondExtensionWild) { + int secondDotPos = otherPattern.patternString.indexOf('.'); + + String firstExtension = this.patternString.substring(firstStarDotPos + 1); // looking for the first extension + String secondExtension = (secondDotPos == -1 ? "" : otherPattern.patternString.substring(secondDotPos)); + boolean isFirstExtensionWildcard = (firstExtension.equals(".*") || firstExtension.isEmpty()); + boolean isSecondExtensionWildcard = (secondExtension.equals(".*") || secondExtension.isEmpty()); + if (!isFirstExtensionWildcard && !isSecondExtensionWildcard) { throw new IllegalArgumentException( - "Cannot combine patterns: " + this.patternString + " and " + pattern2string); + "Cannot combine patterns: " + this.patternString + " and " + otherPattern); } - return this.parser.parse(file2 + (firstExtensionWild ? secondExtension : firstExtension)); + + String firstPath = this.patternString.substring(0, this.patternString.lastIndexOf(this.getSeparator())); + String secondPath = otherPattern.patternString.substring(0, otherPattern.patternString.lastIndexOf(this.getSeparator())); + if (!this.parser.parse(firstPath).matches(PathContainer.parsePath(secondPath))) { + throw new IllegalArgumentException( + "Cannot combine patterns: " + this.patternString + " and " + otherPattern); + } + + String secondFile = (secondDotPos == -1 ? otherPattern.patternString : otherPattern.patternString.substring(0, secondDotPos)); + return this.parser.parse(secondFile + (isFirstExtensionWildcard ? secondExtension : firstExtension)); } @Override diff --git a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java index ebaeb84863..d428b0ec00 100644 --- a/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java +++ b/spring-web/src/test/java/org/springframework/web/util/pattern/PathPatternTests.java @@ -905,34 +905,25 @@ class PathPatternTests { } @Test - void combine() { + void combineEmptyPatternsShouldReturnEmpty() { TestPathCombiner pathMatcher = new TestPathCombiner(); assertThat(pathMatcher.combine("", "")).isEmpty(); + } + + @Test + void combineWithEmptyPatternShouldReturnPattern() { + TestPathCombiner pathMatcher = new TestPathCombiner(); assertThat(pathMatcher.combine("/hotels", "")).isEqualTo("/hotels"); assertThat(pathMatcher.combine("", "/hotels")).isEqualTo("/hotels"); - assertThat(pathMatcher.combine("/hotels/*", "booking")).isEqualTo("/hotels/booking"); - assertThat(pathMatcher.combine("/hotels/*", "/booking")).isEqualTo("/hotels/booking"); + } + + @Test + void combineStaticPatternsShouldConcatenate() { + TestPathCombiner pathMatcher = new TestPathCombiner(); assertThat(pathMatcher.combine("/hotels", "/booking")).isEqualTo("/hotels/booking"); assertThat(pathMatcher.combine("/hotels", "booking")).isEqualTo("/hotels/booking"); assertThat(pathMatcher.combine("/hotels/", "booking")).isEqualTo("/hotels/booking"); - assertThat(pathMatcher.combine("/hotels/*", "{hotel}")).isEqualTo("/hotels/{hotel}"); - assertThat(pathMatcher.combine("/hotels", "{hotel}")).isEqualTo("/hotels/{hotel}"); - assertThat(pathMatcher.combine("/hotels", "{hotel}.*")).isEqualTo("/hotels/{hotel}.*"); - assertThat(pathMatcher.combine("/hotels/*/booking", "{booking}")).isEqualTo("/hotels/*/booking/{booking}"); - assertThat(pathMatcher.combine("/*.html", "/hotel.html")).isEqualTo("/hotel.html"); - assertThat(pathMatcher.combine("/*.html", "/hotel")).isEqualTo("/hotel.html"); - assertThat(pathMatcher.combine("/*.html", "/hotel.*")).isEqualTo("/hotel.html"); - // TODO this seems rather bogus, should we eagerly show an error? - assertThat(pathMatcher.combine("/a/b/c/*.html", "/d/e/f/hotel.*")).isEqualTo("/d/e/f/hotel.html"); - assertThat(pathMatcher.combine("/**", "/*.html")).isEqualTo("/*.html"); - assertThat(pathMatcher.combine("/*", "/*.html")).isEqualTo("/*.html"); - assertThat(pathMatcher.combine("/*.*", "/*.html")).isEqualTo("/*.html"); - // SPR-8858 - assertThat(pathMatcher.combine("/{foo}", "/bar")).isEqualTo("/{foo}/bar"); - // SPR-7970 - assertThat(pathMatcher.combine("/user", "/user")).isEqualTo("/user/user"); // SPR-10062 - assertThat(pathMatcher.combine("/{foo:.*[^0-9].*}", "/edit/")).isEqualTo("/{foo:.*[^0-9].*}/edit/"); assertThat(pathMatcher.combine("/1.0", "/foo/test")).isEqualTo("/1.0/foo/test"); // SPR-10554 // SPR-12975 @@ -941,14 +932,56 @@ class PathPatternTests { assertThat(pathMatcher.combine("/hotel/", "/booking")).isEqualTo("/hotel/booking"); assertThat(pathMatcher.combine("", "/hotel")).isEqualTo("/hotel"); assertThat(pathMatcher.combine("/hotel", "")).isEqualTo("/hotel"); - // TODO Do we need special handling when patterns contain multiple dots? + // SPR-7970 + assertThat(pathMatcher.combine("/user", "/user")).isEqualTo("/user/user"); } @Test - void combineWithTwoFileExtensionPatterns() { + void combineStaticWithMatchingShouldConcatenate() { TestPathCombiner pathMatcher = new TestPathCombiner(); - assertThatIllegalArgumentException().isThrownBy(() -> - pathMatcher.combine("/*.html", "/*.txt")); + assertThat(pathMatcher.combine("/hotels", "*")).isEqualTo("/hotels/*"); + assertThat(pathMatcher.combine("/hotels", "{hotel}")).isEqualTo("/hotels/{hotel}"); + assertThat(pathMatcher.combine("/hotels", "{hotel}.*")).isEqualTo("/hotels/{hotel}.*"); + } + + @Test + void combineMatchingWithStaticShouldMergeWhenWildcardMatch() { + TestPathCombiner pathMatcher = new TestPathCombiner(); + assertThat(pathMatcher.combine("/hotels/*", "booking")).isEqualTo("/hotels/booking"); + assertThat(pathMatcher.combine("/hotels/*", "/booking")).isEqualTo("/hotels/booking"); + assertThat(pathMatcher.combine("/hotels/**", "/booking/rooms")).isEqualTo("/hotels/booking/rooms"); + assertThat(pathMatcher.combine("/*.html", "/hotel.html")).isEqualTo("/hotel.html"); + assertThat(pathMatcher.combine("/*.html", "/hotel")).isEqualTo("/hotel.html"); + // gh-34986 + assertThat(pathMatcher.combine("/*", "/foo/bar")).isEqualTo("/foo/bar"); + assertThat(pathMatcher.combine("/*", "foo/bar")).isEqualTo("/foo/bar"); + } + + @Test + void combineMatchingWithStaticShouldConcatenateWhenNoWildcardMatch() { + TestPathCombiner pathMatcher = new TestPathCombiner(); + // SPR-10062 + assertThat(pathMatcher.combine("/{foo:.*[^0-9].*}", "/edit/")).isEqualTo("/{foo:.*[^0-9].*}/edit/"); + // SPR-8858 + assertThat(pathMatcher.combine("/{foo}", "/bar")).isEqualTo("/{foo}/bar"); + } + + @Test + void combineMatchingPatternsShouldMergeWhenMatch() { + TestPathCombiner pathMatcher = new TestPathCombiner(); + assertThat(pathMatcher.combine("/hotels/*/booking", "{booking}")).isEqualTo("/hotels/*/booking/{booking}"); + assertThat(pathMatcher.combine("/hotels/*", "{hotel}")).isEqualTo("/hotels/{hotel}"); + assertThat(pathMatcher.combine("/*.html", "/hotel.*")).isEqualTo("/hotel.html"); + assertThat(pathMatcher.combine("/**", "/*.html")).isEqualTo("/*.html"); + assertThat(pathMatcher.combine("/*", "/*.html")).isEqualTo("/*.html"); + assertThat(pathMatcher.combine("/*.*", "/*.html")).isEqualTo("/*.html"); + } + + @Test + void combineMatchingPatternsShouldFailWhenNoMatch() { + TestPathCombiner pathMatcher = new TestPathCombiner(); + pathMatcher.combineFails("/*.html", "/*.txt"); + pathMatcher.combineFails("/a/b/c/*.html", "/d/e/f/hotel.*"); } @Test @@ -1268,6 +1301,12 @@ class PathPatternTests { return pattern1.combine(pattern2).getPatternString(); } + public void combineFails(String string1, String string2) { + PathPattern pattern1 = pp.parse(string1); + PathPattern pattern2 = pp.parse(string2); + assertThatIllegalArgumentException().isThrownBy(() -> pattern1.combine(pattern2)); + } + } }