From d4782647a4fda81484339433bff6ddee87c1764d Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 20 Mar 2014 19:44:19 -0400 Subject: [PATCH] Ensure default value of @Payload required is enforced When no @Payload is provided, it is equivalent to @Payload with default attribute values. Since the default value of required=true, then an argument that's not annotated is required. --- .../MethodArgumentNotValidException.java | 58 ++++++++++--- .../support/PayloadArgumentResolver.java | 47 +++++----- .../support/PayloadArgumentResolverTests.java | 86 ++++++++++++------- 3 files changed, 117 insertions(+), 74 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 e463fcaccb..a235f08ef6 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 @@ -33,31 +33,61 @@ import org.springframework.validation.ObjectError; @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() + * Create a new instance with the invalid {@code MethodParameter}. */ - public MethodArgumentNotValidException(Message message, String description) { - super(message, description); + + public MethodArgumentNotValidException(Message message, MethodParameter parameter) { + this(message, parameter, null); } /** - * Create a new instance with a failed validation described by - * the given {@link BindingResult}. + * Create a new instance with the invalid {@code MethodParameter} and a + * {@link org.springframework.validation.BindingResult}. */ - public MethodArgumentNotValidException(Message message, - MethodParameter parameter, BindingResult bindingResult) { - this(message, generateMessage(parameter, bindingResult)); + public MethodArgumentNotValidException(Message message, MethodParameter parameter, + BindingResult bindingResult) { + + super(message, generateMessage(parameter, bindingResult)); + this.parameter = parameter; + this.bindingResult = bindingResult; } + + /** + * Return the MethodParameter that was rejected. + */ + public MethodParameter getMethodParameter() { + return this.parameter; + } + + /** + * Return the BindingResult if the failure is validation-related or {@code null}. + */ + public BindingResult getBindingResult() { + return this.bindingResult; + } + + private static String generateMessage(MethodParameter parameter, BindingResult bindingResult) { - StringBuilder sb = new StringBuilder("Validation failed for parameter at index ") + + StringBuilder sb = new StringBuilder("Invalid parameter at index ") .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("] "); + .append(parameter.getMethod().toGenericString()); + + + if (bindingResult != null) { + sb.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 c8cafcd09e..f6f9e4d244 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 @@ -27,6 +27,8 @@ import org.springframework.util.ClassUtils; import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import org.springframework.validation.BeanPropertyBindingResult; +import org.springframework.validation.BindingResult; +import org.springframework.validation.ObjectError; import org.springframework.validation.SmartValidator; import org.springframework.validation.Validator; @@ -65,16 +67,21 @@ public class PayloadArgumentResolver implements HandlerMethodArgumentResolver { } @Override - public Object resolveArgument(MethodParameter parameter, Message message) throws Exception { - Payload annot = parameter.getParameterAnnotation(Payload.class); + public Object resolveArgument(MethodParameter param, Message message) throws Exception { + + Payload annot = param.getParameterAnnotation(Payload.class); if ((annot != null) && StringUtils.hasText(annot.value())) { throw new IllegalStateException("@Payload SpEL expressions not supported by this resolver."); } - Object target = getTargetPayload(parameter, message); - if (annot != null && isEmptyPayload(target)) { - if (annot.required()) { - throw new MethodArgumentNotValidException(message, createPayloadRequiredExceptionMessage(parameter, target)); + Object target = getTargetPayload(param, message); + if (isEmptyPayload(target)) { + if (annot == null || annot.required()) { + String paramName = param.getParameterName(); + paramName = (paramName == null ? "Arg" + param.getParameterIndex() : paramName); + BindingResult bindingResult = new BeanPropertyBindingResult(target, paramName); + bindingResult.addError(new ObjectError(paramName, "@Payload param is required")); + throw new MethodArgumentNotValidException(message, param, bindingResult); } else { return null; @@ -82,17 +89,18 @@ public class PayloadArgumentResolver implements HandlerMethodArgumentResolver { } if (annot != null) { // Only validate @Payload - validate(message, parameter, target); + validate(message, param, target); } return target; } /** - * 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}. + * Return the payload for the specified message, which can be the payload + * itself if it matches the parameter type or the result of message conversion + * otherwise. + * + *

