Empty path mapping behaves consistently

rstoyanchev · rstoyanchev · commit d1f888a7a963 · 2019-04-09T15:03:43.000-04:00
An empty path mapping in an @RequestMapping now consistently matches to empty paths regardless of whether there are both type and method level, annotations, or method-level only. Closes gh-22543
diff --git a/spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnection.java b/spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnection.java
@@ -63,6 +63,7 @@ public final class MockMvcWebConnection implements WebConnection {
 
 	private final MockMvc mockMvc;
 
+	@Nullable
 	private final String contextPath;
 
 	private WebClient webClient;
@@ -91,7 +92,7 @@ public MockMvcWebConnection(MockMvc mockMvc, WebClient webClient) {
 	 * @param webClient the {@link WebClient} to use (never {@code null})
 	 * @param contextPath the contextPath to use
 	 */
-	public MockMvcWebConnection(MockMvc mockMvc, WebClient webClient, String contextPath) {
+	public MockMvcWebConnection(MockMvc mockMvc, WebClient webClient, @Nullable String contextPath) {
 		Assert.notNull(mockMvc, "MockMvc must not be null");
 		Assert.notNull(webClient, "WebClient must not be null");
 		validateContextPath(contextPath);
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/ForwardController.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/ForwardController.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,7 +28,7 @@ public class ForwardController {
 
 	@RequestMapping("/forward")
 	public String forward() {
-		return "forward:/";
+		return "forward:/a";
 	}
 
 }
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/HelloController.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/HelloController.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,7 +28,7 @@
 @RestController
 public class HelloController {
 
-	@RequestMapping
+	@RequestMapping("/a")
 	public String header(HttpServletRequest request) {
 		return "hello";
 	}
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcConnectionBuilderSupportTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcConnectionBuilderSupportTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -151,7 +151,7 @@ static class Config {
 		@RestController
 		static class ContextPathController {
 
-			@RequestMapping
+			@RequestMapping("/def")
 			public String contextPath(HttpServletRequest request) {
 				return request.getContextPath();
 			}
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebClientBuilderTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebClientBuilderTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -157,7 +157,7 @@ static class Config {
 		@RestController
 		static class ContextPathController {
 
-			@RequestMapping
+			@RequestMapping("/test")
 			public String contextPath(HttpServletRequest request) {
 				return "mvc";
 			}
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnectionTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/MockMvcWebConnectionTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@
 
 import java.io.IOException;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import com.gargoylesoftware.htmlunit.Page;
 import com.gargoylesoftware.htmlunit.WebClient;
 import org.junit.Test;
@@ -44,7 +45,7 @@ public class MockMvcWebConnectionTests {
 
 	@Test
 	public void contextPathNull() throws IOException {
-		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, this.webClient));
+		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, this.webClient, null));
 		Page page = this.webClient.getPage("http://localhost/context/a");
 		assertThat(page.getWebResponse().getStatusCode(), equalTo(200));
 	}
@@ -59,8 +60,21 @@ public void contextPathExplicit() throws IOException {
 	@Test
 	public void contextPathEmpty() throws IOException {
 		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, this.webClient, ""));
-		Page page = this.webClient.getPage("http://localhost/context/a");
-		assertThat(page.getWebResponse().getStatusCode(), equalTo(200));
+		try {
+			this.webClient.getPage("http://localhost/context/a");
+			fail("Empty context path (root context) should not match to a URL with a context path");
+		}
+		catch (FailingHttpStatusCodeException ex) {
+			assertEquals(404, ex.getStatusCode());
+		}
+		this.webClient.setWebConnection(new MockMvcWebConnection(this.mockMvc, this.webClient));
+		try {
+			this.webClient.getPage("http://localhost/context/a");
+			fail("No context is the same providing an empty context path");
+		}
+		catch (FailingHttpStatusCodeException ex) {
+			assertEquals(404, ex.getStatusCode());
+		}
 	}
 
 	@Test
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/webdriver/MockMvcHtmlUnitDriverBuilderTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/webdriver/MockMvcHtmlUnitDriverBuilderTests.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -145,7 +145,7 @@ static class Config {
 		@RestController
 		static class ContextPathController {
 
-			@RequestMapping
+			@RequestMapping("/test")
 			public String contextPath(HttpServletRequest request) {
 				return EXPECTED_BODY;
 			}
diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/RedirectTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/standalone/RedirectTests.java
@@ -115,7 +115,7 @@ public String getPerson(@PathVariable String name, Model model) {
 			return "persons/index";
 		}
 
-		@PostMapping
+		@PostMapping("/persons")
 		public String save(@Valid Person person, Errors errors, RedirectAttributes redirectAttrs) {
 			if (errors.hasErrors()) {
 				return "persons/add";
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
@@ -16,9 +16,9 @@
 
 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.Iterator;
 import java.util.List;
 import java.util.Set;
@@ -29,6 +29,7 @@
 import org.springframework.lang.Nullable;
 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
@@ -40,6 +41,10 @@
  */
 public final class PatternsRequestCondition extends AbstractRequestCondition<PatternsRequestCondition> {
 
+	private static final SortedSet<PathPattern> EMPTY_PATTERNS =
+			new TreeSet<>(Collections.singleton(new PathPatternParser().parse("")));
+
+
 	private final SortedSet<PathPattern> patterns;
 
 
@@ -55,7 +60,7 @@ public PatternsRequestCondition(PathPattern... patterns) {
 	 * Creates a new instance with the given URL patterns.
 	 */
 	public PatternsRequestCondition(List<PathPattern> patterns) {
-		this(new TreeSet<>(patterns));
+		this(patterns.isEmpty() ? EMPTY_PATTERNS : new TreeSet<>(patterns));
 	}
 
 
@@ -89,19 +94,23 @@ protected String getToStringInfix() {
 	 */
 	@Override
 	public PatternsRequestCondition combine(PatternsRequestCondition other) {
-		List<PathPattern> combined = new ArrayList<>();
+		SortedSet<PathPattern> combined;
 		if (!this.patterns.isEmpty() && !other.patterns.isEmpty()) {
+			combined = new TreeSet<>();
 			for (PathPattern pattern1 : this.patterns) {
 				for (PathPattern pattern2 : other.patterns) {
 					combined.add(pattern1.combine(pattern2));
 				}
 			}
 		}
 		else if (!this.patterns.isEmpty()) {
-			combined.addAll(this.patterns);
+			combined = this.patterns;
 		}
 		else if (!other.patterns.isEmpty()) {
-			combined.addAll(other.patterns);
+			combined = other.patterns;
+		}
+		else {
+			combined = EMPTY_PATTERNS;
 		}
 		return new PatternsRequestCondition(combined);
 	}
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
@@ -140,6 +140,19 @@ public void matchPatternContainsExtension() throws Exception {
 		assertNull(match);
 	}
 
+	@Test // gh-22543
+	public void matchWithEmptyPatterns() {
+		PatternsRequestCondition condition = new PatternsRequestCondition();
+		assertEquals(new PatternsRequestCondition(this.parser.parse("")), condition);
+		assertNotNull(condition.getMatchingCondition(MockServerWebExchange.from(get(""))));
+		assertNull(condition.getMatchingCondition(MockServerWebExchange.from(get("/anything"))));
+
+		condition = condition.combine(new PatternsRequestCondition());
+		assertEquals(new PatternsRequestCondition(this.parser.parse("")), condition);
+		assertNotNull(condition.getMatchingCondition(MockServerWebExchange.from(get(""))));
+		assertNull(condition.getMatchingCondition(MockServerWebExchange.from(get("/anything"))));
+	}
+
 	@Test
 	public void compareToConsistentWithEquals() throws Exception {
 		PatternsRequestCondition c1 = createPatternsCondition("/foo*");
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
@@ -34,6 +34,7 @@
 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.PathPatternParser;
 import org.springframework.web.util.pattern.PatternParseException;
 
 import static java.util.Arrays.asList;
@@ -61,7 +62,8 @@ public class RequestMappingInfoTests {
 	public void createEmpty() {
 		RequestMappingInfo info = paths().build();
 
-		assertEquals(0, info.getPatternsCondition().getPatterns().size());
+		PathPattern emptyPattern = (new PathPatternParser()).parse("");
+		assertEquals(Collections.singleton(emptyPattern), info.getPatternsCondition().getPatterns());
 		assertEquals(0, info.getMethodsCondition().getMethods().size());
 		assertEquals(true, info.getConsumesCondition().isEmpty());
 		assertEquals(true, info.getProducesCondition().isEmpty());
diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java
@@ -253,14 +253,12 @@ public void handleMatchBestMatchingPatternAttribute() {
 		assertSame(handlerMethod, mapped);
 	}
 
-	@Test
+	@Test // gh-22543
 	public void handleMatchBestMatchingPatternAttributeNoPatternsDefined() {
-		RequestMappingInfo key = paths().build();
-		ServerWebExchange exchange = MockServerWebExchange.from(get("/1/2"));
-		this.handlerMapping.handleMatch(key, handlerMethod, exchange);
-
-		PathPattern bestMatch = (PathPattern) exchange.getAttributes().get(BEST_MATCHING_PATTERN_ATTRIBUTE);
-		assertEquals("/1/2", bestMatch.getPatternString());
+		ServerWebExchange exchange = MockServerWebExchange.from(get(""));
+		this.handlerMapping.handleMatch(paths().build(), handlerMethod, exchange);
+		PathPattern pattern = (PathPattern) exchange.getAttributes().get(BEST_MATCHING_PATTERN_ATTRIBUTE);
+		assertEquals("", pattern.getPatternString());
 	}
 
 	@Test
diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/PatternsRequestCondition.java
@@ -135,6 +135,9 @@ private PatternsRequestCondition(Set<String> patterns, PatternsRequestCondition
 
 
 	private static Set<String> prependLeadingSlash(Collection<String> patterns) {
+		if (patterns.isEmpty()) {
+			return Collections.singleton("");
+		}
 		Set<String> result = new LinkedHashSet<>(patterns.size());
 		for (String pattern : patterns) {
 			if (StringUtils.hasLength(pattern) && !pattern.startsWith("/")) {
diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/PatternsRequestConditionTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/PatternsRequestConditionTests.java
@@ -115,9 +115,7 @@ public void matchSuffixPattern() {
 		assertEquals("/{foo}", match.getPatterns().iterator().next());
 	}
 
-	// SPR-8410
-
-	@Test
+	@Test // SPR-8410
 	public void matchSuffixPatternUsingFileExtensions() {
 		String[] patterns = new String[] {"/jobs/{jobName}"};
 		List<String> extensions = Arrays.asList("json");
@@ -183,6 +181,19 @@ public void matchPatternContainsExtension() {
 		assertNull(match);
 	}
 
+	@Test // gh-22543
+	public void matchWithEmptyPatterns() {
+		PatternsRequestCondition condition = new PatternsRequestCondition();
+		assertEquals(new PatternsRequestCondition(""), condition);
+		assertNotNull(condition.getMatchingCondition(new MockHttpServletRequest("GET", "")));
+		assertNull(condition.getMatchingCondition(new MockHttpServletRequest("GET", "/anything")));
+
+		condition = condition.combine(new PatternsRequestCondition());
+		assertEquals(new PatternsRequestCondition(""), condition);
+		assertNotNull(condition.getMatchingCondition(new MockHttpServletRequest("GET", "")));
+		assertNull(condition.getMatchingCondition(new MockHttpServletRequest("GET", "/anything")));
+	}
+
 	@Test
 	public void compareEqualPatterns() {
 		PatternsRequestCondition c1 = new PatternsRequestCondition("/foo*");
diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java
@@ -295,14 +295,12 @@ public void handleMatchBestMatchingPatternAttribute() {
 		assertEquals("/{path1}/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
-	@Test
+	@Test // gh-22543
 	public void handleMatchBestMatchingPatternAttributeNoPatternsDefined() {
-		RequestMappingInfo key = RequestMappingInfo.paths().build();
-		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
-
-		this.handlerMapping.handleMatch(key, "/1/2", request);
-
-		assertEquals("/1/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
+		String path = "";
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", path);
+		this.handlerMapping.handleMatch(RequestMappingInfo.paths().build(), path, request);
+		assertEquals(path, request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
 	@Test
diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoTests.java
@@ -49,7 +49,7 @@ public class RequestMappingInfoTests {
 	public void createEmpty() {
 		RequestMappingInfo info = paths().build();
 
-		assertEquals(0, info.getPatternsCondition().getPatterns().size());
+		assertEquals(Collections.singleton(""), info.getPatternsCondition().getPatterns()); // gh-22543
 		assertEquals(0, info.getMethodsCondition().getMethods().size());
 		assertEquals(true, info.getConsumesCondition().isEmpty());
 		assertEquals(true, info.getProducesCondition().isEmpty());
diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java
@@ -796,13 +796,19 @@ public void equivalentMappingsWithSameMethodName() {
 			.hasMessageContaining("Ambiguous mapping");
 	}
 
-	@Test
+	@Test // gh-22543
 	public void unmappedPathMapping() throws Exception {
 		initServletWithControllers(UnmappedPathController.class);
 
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bogus-unmapped");
 		MockHttpServletResponse response = new MockHttpServletResponse();
 		getServlet().service(request, response);
+		assertEquals(404, response.getStatus());
+
+		request = new MockHttpServletRequest("GET", "");
+		response = new MockHttpServletResponse();
+		getServlet().service(request, response);
+		assertEquals(200, response.getStatus());
 		assertEquals("get", response.getContentAsString());
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright 2002-2016 the original author or authors.`
	`2`	`+ * Copyright 2002-2019 the original author or authors.`
`3`	`3`	`*`
`4`	`4`	`* Licensed under the Apache License, Version 2.0 (the "License");`
`5`	`5`	`* you may not use this file except in compliance with the License.`
`@@ -28,7 +28,7 @@ public class ForwardController {`
`28`	`28`
`29`	`29`	`@RequestMapping("/forward")`
`30`	`30`	`public String forward() {`
`31`		`- return "forward:/";`
	`31`	`+ return "forward:/a";`
`32`	`32`	`}`
`33`	`33`
`34`	`34`	`}`
Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ public String getPerson(@PathVariable String name, Model model) {`
`115`	`115`	`return "persons/index";`
`116`	`116`	`}`
`117`	`117`
`118`		`- @PostMapping`
	`118`	`+ @PostMapping("/persons")`
`119`	`119`	`public String save(@Valid Person person, Errors errors, RedirectAttributes redirectAttrs) {`
`120`	`120`	`if (errors.hasErrors()) {`
`121`	`121`	`return "persons/add";`