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 30798265f8e..d332a58f1a6 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.Collections; import java.util.Comparator; import java.util.Iterator; import java.util.List; @@ -27,14 +26,11 @@ import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.springframework.http.server.reactive.PathContainer; import org.springframework.lang.Nullable; -import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.pattern.PathPattern; -import org.springframework.web.util.pattern.PathPatternParser; /** * A logical disjunction (' || ') request condition that matches a request @@ -46,48 +42,32 @@ import org.springframework.web.util.pattern.PathPatternParser; */ public final class PatternsRequestCondition extends AbstractRequestCondition { - private final Set patterns; - - private final PathPatternParser parser; + private final SortedSet patterns; /** * Creates a new instance with the given URL patterns. - * Each pattern is prepended with "/" if not already. * @param patterns 0 or more URL patterns; if 0 the condition will match to every request. */ - public PatternsRequestCondition(String... patterns) { - this(patterns, null); + public PatternsRequestCondition(PathPattern... patterns) { + this(Arrays.asList(patterns)); } /** - * Creates a new instance with the given URL patterns. - * Each pattern that is not empty and does not start with "/" is pre-pended with "/". - * @param patterns the URL patterns to use; if 0, the condition will match to every request. - * @param patternParser for parsing string patterns + * Creates a new instance with the given {@code Stream} of URL patterns. */ - public PatternsRequestCondition(String[] patterns, PathPatternParser patternParser) { - this(Arrays.asList(patterns), patternParser); + public PatternsRequestCondition(List patterns) { + this(toSortedSet(patterns)); } - /** - * Private constructor accepting a collection of raw patterns. - */ - private PatternsRequestCondition(Collection patterns, PathPatternParser parser) { - this.parser = (parser != null ? parser : new PathPatternParser()); - this.patterns = toSortedSet(patterns.stream().map(pattern -> parse(pattern, this.parser))); - } - - private static PathPattern parse(String pattern, PathPatternParser parser) { - if (StringUtils.hasText(pattern) && !pattern.startsWith("/")) { - pattern = "/" + pattern; - } - return parser.parse(pattern); + private static SortedSet toSortedSet(Collection patterns) { + TreeSet sorted = new TreeSet<>(getPatternComparator()); + sorted.addAll(patterns); + return sorted; } - private static Set toSortedSet(Stream stream) { - return Collections.unmodifiableSet(stream.sorted() - .collect(Collectors.toCollection(() -> new TreeSet<>(getPatternComparator())))); + private PatternsRequestCondition(SortedSet patterns) { + this.patterns = patterns; } private static Comparator getPatternComparator() { @@ -97,14 +77,6 @@ public final class PatternsRequestCondition extends AbstractRequestCondition patterns, PathPatternParser patternParser) { - this.patterns = toSortedSet(patterns.stream()); - this.parser = patternParser; - } - public Set getPatterns() { return this.patterns; } @@ -145,10 +117,7 @@ public final class PatternsRequestCondition extends AbstractRequestCondition matches = getMatchingPatterns(exchange); return matches.isEmpty() ? null : - new PatternsRequestCondition(new ArrayList(matches), this.parser); + new PatternsRequestCondition(matches); } /** diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java index da1fc5df623..e383a6105b6 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java @@ -16,6 +16,10 @@ package org.springframework.web.reactive.result.method; +import java.util.Arrays; +import java.util.List; +import java.util.stream.Collectors; + import org.springframework.lang.Nullable; import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.RequestMethod; @@ -29,6 +33,7 @@ import org.springframework.web.reactive.result.condition.RequestCondition; import org.springframework.web.reactive.result.condition.RequestConditionHolder; import org.springframework.web.reactive.result.condition.RequestMethodsRequestCondition; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.util.pattern.PathPattern; import org.springframework.web.util.pattern.PathPatternParser; /** @@ -478,8 +483,9 @@ public final class RequestMappingInfo implements RequestCondition parse(String[] paths, PathPatternParser parser) { + return Arrays + .stream(paths) + .map(path -> { + if (StringUtils.hasText(path) && !path.startsWith("/")) { + path = "/" + path; + } + return parser.parse(path); + }) + .collect(Collectors.toList()); + } } 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 19bb9a208b5..5e10baaf91e 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 @@ -18,6 +18,7 @@ package org.springframework.web.reactive.result.condition; import java.util.Arrays; import java.util.Iterator; +import java.util.stream.Collectors; import org.junit.Test; @@ -38,15 +39,11 @@ import static org.springframework.mock.http.server.reactive.test.MockServerHttpR */ public class PatternsRequestConditionTests { - @Test - public void prependSlash() { - PatternsRequestCondition c = new PatternsRequestCondition("foo"); - assertEquals("/foo", c.getPatterns().iterator().next().getPatternString()); - } + private final PathPatternParser parser = new PathPatternParser(); @Test public void prependNonEmptyPatternsOnly() { - PatternsRequestCondition c = new PatternsRequestCondition(""); + PatternsRequestCondition c = createPatternsCondition(""); assertEquals("Do not prepend empty patterns (SPR-8255)", "", c.getPatterns().iterator().next().getPatternString()); } @@ -56,33 +53,33 @@ public class PatternsRequestConditionTests { PatternsRequestCondition c1 = new PatternsRequestCondition(); PatternsRequestCondition c2 = new PatternsRequestCondition(); - assertEquals(new PatternsRequestCondition(""), c1.combine(c2)); + assertEquals(createPatternsCondition(), c1.combine(c2)); } @Test public void combineOnePatternWithEmptySet() { - PatternsRequestCondition c1 = new PatternsRequestCondition("/type1", "/type2"); + PatternsRequestCondition c1 = createPatternsCondition("/type1", "/type2"); PatternsRequestCondition c2 = new PatternsRequestCondition(); - assertEquals(new PatternsRequestCondition("/type1", "/type2"), c1.combine(c2)); + assertEquals(createPatternsCondition("/type1", "/type2"), c1.combine(c2)); c1 = new PatternsRequestCondition(); - c2 = new PatternsRequestCondition("/method1", "/method2"); + c2 = createPatternsCondition("/method1", "/method2"); - assertEquals(new PatternsRequestCondition("/method1", "/method2"), c1.combine(c2)); + assertEquals(createPatternsCondition("/method1", "/method2"), c1.combine(c2)); } @Test public void combineMultiplePatterns() { - PatternsRequestCondition c1 = new PatternsRequestCondition("/t1", "/t2"); - PatternsRequestCondition c2 = new PatternsRequestCondition("/m1", "/m2"); + PatternsRequestCondition c1 = createPatternsCondition("/t1", "/t2"); + PatternsRequestCondition c2 = createPatternsCondition("/m1", "/m2"); - assertEquals(new PatternsRequestCondition("/t1/m1", "/t1/m2", "/t2/m1", "/t2/m2"), c1.combine(c2)); + assertEquals(createPatternsCondition("/t1/m1", "/t1/m2", "/t2/m1", "/t2/m2"), c1.combine(c2)); } @Test public void matchDirectPath() throws Exception { - PatternsRequestCondition condition = new PatternsRequestCondition("/foo"); + PatternsRequestCondition condition = createPatternsCondition("/foo"); PatternsRequestCondition match = condition.getMatchingCondition(get("/foo").toExchange()); assertNotNull(match); @@ -90,7 +87,7 @@ public class PatternsRequestConditionTests { @Test public void matchPattern() throws Exception { - PatternsRequestCondition condition = new PatternsRequestCondition("/foo/*"); + PatternsRequestCondition condition = createPatternsCondition("/foo/*"); PatternsRequestCondition match = condition.getMatchingCondition(get("/foo/bar").toExchange()); assertNotNull(match); @@ -98,9 +95,9 @@ public class PatternsRequestConditionTests { @Test public void matchSortPatterns() throws Exception { - PatternsRequestCondition condition = new PatternsRequestCondition("/*/*", "/foo/bar", "/foo/*"); + PatternsRequestCondition condition = createPatternsCondition("/*/*", "/foo/bar", "/foo/*"); PatternsRequestCondition match = condition.getMatchingCondition(get("/foo/bar").toExchange()); - PatternsRequestCondition expected = new PatternsRequestCondition("/foo/bar", "/foo/*", "/*/*"); + PatternsRequestCondition expected = createPatternsCondition("/foo/bar", "/foo/*", "/*/*"); assertEquals(expected, match); } @@ -109,14 +106,14 @@ public class PatternsRequestConditionTests { public void matchTrailingSlash() throws Exception { MockServerWebExchange exchange = get("/foo/").toExchange(); - PatternsRequestCondition condition = new PatternsRequestCondition("/foo"); + PatternsRequestCondition condition = createPatternsCondition("/foo"); PatternsRequestCondition match = condition.getMatchingCondition(exchange); assertNotNull(match); assertEquals("Should match by default", "/foo", match.getPatterns().iterator().next().getPatternString()); - condition = new PatternsRequestCondition(new String[] {"/foo"}, null); + condition = createPatternsCondition("/foo"); match = condition.getMatchingCondition(exchange); assertNotNull(match); @@ -125,7 +122,7 @@ public class PatternsRequestConditionTests { PathPatternParser parser = new PathPatternParser(); parser.setMatchOptionalTrailingSlash(false); - condition = new PatternsRequestCondition(new String[] {"/foo"}, parser); + condition = new PatternsRequestCondition(parser.parse("/foo")); match = condition.getMatchingCondition(get("/foo/").toExchange()); assertNull(match); @@ -133,7 +130,7 @@ public class PatternsRequestConditionTests { @Test public void matchPatternContainsExtension() throws Exception { - PatternsRequestCondition condition = new PatternsRequestCondition("/foo.jpg"); + PatternsRequestCondition condition = createPatternsCondition("/foo.jpg"); PatternsRequestCondition match = condition.getMatchingCondition(get("/foo.html").toExchange()); assertNull(match); @@ -141,15 +138,15 @@ public class PatternsRequestConditionTests { @Test public void compareEqualPatterns() throws Exception { - PatternsRequestCondition c1 = new PatternsRequestCondition("/foo*"); - PatternsRequestCondition c2 = new PatternsRequestCondition("/foo*"); + PatternsRequestCondition c1 = createPatternsCondition("/foo*"); + PatternsRequestCondition c2 = createPatternsCondition("/foo*"); assertEquals(0, c1.compareTo(c2, get("/foo").toExchange())); } @Test public void equallyMatchingPatternsAreBothPresent() throws Exception { - PatternsRequestCondition c = new PatternsRequestCondition("/a", "/b"); + PatternsRequestCondition c = createPatternsCondition("/a", "/b"); assertEquals(2, c.getPatterns().size()); Iterator itr = c.getPatterns().iterator(); assertEquals("/a", itr.next().getPatternString()); @@ -158,8 +155,8 @@ public class PatternsRequestConditionTests { @Test public void comparePatternSpecificity() throws Exception { - PatternsRequestCondition c1 = new PatternsRequestCondition("/fo*"); - PatternsRequestCondition c2 = new PatternsRequestCondition("/foo"); + PatternsRequestCondition c1 = createPatternsCondition("/fo*"); + PatternsRequestCondition c2 = createPatternsCondition("/foo"); assertEquals(1, c1.compareTo(c2, get("/foo").toExchange())); } @@ -168,8 +165,8 @@ public class PatternsRequestConditionTests { public void compareNumberOfMatchingPatterns() throws Exception { ServerWebExchange exchange = get("/foo.html").toExchange(); - PatternsRequestCondition c1 = new PatternsRequestCondition("/foo.*", "/foo.jpeg"); - PatternsRequestCondition c2 = new PatternsRequestCondition("/foo.*", "/foo.html"); + PatternsRequestCondition c1 = createPatternsCondition("/foo.*", "/foo.jpeg"); + PatternsRequestCondition c2 = createPatternsCondition("/foo.*", "/foo.html"); PatternsRequestCondition match1 = c1.getMatchingCondition(exchange); PatternsRequestCondition match2 = c2.getMatchingCondition(exchange); @@ -178,4 +175,11 @@ public class PatternsRequestConditionTests { assertEquals(1, match1.compareTo(match2, exchange)); } + private PatternsRequestCondition createPatternsCondition(String... patterns) { + return new PatternsRequestCondition(Arrays + .stream(patterns) + .map(rawPattern -> this.parser.parse(rawPattern)) + .collect(Collectors.toList())); + } + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/RequestMappingInfoTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/RequestMappingInfoTests.java index c5ff5245de7..14c95e01b44 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/RequestMappingInfoTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/RequestMappingInfoTests.java @@ -16,12 +16,15 @@ package org.springframework.web.reactive.result.condition; +import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; import org.junit.Ignore; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; @@ -30,6 +33,8 @@ import org.springframework.mock.http.server.reactive.test.MockServerWebExchange; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.reactive.result.method.RequestMappingInfo; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.util.pattern.PathPattern; +import org.springframework.web.util.pattern.PatternParseException; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; @@ -46,6 +51,9 @@ import static org.springframework.web.reactive.result.method.RequestMappingInfo. */ public class RequestMappingInfoTests { + @Rule + public ExpectedException thrown = ExpectedException.none(); + // TODO: CORS pre-flight (see @Ignore) @@ -62,6 +70,21 @@ public class RequestMappingInfoTests { assertNull(info.getCustomCondition()); } + @Test + public void throwWhenInvalidPattern() { + this.thrown.expect(PatternParseException.class); + this.thrown.expectMessage("Expected close capture character after variable name }"); + paths("/{foo").build(); + } + + @Test + public void prependPatternWithSlash() { + RequestMappingInfo actual = paths("foo").build(); + List patterns = new ArrayList<>(actual.getPatternsCondition().getPatterns()); + assertEquals(1, patterns.size()); + assertEquals("/foo", patterns.get(0).getPatternString()); + } + @Test public void matchPatternsCondition() { MockServerWebExchange exchange = MockServerHttpRequest.get("/foo").toExchange();