While the payload of a {@link Message} cannot be {@code null} by design, + * this method may return {@code null} if the message converter returns that. */ protected Object getTargetPayload(MethodParameter parameter, Message message) { Class sourceClass = message.getPayload().getClass(); @@ -146,19 +154,4 @@ 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 9928988d8a..e02f81966f 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 @@ -46,33 +46,52 @@ import org.springframework.validation.annotation.Validated; */ public class PayloadArgumentResolverTests { - @Rule - public final ExpectedException thrown = ExpectedException.none(); - private PayloadArgumentResolver resolver; private Method payloadMethod; - private Method simpleMethod; + + private MethodParameter paramAnnotated; + + private MethodParameter paramAnnotatedNotRequired; + + private MethodParameter paramAnnotatedRequired; + + private MethodParameter paramWithSpelExpression; + + private MethodParameter paramNotAnnotated; + + private MethodParameter paramValidatedNotAnnotated; + private MethodParameter paramValidated; + + @Rule + public final ExpectedException thrown = ExpectedException.none(); + + @Before public void setup() throws Exception { + this.resolver = new PayloadArgumentResolver(new StringMessageConverter(), testValidator()); payloadMethod = PayloadArgumentResolverTests.class.getDeclaredMethod("handleMessage", - String.class, String.class, Locale.class, String.class, String.class); - simpleMethod = PayloadArgumentResolverTests.class.getDeclaredMethod("handleAnotherMessage", - String.class, String.class); + String.class, String.class, Locale.class, String.class, String.class, String.class, String.class); - this.paramValidated = getMethodParameter(payloadMethod, 4); + this.paramAnnotated = getMethodParameter(this.payloadMethod, 0); + this.paramAnnotatedNotRequired = getMethodParameter(this.payloadMethod, 1); + this.paramAnnotatedRequired = getMethodParameter(payloadMethod, 2); + this.paramWithSpelExpression = getMethodParameter(payloadMethod, 3); + this.paramValidated = getMethodParameter(this.payloadMethod, 4); this.paramValidated.initParameterNameDiscovery(new LocalVariableTableParameterNameDiscoverer()); + this.paramValidatedNotAnnotated = getMethodParameter(this.payloadMethod, 5); + this.paramNotAnnotated = getMethodParameter(this.payloadMethod, 6); } @Test public void resolveRequired() throws Exception { Message message = MessageBuilder.withPayload("ABC".getBytes()).build(); - Object actual = this.resolver.resolveArgument(getMethodParameter(payloadMethod, 0), message); + Object actual = this.resolver.resolveArgument(paramAnnotated, message); assertEquals("ABC", actual); } @@ -82,21 +101,28 @@ public class PayloadArgumentResolverTests { Message message = MessageBuilder.withPayload("").build(); thrown.expect(MethodArgumentNotValidException.class); // Required but empty - this.resolver.resolveArgument(getMethodParameter(payloadMethod, 0), message); + this.resolver.resolveArgument(paramAnnotated, message); + } + + @Test + public void resolveRequiredEmptyNonAnnotatedParameter() throws Exception { + Message message = MessageBuilder.withPayload("").build(); + + thrown.expect(MethodArgumentNotValidException.class); // Required but empty + this.resolver.resolveArgument(this.paramNotAnnotated, message); } @Test public void resolveNotRequired() throws Exception { - MethodParameter paramNotRequired = getMethodParameter(payloadMethod, 1); Message emptyByteArrayMessage = MessageBuilder.withPayload(new byte[0]).build(); - assertNull(this.resolver.resolveArgument(paramNotRequired, emptyByteArrayMessage)); + assertNull(this.resolver.resolveArgument(this.paramAnnotatedNotRequired, emptyByteArrayMessage)); Message emptyStringMessage = MessageBuilder.withPayload("").build(); - assertNull(this.resolver.resolveArgument(paramNotRequired, emptyStringMessage)); + assertNull(this.resolver.resolveArgument(this.paramAnnotatedNotRequired, emptyStringMessage)); Message notEmptyMessage = MessageBuilder.withPayload("ABC".getBytes()).build(); - assertEquals("ABC", this.resolver.resolveArgument(paramNotRequired, notEmptyMessage)); + assertEquals("ABC", this.resolver.resolveArgument(this.paramAnnotatedNotRequired, notEmptyMessage)); } @Test @@ -106,7 +132,7 @@ public class PayloadArgumentResolverTests { // 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); + this.resolver.resolveArgument(this.paramAnnotatedRequired, notEmptyMessage); } @Test @@ -114,7 +140,7 @@ public class PayloadArgumentResolverTests { Message message = MessageBuilder.withPayload("ABC".getBytes()).build(); thrown.expect(IllegalStateException.class); - this.resolver.resolveArgument(getMethodParameter(payloadMethod, 3), message); + this.resolver.resolveArgument(paramWithSpelExpression, message); } @Test @@ -142,17 +168,14 @@ public class PayloadArgumentResolverTests { @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)); + assertEquals("ABC", this.resolver.resolveArgument(this.paramNotAnnotated, notEmptyMessage)); + + Message emptyStringMessage = MessageBuilder.withPayload("").build(); + thrown.expect(MethodArgumentNotValidException.class); + this.resolver.resolveArgument(this.paramValidated, emptyStringMessage); + } @Test @@ -160,7 +183,8 @@ public class PayloadArgumentResolverTests { // See testValidator() Message message = MessageBuilder.withPayload("invalidValue".getBytes()).build(); - assertEquals("invalidValue", this.resolver.resolveArgument(getMethodParameter(simpleMethod, 1), message)); + assertEquals("invalidValue", + this.resolver.resolveArgument(paramValidatedNotAnnotated, message)); } private Validator testValidator() { @@ -191,13 +215,9 @@ public class PayloadArgumentResolverTests { @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) { + @Validated @Payload String validParam, + @Validated String validParamNotAnnotated, + String paramNotAnnotated) { } }