Skip to content

Commit fa6e329

Browse files
bclozelbclozel
committed
Merge branch '6.2.x'
2 parents 7e035f5 + 4a46d95 commit fa6e329

File tree

2 files changed

+133
-49
lines changed

2 files changed

+133
-49
lines changed

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

Lines changed: 70 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -364,19 +364,49 @@ public int compareTo(@Nullable PathPattern otherPattern) {
364364
}
365365

366366
/**
367-
* Combine this pattern with another.
367+
* Combine this pattern with the one given as parameter, returning a new
368+
* {@code PathPattern} instance that concatenates or merges both.
369+
* This operation is not commutative, meaning {@code pattern1.combine(pattern2)}
370+
* is not equal to {@code pattern2.combine(pattern1)}.
371+
*
372+
* <p>Combining two "fixed" patterns effectively concatenates them:
373+
* <ul>
374+
* <li><code> "/projects" + "/spring-framework" => "/projects/spring-framework"</code>
375+
* </ul>
376+
* Combining a "fixed" pattern with a "matching" pattern concatenates them:
377+
* <ul>
378+
* <li><code> "/projects" + "/{project}" => "/projects/{project}"</code>
379+
* </ul>
380+
* Combining a "matching" pattern with a "fixed" pattern merges them:
381+
* <ul>
382+
* <li><code> "/projects/&#42;" + "/spring-framework" => "/projects/spring-framework"</code>
383+
* <li><code> "/projects/&#42;.html" + "/spring-framework.html" => "/projects/spring-framework.html"</code>
384+
* </ul>
385+
* Combining two "matching" patterns merges them:
386+
* <ul>
387+
* <li><code> "/projects/&#42;&#42;" + "/&#42;.html" => "/projects/&#42;.html"</code>
388+
* <li><code> "/projects/&#42;" + "/{project}" => "/projects/{project}"</code>
389+
* <li><code> "/projects/&#42;.html" + "/spring-framework.&#42;" => "/projects/spring-framework.html"</code>
390+
* </ul>
391+
* Note, if a pattern does not end with a "matching" segment, it is considered as a "fixed" one:
392+
* <ul>
393+
* <li><code> "/projects/&#42;/releases" + "/{id}" => "/projects/&#42;/releases/{id}"</code>
394+
* </ul>
395+
* @param otherPattern the pattern to be combined with the current one
396+
* @return the new {@code PathPattern} that combines both patterns
397+
* @throws IllegalArgumentException if the combination is not allowed
368398
*/
369-
public PathPattern combine(PathPattern pattern2string) {
370-
// If one of them is empty the result is the other. If both empty the result is ""
399+
public PathPattern combine(PathPattern otherPattern) {
400+
// If one of them is empty, the result is the other. If both are empty, the result is ""
371401
if (!StringUtils.hasLength(this.patternString)) {
372-
if (!StringUtils.hasLength(pattern2string.patternString)) {
402+
if (!StringUtils.hasLength(otherPattern.patternString)) {
373403
return this.parser.parse("");
374404
}
375405
else {
376-
return pattern2string;
406+
return otherPattern;
377407
}
378408
}
379-
else if (!StringUtils.hasLength(pattern2string.patternString)) {
409+
else if (!StringUtils.hasLength(otherPattern.patternString)) {
380410
return this;
381411
}
382412

@@ -385,40 +415,55 @@ else if (!StringUtils.hasLength(pattern2string.patternString)) {
385415
// However:
386416
// /usr + /user => /usr/user
387417
// /{foo} + /bar => /{foo}/bar
388-
if (!this.patternString.equals(pattern2string.patternString) && this.capturedVariableCount == 0 &&
389-
matches(PathContainer.parsePath(pattern2string.patternString))) {
390-
return pattern2string;
418+
if (!this.patternString.equals(otherPattern.patternString) && this.capturedVariableCount == 0 &&
419+
matches(PathContainer.parsePath(otherPattern.patternString))) {
420+
return otherPattern;
391421
}
392422

393423
// /hotels/* + /booking => /hotels/booking
394424
// /hotels/* + booking => /hotels/booking
395425
if (this.endsWithSeparatorWildcard) {
396-
return this.parser.parse(concat(
397-
this.patternString.substring(0, this.patternString.length() - 2),
398-
pattern2string.patternString));
426+
String prefix = this.patternString.length() > 2 ?
427+
this.patternString.substring(0, this.patternString.length() - 2) :
428+
String.valueOf(this.getSeparator());
429+
return this.parser.parse(concat(prefix, otherPattern.patternString));
430+
}
431+
432+
// /hotels/** + "/booking/rooms => /hotels/booking/rooms
433+
if (this.catchAll) {
434+
return this.parser.parse(concat(this.patternString.substring(0, this.patternString.length() - 3),
435+
otherPattern.patternString));
399436
}
400437

401438
// /hotels + /booking => /hotels/booking
402439
// /hotels + booking => /hotels/booking
403-
int starDotPos1 = this.patternString.indexOf("*."); // Are there any file prefix/suffix things to consider?
404-
if (this.capturedVariableCount != 0 || starDotPos1 == -1 || getSeparator() == '.') {
405-
return this.parser.parse(concat(this.patternString, pattern2string.patternString));
440+
int firstStarDotPos = this.patternString.indexOf("*."); // Are there any file prefix/suffix things to consider?
441+
if (this.capturedVariableCount != 0 || firstStarDotPos == -1 || getSeparator() == '.') {
442+
return this.parser.parse(concat(this.patternString, otherPattern.patternString));
406443
}
407444

408445
// /*.html + /hotel => /hotel.html
409446
// /*.html + /hotel.* => /hotel.html
410-
String firstExtension = this.patternString.substring(starDotPos1 + 1); // looking for the first extension
411-
String p2string = pattern2string.patternString;
412-
int dotPos2 = p2string.indexOf('.');
413-
String file2 = (dotPos2 == -1 ? p2string : p2string.substring(0, dotPos2));
414-
String secondExtension = (dotPos2 == -1 ? "" : p2string.substring(dotPos2));
415-
boolean firstExtensionWild = (firstExtension.equals(".*") || firstExtension.isEmpty());
416-
boolean secondExtensionWild = (secondExtension.equals(".*") || secondExtension.isEmpty());
417-
if (!firstExtensionWild && !secondExtensionWild) {
447+
int secondDotPos = otherPattern.patternString.indexOf('.');
448+
449+
String firstExtension = this.patternString.substring(firstStarDotPos + 1); // looking for the first extension
450+
String secondExtension = (secondDotPos == -1 ? "" : otherPattern.patternString.substring(secondDotPos));
451+
boolean isFirstExtensionWildcard = (firstExtension.equals(".*") || firstExtension.isEmpty());
452+
boolean isSecondExtensionWildcard = (secondExtension.equals(".*") || secondExtension.isEmpty());
453+
if (!isFirstExtensionWildcard && !isSecondExtensionWildcard) {
418454
throw new IllegalArgumentException(
419-
"Cannot combine patterns: " + this.patternString + " and " + pattern2string);
455+
"Cannot combine patterns: " + this.patternString + " and " + otherPattern);
420456
}
421-
return this.parser.parse(file2 + (firstExtensionWild ? secondExtension : firstExtension));
457+
458+
String firstPath = this.patternString.substring(0, this.patternString.lastIndexOf(this.getSeparator()));
459+
String secondPath = otherPattern.patternString.substring(0, otherPattern.patternString.lastIndexOf(this.getSeparator()));
460+
if (!this.parser.parse(firstPath).matches(PathContainer.parsePath(secondPath))) {
461+
throw new IllegalArgumentException(
462+
"Cannot combine patterns: " + this.patternString + " and " + otherPattern);
463+
}
464+
465+
String secondFile = (secondDotPos == -1 ? otherPattern.patternString : otherPattern.patternString.substring(0, secondDotPos));
466+
return this.parser.parse(secondFile + (isFirstExtensionWildcard ? secondExtension : firstExtension));
422467
}
423468

424469
@Override

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

Lines changed: 63 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -694,34 +694,25 @@ void extractUriTemplateVarsRegexCapturingGroups() {
694694
}
695695

696696
@Test
697-
void combine() {
697+
void combineEmptyPatternsShouldReturnEmpty() {
698698
TestPathCombiner pathMatcher = new TestPathCombiner();
699699
assertThat(pathMatcher.combine("", "")).isEmpty();
700+
}
701+
702+
@Test
703+
void combineWithEmptyPatternShouldReturnPattern() {
704+
TestPathCombiner pathMatcher = new TestPathCombiner();
700705
assertThat(pathMatcher.combine("/hotels", "")).isEqualTo("/hotels");
701706
assertThat(pathMatcher.combine("", "/hotels")).isEqualTo("/hotels");
702-
assertThat(pathMatcher.combine("/hotels/*", "booking")).isEqualTo("/hotels/booking");
703-
assertThat(pathMatcher.combine("/hotels/*", "/booking")).isEqualTo("/hotels/booking");
707+
}
708+
709+
@Test
710+
void combineStaticPatternsShouldConcatenate() {
711+
TestPathCombiner pathMatcher = new TestPathCombiner();
704712
assertThat(pathMatcher.combine("/hotels", "/booking")).isEqualTo("/hotels/booking");
705713
assertThat(pathMatcher.combine("/hotels", "booking")).isEqualTo("/hotels/booking");
706714
assertThat(pathMatcher.combine("/hotels/", "booking")).isEqualTo("/hotels/booking");
707-
assertThat(pathMatcher.combine("/hotels/*", "{hotel}")).isEqualTo("/hotels/{hotel}");
708-
assertThat(pathMatcher.combine("/hotels", "{hotel}")).isEqualTo("/hotels/{hotel}");
709-
assertThat(pathMatcher.combine("/hotels", "{hotel}.*")).isEqualTo("/hotels/{hotel}.*");
710-
assertThat(pathMatcher.combine("/hotels/*/booking", "{booking}")).isEqualTo("/hotels/*/booking/{booking}");
711-
assertThat(pathMatcher.combine("/*.html", "/hotel.html")).isEqualTo("/hotel.html");
712-
assertThat(pathMatcher.combine("/*.html", "/hotel")).isEqualTo("/hotel.html");
713-
assertThat(pathMatcher.combine("/*.html", "/hotel.*")).isEqualTo("/hotel.html");
714-
// TODO this seems rather bogus, should we eagerly show an error?
715-
assertThat(pathMatcher.combine("/a/b/c/*.html", "/d/e/f/hotel.*")).isEqualTo("/d/e/f/hotel.html");
716-
assertThat(pathMatcher.combine("/**", "/*.html")).isEqualTo("/*.html");
717-
assertThat(pathMatcher.combine("/*", "/*.html")).isEqualTo("/*.html");
718-
assertThat(pathMatcher.combine("/*.*", "/*.html")).isEqualTo("/*.html");
719-
// SPR-8858
720-
assertThat(pathMatcher.combine("/{foo}", "/bar")).isEqualTo("/{foo}/bar");
721-
// SPR-7970
722-
assertThat(pathMatcher.combine("/user", "/user")).isEqualTo("/user/user");
723715
// SPR-10062
724-
assertThat(pathMatcher.combine("/{foo:.*[^0-9].*}", "/edit/")).isEqualTo("/{foo:.*[^0-9].*}/edit/");
725716
assertThat(pathMatcher.combine("/1.0", "/foo/test")).isEqualTo("/1.0/foo/test");
726717
// SPR-10554
727718
// SPR-12975
@@ -730,14 +721,56 @@ void combine() {
730721
assertThat(pathMatcher.combine("/hotel/", "/booking")).isEqualTo("/hotel/booking");
731722
assertThat(pathMatcher.combine("", "/hotel")).isEqualTo("/hotel");
732723
assertThat(pathMatcher.combine("/hotel", "")).isEqualTo("/hotel");
733-
// TODO Do we need special handling when patterns contain multiple dots?
724+
// SPR-7970
725+
assertThat(pathMatcher.combine("/user", "/user")).isEqualTo("/user/user");
734726
}
735727

736728
@Test
737-
void combineWithTwoFileExtensionPatterns() {
729+
void combineStaticWithMatchingShouldConcatenate() {
738730
TestPathCombiner pathMatcher = new TestPathCombiner();
739-
assertThatIllegalArgumentException().isThrownBy(() ->
740-
pathMatcher.combine("/*.html", "/*.txt"));
731+
assertThat(pathMatcher.combine("/hotels", "*")).isEqualTo("/hotels/*");
732+
assertThat(pathMatcher.combine("/hotels", "{hotel}")).isEqualTo("/hotels/{hotel}");
733+
assertThat(pathMatcher.combine("/hotels", "{hotel}.*")).isEqualTo("/hotels/{hotel}.*");
734+
}
735+
736+
@Test
737+
void combineMatchingWithStaticShouldMergeWhenWildcardMatch() {
738+
TestPathCombiner pathMatcher = new TestPathCombiner();
739+
assertThat(pathMatcher.combine("/hotels/*", "booking")).isEqualTo("/hotels/booking");
740+
assertThat(pathMatcher.combine("/hotels/*", "/booking")).isEqualTo("/hotels/booking");
741+
assertThat(pathMatcher.combine("/hotels/**", "/booking/rooms")).isEqualTo("/hotels/booking/rooms");
742+
assertThat(pathMatcher.combine("/*.html", "/hotel.html")).isEqualTo("/hotel.html");
743+
assertThat(pathMatcher.combine("/*.html", "/hotel")).isEqualTo("/hotel.html");
744+
// gh-34986
745+
assertThat(pathMatcher.combine("/*", "/foo/bar")).isEqualTo("/foo/bar");
746+
assertThat(pathMatcher.combine("/*", "foo/bar")).isEqualTo("/foo/bar");
747+
}
748+
749+
@Test
750+
void combineMatchingWithStaticShouldConcatenateWhenNoWildcardMatch() {
751+
TestPathCombiner pathMatcher = new TestPathCombiner();
752+
// SPR-10062
753+
assertThat(pathMatcher.combine("/{foo:.*[^0-9].*}", "/edit/")).isEqualTo("/{foo:.*[^0-9].*}/edit/");
754+
// SPR-8858
755+
assertThat(pathMatcher.combine("/{foo}", "/bar")).isEqualTo("/{foo}/bar");
756+
}
757+
758+
@Test
759+
void combineMatchingPatternsShouldMergeWhenMatch() {
760+
TestPathCombiner pathMatcher = new TestPathCombiner();
761+
assertThat(pathMatcher.combine("/hotels/*/booking", "{booking}")).isEqualTo("/hotels/*/booking/{booking}");
762+
assertThat(pathMatcher.combine("/hotels/*", "{hotel}")).isEqualTo("/hotels/{hotel}");
763+
assertThat(pathMatcher.combine("/*.html", "/hotel.*")).isEqualTo("/hotel.html");
764+
assertThat(pathMatcher.combine("/**", "/*.html")).isEqualTo("/*.html");
765+
assertThat(pathMatcher.combine("/*", "/*.html")).isEqualTo("/*.html");
766+
assertThat(pathMatcher.combine("/*.*", "/*.html")).isEqualTo("/*.html");
767+
}
768+
769+
@Test
770+
void combineMatchingPatternsShouldFailWhenNoMatch() {
771+
TestPathCombiner pathMatcher = new TestPathCombiner();
772+
pathMatcher.combineFails("/*.html", "/*.txt");
773+
pathMatcher.combineFails("/a/b/c/*.html", "/d/e/f/hotel.*");
741774
}
742775

743776
@Test
@@ -1053,6 +1086,12 @@ public String combine(String string1, String string2) {
10531086
return pattern1.combine(pattern2).getPatternString();
10541087
}
10551088

1089+
public void combineFails(String string1, String string2) {
1090+
PathPattern pattern1 = pp.parse(string1);
1091+
PathPattern pattern2 = pp.parse(string2);
1092+
assertThatIllegalArgumentException().isThrownBy(() -> pattern1.combine(pattern2));
1093+
}
1094+
10561095
}
10571096

10581097
}

0 commit comments

Comments
 (0)