From 83ce399c4703a44823ba78edab6b65ded4c15b93 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 15 Apr 2011 06:42:18 +0000 Subject: [PATCH] SPR-8248 --- .../handler/AbstractHandlerMethodMapping.java | 28 +++++++------ .../RequestMappingHandlerMethodMapping.java | 5 ++- .../support/ViewMethodReturnValueHandler.java | 6 +-- .../condition/ConsumesRequestCondition.java | 11 ++++- .../handler/HandlerMethodMappingTests.java | 2 +- .../ServletAnnotationControllerTests.java | 1 + ...estMappingHandlerMethodDetectionTests.java | 40 ++++++++++++++----- .../annotation/ServletHandlerMethodTests.java | 28 ++++++------- 8 files changed, 78 insertions(+), 43 deletions(-) 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 09819b692c..25c3918920 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 @@ -97,29 +97,30 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap /** * Detect and register handler methods for the specified handler. */ - private void detectHandlerMethods(final String handlerName) { - Class handlerType = getApplicationContext().getType(handlerName); + private void detectHandlerMethods(final String beanName) { + Class handlerType = getApplicationContext().getType(beanName); Set methods = HandlerMethodSelector.selectMethods(handlerType, new MethodFilter() { public boolean matches(Method method) { - return getKeyForMethod(method) != null; + return getKeyForMethod(beanName, method) != null; } }); for (Method method : methods) { - T key = getKeyForMethod(method); - HandlerMethod handlerMethod = new HandlerMethod(handlerName, getApplicationContext(), method); + T key = getKeyForMethod(beanName, method); + HandlerMethod handlerMethod = new HandlerMethod(beanName, getApplicationContext(), method); registerHandlerMethod(key, handlerMethod); } } /** - * Provides a lookup key for the given method. A method for which no key can be determined is + * Provides a lookup key for the given bean method. A method for which no key can be determined is * not considered a handler method. * + * @param beanName the name of the bean the method belongs to * @param method the method to create a key for * @return the lookup key, or {@code null} if the method has none */ - protected abstract T getKeyForMethod(Method method); + protected abstract T getKeyForMethod(String beanName, Method method); /** * Registers a {@link HandlerMethod} under the given key. @@ -133,12 +134,13 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap Assert.notNull(handlerMethod, "'handlerMethod' must not be null"); HandlerMethod mappedHandlerMethod = handlerMethods.get(key); if (mappedHandlerMethod != null && !mappedHandlerMethod.equals(handlerMethod)) { - throw new IllegalStateException("Cannot map " + handlerMethod + " to \"" + key + "\": There is already " - + mappedHandlerMethod + " mapped."); + throw new IllegalStateException("Ambiguous mapping found. Cannot map '" + handlerMethod.getBean() + + "' bean method \n" + handlerMethod + "\nto " + key + ": There is already '" + + mappedHandlerMethod.getBean() + "' bean method\n" + mappedHandlerMethod + " mapped."); } handlerMethods.put(key, handlerMethod); - if (logger.isDebugEnabled()) { - logger.debug("Mapped \"" + key + "\" onto " + handlerMethod); + if (logger.isInfoEnabled()) { + logger.info("Mapped \"" + key + "\" onto " + handlerMethod); } } @@ -149,14 +151,14 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap return null; } if (logger.isDebugEnabled()) { - logger.debug("Looking up handler method for [" + key + "]"); + logger.debug("Looking up handler method with key [" + key + "]"); } HandlerMethod handlerMethod = lookupHandlerMethod(key, request); if (logger.isDebugEnabled()) { if (handlerMethod != null) { - logger.debug("Returning [" + handlerMethod + "] as best match for [" + key + "]"); + logger.debug("Returning handler method [" + handlerMethod + "]"); } else { logger.debug("Did not find handler method for [" + key + "]"); diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodMapping.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodMapping.java index c602fd87ae..5300a6c48c 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodMapping.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodMapping.java @@ -161,16 +161,17 @@ public class RequestMappingHandlerMethodMapping extends AbstractHandlerMethodMap * Type-level {@link RequestMapping @RequestMapping} annotations are also detected and their * attributes combined with method-level {@link RequestMapping @RequestMapping} attributes. * + * @param beanName the name of the bean the method belongs to * @param method the method to create a key for * @return the key, or {@code null} * @see RequestKey#combine(RequestKey, PathMatcher) */ @Override - protected RequestKey getKeyForMethod(Method method) { + protected RequestKey getKeyForMethod(String beanName, Method method) { RequestMapping annotation = AnnotationUtils.findAnnotation(method, RequestMapping.class); if (annotation != null) { RequestKey methodKey = RequestKey.createFromRequestMapping(annotation); - RequestMapping typeAnnot = AnnotationUtils.findAnnotation(method.getDeclaringClass(), RequestMapping.class); + RequestMapping typeAnnot = getApplicationContext().findAnnotationOnBean(beanName, RequestMapping.class); if (typeAnnot != null) { RequestKey typeKey = RequestKey.createFromRequestMapping(typeAnnot); return typeKey.combine(methodKey, pathMatcher); diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/ViewMethodReturnValueHandler.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/ViewMethodReturnValueHandler.java index 2db1978c1b..76efe6fee5 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/ViewMethodReturnValueHandler.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/annotation/support/ViewMethodReturnValueHandler.java @@ -25,9 +25,9 @@ import org.springframework.web.servlet.View; /** * Handles return values that are of type {@link View} or {@link String} (i.e. a logical view name). - *

Since {@link String} return value can be interpeted in multiple ways, this resolver should be ordered - * after return value handlers that recognize annotated return values such as the - * {@link ModelAttributeMethodProcessor} and the {@link RequestResponseBodyMethodProcessor}. + *

Since a {@link String} return value may handled in different ways, especially in combination with method + * annotations, this handler should be registered after return value handlers that look for method annotations + * such as the {@link ModelAttributeMethodProcessor} and the {@link RequestResponseBodyMethodProcessor}. * * @author Rossen Stoyanchev * @since 3.1 diff --git a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/condition/ConsumesRequestCondition.java b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/condition/ConsumesRequestCondition.java index 4fb4e18ec7..5e9cf0d993 100644 --- a/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/condition/ConsumesRequestCondition.java +++ b/org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/mvc/method/condition/ConsumesRequestCondition.java @@ -60,4 +60,13 @@ class ConsumesRequestCondition extends AbstractRequestCondition { return false; } -} + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + if (mediaType != null) { + builder.append(mediaType.toString()); + } + return builder.toString(); + } + +} \ No newline at end of file diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java index 4f02a0f035..35011ed822 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/handler/HandlerMethodMappingTests.java @@ -103,7 +103,7 @@ public class HandlerMethodMappingTests { } @Override - protected String getKeyForMethod(Method method) { + protected String getKeyForMethod(String beanName, Method method) { String methodName = method.getName(); return methodName.startsWith("handler") ? methodName : null; } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java index 6cfa55ea77..c3f486ec5e 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/annotation/ServletAnnotationControllerTests.java @@ -1157,6 +1157,7 @@ public class ServletAnnotationControllerTests { MockHttpServletResponse response = new MockHttpServletResponse(); try { servlet.service(request, response); + fail("Didn't fail with due to ambiguous method mapping"); } catch (NestedServletException ex) { assertTrue(ex.getCause() instanceof IllegalStateException); diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodDetectionTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodDetectionTests.java index 37f3d8d33a..f9638a5560 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodDetectionTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodDetectionTests.java @@ -63,7 +63,8 @@ public class RequestMappingHandlerMethodDetectionTests { { new MappingInterfaceController(), false}, { new MappingAbstractClassController(), false}, { new ParameterizedInterfaceController(), false }, - { new MappingParameterizedInterfaceController(), false }, + { new MappingParameterizedInterfaceController(), false }, + { new MappingClassController(), false }, { new MappingAbstractClassController(), true}, { new PlainController(), true} }); @@ -80,7 +81,7 @@ public class RequestMappingHandlerMethodDetectionTests { @Test public void detectAndMapHandlerMethod() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/handle"); + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/type/handle"); TestRequestMappingHandlerMethodMapping mapping = createHandlerMapping(handler.getClass(), useAutoProxy); HandlerMethod handlerMethod = mapping.getHandlerInternal(request); @@ -110,55 +111,76 @@ public class RequestMappingHandlerMethodDetectionTests { } } + /* Annotation on interface method */ + @Controller public interface MappingInterface { @RequestMapping(value="/handle", method = RequestMethod.GET) void handle(); } - + @RequestMapping(value="/type") public static class MappingInterfaceController implements MappingInterface { public void handle() { } } - + + /* Annotation on abstract class method */ + @Controller public static abstract class MappingAbstractClass { @RequestMapping(value = "/handle", method = RequestMethod.GET) public abstract void handle(); } - + @RequestMapping(value="/type") public static class MappingAbstractClassController extends MappingAbstractClass { public void handle() { } } + /* Annotation on parameterized controller method */ + @Controller public interface ParameterizedInterface { void handle(T object); } - + @RequestMapping(value="/type") public static class ParameterizedInterfaceController implements ParameterizedInterface { @RequestMapping(value = "/handle", method = RequestMethod.GET) public void handle(TestBean object) { } } + /* Annotation on parameterized interface method */ + @Controller public interface MappingParameterizedInterface { @RequestMapping(value = "/handle", method = RequestMethod.GET) void handle(T object); } - + @RequestMapping(value="/type") public static class MappingParameterizedInterfaceController implements MappingParameterizedInterface { public void handle(TestBean object) { } } + /* Type + method annotations, method in parent class only (SPR-8248) */ + @Controller - public static class PlainController { - public PlainController() { + public static class MappingClass { + @RequestMapping(value = "/handle", method = RequestMethod.GET) + public void handle(TestBean object) { } + } + @RequestMapping(value="/type") + public static class MappingClassController extends MappingClass { + // Method in parent class only + } + /* Annotations on controller class */ + + @Controller + @RequestMapping(value="/type") + public static class PlainController { @RequestMapping(value = "/handle", method = RequestMethod.GET) public void handle() { } diff --git a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletHandlerMethodTests.java b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletHandlerMethodTests.java index 05df5acf92..63b0982925 100644 --- a/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletHandlerMethodTests.java +++ b/org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletHandlerMethodTests.java @@ -16,6 +16,14 @@ package org.springframework.web.servlet.mvc.method.annotation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + import java.beans.PropertyEditorSupport; import java.io.IOException; import java.io.Serializable; @@ -40,6 +48,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; + import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; @@ -53,7 +62,6 @@ import javax.xml.bind.annotation.XmlRootElement; import org.junit.Ignore; import org.junit.Test; - import org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator; import org.springframework.aop.interceptor.SimpleTraceInterceptor; import org.springframework.aop.support.DefaultPointcutAdvisor; @@ -133,9 +141,6 @@ import org.springframework.web.servlet.mvc.annotation.ResponseStatusExceptionRes import org.springframework.web.servlet.mvc.method.annotation.support.ServletWebArgumentResolverAdapter; import org.springframework.web.servlet.mvc.support.DefaultHandlerExceptionResolver; import org.springframework.web.servlet.view.InternalResourceViewResolver; -import org.springframework.web.util.NestedServletException; - -import static org.junit.Assert.*; /** * The origin of this test fixture is {@link ServletHandlerMethodTests} with tests in this class adapted to run @@ -771,18 +776,13 @@ public class ServletHandlerMethodTests { @Test public void equivalentMappingsWithSameMethodName() throws Exception { - initDispatcherServlet(ChildController.class, null); - servlet.init(new MockServletConfig()); - - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/child/test"); - request.addParameter("childId", "100"); - MockHttpServletResponse response = new MockHttpServletResponse(); try { - servlet.service(request, response); + initDispatcherServlet(ChildController.class, null); + fail("Expected 'method already mapped' error"); } - catch (NestedServletException ex) { - assertTrue(ex.getCause() instanceof IllegalStateException); - assertTrue(ex.getCause().getMessage().contains("doGet")); + catch (BeanCreationException e) { + assertTrue(e.getCause() instanceof IllegalStateException); + assertTrue(e.getCause().getMessage().contains("Ambiguous mapping")); } }