Revised InvocableHandlerMethod exception handling

Issue: SPR-11281
(cherry picked from commit 1a1c72c)

Author: jhoeller

spring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -47,15 +47,15 @@
  */
 public class InvocableHandlerMethod extends HandlerMethod {
 
-	private HandlerMethodArgumentResolverComposite argumentResolvers = new HandlerMethodArgumentResolverComposite();
-
 	private WebDataBinderFactory dataBinderFactory;
 
+	private HandlerMethodArgumentResolverComposite argumentResolvers = new HandlerMethodArgumentResolverComposite();
+
 	private ParameterNameDiscoverer parameterNameDiscoverer = new LocalVariableTableParameterNameDiscoverer();
 
 
 	/**
-	 * Creates an instance from the given handler and method.
+	 * Create an instance from the given handler and method.
 	 */
 	public InvocableHandlerMethod(Object bean, Method method) {
 		super(bean, method);
@@ -69,19 +69,19 @@ public InvocableHandlerMethod(HandlerMethod handlerMethod) {
 	}
 
 	/**
-	 * Constructs a new handler method with the given bean instance, method name and parameters.
+	 * Construct a new handler method with the given bean instance, method name and parameters.
 	 * @param bean the object bean
 	 * @param methodName the method name
 	 * @param parameterTypes the method parameter types
 	 * @throws NoSuchMethodException when the method cannot be found
 	 */
-	public InvocableHandlerMethod(
-			Object bean, String methodName, Class<?>... parameterTypes) throws NoSuchMethodException {
+	public InvocableHandlerMethod(Object bean, String methodName, Class<?>... parameterTypes) throws NoSuchMethodException {
 		super(bean, methodName, parameterTypes);
 	}
 
+
 	/**
-	 * Sets the {@link WebDataBinderFactory} to be passed to argument resolvers allowing them to create
+	 * Set the {@link WebDataBinderFactory} to be passed to argument resolvers allowing them to create
 	 * a {@link WebDataBinder} for data binding and type conversion purposes.
 	 * @param dataBinderFactory the data binder factory.
 	 */
@@ -97,78 +97,74 @@ public void setHandlerMethodArgumentResolvers(HandlerMethodArgumentResolverCompo
 	}
 
 	/**
-	 * Set the ParameterNameDiscoverer for resolving parameter names when needed (e.g. default request attribute name).
-	 * <p>Default is an {@link org.springframework.core.LocalVariableTableParameterNameDiscoverer} instance.
+	 * Set the ParameterNameDiscoverer for resolving parameter names when needed
+	 * (e.g. default request attribute name).
+	 * <p>Default is a {@link org.springframework.core.LocalVariableTableParameterNameDiscoverer} instance.
 	 */
 	public void setParameterNameDiscoverer(ParameterNameDiscoverer parameterNameDiscoverer) {
 		this.parameterNameDiscoverer = parameterNameDiscoverer;
 	}
 
+
 	/**
 	 * Invoke the method after resolving its argument values in the context of the given request. <p>Argument
 	 * values are commonly resolved through {@link HandlerMethodArgumentResolver}s. The {@code provideArgs}
 	 * parameter however may supply argument values to be used directly, i.e. without argument resolution.
 	 * Examples of provided argument values include a {@link WebDataBinder}, a {@link SessionStatus}, or
 	 * a thrown exception instance. Provided argument values are checked before argument resolvers.
-	 *
 	 * @param request the current request
 	 * @param mavContainer the ModelAndViewContainer for this request
 	 * @param providedArgs "given" arguments matched by type, not resolved
 	 * @return the raw value returned by the invoked method
 	 * @exception Exception raised if no suitable argument resolver can be found, or the method raised an exception
 	 */
-	public final Object invokeForRequest(NativeWebRequest request,
-										 ModelAndViewContainer mavContainer,
-										 Object... providedArgs) throws Exception {
-		Object[] args = getMethodArgumentValues(request, mavContainer, providedArgs);
+	public final Object invokeForRequest(NativeWebRequest request, ModelAndViewContainer mavContainer,
+			Object... providedArgs) throws Exception {
 
+		Object[] args = getMethodArgumentValues(request, mavContainer, providedArgs);
 		if (logger.isTraceEnabled()) {
-			StringBuilder builder = new StringBuilder("Invoking [");
-			builder.append(this.getMethod().getName()).append("] method with arguments ");
-			builder.append(Arrays.asList(args));
-			logger.trace(builder.toString());
+			StringBuilder sb = new StringBuilder("Invoking [");
+			sb.append(getBeanType().getSimpleName()).append(".");
+			sb.append(getMethod().getName()).append("] method with arguments ");
+			sb.append(Arrays.asList(args));
+			logger.trace(sb.toString());
 		}
-
 		Object returnValue = invoke(args);
-
 		if (logger.isTraceEnabled()) {
-			logger.trace("Method [" + this.getMethod().getName() + "] returned [" + returnValue + "]");
+			logger.trace("Method [" + getMethod().getName() + "] returned [" + returnValue + "]");
 		}
-
 		return returnValue;
 	}
 
 	/**
 	 * Get the method argument values for the current request.
 	 */
-	private Object[] getMethodArgumentValues(
-			NativeWebRequest request, ModelAndViewContainer mavContainer,
+	private Object[] getMethodArgumentValues(NativeWebRequest request, ModelAndViewContainer mavContainer,
 			Object... providedArgs) throws Exception {
 
 		MethodParameter[] parameters = getMethodParameters();
 		Object[] args = new Object[parameters.length];
 		for (int i = 0; i < parameters.length; i++) {
 			MethodParameter parameter = parameters[i];
-			parameter.initParameterNameDiscovery(parameterNameDiscoverer);
+			parameter.initParameterNameDiscovery(this.parameterNameDiscoverer);
 			GenericTypeResolver.resolveParameterType(parameter, getBean().getClass());
-
 			args[i] = resolveProvidedArgument(parameter, providedArgs);
 			if (args[i] != null) {
 				continue;
 			}
-
-			if (argumentResolvers.supportsParameter(parameter)) {
+			if (this.argumentResolvers.supportsParameter(parameter)) {
 				try {
-					args[i] = argumentResolvers.resolveArgument(parameter, mavContainer, request, dataBinderFactory);
+					args[i] = this.argumentResolvers.resolveArgument(
+							parameter, mavContainer, request, this.dataBinderFactory);
 					continue;
-				} catch (Exception ex) {
+				}
+				catch (Exception ex) {
 					if (logger.isTraceEnabled()) {
 						logger.trace(getArgumentResolutionErrorMessage("Error resolving argument", i), ex);
 					}
 					throw ex;
 				}
 			}
-
 			if (args[i] == null) {
 				String msg = getArgumentResolutionErrorMessage("No suitable resolver for argument", i);
 				throw new IllegalStateException(msg);
@@ -214,17 +210,17 @@ private Object resolveProvidedArgument(MethodParameter parameter, Object... prov
 	 * Invoke the handler method with the given argument values.
 	 */
 	private Object invoke(Object... args) throws Exception {
-		ReflectionUtils.makeAccessible(this.getBridgedMethod());
+		ReflectionUtils.makeAccessible(getBridgedMethod());
 		try {
 			return getBridgedMethod().invoke(getBean(), args);
 		}
-		catch (IllegalArgumentException e) {
-			String msg = getInvocationErrorMessage(e.getMessage(), args);
-			throw new IllegalArgumentException(msg, e);
+		catch (IllegalArgumentException ex) {
+			assertTargetBean(getBridgedMethod(), getBean(), args);
+			throw new IllegalStateException(getInvocationErrorMessage(ex.getMessage(), args), ex);
 		}
-		catch (InvocationTargetException e) {
+		catch (InvocationTargetException ex) {
 			// Unwrap for HandlerExceptionResolvers ...
-			Throwable targetException = e.getTargetException();
+			Throwable targetException = ex.getTargetException();
 			if (targetException instanceof RuntimeException) {
 				throw (RuntimeException) targetException;
 			}
@@ -241,6 +237,25 @@ else if (targetException instanceof Exception) {
 		}
 	}
 
+	/**
+	 * Assert that the target bean class is an instance of the class where the given
+	 * method is declared. In some cases the actual controller instance at request-
+	 * processing time may be a JDK dynamic proxy (lazy initialization, prototype
+	 * beans, and others). {@code @Controller}'s that require proxying should prefer
+	 * class-based proxy mechanisms.
+	 */
+	private void assertTargetBean(Method method, Object targetBean, Object[] args) {
+		Class<?> methodDeclaringClass = method.getDeclaringClass();
+		Class<?> targetBeanClass = targetBean.getClass();
+		if (!methodDeclaringClass.isAssignableFrom(targetBeanClass)) {
+			String msg = "The mapped controller method class '" + methodDeclaringClass.getName() +
+					"' is not an instance of the actual controller bean instance '" +
+					targetBeanClass.getName() + "'. If the controller requires proxying " +
+					"(e.g. due to @Transactional), please use class-based proxying.";
+			throw new IllegalStateException(getInvocationErrorMessage(msg, args));
+		}
+	}
+
 	private String getInvocationErrorMessage(String message, Object[] resolvedArgs) {
 		StringBuilder sb = new StringBuilder(getDetailedErrorMessage(message));
 		sb.append("Resolved arguments: \n");

spring-web/src/test/java/org/springframework/web/method/support/InvocableHandlerMethodTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -45,13 +45,15 @@ public class InvocableHandlerMethodTests {
 
 	private NativeWebRequest webRequest;
 
+
 	@Before
 	public void setUp() throws Exception {
 		Method method = Handler.class.getDeclaredMethod("handle", Integer.class, String.class);
 		this.handlerMethod = new InvocableHandlerMethod(new Handler(), method);
 		this.webRequest = new ServletWebRequest(new MockHttpServletRequest(), new MockHttpServletResponse());
 	}
 
+
 	@Test
 	public void resolveArg() throws Exception {
 		StubArgumentResolver intResolver = new StubArgumentResolver(Integer.class, 99);
@@ -63,11 +65,9 @@ public void resolveArg() throws Exception {
 		handlerMethod.setHandlerMethodArgumentResolvers(composite);
 
 		Object returnValue = handlerMethod.invokeForRequest(webRequest, null);
-
 		assertEquals(1, intResolver.getResolvedParameters().size());
 		assertEquals(1, stringResolver.getResolvedParameters().size());
 		assertEquals("99-value", returnValue);
-
 		assertEquals("intArg", intResolver.getResolvedParameters().get(0).getParameterName());
 		assertEquals("stringArg", stringResolver.getResolvedParameters().get(0).getParameterName());
 	}
@@ -83,7 +83,6 @@ public void resolveNullArg() throws Exception {
 		handlerMethod.setHandlerMethodArgumentResolvers(composite);
 
 		Object returnValue = handlerMethod.invokeForRequest(webRequest, null);
-
 		assertEquals(1, intResolver.getResolvedParameters().size());
 		assertEquals(1, stringResolver.getResolvedParameters().size());
 		assertEquals("null-null", returnValue);
@@ -94,7 +93,8 @@ public void cannotResolveArg() throws Exception {
 		try {
 			handlerMethod.invokeForRequest(webRequest, null);
 			fail("Expected exception");
-		} catch (IllegalStateException ex) {
+		}
+		catch (IllegalStateException ex) {
 			assertTrue(ex.getMessage().contains("No suitable resolver for argument [0] [type=java.lang.Integer]"));
 		}
 	}
@@ -118,7 +118,6 @@ public void resolveProvidedArgFirst() throws Exception {
 		handlerMethod.setHandlerMethodArgumentResolvers(composite);
 
 		Object returnValue = handlerMethod.invokeForRequest(webRequest, null, 2, "value2");
-
 		assertEquals("2-value2", returnValue);
 	}
 
@@ -131,9 +130,9 @@ public void exceptionInResolvingArg() throws Exception {
 		try {
 			handlerMethod.invokeForRequest(webRequest, null);
 			fail("Expected exception");
-		} catch (HttpMessageNotReadableException ex) {
-			// Expected..
-			// Allow HandlerMethodArgumentResolver exceptions to propagate..
+		}
+		catch (HttpMessageNotReadableException ex) {
+			// expected -  allow HandlerMethodArgumentResolver exceptions to propagate
 		}
 	}
 
@@ -150,7 +149,8 @@ public void illegalArgumentException() throws Exception {
 		try {
 			handlerMethod.invokeForRequest(webRequest, null);
 			fail("Expected exception");
-		} catch (IllegalArgumentException ex) {
+		}
+		catch (IllegalStateException ex) {
 			assertNotNull("Exception not wrapped", ex.getCause());
 			assertTrue(ex.getCause() instanceof IllegalArgumentException);
 			assertTrue(ex.getMessage().contains("Controller ["));
@@ -166,28 +166,32 @@ public void invocationTargetException() throws Exception {
 		Throwable expected = new RuntimeException("error");
 		try {
 			invokeExceptionRaisingHandler(expected);
-		} catch (RuntimeException actual) {
+		}
+		catch (RuntimeException actual) {
 			assertSame(expected, actual);
 		}
 
 		expected = new Error("error");
 		try {
 			invokeExceptionRaisingHandler(expected);
-		} catch (Error actual) {
+		}
+		catch (Error actual) {
 			assertSame(expected, actual);
 		}
 
 		expected = new Exception("error");
 		try {
 			invokeExceptionRaisingHandler(expected);
-		} catch (Exception actual) {
+		}
+		catch (Exception actual) {
 			assertSame(expected, actual);
 		}
 
 		expected = new Throwable("error");
 		try {
 			invokeExceptionRaisingHandler(expected);
-		} catch (IllegalStateException actual) {
+		}
+		catch (IllegalStateException actual) {
 			assertNotNull(actual.getCause());
 			assertSame(expected, actual.getCause());
 			assertTrue(actual.getMessage().contains("Failed to invoke controller method"));
@@ -201,6 +205,7 @@ private void invokeExceptionRaisingHandler(Throwable expected) throws Exception
 		fail("Expected exception");
 	}
 
+
 	@SuppressWarnings("unused")
 	private static class Handler {
 
@@ -209,6 +214,7 @@ public String handle(Integer intArg, String stringArg) {
 		}
 	}
 
+
 	@SuppressWarnings("unused")
 	private static class ExceptionRaisingHandler {
 
@@ -221,9 +227,9 @@ public ExceptionRaisingHandler(Throwable t) {
 		public void raiseException() throws Throwable {
 			throw t;
 		}
-
 	}
 
+
 	private static class ExceptionRaisingArgumentResolver implements HandlerMethodArgumentResolver {
 
 		@Override