Improve no-match handling for @RequestMapping methods

Issue: SPR-9603

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java

@@ -54,10 +54,10 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe
 	}
 
 	/**
-	 * Check if the given RequestMappingInfo matches the current request and 
-	 * return a (potentially new) instance with conditions that match the 
+	 * Check if the given RequestMappingInfo matches the current request and
+	 * return a (potentially new) instance with conditions that match the
 	 * current request -- for example with a subset of URL patterns.
-	 * @returns an info in case of a match; or {@code null} otherwise. 
+	 * @returns an info in case of a match; or {@code null} otherwise.
 	 */
 	@Override
 	protected RequestMappingInfo getMatchingMapping(RequestMappingInfo info, HttpServletRequest request) {
@@ -88,7 +88,7 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe
 		Set<String> patterns = info.getPatternsCondition().getPatterns();
 		String bestPattern = patterns.isEmpty() ? lookupPath : patterns.iterator().next();
 		request.setAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE, bestPattern);
-		
+
 		Map<String, String> uriTemplateVariables = getPathMatcher().extractUriTemplateVariables(bestPattern, lookupPath);
 		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVariables);
 
@@ -101,40 +101,57 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe
 	/**
 	 * Iterate all RequestMappingInfos once again, look if any match by URL at
 	 * least and raise exceptions accordingly.
-	 * 
-	 * @throws HttpRequestMethodNotSupportedException 
+	 *
+	 * @throws HttpRequestMethodNotSupportedException
 	 * 		if there are matches by URL but not by HTTP method
-	 * @throws HttpMediaTypeNotAcceptableException 
+	 * @throws HttpMediaTypeNotAcceptableException
 	 * 		if there are matches by URL but not by consumable media types
-	 * @throws HttpMediaTypeNotAcceptableException 
+	 * @throws HttpMediaTypeNotAcceptableException
 	 * 		if there are matches by URL but not by producible media types
 	 */
 	@Override
