Remove PathPatternParser from PatternsRequestCondition

Since `PathPattern.combine` now returns another `PathPattern` instance
(it was previously returning a String instance), we can now safely
remove the parser instance included in `PatternsRequestCondition`.

Issue: SPR-15663

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<PatternsRequestCondition> {
 
-	private final Set<PathPattern> patterns;
-
-	private final PathPatternParser parser;
+	private final SortedSet<PathPattern> 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<PathPattern> patterns) {
+		this(toSortedSet(patterns));
 	}
 
-	/**
-	 * Private constructor accepting a collection of raw patterns.
-	 */
-	private PatternsRequestCondition(Collection<String> 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<PathPattern> toSortedSet(Collection<PathPattern> patterns) {
+		TreeSet<PathPattern> sorted = new TreeSet<>(getPatternComparator());
+		sorted.addAll(patterns);
+		return sorted;
 	}
 
-	private static Set<PathPattern> toSortedSet(Stream<PathPattern> stream) {
-		return Collections.unmodifiableSet(stream.sorted()
-				.collect(Collectors.toCollection(() -> new TreeSet<>(getPatternComparator()))));
+	private PatternsRequestCondition(SortedSet<PathPattern> patterns) {
+		this.patterns = patterns;
 	}
 
 	private static Comparator<PathPattern> getPatternComparator() {
@@ -97,14 +77,6 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 		};
 	}
 
-	/**
-	 * Private constructor accepting a list of path patterns.
-	 */
-	private PatternsRequestCondition(List<PathPattern> patterns, PathPatternParser patternParser) {
-		this.patterns = toSortedSet(patterns.stream());
-		this.parser = patternParser;
-	}
-
 	public Set<PathPattern> getPatterns() {
 		return this.patterns;
 	}
@@ -145,10 +117,7 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 		else if (!other.patterns.isEmpty()) {
 			combined.addAll(other.patterns);
 		}
-		else {
-			combined.add(this.parser.parse(""));
-		}
-		return new PatternsRequestCondition(combined, this.parser);
+		return new PatternsRequestCondition(combined);
 	}
 
 	/**
@@ -167,7 +136,7 @@ public final class PatternsRequestCondition extends AbstractRequestCondition<Pat
 		}
 		SortedSet<PathPattern> matches = getMatchingPatterns(exchange);
 		return matches.isEmpty() ? null :
-				new PatternsRequestCondition(new ArrayList<PathPattern>(matches), this.parser);
+				new PatternsRequestCondition(matches);
 	}
 
 	/**

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<RequestMapping
 		public RequestMappingInfo build() {
 			RequestedContentTypeResolver contentTypeResolver = this.options.getContentTypeResolver();
 
-			PatternsRequestCondition patternsCondition = new PatternsRequestCondition(
-					this.paths, this.options.getPatternParser());
+			PathPatternParser parser = this.options.getPatternParser() != null ?
+					this.options.getPatternParser() : new PathPatternParser();
+			PatternsRequestCondition patternsCondition = new PatternsRequestCondition(parse(this.paths, parser));
 
 			return new RequestMappingInfo(this.mappingName, patternsCondition,
 					new RequestMethodsRequestCondition(methods),
@@ -489,6 +495,18 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 					new ProducesRequestCondition(this.produces, this.headers, contentTypeResolver),
 					this.customCondition);
 		}
+
+		private static List<PathPattern> 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());
+		}
 	}
 
 

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

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<PathPattern> 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();