From 52c3f713bf0f467a43fa55c9be425447d371531b Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Wed, 19 Mar 2014 17:09:41 -0700 Subject: [PATCH] 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 --- .../MethodArgumentNotValidException.java | 38 +++--- .../support/PayloadArgumentResolver.java | 73 ++++++++--- .../support/PayloadArgumentResolverTests.java | 122 ++++++++++++++---- ...mpAnnotationMethodMessageHandlerTests.java | 20 ++- 4 files changed, 188 insertions(+), 65 deletions(-) diff --git a/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/MethodArgumentNotValidException.java b/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/MethodArgumentNotValidException.java index 06d0f838a7..e463fcaccb 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/MethodArgumentNotValidException.java +++ b/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(); diff --git a/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolver.java b/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolver.java index ba1fa9c8eb..c8cafcd09e 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolver.java +++ b/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(); + } + } diff --git a/spring-messaging/src/test/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolverTests.java b/spring-messaging/src/test/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolverTests.java index acf88bcf8c..9928988d8a 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/handler/annotation/support/PayloadArgumentResolverTests.java +++ b/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) { + } + } diff --git a/spring-messaging/src/test/java/org/springframework/messaging/simp/annotation/support/SimpAnnotationMethodMessageHandlerTests.java b/spring-messaging/src/test/java/org/springframework/messaging/simp/annotation/support/SimpAnnotationMethodMessageHandlerTests.java index b118c6dcbf..559892273c 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/simp/annotation/support/SimpAnnotationMethodMessageHandlerTests.java +++ b/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+"'"); } } }