From e9208981a642f1e98863446174ce5285f5ecf993 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Mon, 30 Jan 2012 19:52:20 -0500 Subject: [PATCH] SPR-9062 Fix bug with ambiguous path and HTTP method request mappings A direct path match with incorrect HTTP request method was causing another request mapping with a pattern and a correct HTTP method to be ignored. The bug affects the new @MVC support classes (i.e. RequestMappingHandlerMapping). --- .../resources/changelog.txt | 1 + .../handler/AbstractHandlerMethodMapping.java | 31 ++++++++++++------- ...nnotationControllerHandlerMethodTests.java | 27 ++++++++++++++++ 3 files changed, 48 insertions(+), 11 deletions(-) diff --git a/build-spring-framework/resources/changelog.txt b/build-spring-framework/resources/changelog.txt index ce2299703ba..c61372a70e7 100644 --- a/build-spring-framework/resources/changelog.txt +++ b/build-spring-framework/resources/changelog.txt @@ -28,6 +28,7 @@ Changes in version 3.1.1 (2012-02-06) * preserve quotes in MediaType parameters * make flash attributes available in the model of ParameterizableViewController and UrlFilenameViewController * add property to RedirectView to disable expanding URI variables in redirect URL +* fix request mapping bug involving direct vs pattern path matches with HTTP methods Changes in version 3.1 GA (2011-12-12) -------------------------------------- diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java index a47814c501b..be8fd6d6107 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java @@ -18,12 +18,14 @@ package org.springframework.web.servlet.handler; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; + import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -237,18 +239,16 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap * @see #handleNoMatch(Set, String, HttpServletRequest) */ protected HandlerMethod lookupHandlerMethod(String lookupPath, HttpServletRequest request) throws Exception { - List mappings = urlMap.get(lookupPath); - if (mappings == null) { - mappings = new ArrayList(handlerMethods.keySet()); - } - List matches = new ArrayList(); - - for (T mapping : mappings) { - T match = getMatchingMapping(mapping, request); - if (match != null) { - matches.add(new Match(match, handlerMethods.get(mapping))); - } + + List directPathMatches = this.urlMap.get(lookupPath); + if (directPathMatches != null) { + addMatchingMappings(directPathMatches, matches, request); + } + + if (matches.isEmpty()) { + // No choice but to go through all mappings + addMatchingMappings(this.handlerMethods.keySet(), matches, request); } if (!matches.isEmpty()) { @@ -279,6 +279,15 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } } + private void addMatchingMappings(Collection mappings, List matches, HttpServletRequest request) { + for (T mapping : mappings) { + T match = getMatchingMapping(mapping, request); + if (match != null) { + matches.add(new Match(match, handlerMethods.get(mapping))); + } + } + } + /** * Check if a mapping matches the current request and return a (potentially * new) mapping with conditions relevant to the current request. diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index 879961db773..ef81648a078 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -1182,6 +1182,19 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl assertEquals("myParam-42", response.getContentAsString()); } + // SPR-9062 + + @Test + public void ambiguousPathAndRequestMethod() throws Exception { + initServletWithControllers(AmbiguousPathAndRequestMethodController.class); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bug/EXISTING"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertEquals(200, response.getStatus()); + assertEquals("Pattern", response.getContentAsString()); + } + @Test public void bridgeMethods() throws Exception { initServletWithControllers(TestControllerImpl.class); @@ -2549,6 +2562,20 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl } } + @Controller + static class AmbiguousPathAndRequestMethodController { + + @RequestMapping(value = "/bug/EXISTING", method = RequestMethod.POST) + public void directMatch(Writer writer) throws IOException { + writer.write("Direct"); + } + + @RequestMapping(value = "/bug/{type}", method = RequestMethod.GET) + public void patternMatch(Writer writer) throws IOException { + writer.write("Pattern"); + } + } + @Controller @RequestMapping("/test*") public static class BindingCookieValueController {