Fix out of bounds exception for PathPattern#combine

Prior to this commit, combining the "/*" and  "/x/y" path patterns
would result in a `StringIndexOutOfBoundsException`.

This commit fixes this problem and revisits the implementation for
better consistency:

* "/*" + "/x/y" is now "/x/y"
* "/x/*.html" + "/y/file.*" is now rejected because they don't share the
  same prefix.

This change also adds the relevant Javadoc to the `PathPattern#combine`
method.

Fixes gh-34986

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

@@ -374,19 +374,49 @@ public class PathPattern implements Comparable<PathPattern> {
 	}
 
 	/**
-	 * 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)}.
+	 *
+	 * <p>Combining two "fixed" patterns effectively concatenates them:
+	 * <ul>
+	 *     <li><code> "/projects" + "/spring-framework" => "/projects/spring-framework"</code>
+ *     </ul>
+	 * Combining a "fixed" pattern with a "matching" pattern concatenates them:
+	 * <ul>
+	 *     <li><code> "/projects" + "/{project}" => "/projects/{project}"</code>
+	 * </ul>
+	 * Combining a "matching" pattern with a "fixed" pattern merges them:
+	 * <ul>
+	 *     <li><code> "/projects/&#42;" + "/spring-framework" => "/projects/spring-framework"</code>
+	 *     <li><code> "/projects/&#42;.html" + "/spring-framework.html" => "/projects/spring-framework.html"</code>
+	 * </ul>
+	 * Combining two "matching" patterns merges them:
+	 * <ul>
+	 *     <li><code> "/projects/&#42;&#42;" + "/&#42;.html" => "/projects/&#42;.html"</code>
+	 *     <li><code> "/projects/&#42;" + "/{project}" => "/projects/{project}"</code>
+	 *     <li><code> "/projects/&#42;.html" + "/spring-framework.&#42;" => "/projects/spring-framework.html"</code>
+	 * </ul>
+	 * Note, if a pattern does not end with a "matching" segment, it is considered as a "fixed" one:
+	 * <ul>
+	 *     <li><code> "/projects/&#42;/releases" + "/{id}" => "/projects/&#42;/releases/{id}"</code>
+	 * </ul>
+	 * @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<PathPattern> {
 		// 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

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));
+		}
+
 	}
 
 }