SPR-8248

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java

@@ -97,29 +97,30 @@ public abstract class AbstractHandlerMethodMapping<T> 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<Method> 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<T> 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<T> 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 + "]");

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);

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).
- * <p>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}.
+ * <p>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

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();
+	}
+	
 }

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;
 		}

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);

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMethodDetectionTests.java

@@ -64,6 +64,7 @@ public class RequestMappingHandlerMethodDetectionTests {
 				{ new MappingAbstractClassController(), false}, 
 				{ new ParameterizedInterfaceController(), 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<T> {
 		void handle(T object);
 	}
-
+	@RequestMapping(value="/type")
 	public static class ParameterizedInterfaceController implements ParameterizedInterface<TestBean> {
 		@RequestMapping(value = "/handle", method = RequestMethod.GET)
 		public void handle(TestBean object) {
 		}
 	}
 
+	/* Annotation on parameterized interface method */
+
 	@Controller
 	public interface MappingParameterizedInterface<T> {
 		@RequestMapping(value = "/handle", method = RequestMethod.GET)
 		void handle(T object);
 	}
-
+	@RequestMapping(value="/type")
 	public static class MappingParameterizedInterfaceController implements MappingParameterizedInterface<TestBean> {
 		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() {
 		}

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"));
 		}
 	}