From f78bef906d1f79a2949da11b2d170ee3a2551f88 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 28 Aug 2012 16:09:46 -0400 Subject: [PATCH] Improve no-match handling for @RequestMapping methods Issue: SPR-9603 --- .../RequestMappingInfoHandlerMapping.java | 81 ++++++++++++++----- ...RequestMappingInfoHandlerMappingTests.java | 54 +++++++++---- 2 files changed, 97 insertions(+), 38 deletions(-) diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java index a98b2db2b55..b2a182a67da 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java +++ b/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 patterns = info.getPatternsCondition().getPatterns(); String bestPattern = patterns.isEmpty() ? lookupPath : patterns.iterator().next(); request.setAttribute(BEST_MATCHING_PATTERN_ATTRIBUTE, bestPattern); - + Map 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 requestMappingInfos, - String lookupPath, - HttpServletRequest request) throws ServletException { + protected HandlerMethod handleNoMatch(Set requestMappingInfos, + String lookupPath, HttpServletRequest request) throws ServletException { + Set allowedMethods = new HashSet(6); - Set consumableMediaTypes = new HashSet(); - Set producibleMediaTypes = new HashSet(); + + Set patternMatches = new HashSet(); + Set patternAndMethodMatches = new HashSet(); + 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 consumableMediaTypes; + Set 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 getConsumableMediaTypes(HttpServletRequest request, Set partialMatches) { + Set result = new HashSet(); + for (RequestMappingInfo partialMatch : partialMatches) { + if (partialMatch.getConsumesCondition().getMatchingCondition(request) == null) { + result.addAll(partialMatch.getConsumesCondition().getConsumableMediaTypes()); + } + } + return result; + } + + private Set getProdicubleMediaTypes(HttpServletRequest request, Set partialMatches) { + Set result = new HashSet(); + for (RequestMappingInfo partialMatch : partialMatches) { + if (partialMatch.getProducesCondition().getMatchingCondition(request) == null) { + result.addAll(partialMatch.getProducesCondition().getProducibleMediaTypes()); + } + } + return result; + } + } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java index d6562766e76..1fe3a892aa7 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMappingTests.java +++ b/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 uriVariables = + Map uriVariables = (Map) 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 { } } } - + } \ No newline at end of file