From a8ea472121d859ebc063f6e22516d4f739100b9d Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 28 Jun 2023 19:08:39 +0100 Subject: [PATCH] Refactoring in MethodValidationResult Remove throwIfViolationsPresent and replace with static factory methods on MethodValidationException taking MethodValidationResult, which makes handling more explicit and allows choice of what exception to raise. Update MethodValidationResult to expose the target, the method, and forReturnValue flag, so the code handling an exception will have access to all details. See gh-30644 --- .../DefaultMethodValidator.java | 52 ++++++------ .../MethodValidationAdapter.java | 48 ++--------- .../MethodValidationException.java | 81 ++++++++++++------- .../MethodValidationInterceptor.java | 14 +++- .../MethodValidationResult.java | 32 ++++++-- .../annotation/HandlerMethodValidator.java | 11 +-- 6 files changed, 125 insertions(+), 113 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/validation/beanvalidation/DefaultMethodValidator.java b/spring-context/src/main/java/org/springframework/validation/beanvalidation/DefaultMethodValidator.java index 1e937661a0..1588056191 100644 --- a/spring-context/src/main/java/org/springframework/validation/beanvalidation/DefaultMethodValidator.java +++ b/spring-context/src/main/java/org/springframework/validation/beanvalidation/DefaultMethodValidator.java @@ -46,47 +46,53 @@ public class DefaultMethodValidator implements MethodValidator { @Override public void validateArguments( - Object target, Method method, @Nullable MethodParameter[] parameters, Object[] arguments, - Class[] groups) { + Object target, Method method, @Nullable MethodParameter[] parameters, + Object[] arguments, Class[] groups) { - handleArgumentsValidationResult(target, method, arguments, groups, - this.adapter.validateMethodArguments(target, method, parameters, arguments, groups)); - } + MethodValidationResult validationResult = + this.adapter.validateMethodArguments(target, method, parameters, arguments, groups); - public void validateReturnValue( - Object target, Method method, @Nullable MethodParameter returnType, @Nullable Object returnValue, - Class[] groups) { - - handleReturnValueValidationResult(target, method, returnValue, groups, - this.adapter.validateMethodReturnValue(target, method, returnType, returnValue, groups)); + handleArgumentsResult(arguments, groups, validationResult); } /** * Subclasses can override this to handle the result of argument validation. - * By default, {@link MethodValidationResult#throwIfViolationsPresent()} is called. - * @param bean the target Object for method invocation - * @param method the target method + * By default, throws {@link MethodValidationException} if there are errors. * @param arguments the candidate argument values to validate * @param groups groups for validation determined via + * @param validationResult the result from validation */ - protected void handleArgumentsValidationResult( - Object bean, Method method, Object[] arguments, Class[] groups, MethodValidationResult result) { + protected void handleArgumentsResult( + Object[] arguments, Class[] groups, MethodValidationResult validationResult) { - result.throwIfViolationsPresent(); + if (validationResult.hasViolations()) { + throw MethodValidationException.forResult(validationResult); + } + } + + public void validateReturnValue( + Object target, Method method, @Nullable MethodParameter returnType, + @Nullable Object returnValue, Class[] groups) { + + MethodValidationResult validationResult = + this.adapter.validateMethodReturnValue(target, method, returnType, returnValue, groups); + + handleReturnValueResult(returnValue, groups, validationResult); } /** * Subclasses can override this to handle the result of return value validation. - * By default, {@link MethodValidationResult#throwIfViolationsPresent()} is called. - * @param bean the target Object for method invocation - * @param method the target method + * By default, throws {@link MethodValidationException} if there are errors. * @param returnValue the return value to validate * @param groups groups for validation determined via + * @param validationResult the result from validation */ - protected void handleReturnValueValidationResult( - Object bean, Method method, @Nullable Object returnValue, Class[] groups, MethodValidationResult result) { + protected void handleReturnValueResult( + @Nullable Object returnValue, Class[] groups, MethodValidationResult validationResult) { - result.throwIfViolationsPresent(); + if (validationResult.hasViolations()) { + throw MethodValidationException.forResult(validationResult); + } } } diff --git a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationAdapter.java b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationAdapter.java index 7ea1936672..2b349651df 100644 --- a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationAdapter.java +++ b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationAdapter.java @@ -18,7 +18,6 @@ package org.springframework.validation.beanvalidation; import java.lang.reflect.Method; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; import java.util.Iterator; import java.util.LinkedHashMap; @@ -74,8 +73,6 @@ public class MethodValidationAdapter { private static final Comparator RESULT_COMPARATOR = new ResultComparator(); - private static final MethodValidationResult EMPTY_RESULT = new EmptyMethodValidationResult(); - private final Supplier validator; @@ -232,7 +229,8 @@ public class MethodValidationAdapter { Method bridgedMethod = BridgeMethodResolver.findBridgedMethod(mostSpecificMethod); result = execVal.validateParameters(target, bridgedMethod, arguments, groups); } - return (result.isEmpty() ? EMPTY_RESULT : + return (result.isEmpty() ? + MethodValidationException.forEmptyResult(target, method, true) : createException(target, method, result, i -> parameters != null ? parameters[i] : new MethodParameter(method, i), i -> arguments[i], @@ -257,7 +255,8 @@ public class MethodValidationAdapter { ExecutableValidator execVal = this.validator.get().forExecutables(); Set> result = execVal.validateReturnValue(target, method, returnValue, groups); - return (result.isEmpty() ? EMPTY_RESULT : + return (result.isEmpty() ? + MethodValidationException.forEmptyResult(target, method, true) : createException(target, method, result, i -> returnType != null ? returnType : new MethodParameter(method, -1), i -> returnValue, @@ -310,7 +309,7 @@ public class MethodValidationAdapter { cascadedViolations.forEach((node, builder) -> validatonResultList.add(builder.build())); validatonResultList.sort(RESULT_COMPARATOR); - return new MethodValidationException(target, method, violations, validatonResultList, forReturnValue); + return new MethodValidationException(target, method, forReturnValue, violations, validatonResultList); } /** @@ -533,41 +532,4 @@ public class MethodValidationAdapter { } } - - /** - * {@link MethodValidationResult} to use when there are no violations. - */ - private static final class EmptyMethodValidationResult implements MethodValidationResult { - - @Override - public Set> getConstraintViolations() { - return Collections.emptySet(); - } - - @Override - public List getAllValidationResults() { - return Collections.emptyList(); - } - - @Override - public List getValueResults() { - return Collections.emptyList(); - } - - @Override - public List getBeanResults() { - return Collections.emptyList(); - } - - @Override - public void throwIfViolationsPresent() { - } - - @Override - public String toString() { - return "MethodValidationResult (0 violations)"; - } - - } - } diff --git a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationException.java b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationException.java index 6f2bb6f0f8..a452e0f772 100644 --- a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationException.java +++ b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationException.java @@ -17,6 +17,7 @@ package org.springframework.validation.beanvalidation; import java.lang.reflect.Method; +import java.util.Collections; import java.util.List; import java.util.Set; @@ -32,7 +33,6 @@ import org.springframework.util.Assert; * {@link org.springframework.context.MessageSourceResolvable} and grouped by * method parameter. * - * * @author Rossen Stoyanchev * @since 6.1 * @see ParameterValidationResult @@ -51,49 +51,55 @@ public class MethodValidationException extends ConstraintViolationException impl private final boolean forReturnValue; - public MethodValidationException( - Object target, Method method, Set> violations, - List validationResults, boolean forReturnValue) { + /** + * Package private constructor for {@link MethodValidationAdapter}. + */ + MethodValidationException( + Object target, Method method, boolean forReturnValue, + Set> violations, List results) { super(violations); - Assert.notEmpty(violations, "'violations' must not be empty"); + + Assert.notNull(violations, "'violations' is required"); + Assert.notNull(results, "'results' is required"); + this.target = target; this.method = method; - this.allValidationResults = validationResults; + this.allValidationResults = results; this.forReturnValue = forReturnValue; } - /** - * Return the target of the method invocation to which validation was applied. + * Private constructor copying from another {@code MethodValidationResult}. */ - public Object getTarget() { - return this.target; + private MethodValidationException(MethodValidationResult other) { + this(other.getTarget(), other.getMethod(), other.isForReturnValue(), + other.getConstraintViolations(), other.getAllValidationResults()); } - /** - * Return the method to which validation was applied. - */ - public Method getMethod() { - return this.method; - } - /** - * Whether the violations are for a return value. - * If true the violations are from validating a return value. - * If false the violations are from validating method arguments. - */ - public boolean isForReturnValue() { - return this.forReturnValue; - } - - // re-declare parent class method for NonNull treatment of interface + // re-declare getConstraintViolations as NonNull @Override public Set> getConstraintViolations() { return super.getConstraintViolations(); } + @Override + public Object getTarget() { + return this.target; + } + + @Override + public Method getMethod() { + return this.method; + } + + @Override + public boolean isForReturnValue() { + return this.forReturnValue; + } + @Override public List getAllValidationResults() { return this.allValidationResults; @@ -114,15 +120,28 @@ public class MethodValidationException extends ConstraintViolationException impl .toList(); } - @Override - public void throwIfViolationsPresent() { - throw this; - } - @Override public String toString() { return "MethodValidationResult (" + getConstraintViolations().size() + " violations) " + "for " + this.method.toGenericString(); } + + /** + * Create an exception copying from the given result, or return the same + * instance if it is a {@code MethodValidationException} already. + */ + public static MethodValidationException forResult(MethodValidationResult result) { + return (result instanceof MethodValidationException ex ? ex : new MethodValidationException(result)); + } + + /** + * Create an exception for validation without errors. + */ + public static MethodValidationException forEmptyResult(Object target, Method method, boolean forReturnValue) { + return new MethodValidationException( + target, method, forReturnValue, Collections.emptySet(), Collections.emptyList()); + } + + } diff --git a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationInterceptor.java b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationInterceptor.java index c596463a52..da2658e785 100644 --- a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationInterceptor.java +++ b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationInterceptor.java @@ -104,13 +104,19 @@ public class MethodValidationInterceptor implements MethodInterceptor { Method method = invocation.getMethod(); Class[] groups = determineValidationGroups(invocation); - this.delegate.validateMethodArguments(target, method, null, invocation.getArguments(), groups) - .throwIfViolationsPresent(); + MethodValidationResult result; + + result = this.delegate.validateMethodArguments(target, method, null, invocation.getArguments(), groups); + if (result.hasViolations()) { + throw MethodValidationException.forResult(result); + } Object returnValue = invocation.proceed(); - this.delegate.validateMethodReturnValue(target, method, null, returnValue, groups) - .throwIfViolationsPresent(); + result = this.delegate.validateMethodReturnValue(target, method, null, returnValue, groups); + if (result.hasViolations()) { + throw MethodValidationException.forResult(result); + } return returnValue; } diff --git a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationResult.java b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationResult.java index f197417ca9..33cc7b0433 100644 --- a/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationResult.java +++ b/spring-context/src/main/java/org/springframework/validation/beanvalidation/MethodValidationResult.java @@ -16,6 +16,7 @@ package org.springframework.validation.beanvalidation; +import java.lang.reflect.Method; import java.util.List; import java.util.Set; @@ -39,6 +40,30 @@ import jakarta.validation.ConstraintViolation; */ public interface MethodValidationResult { + /** + * Return the target of the method invocation to which validation was applied. + */ + Object getTarget(); + + /** + * Return the method to which validation was applied. + */ + Method getMethod(); + + /** + * Whether the violations are for a return value. + * If true the violations are from validating a return value. + * If false the violations are from validating method arguments. + */ + boolean isForReturnValue(); + + /** + * Whether the result contains any {@link ConstraintViolation}s. + */ + default boolean hasViolations() { + return !getConstraintViolations().isEmpty(); + } + /** * Returns the set of constraint violations reported during a validation. * @return the {@code Set} of {@link ConstraintViolation}s, or an empty Set @@ -72,11 +97,4 @@ public interface MethodValidationResult { */ List getBeanResults(); - /** - * Check if {@link #getConstraintViolations()} is empty, and if not, raise - * {@link MethodValidationException}. - * @throws MethodValidationException if the result contains any violations - */ - void throwIfViolationsPresent(); - } diff --git a/spring-web/src/main/java/org/springframework/web/method/annotation/HandlerMethodValidator.java b/spring-web/src/main/java/org/springframework/web/method/annotation/HandlerMethodValidator.java index 6478c8d893..41c5451fe1 100644 --- a/spring-web/src/main/java/org/springframework/web/method/annotation/HandlerMethodValidator.java +++ b/spring-web/src/main/java/org/springframework/web/method/annotation/HandlerMethodValidator.java @@ -16,8 +16,6 @@ package org.springframework.web.method.annotation; -import java.lang.reflect.Method; - import jakarta.validation.Validator; import org.springframework.core.Conventions; @@ -28,6 +26,7 @@ import org.springframework.validation.BindingResult; import org.springframework.validation.MessageCodesResolver; import org.springframework.validation.beanvalidation.DefaultMethodValidator; import org.springframework.validation.beanvalidation.MethodValidationAdapter; +import org.springframework.validation.beanvalidation.MethodValidationException; import org.springframework.validation.beanvalidation.MethodValidationResult; import org.springframework.validation.beanvalidation.MethodValidator; import org.springframework.validation.beanvalidation.ParameterErrors; @@ -53,8 +52,8 @@ public final class HandlerMethodValidator extends DefaultMethodValidator { @Override - protected void handleArgumentsValidationResult( - Object bean, Method method, Object[] arguments, Class[] groups, MethodValidationResult result) { + protected void handleArgumentsResult( + Object[] arguments, Class[] groups, MethodValidationResult result) { if (result.getConstraintViolations().isEmpty()) { return; @@ -76,7 +75,9 @@ public final class HandlerMethodValidator extends DefaultMethodValidator { return; } } - result.throwIfViolationsPresent(); + if (result.hasViolations()) { + throw MethodValidationException.forResult(result); + } } private String determineObjectName(MethodParameter param, @Nullable Object argument) {