From 976b4f353387f10d8f4f406236348d8989c636ea Mon Sep 17 00:00:00 2001 From: vatsal Date: Wed, 26 Jun 2024 20:00:01 -0700 Subject: [PATCH 1/2] Fix return value validation Fix argument in call to applyReturnValueValidation() method in MethodValidationInterceptor.java. Method argument was passed instead of the return value of the method that was being validated. See gh-33105 --- .../MethodValidationInterceptor.java | 2 +- .../MethodValidationProxyTests.java | 91 ++++++++++++++++--- 2 files changed, 81 insertions(+), 12 deletions(-) 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 fb791ae5f3..5023615270 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 @@ -174,7 +174,7 @@ public class MethodValidationInterceptor implements MethodInterceptor { Object returnValue = invocation.proceed(); if (this.adaptViolations) { - this.validationAdapter.applyReturnValueValidation(target, method, null, arguments, groups); + this.validationAdapter.applyReturnValueValidation(target, method, null, returnValue, groups); } else { violations = this.validationAdapter.invokeValidatorForReturnValue(target, method, returnValue, groups); diff --git a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java index cc33a5162f..b2fb1f0fc1 100644 --- a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java +++ b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java @@ -20,8 +20,10 @@ import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.reflect.Method; -import jakarta.validation.ValidationException; +import jakarta.validation.ConstraintViolationException; +import jakarta.validation.Validation; import jakarta.validation.Validator; +import jakarta.validation.ValidatorFactory; import jakarta.validation.constraints.Max; import jakarta.validation.constraints.NotNull; import jakarta.validation.groups.Default; @@ -45,7 +47,9 @@ import org.springframework.scheduling.annotation.AsyncAnnotationAdvisor; import org.springframework.scheduling.annotation.AsyncAnnotationBeanPostProcessor; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; +import org.springframework.util.function.SingletonSupplier; import org.springframework.validation.annotation.Validated; +import org.springframework.validation.method.MethodValidationException; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -66,7 +70,19 @@ class MethodValidationProxyTests { ProxyFactory factory = new ProxyFactory(bean); factory.addAdvice(new MethodValidationInterceptor()); factory.addAdvisor(new AsyncAnnotationAdvisor()); - doTestProxyValidation((MyValidInterface) factory.getProxy()); + doTestProxyValidation((MyValidInterface) factory.getProxy(), ConstraintViolationException.class); + } + + @Test + @SuppressWarnings("unchecked") + public void testMethodValidationInterceptorWithAdaptConstraintViolations() { + MyValidBean bean = new MyValidBean(); + ProxyFactory factory = new ProxyFactory(bean); + try (ValidatorFactory validatorFactory = Validation.buildDefaultValidatorFactory()) { + factory.addAdvice(new MethodValidationInterceptor(SingletonSupplier.of(validatorFactory.getValidator()), true)); + factory.addAdvisor(new AsyncAnnotationAdvisor()); + doTestProxyValidation((MyValidInterface) factory.getProxy(), MethodValidationException.class); + } } @Test @@ -79,7 +95,26 @@ class MethodValidationProxyTests { context.registerSingleton("aapp", AsyncAnnotationBeanPostProcessor.class, pvs); context.registerSingleton("bean", MyValidBean.class); context.refresh(); - doTestProxyValidation(context.getBean("bean", MyValidInterface.class)); + doTestProxyValidation(context.getBean("bean", MyValidInterface.class), ConstraintViolationException.class); + context.close(); + } + + @Test + @SuppressWarnings("unchecked") + public void testMethodValidationPostProcessorWithAdaptConstraintViolations() { + StaticApplicationContext context = new StaticApplicationContext(); + context.registerBean(MethodValidationPostProcessor.class, () -> { + MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); + postProcessor.setAdaptConstraintViolations(true); + return postProcessor; + }); + + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("beforeExistingAdvisors", false); + context.registerSingleton("aapp", AsyncAnnotationBeanPostProcessor.class, pvs); + context.registerSingleton("bean", MyValidBean.class); + context.refresh(); + doTestProxyValidation(context.getBean("bean", MyValidInterface.class), MethodValidationException.class); context.close(); } @@ -90,21 +125,38 @@ class MethodValidationProxyTests { context.registerBean(MyValidInterface.class, () -> ProxyFactory.getProxy(MyValidInterface.class, new MyValidClientInterfaceMethodInterceptor())); context.refresh(); - doTestProxyValidation(context.getBean(MyValidInterface.class)); + doTestProxyValidation(context.getBean(MyValidInterface.class), ConstraintViolationException.class); + context.close(); + } + + @Test + @SuppressWarnings("unchecked") + public void testMethodValidationPostProcessorForInterfaceOnlyProxyWithAdaptConstraintViolations() { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); + context.registerBean(MethodValidationPostProcessor.class, () -> { + MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); + postProcessor.setAdaptConstraintViolations(true); + return postProcessor; + }); + + context.registerBean(MyValidInterface.class, () -> + ProxyFactory.getProxy(MyValidInterface.class, new MyValidClientInterfaceMethodInterceptor())); + context.refresh(); + doTestProxyValidation(context.getBean(MyValidInterface.class), MethodValidationException.class); context.close(); } @SuppressWarnings("DataFlowIssue") - private void doTestProxyValidation(MyValidInterface proxy) { + private void doTestProxyValidation(MyValidInterface proxy, Class expectedExceptionClass) { assertThat(proxy.myValidMethod("value", 5)).isNotNull(); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myValidMethod("value", 15)); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myValidMethod(null, 5)); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myValidMethod("value", 0)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myValidMethod("value", 15)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myValidMethod(null, 5)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myValidMethod("value", 0)); proxy.myValidAsyncMethod("value", 5); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myValidAsyncMethod("value", 15)); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myValidAsyncMethod(null, 5)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myValidAsyncMethod("value", 15)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myValidAsyncMethod(null, 5)); assertThat(proxy.myGenericMethod("myValue")).isEqualTo("myValue"); - assertThatExceptionOfType(ValidationException.class).isThrownBy(() -> proxy.myGenericMethod(null)); + assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> proxy.myGenericMethod(null)); } @Test @@ -122,6 +174,11 @@ class MethodValidationProxyTests { doTestLazyValidatorForMethodValidation(LazyMethodValidationConfigWithValidatorProvider.class); } + @Test + void testLazyValidatorForMethodValidationWithAdaptConstraintViolations() { + doTestLazyValidatorForMethodValidation(LazyMethodValidationConfigWithAdaptConstraintViolations.class); + } + private void doTestLazyValidatorForMethodValidation(Class configClass) { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); context.register(configClass, CustomValidatorBean.class, MyValidBean.class, MyValidFactoryBean.class); @@ -278,4 +335,16 @@ class MethodValidationProxyTests { } } + @Configuration + public static class LazyMethodValidationConfigWithAdaptConstraintViolations { + + @Bean + public static MethodValidationPostProcessor methodValidationPostProcessor(ObjectProvider validator) { + MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); + postProcessor.setValidatorProvider(validator); + postProcessor.setAdaptConstraintViolations(true); + return postProcessor; + } + } + } From c74666a883faf88d62159a6330c9b87229513d37 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 28 Jun 2024 15:32:43 +0100 Subject: [PATCH 2/2] Polishing contribution Closes gh-33105 --- .../MethodValidationProxyTests.java | 99 +++++-------------- 1 file changed, 23 insertions(+), 76 deletions(-) diff --git a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java index b2fb1f0fc1..90224491f4 100644 --- a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java +++ b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationProxyTests.java @@ -23,13 +23,14 @@ import java.lang.reflect.Method; import jakarta.validation.ConstraintViolationException; import jakarta.validation.Validation; import jakarta.validation.Validator; -import jakarta.validation.ValidatorFactory; import jakarta.validation.constraints.Max; import jakarta.validation.constraints.NotNull; import jakarta.validation.groups.Default; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.MutablePropertyValues; @@ -47,7 +48,6 @@ import org.springframework.scheduling.annotation.AsyncAnnotationAdvisor; import org.springframework.scheduling.annotation.AsyncAnnotationBeanPostProcessor; import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; -import org.springframework.util.function.SingletonSupplier; import org.springframework.validation.annotation.Validated; import org.springframework.validation.method.MethodValidationException; @@ -63,63 +63,44 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; */ class MethodValidationProxyTests { - @Test + @ParameterizedTest + @ValueSource(booleans = {true, false}) @SuppressWarnings("unchecked") - public void testMethodValidationInterceptor() { + void testMethodValidationInterceptor(boolean adaptViolations) { MyValidBean bean = new MyValidBean(); ProxyFactory factory = new ProxyFactory(bean); - factory.addAdvice(new MethodValidationInterceptor()); + factory.addAdvice(adaptViolations ? + new MethodValidationInterceptor(() -> Validation.buildDefaultValidatorFactory().getValidator(), true) : + new MethodValidationInterceptor()); factory.addAdvisor(new AsyncAnnotationAdvisor()); - doTestProxyValidation((MyValidInterface) factory.getProxy(), ConstraintViolationException.class); + doTestProxyValidation((MyValidInterface) factory.getProxy(), + (adaptViolations ? MethodValidationException.class : ConstraintViolationException.class)); } - @Test + @ParameterizedTest + @ValueSource(booleans = {true, false}) @SuppressWarnings("unchecked") - public void testMethodValidationInterceptorWithAdaptConstraintViolations() { - MyValidBean bean = new MyValidBean(); - ProxyFactory factory = new ProxyFactory(bean); - try (ValidatorFactory validatorFactory = Validation.buildDefaultValidatorFactory()) { - factory.addAdvice(new MethodValidationInterceptor(SingletonSupplier.of(validatorFactory.getValidator()), true)); - factory.addAdvisor(new AsyncAnnotationAdvisor()); - doTestProxyValidation((MyValidInterface) factory.getProxy(), MethodValidationException.class); - } - } - - @Test - @SuppressWarnings("unchecked") - public void testMethodValidationPostProcessor() { + void testMethodValidationPostProcessor(boolean adaptViolations) { StaticApplicationContext context = new StaticApplicationContext(); - context.registerSingleton("mvpp", MethodValidationPostProcessor.class); + context.registerBean(MethodValidationPostProcessor.class, adaptViolations ? + () -> { + MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); + postProcessor.setAdaptConstraintViolations(true); + return postProcessor; + } : + MethodValidationPostProcessor::new); MutablePropertyValues pvs = new MutablePropertyValues(); pvs.add("beforeExistingAdvisors", false); context.registerSingleton("aapp", AsyncAnnotationBeanPostProcessor.class, pvs); context.registerSingleton("bean", MyValidBean.class); context.refresh(); - doTestProxyValidation(context.getBean("bean", MyValidInterface.class), ConstraintViolationException.class); - context.close(); - } - - @Test - @SuppressWarnings("unchecked") - public void testMethodValidationPostProcessorWithAdaptConstraintViolations() { - StaticApplicationContext context = new StaticApplicationContext(); - context.registerBean(MethodValidationPostProcessor.class, () -> { - MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); - postProcessor.setAdaptConstraintViolations(true); - return postProcessor; - }); - - MutablePropertyValues pvs = new MutablePropertyValues(); - pvs.add("beforeExistingAdvisors", false); - context.registerSingleton("aapp", AsyncAnnotationBeanPostProcessor.class, pvs); - context.registerSingleton("bean", MyValidBean.class); - context.refresh(); - doTestProxyValidation(context.getBean("bean", MyValidInterface.class), MethodValidationException.class); + doTestProxyValidation(context.getBean("bean", MyValidInterface.class), + adaptViolations ? MethodValidationException.class : ConstraintViolationException.class); context.close(); } @Test // gh-29782 - public void testMethodValidationPostProcessorForInterfaceOnlyProxy() { + void testMethodValidationPostProcessorForInterfaceOnlyProxy() { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); context.register(MethodValidationPostProcessor.class); context.registerBean(MyValidInterface.class, () -> @@ -129,23 +110,6 @@ class MethodValidationProxyTests { context.close(); } - @Test - @SuppressWarnings("unchecked") - public void testMethodValidationPostProcessorForInterfaceOnlyProxyWithAdaptConstraintViolations() { - AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); - context.registerBean(MethodValidationPostProcessor.class, () -> { - MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); - postProcessor.setAdaptConstraintViolations(true); - return postProcessor; - }); - - context.registerBean(MyValidInterface.class, () -> - ProxyFactory.getProxy(MyValidInterface.class, new MyValidClientInterfaceMethodInterceptor())); - context.refresh(); - doTestProxyValidation(context.getBean(MyValidInterface.class), MethodValidationException.class); - context.close(); - } - @SuppressWarnings("DataFlowIssue") private void doTestProxyValidation(MyValidInterface proxy, Class expectedExceptionClass) { assertThat(proxy.myValidMethod("value", 5)).isNotNull(); @@ -174,11 +138,6 @@ class MethodValidationProxyTests { doTestLazyValidatorForMethodValidation(LazyMethodValidationConfigWithValidatorProvider.class); } - @Test - void testLazyValidatorForMethodValidationWithAdaptConstraintViolations() { - doTestLazyValidatorForMethodValidation(LazyMethodValidationConfigWithAdaptConstraintViolations.class); - } - private void doTestLazyValidatorForMethodValidation(Class configClass) { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(); context.register(configClass, CustomValidatorBean.class, MyValidBean.class, MyValidFactoryBean.class); @@ -335,16 +294,4 @@ class MethodValidationProxyTests { } } - @Configuration - public static class LazyMethodValidationConfigWithAdaptConstraintViolations { - - @Bean - public static MethodValidationPostProcessor methodValidationPostProcessor(ObjectProvider validator) { - MethodValidationPostProcessor postProcessor = new MethodValidationPostProcessor(); - postProcessor.setValidatorProvider(validator); - postProcessor.setAdaptConstraintViolations(true); - return postProcessor; - } - } - }