Fix handling of required payload.

A payload that is required will now throw an appropriate exception
regardless of if a conversion is required or not.

isEmptyPayload now takes the payload instead of the message
so that both the original payload and the converted payload, if
necessary, share the same logic.

JSR-303 validation is now consistently applied.

Issue: SPR-11577

spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/MethodArgumentNotValidException.java

@@ -23,31 +23,39 @@ import org.springframework.validation.BindingResult;
 import org.springframework.validation.ObjectError;
 
 /**
- * Exception to be thrown when validation on an method parameter annotated with {@code @Valid} fails.
+ * Exception to be thrown when a method argument is not valid. For instance, this
+ * can be issued if a validation on a method parameter annotated with
+ * {@code @Valid} fails.
+ *
  * @author Brian Clozel
  * @since 4.0.1
  */
 @SuppressWarnings("serial")
 public class MethodArgumentNotValidException extends MessagingException {
 
-	private final MethodParameter parameter;
-
-	private final BindingResult bindingResult;
-
+	/**
+	 * Create a new message with the given description.
+	 * @see #getMessage()
+	 */
+	public MethodArgumentNotValidException(Message<?> message, String description) {
+		super(message, description);
+	}
 
-	public MethodArgumentNotValidException(Message<?> message, MethodParameter parameter, BindingResult bindingResult) {
-		super(message);
-		this.parameter = parameter;
-		this.bindingResult = bindingResult;
+	/**
+	 * Create a new instance with a failed validation described by
+	 * the given {@link BindingResult}.
+	 */
+	public MethodArgumentNotValidException(Message<?> message,
+			MethodParameter parameter, BindingResult bindingResult) {
+		this(message, generateMessage(parameter, bindingResult));
 	}
 
-	@Override
-	public String getMessage() {
+	private static String generateMessage(MethodParameter parameter, BindingResult bindingResult) {
 		StringBuilder sb = new StringBuilder("Validation failed for parameter at index ")
-				.append(this.parameter.getParameterIndex()).append(" in method: ")
-				.append(this.parameter.getMethod().toGenericString())
-				.append(", with ").append(this.bindingResult.getErrorCount()).append(" error(s): ");
-		for (ObjectError error : this.bindingResult.getAllErrors()) {
+				.append(parameter.getParameterIndex()).append(" in method: ")
+				.append(parameter.getMethod().toGenericString())
+				.append(", with ").append(bindingResult.getErrorCount()).append(" error(s): ");
+		for (ObjectError error : bindingResult.getAllErrors()) {
 			sb.append("[").append(error).append("] ");
 		}
 		return sb.toString();

spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolver.java

@@ -42,6 +42,7 @@ import java.lang.annotation.Annotation;
  *
  * @author Rossen Stoyanchev
  * @author Brian Clozel
+ * @author Stephane Nicoll
  * @since 4.0
  */
 public class PayloadArgumentResolver implements HandlerMethodArgumentResolver {
@@ -65,36 +66,53 @@ public class PayloadArgumentResolver implements HandlerMethodArgumentResolver {
 
 	@Override
 	public Object resolveArgument(MethodParameter parameter, Message<?> message) throws Exception {
-
-		Class<?> sourceClass = message.getPayload().getClass();
-		Class<?> targetClass = parameter.getParameterType();
-
-		if (ClassUtils.isAssignable(targetClass,sourceClass)) {
-			return message.getPayload();
-		}
-
 		Payload annot = parameter.getParameterAnnotation(Payload.class);
+		if ((annot != null) && StringUtils.hasText(annot.value())) {
+			throw new IllegalStateException("@Payload SpEL expressions not supported by this resolver.");
+		}
 
-		if (isEmptyPayload(message)) {
-			if ((annot != null) && !annot.required()) {
+		Object target = getTargetPayload(parameter, message);
+		if (annot != null && isEmptyPayload(target)) {
+			if (annot.required()) {
+				throw new MethodArgumentNotValidException(message, createPayloadRequiredExceptionMessage(parameter, target));
+			}
+			else {
 				return null;
 			}
 		}
 
-		if ((annot != null) && StringUtils.hasText(annot.value())) {
-			throw new IllegalStateException("@Payload SpEL expressions not supported by this resolver.");
+		if (annot != null) { // Only validate @Payload
+			validate(message, parameter, target);
 		}
-
-		Object target = this.converter.fromMessage(message, targetClass);
-		validate(message, parameter, target);
-
 		return target;
 	}
 
-	protected boolean isEmptyPayload(Message<?> message) {
-		Object payload = message.getPayload();
-		if (payload instanceof byte[]) {
-			return ((byte[]) message.getPayload()).length == 0;
+	/**
+	 * Return the target payload to handle for the specified message. Can either
+	 * be the payload itself if the parameter type supports it or the converted
+	 * one otherwise. While the payload of a {@link Message} cannot be null by
+	 * design, this method may return a {@code null} payload if the conversion
+	 * result is {@code null}.
+	 */
+	protected Object getTargetPayload(MethodParameter parameter, Message<?> message) {
+		Class<?> sourceClass = message.getPayload().getClass();
+		Class<?> targetClass = parameter.getParameterType();
+		if (ClassUtils.isAssignable(targetClass,sourceClass)) {
+			return message.getPayload();
+		}
+		return this.converter.fromMessage(message, targetClass);
+	}
+
+	/**
+	 * Specify if the given {@code payload} is empty.
+	 * @param payload the payload to check (can be {@code null})
+	 */
+	protected boolean isEmptyPayload(Object payload) {
+		if (payload == null) {
+			return true;
+		}
+		else if (payload instanceof byte[]) {
+			return ((byte[]) payload).length == 0;
 		}
 		else if (payload instanceof String) {
 			return ((String) payload).trim().equals("");
@@ -128,4 +146,19 @@ public class PayloadArgumentResolver implements HandlerMethodArgumentResolver {
 		}
 	}
 
+	private String createPayloadRequiredExceptionMessage(MethodParameter parameter, Object payload) {
+		String name = parameter.getParameterName() != null
+				? parameter.getParameterName() : "arg" + parameter.getParameterIndex();
+		StringBuilder sb = new StringBuilder("Payload parameter '").append(name)
+				.append(" at index ").append(parameter.getParameterIndex()).append(" ");
+		if (payload == null) {
+			sb.append("could not be converted to '").append(parameter.getParameterType().getName())
+					.append("' and is required");
+		}
+		else {
+			sb.append("is required");
+		}
+		return sb.toString();
+	}
+
 }

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

@@ -16,51 +16,55 @@
 
 package org.springframework.messaging.handler.annotation.support;
 
+import static org.junit.Assert.*;
+
 import java.lang.reflect.Method;
+import java.util.Locale;
 
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
 import org.springframework.core.LocalVariableTableParameterNameDiscoverer;
 import org.springframework.core.MethodParameter;
 import org.springframework.messaging.Message;
+import org.springframework.messaging.converter.StringMessageConverter;
 import org.springframework.messaging.handler.annotation.Payload;
 import org.springframework.messaging.support.MessageBuilder;
-import org.springframework.messaging.converter.StringMessageConverter;
-import org.springframework.util.StringUtils;
+import org.springframework.util.Assert;
 import org.springframework.validation.Errors;
 import org.springframework.validation.Validator;
 import org.springframework.validation.annotation.Validated;
 
-import static org.junit.Assert.*;
-
 /**
  * Test fixture for {@link PayloadArgumentResolver}.
  *
  * @author Rossen Stoyanchev
  * @author Brian Clozel
+ * @author Stephane Nicoll
  */
 public class PayloadArgumentResolverTests {
 
+	@Rule
+	public final ExpectedException thrown = ExpectedException.none();
+
 	private PayloadArgumentResolver resolver;
 
-	private MethodParameter param;
-	private MethodParameter paramNotRequired;
-	private MethodParameter paramWithSpelExpression;
+	private Method payloadMethod;
+	private Method simpleMethod;
 	private MethodParameter paramValidated;
 
-
 	@Before
 	public void setup() throws Exception {
-
 		this.resolver = new PayloadArgumentResolver(new StringMessageConverter(), testValidator());
 
-		Method method = PayloadArgumentResolverTests.class.getDeclaredMethod("handleMessage",
-				String.class, String.class, String.class, String.class);
+		payloadMethod = PayloadArgumentResolverTests.class.getDeclaredMethod("handleMessage",
+				String.class, String.class, Locale.class, String.class, String.class);
+		simpleMethod = PayloadArgumentResolverTests.class.getDeclaredMethod("handleAnotherMessage",
+				String.class, String.class);
 
-		this.param = new MethodParameter(method , 0);
-		this.paramNotRequired = new MethodParameter(method , 1);
-		this.paramWithSpelExpression = new MethodParameter(method , 2);
-		this.paramValidated = new MethodParameter(method , 3);
+		this.paramValidated = getMethodParameter(payloadMethod, 4);
 		this.paramValidated.initParameterNameDiscovery(new LocalVariableTableParameterNameDiscoverer());
 	}
 
@@ -68,25 +72,49 @@ public class PayloadArgumentResolverTests {
 	@Test
 	public void resolveRequired() throws Exception {
 		Message<?> message = MessageBuilder.withPayload("ABC".getBytes()).build();
-		Object actual = this.resolver.resolveArgument(this.param, message);
+		Object actual = this.resolver.resolveArgument(getMethodParameter(payloadMethod, 0), message);
 
 		assertEquals("ABC", actual);
 	}
 
+	@Test
+	public void resolveRequiredEmpty() throws Exception {
+		Message<?> message = MessageBuilder.withPayload("").build();
+
+		thrown.expect(MethodArgumentNotValidException.class); // Required but empty
+		this.resolver.resolveArgument(getMethodParameter(payloadMethod, 0), message);
+	}
+
 	@Test
 	public void resolveNotRequired() throws Exception {
+		MethodParameter paramNotRequired = getMethodParameter(payloadMethod, 1);
 
 		Message<?> emptyByteArrayMessage = MessageBuilder.withPayload(new byte[0]).build();
-		assertNull(this.resolver.resolveArgument(this.paramNotRequired, emptyByteArrayMessage));
+		assertNull(this.resolver.resolveArgument(paramNotRequired, emptyByteArrayMessage));
+
+		Message<?> emptyStringMessage = MessageBuilder.withPayload("").build();
+		assertNull(this.resolver.resolveArgument(paramNotRequired, emptyStringMessage));
 
 		Message<?> notEmptyMessage = MessageBuilder.withPayload("ABC".getBytes()).build();
-		assertEquals("ABC", this.resolver.resolveArgument(this.paramNotRequired, notEmptyMessage));
+		assertEquals("ABC", this.resolver.resolveArgument(paramNotRequired, notEmptyMessage));
 	}
 
-	@Test(expected=IllegalStateException.class)
+	@Test
+	public void resolveNonConvertibleParam() throws Exception {
+		Message<?> notEmptyMessage = MessageBuilder.withPayload(123).build();
+
+		// Could not convert from int to Locale so will be "empty" after conversion
+	  	thrown.expect(MethodArgumentNotValidException.class);
+		thrown.expectMessage(Locale.class.getName()); // reference to the type that could not be converted
+		this.resolver.resolveArgument(getMethodParameter(payloadMethod, 2), notEmptyMessage);
+	}
+
+	@Test
 	public void resolveSpelExpressionNotSupported() throws Exception {
 		Message<?> message = MessageBuilder.withPayload("ABC".getBytes()).build();
-		this.resolver.resolveArgument(this.paramWithSpelExpression, message);
+
+		thrown.expect(IllegalStateException.class);
+		this.resolver.resolveArgument(getMethodParameter(payloadMethod, 3), message);
 	}
 
 	@Test
@@ -95,12 +123,46 @@ public class PayloadArgumentResolverTests {
 		this.resolver.resolveArgument(this.paramValidated, message);
 	}
 
-	@Test(expected=MethodArgumentNotValidException.class)
+	@Test
 	public void resolveFailValidation() throws Exception {
-		Message<?> message = MessageBuilder.withPayload("".getBytes()).build();
+		// See testValidator()
+		Message<?> message = MessageBuilder.withPayload("invalidValue".getBytes()).build();
+
+		thrown.expect(MethodArgumentNotValidException.class);
 		this.resolver.resolveArgument(this.paramValidated, message);
 	}
 
+	@Test
+	public void resolveFailValidationNoConversionNecessary() throws Exception {
+		Message<?> message = MessageBuilder.withPayload("invalidValue").build();
+
+		thrown.expect(MethodArgumentNotValidException.class);
+		this.resolver.resolveArgument(this.paramValidated, message);
+	}
+
+	@Test
+	public void resolveNonAnnotatedParameter() throws Exception {
+		MethodParameter paramNotRequired = getMethodParameter(simpleMethod, 0);
+
+		Message<?> emptyByteArrayMessage = MessageBuilder.withPayload(new byte[0]).build();
+		assertEquals("", this.resolver.resolveArgument(paramNotRequired, emptyByteArrayMessage));
+
+		Message<?> emptyStringMessage = MessageBuilder.withPayload("").build();
+		assertEquals("", this.resolver.resolveArgument(paramNotRequired, emptyStringMessage));
+
+
+		Message<?> notEmptyMessage = MessageBuilder.withPayload("ABC".getBytes()).build();
+		assertEquals("ABC", this.resolver.resolveArgument(paramNotRequired, notEmptyMessage));
+	}
+
+	@Test
+	public void resolveNonAnnotatedParameterFailValidation() throws Exception {
+		// See testValidator()
+		Message<?> message = MessageBuilder.withPayload("invalidValue".getBytes()).build();
+
+		assertEquals("invalidValue", this.resolver.resolveArgument(getMethodParameter(simpleMethod, 1), message));
+	}
+
 	private Validator testValidator() {
 
 		return new Validator() {
@@ -111,19 +173,31 @@ public class PayloadArgumentResolverTests {
 			@Override
 			public void validate(Object target, Errors errors) {
 				String value = (String) target;
-				if (StringUtils.isEmpty(value.toString())) {
-					errors.reject("empty value");
+				if ("invalidValue".equals(value)) {
+					errors.reject("invalid value");
 				}
 			}
 		};
 	}
 
+	private MethodParameter getMethodParameter(Method method, int index) {
+		Assert.notNull(method, "Method must be set");
+		return new MethodParameter(method, index);
+	}
+
 	@SuppressWarnings("unused")
 	private void handleMessage(
 			@Payload String param,
 			@Payload(required=false) String paramNotRequired,
+			@Payload(required=true) Locale nonConvertibleRequiredParam,
 			@Payload("foo.bar") String paramWithSpelExpression,
 			@Validated @Payload String validParam) {
 	}
 
+	@SuppressWarnings("unused")
+	private void handleAnotherMessage(
+			String param,
+			@Validated String validParam) {
+	}
+
 }

spring-messaging/src/test/java/org/springframework/messaging/simp/annotation/support/SimpAnnotationMethodMessageHandlerTests.java

@@ -35,7 +35,6 @@ import org.springframework.messaging.simp.SimpMessagingTemplate;
 import org.springframework.messaging.simp.annotation.SubscribeMapping;
 import org.springframework.messaging.support.MessageBuilder;
 import org.springframework.stereotype.Controller;
-import org.springframework.util.StringUtils;
 import org.springframework.validation.Errors;
 import org.springframework.validation.Validator;
 import org.springframework.validation.annotation.Validated;
@@ -51,6 +50,8 @@ import static org.junit.Assert.*;
  */
 public class SimpAnnotationMethodMessageHandlerTests {
 
+	private static final String TEST_INVALID_VALUE = "invalidValue";
+
 	private TestSimpAnnotationMethodMessageHandler messageHandler;
 
 	private TestController testController;
@@ -64,7 +65,7 @@ public class SimpAnnotationMethodMessageHandlerTests {
 
 		this.messageHandler = new TestSimpAnnotationMethodMessageHandler(brokerTemplate, channel, channel);
 		this.messageHandler.setApplicationContext(new StaticApplicationContext());
-		this.messageHandler.setValidator(new StringNotEmptyValidator());
+		this.messageHandler.setValidator(new StringTestValidator(TEST_INVALID_VALUE));
 		this.messageHandler.afterPropertiesSet();
 
 		testController = new TestController();
@@ -126,7 +127,7 @@ public class SimpAnnotationMethodMessageHandlerTests {
 	public void validationError() {
 		SimpMessageHeaderAccessor headers = SimpMessageHeaderAccessor.create();
 		headers.setDestination("/pre/validation/payload");
-		Message<?> message = MessageBuilder.withPayload(new byte[0]).setHeaders(headers).build();
+		Message<?> message = MessageBuilder.withPayload(TEST_INVALID_VALUE.getBytes()).setHeaders(headers).build();
 		this.messageHandler.handleMessage(message);
 		assertEquals("handleValidationException", this.testController.method);
 	}
@@ -196,7 +197,14 @@ public class SimpAnnotationMethodMessageHandlerTests {
 		}
 	}
 
-	private static class StringNotEmptyValidator implements Validator {
+	private static class StringTestValidator implements Validator {
+
+		private final String invalidValue;
+
+		private StringTestValidator(String invalidValue) {
+			this.invalidValue = invalidValue;
+		}
+
 		@Override
 		public boolean supports(Class<?> clazz) {
 			return String.class.isAssignableFrom(clazz);
@@ -204,8 +212,8 @@ public class SimpAnnotationMethodMessageHandlerTests {
 		@Override
 		public void validate(Object target, Errors errors) {
 			String value = (String) target;
-			if (StringUtils.isEmpty(value.toString())) {
-				errors.reject("empty value");
+			if (invalidValue.equals(value)) {
+				errors.reject("invalid value '"+invalidValue+"'");
 			}
 		}
 	}