-	protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> requestMappingInfos, 
-										  String lookupPath, 
-										  HttpServletRequest request) throws ServletException {
+	protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> requestMappingInfos,
+			String lookupPath, HttpServletRequest request) throws ServletException {
+
 		Set<String> allowedMethods = new HashSet<String>(6);
-		Set<MediaType> consumableMediaTypes = new HashSet<MediaType>();
-		Set<MediaType> producibleMediaTypes = new HashSet<MediaType>();
+
+		Set<RequestMappingInfo> patternMatches = new HashSet<RequestMappingInfo>();
+		Set<RequestMappingInfo> patternAndMethodMatches = new HashSet<RequestMappingInfo>();
+
 		for (RequestMappingInfo info : requestMappingInfos) {
 			if (info.getPatternsCondition().getMatchingCondition(request) != null) {
-				if (info.getMethodsCondition().getMatchingCondition(request) == null) {
+				patternMatches.add(info);
+				if (info.getMethodsCondition().getMatchingCondition(request) != null) {
+					patternAndMethodMatches.add(info);
+				}
+				else {
 					for (RequestMethod method : info.getMethodsCondition().getMethods()) {
 						allowedMethods.add(method.name());
 					}
 				}
-				if (info.getConsumesCondition().getMatchingCondition(request) == null) {
-					consumableMediaTypes.addAll(info.getConsumesCondition().getConsumableMediaTypes());
-				}
-				if (info.getProducesCondition().getMatchingCondition(request) == null) {
-					producibleMediaTypes.addAll(info.getProducesCondition().getProducibleMediaTypes());
-				}
 			}
 		}
-		if (!allowedMethods.isEmpty()) {
+
+		if (patternMatches.isEmpty()) {
+			return null;
+		}
+		else if (patternAndMethodMatches.isEmpty() && !allowedMethods.isEmpty()) {
 			throw new HttpRequestMethodNotSupportedException(request.getMethod(), allowedMethods);
 		}
-		else if (!consumableMediaTypes.isEmpty()) {
+
+		Set<MediaType> consumableMediaTypes;
+		Set<MediaType> producibleMediaTypes;
+
+		if (patternAndMethodMatches.isEmpty()) {
+			consumableMediaTypes = getConsumableMediaTypes(request, patternMatches);
+			producibleMediaTypes = getProdicubleMediaTypes(request, patternMatches);
+		}
+		else {
+			consumableMediaTypes = getConsumableMediaTypes(request, patternAndMethodMatches);
+			producibleMediaTypes = getProdicubleMediaTypes(request, patternAndMethodMatches);
+		}
+
+		if (!consumableMediaTypes.isEmpty()) {
 			MediaType contentType = null;
 			if (StringUtils.hasLength(request.getContentType())) {
 				contentType = MediaType.parseMediaType(request.getContentType());
@@ -149,4 +166,24 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe
 		}
 	}
 
+	private Set<MediaType> getConsumableMediaTypes(HttpServletRequest request, Set<RequestMappingInfo> partialMatches) {
+		Set<MediaType> result = new HashSet<MediaType>();
+		for (RequestMappingInfo partialMatch : partialMatches) {
+			if (partialMatch.getConsumesCondition().getMatchingCondition(request) == null) {
+				result.addAll(partialMatch.getConsumesCondition().getConsumableMediaTypes());
+			}
+		}
+		return result;
+	}
+
+	private Set<MediaType> getProdicubleMediaTypes(HttpServletRequest request, Set<RequestMappingInfo> partialMatches) {
+		Set<MediaType> result = new HashSet<MediaType>();
+		for (RequestMappingInfo partialMatch : partialMatches) {
+			if (partialMatch.getProducesCondition().getMatchingCondition(request) == null) {
+				result.addAll(partialMatch.getProducesCondition().getProducibleMediaTypes());
+			}
+		}
+		return result;
+	}
+
 }

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java

@@ -131,7 +131,7 @@ public class RequestMappingInfoHandlerMappingTests {
 		HandlerMethod hm = (HandlerMethod) this.mapping.getHandler(request).getHandler();
 		assertEquals(this.fooParamMethod.getMethod(), hm.getMethod());
 	}
-	
+
 	@Test
 	public void requestMethodNotAllowed() throws Exception {
 		try {
@@ -143,7 +143,18 @@ public class RequestMappingInfoHandlerMappingTests {
 			assertArrayEquals("Invalid supported methods", new String[]{"GET", "HEAD"}, ex.getSupportedMethods());
 		}
 	}
-	
+
+	// SPR-9603
+
+	@Test(expected=HttpMediaTypeNotAcceptableException.class)
+	public void requestMethodMatchFalsePositive() throws Exception {
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/users");
+		request.addHeader("Accept", "application/xml");
+
+		this.mapping.registerHandler(new UserController());
+		this.mapping.getHandler(request);
+	}
+
 	@Test
 	public void mediaTypeNotSupported() throws Exception {
 		testMediaTypeNotSupported("/person/1");
@@ -159,11 +170,11 @@ public class RequestMappingInfoHandlerMappingTests {
 			fail("HttpMediaTypeNotSupportedException expected");
 		}
 		catch (HttpMediaTypeNotSupportedException ex) {
-			assertEquals("Invalid supported consumable media types", 
+			assertEquals("Invalid supported consumable media types",
 					Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
 		}
 	}
-	
+
 	@Test
 	public void mediaTypeNotAccepted() throws Exception {
 		testMediaTypeNotAccepted("/persons");
@@ -179,7 +190,7 @@ public class RequestMappingInfoHandlerMappingTests {
 			fail("HttpMediaTypeNotAcceptableException expected");
 		}
 		catch (HttpMediaTypeNotAcceptableException ex) {
-			assertEquals("Invalid supported producible media types", 
+			assertEquals("Invalid supported producible media types",
 					Arrays.asList(new MediaType("application", "xml")), ex.getSupportedMediaTypes());
 		}
 	}
@@ -191,12 +202,12 @@ public class RequestMappingInfoHandlerMappingTests {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 		String lookupPath = new UrlPathHelper().getLookupPathForRequest(request);
 		this.mapping.handleMatch(key, lookupPath, request);
-		
+
 		@SuppressWarnings("unchecked")
-		Map<String, String> uriVariables = 
+		Map<String, String> uriVariables =
 			(Map<String, String>) request.getAttribute(
 					HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE);
-		
+
 		assertNotNull(uriVariables);
 		assertEquals("1", uriVariables.get("path1"));
 		assertEquals("2", uriVariables.get("path2"));
@@ -209,7 +220,7 @@ public class RequestMappingInfoHandlerMappingTests {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 
 		this.mapping.handleMatch(key, "/1/2", request);
-		
+
 		assertEquals("/{path1}/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
@@ -220,7 +231,7 @@ public class RequestMappingInfoHandlerMappingTests {
 		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/1/2");
 
 		this.mapping.handleMatch(key, "/1/2", request);
-		
+
 		assertEquals("/1/2", request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE));
 	}
 
@@ -237,10 +248,10 @@ public class RequestMappingInfoHandlerMappingTests {
 		request.addHeader("Accept", "application/json");
 		this.mapping.getHandler(request);
 
-		assertNull("Negated expression should not be listed as a producible type", 
+		assertNull("Negated expression should not be listed as a producible type",
 				request.getAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE));
 	}
-	
+
 	@Test
 	public void mappedInterceptors() throws Exception {
 		String path = "/foo";
@@ -261,14 +272,13 @@ public class RequestMappingInfoHandlerMappingTests {
 		assertNull(chain);
 	}
 
-	@SuppressWarnings("unused")
 	@Controller
 	private static class Handler {
 
 		@RequestMapping(value = "/foo", method = RequestMethod.GET)
 		public void foo() {
 		}
-		
+
 		@RequestMapping(value = "/foo", method = RequestMethod.GET, params="p")
 		public void fooParam() {
 		}
@@ -301,12 +311,24 @@ public class RequestMappingInfoHandlerMappingTests {
 		}
 	}
 
+	@Controller
+	private static class UserController {
+
+		@RequestMapping(value = "/users", method = RequestMethod.GET, produces = "application/json")
+		public void getUser() {
+		}
+
+		@RequestMapping(value = "/users", method = RequestMethod.PUT)
+		public void saveUser() {
+		}
+	}
+
 	private static class TestRequestMappingInfoHandlerMapping extends RequestMappingInfoHandlerMapping {
 
 		public void registerHandler(Object handler) {
 			super.detectHandlerMethods(handler);
 		}
-		
+
 		@Override
 		protected boolean isHandler(Class<?> beanType) {
 			return AnnotationUtils.findAnnotation(beanType, RequestMapping.class) != null;
@@ -329,5 +351,5 @@ public class RequestMappingInfoHandlerMappingTests {
 			}
 		}
 	}
-	
+
 }