Consistent InvocableHandlerMethod implementations

This commit makes the 3 existing InvocableHandlerMethod types more
consistent and comparable with each other.

1. Use of consistent method names and method order.

2. Consistent error formatting.

3. Explicit for loops for resolving argument values in webflux variant
because that makes it more readable, creates less garabage, and it's
the only way to bring consistency since the other two variants cannot
throw exceptions inside Optional lambdas (vs webflux variant which can
wrap it in a Mono).

4. Use package private HandlerMethodArgumentComposite in webflux
variant in order to pick up the resolver argument caching that the
other two variants have.

5. Polish tests.

6. Add missing tests for messaging variant.

Author: rstoyanchev

spring-messaging/src/main/java/org/springframework/messaging/handler/invocation/HandlerMethodArgumentResolverComposite.java

@@ -57,9 +57,7 @@ public HandlerMethodArgumentResolverComposite addResolver(HandlerMethodArgumentR
 	 */
 	public HandlerMethodArgumentResolverComposite addResolvers(@Nullable HandlerMethodArgumentResolver... resolvers) {
 		if (resolvers != null) {
-			for (HandlerMethodArgumentResolver resolver : resolvers) {
-				this.argumentResolvers.add(resolver);
-			}
+			Collections.addAll(this.argumentResolvers, resolvers);
 		}
 		return this;
 	}
@@ -92,30 +90,36 @@ public void clear() {
 
 
 	/**
-	 * Whether the given {@linkplain MethodParameter method parameter} is supported by any registered
-	 * {@link HandlerMethodArgumentResolver}.
+	 * Whether the given {@linkplain MethodParameter method parameter} is
+	 * supported by any registered {@link HandlerMethodArgumentResolver}.
 	 */
 	@Override
 	public boolean supportsParameter(MethodParameter parameter) {
 		return getArgumentResolver(parameter) != null;
 	}
 
 	/**
-	 * Iterate over registered {@link HandlerMethodArgumentResolver HandlerMethodArgumentResolvers} and invoke the one that supports it.
-	 * @throws IllegalStateException if no suitable {@link HandlerMethodArgumentResolver} is found.
+	 * Iterate over registered
+	 * {@link HandlerMethodArgumentResolver HandlerMethodArgumentResolvers}
+	 * and invoke the one that supports it.
+	 * @throws IllegalStateException if no suitable
+	 * {@link HandlerMethodArgumentResolver} is found.
 	 */
 	@Override
 	@Nullable
 	public Object resolveArgument(MethodParameter parameter, Message<?> message) throws Exception {
 		HandlerMethodArgumentResolver resolver = getArgumentResolver(parameter);
 		if (resolver == null) {
-			throw new IllegalStateException("Unknown parameter type [" + parameter.getParameterType().getName() + "]");
+			throw new IllegalStateException(
+					"Unsupported parameter type [" + parameter.getParameterType().getName() + "]." +
+							" supportsParameter should be called first.");
 		}
 		return resolver.resolveArgument(parameter, message);
 	}
 
 	/**
-	 * Find a registered {@link HandlerMethodArgumentResolver} that supports the given method parameter.
+	 * Find a registered {@link HandlerMethodArgumentResolver} that supports
+	 * the given method parameter.
 	 */
 	@Nullable
 	private HandlerMethodArgumentResolver getArgumentResolver(MethodParameter parameter) {

spring-messaging/src/main/java/org/springframework/messaging/handler/invocation/InvocableHandlerMethod.java

@@ -20,6 +20,8 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.Type;
 import java.util.Arrays;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 
 import org.springframework.core.DefaultParameterNameDiscoverer;
 import org.springframework.core.MethodParameter;
@@ -28,22 +30,25 @@
 import org.springframework.lang.Nullable;
 import org.springframework.messaging.Message;
 import org.springframework.messaging.handler.HandlerMethod;
-import org.springframework.util.ClassUtils;
+import org.springframework.util.ObjectUtils;
 import org.springframework.util.ReflectionUtils;
+import org.springframework.util.StringUtils;
 
 /**
- * Provides a method for invoking the handler method for a given message after resolving its
- * method argument values through registered {@link HandlerMethodArgumentResolver HandlerMethodArgumentResolvers}.
- *
- * <p>Use {@link #setMessageMethodArgumentResolvers} to customize the list of argument resolvers.
+ * Extension of {@link HandlerMethod} that invokes the underlying method with
+ * argument values resolved from the current HTTP request through a list of
+ * {@link HandlerMethodArgumentResolver}.
  *
  * @author Rossen Stoyanchev
  * @author Juergen Hoeller
  * @since 4.0
  */
 public class InvocableHandlerMethod extends HandlerMethod {
 
-	private HandlerMethodArgumentResolverComposite argumentResolvers = new HandlerMethodArgumentResolverComposite();
+	private static final Object[] EMPTY_ARGS = new Object[0];
+
+
+	private HandlerMethodArgumentResolverComposite resolvers = new HandlerMethodArgumentResolverComposite();
 
 	private ParameterNameDiscoverer parameterNameDiscoverer = new DefaultParameterNameDiscoverer();
 
@@ -80,7 +85,7 @@ public InvocableHandlerMethod(Object bean, String methodName, Class<?>... parame
 	 * Set {@link HandlerMethodArgumentResolver HandlerMethodArgumentResolvers} to use to use for resolving method argument values.
 	 */
 	public void setMessageMethodArgumentResolvers(HandlerMethodArgumentResolverComposite argumentResolvers) {
-		this.argumentResolvers = argumentResolvers;
+		this.resolvers = argumentResolvers;
 	}
 
 	/**
@@ -113,15 +118,9 @@ public void setParameterNameDiscoverer(ParameterNameDiscoverer parameterNameDisc
 	public Object invoke(Message<?> message, Object... providedArgs) throws Exception {
 		Object[] args = getMethodArgumentValues(message, providedArgs);
 		if (logger.isTraceEnabled()) {
-			logger.trace("Invoking '" + ClassUtils.getQualifiedMethodName(getMethod(), getBeanType()) +
-					"' with arguments " + Arrays.toString(args));
+			logger.trace("Arguments: " + Arrays.toString(args));
 		}
-		Object returnValue = doInvoke(args);
-		if (logger.isTraceEnabled()) {
-			logger.trace("Method [" + ClassUtils.getQualifiedMethodName(getMethod(), getBeanType()) +
-					"] returned [" + returnValue + "]");
-		}
-		return returnValue;
+		return doInvoke(args);
 	}
 
 	/**
@@ -131,53 +130,55 @@ public Object invoke(Message<?> message, Object... providedArgs) throws Exceptio
 	 * @since 5.1.2
 	 */
 	protected Object[] getMethodArgumentValues(Message<?> message, Object... providedArgs) throws Exception {
+		if (ObjectUtils.isEmpty(getMethodParameters())) {
+			return EMPTY_ARGS;
+		}
 		MethodParameter[] parameters = getMethodParameters();
 		Object[] args = new Object[parameters.length];
 		for (int i = 0; i < parameters.length; i++) {
 			MethodParameter parameter = parameters[i];
 			parameter.initParameterNameDiscovery(this.parameterNameDiscoverer);
-			args[i] = resolveProvidedArgument(parameter, providedArgs);
+			args[i] = findProvidedArgument(parameter, providedArgs);
 			if (args[i] != null) {
 				continue;
 			}
-			if (this.argumentResolvers.supportsParameter(parameter)) {
-				try {
-					args[i] = this.argumentResolvers.resolveArgument(parameter, message);
-					continue;
-				}
-				catch (Exception ex) {
-					if (logger.isDebugEnabled()) {
-						logger.debug(getArgumentResolutionErrorMessage("Failed to resolve", i), ex);
+			if (!this.resolvers.supportsParameter(parameter)) {
+				throw new MethodArgumentResolutionException(
+						message, parameter, formatArgumentError(parameter, "No suitable resolver"));
+			}
+			try {
+				args[i] = this.resolvers.resolveArgument(parameter, message);
+			}
+			catch (Exception ex) {
+				// Leave stack trace for later, exception may actually be resolved and handled..
+				if (logger.isDebugEnabled()) {
+					String error = ex.getMessage();
+					if (error != null && !error.contains(parameter.getExecutable().toGenericString())) {
+						logger.debug(formatArgumentError(parameter, error));
 					}
-					throw ex;
 				}
-			}
-			if (args[i] == null) {
-				throw new MethodArgumentResolutionException(message, parameter,
-						getArgumentResolutionErrorMessage("No suitable resolver for", i));
+				throw ex;
 			}
 		}
 		return args;
 	}
 
-	private String getArgumentResolutionErrorMessage(String text, int index) {
-		Class<?> paramType = getMethodParameters()[index].getParameterType();
-		return text + " argument " + index + " of type '" + paramType.getName() + "'";
-	}
-
-	/**
-	 * Attempt to resolve a method parameter from the list of provided argument values.
-	 */
 	@Nullable
-	private Object resolveProvidedArgument(MethodParameter parameter, Object... providedArgs) {
-		for (Object providedArg : providedArgs) {
-			if (parameter.getParameterType().isInstance(providedArg)) {
-				return providedArg;
+	private Object findProvidedArgument(MethodParameter parameter, @Nullable Object... providedArgs) {
+		if (!ObjectUtils.isEmpty(providedArgs)) {
+			for (Object providedArg : providedArgs) {
+				if (parameter.getParameterType().isInstance(providedArg)) {
+					return providedArg;
+				}
 			}
 		}
 		return null;
 	}
 
+	private static String formatArgumentError(MethodParameter param, String message) {
+		return "Could not resolve parameter [" + param.getParameterIndex() + "] in " +
+				param.getExecutable().toGenericString() + (StringUtils.hasText(message) ? ": " + message : "");
+	}
 
 	/**
 	 * Invoke the handler method with the given argument values.
@@ -191,7 +192,7 @@ protected Object doInvoke(Object... args) throws Exception {
 		catch (IllegalArgumentException ex) {
 			assertTargetBean(getBridgedMethod(), getBean(), args);
 			String text = (ex.getMessage() != null ? ex.getMessage() : "Illegal argument");
-			throw new IllegalStateException(getInvocationErrorMessage(text, args), ex);
+			throw new IllegalStateException(formatInvokeError(text, args), ex);
 		}
 		catch (InvocationTargetException ex) {
 			// Unwrap for HandlerExceptionResolvers ...
@@ -206,8 +207,7 @@ else if (targetException instanceof Exception) {
 				throw (Exception) targetException;
 			}
 			else {
-				String text = getInvocationErrorMessage("Failed to invoke handler method", args);
-				throw new IllegalStateException(text, targetException);
+				throw new IllegalStateException(formatInvokeError("Invocation failure", args), targetException);
 			}
 		}
 	}
@@ -227,38 +227,23 @@ private void assertTargetBean(Method method, Object targetBean, Object[] args) {
 					"' is not an instance of the actual endpoint bean class '" +
 					targetBeanClass.getName() + "'. If the endpoint requires proxying " +
 					"(e.g. due to @Transactional), please use class-based proxying.";
-			throw new IllegalStateException(getInvocationErrorMessage(text, args));
+			throw new IllegalStateException(formatInvokeError(text, args));
 		}
 	}
 
-	private String getInvocationErrorMessage(String text, Object[] resolvedArgs) {
-		StringBuilder sb = new StringBuilder(getDetailedErrorMessage(text));
-		sb.append("Resolved arguments: \n");
-		for (int i = 0; i < resolvedArgs.length; i++) {
-			sb.append("[").append(i).append("] ");
-			if (resolvedArgs[i] == null) {
-				sb.append("[null] \n");
-			}
-			else {
-				sb.append("[type=").append(resolvedArgs[i].getClass().getName()).append("] ");
-				sb.append("[value=").append(resolvedArgs[i]).append("]\n");
-			}
-		}
-		return sb.toString();
-	}
+	private String formatInvokeError(String text, Object[] args) {
 
-	/**
-	 * Adds HandlerMethod details such as the bean type and method signature to the message.
-	 * @param text error message to append the HandlerMethod details to
-	 */
-	protected String getDetailedErrorMessage(String text) {
-		StringBuilder sb = new StringBuilder(text).append("\n");
-		sb.append("HandlerMethod details: \n");
-		sb.append("Endpoint [").append(getBeanType().getName()).append("]\n");
-		sb.append("Method [").append(getBridgedMethod().toGenericString()).append("]\n");
-		return sb.toString();
-	}
+		String formattedArgs = IntStream.range(0, args.length)
+				.mapToObj(i -> (args[i] != null ?
+						"[" + i + "] [type=" + args[i].getClass().getName() + "] [value=" + args[i] + "]" :
+						"[" + i + "] [null]"))
+				.collect(Collectors.joining(",\n", " ", " "));
 
+		return text + "\n" +
+				"Endpoint [" + getBeanType().getName() + "]\n" +
+				"Method [" + getBridgedMethod().toGenericString() + "] " +
+				"with argument values:\n" + formattedArgs;
+	}
 
 	MethodParameter getAsyncReturnValueType(@Nullable Object returnValue) {
 		return new AsyncResultMethodParameter(returnValue);

spring-messaging/src/test/java/org/springframework/messaging/handler/annotation/support/DefaultMessageHandlerMethodFactoryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -149,7 +149,7 @@ public void overrideArgumentResolvers() throws Exception {
 				createInvocableHandlerMethod(instance, "simpleString", String.class);
 
 		thrown.expect(MethodArgumentResolutionException.class);
-		thrown.expectMessage("No suitable resolver for");
+		thrown.expectMessage("No suitable resolver");
 		invocableHandlerMethod2.invoke(message);
 	}