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 a15f0726fdd0..c069d96e85cf 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 @@ -49,6 +49,7 @@ import org.springframework.core.ParameterNameDiscoverer; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.function.SingletonSupplier; import org.springframework.validation.BeanPropertyBindingResult; @@ -302,7 +303,7 @@ private MethodValidationResult adaptViolations( Function argumentFunction) { Map paramViolations = new LinkedHashMap<>(); - Map beanViolations = new LinkedHashMap<>(); + Map beanViolations = new LinkedHashMap<>(); for (ConstraintViolation violation : violations) { Iterator itr = violation.getPropertyPath().iterator(); @@ -328,10 +329,37 @@ else if (node.getKind().equals(ElementKind.RETURN_VALUE)) { .addViolation(violation); } else { - Object leafBean = violation.getLeafBean(); - BeanResultKey key = new BeanResultKey(node, leafBean); + // If the argument is a container of elements, we need the specific element, + // but the only option is to check for a parent container index/key in the + // next part of the property path. + Path.Node paramNode = node; + node = itr.next(); + + Object bean; + Object container; + Integer containerIndex = node.getIndex(); + Object containerKey = node.getKey(); + if (containerIndex != null && arg instanceof List list) { + bean = list.get(containerIndex); + container = list; + } + else if (containerIndex != null && arg instanceof Object[] array) { + bean = array[containerIndex]; + container = array; + } + else if (containerKey != null && arg instanceof Map map) { + bean = map.get(containerKey); + container = map; + } + else { + Assert.state(!node.isInIterable(), "No way to unwrap Iterable without index"); + bean = arg; + container = null; + } + beanViolations - .computeIfAbsent(key, k -> new BeanResultBuilder(parameter, arg, itr.next(), leafBean)) + .computeIfAbsent(paramNode, k -> + new BeanResultBuilder(parameter, bean, container, containerIndex, containerKey)) .addViolation(violation); } break; @@ -448,13 +476,16 @@ private final class BeanResultBuilder { private final Set> violations = new LinkedHashSet<>(); - public BeanResultBuilder(MethodParameter param, @Nullable Object arg, Path.Node node, @Nullable Object leafBean) { + public BeanResultBuilder( + MethodParameter param, @Nullable Object bean, @Nullable Object container, + @Nullable Integer containerIndex, @Nullable Object containerKey) { + this.parameter = param; - this.bean = leafBean; - this.container = (arg != null && !arg.equals(leafBean) ? arg : null); - this.containerIndex = node.getIndex(); - this.containerKey = node.getKey(); - this.errors = createBindingResult(param, leafBean); + this.bean = bean; + this.container = container; + this.containerIndex = containerIndex; + this.containerKey = containerKey; + this.errors = createBindingResult(param, this.bean); } public void addViolation(ConstraintViolation violation) { @@ -470,22 +501,6 @@ public ParameterErrors build() { } - /** - * Unique key for cascaded violations associated with a bean. - *

The bean may be an element within a container such as a List, Set, array, - * Map, Optional, and others. In that case the {@link Path.Node} represents - * the container element with its index or key, if applicable, while the - * {@link ConstraintViolation#getLeafBean() leafBean} is the actual - * element instance. The pair should be unique. For example in a Set, the - * node is the same but element instances are unique. In a List or Map the - * node is further qualified by an index or key while element instances - * may be the same. - * @param node the path to the bean associated with the violation - * @param leafBean the bean instance - */ - record BeanResultKey(Path.Node node, Object leafBean) { } - - /** * Default algorithm to select an object name, as described in * {@link #setObjectNameResolver(ObjectNameResolver)}. diff --git a/spring-context/src/main/java/org/springframework/validation/method/ParameterErrors.java b/spring-context/src/main/java/org/springframework/validation/method/ParameterErrors.java index e601942bf2aa..bf8f7b1c1e49 100644 --- a/spring-context/src/main/java/org/springframework/validation/method/ParameterErrors.java +++ b/spring-context/src/main/java/org/springframework/validation/method/ParameterErrors.java @@ -32,12 +32,11 @@ * {@link Errors#getAllErrors()}, but this subclass provides access to the same * as {@link FieldError}s. * - *

When the method parameter is a container with multiple elements such as a - * {@link List}, {@link java.util.Set}, array, {@link java.util.Map}, or others, - * then a separate {@link ParameterErrors} is created for each element that has - * errors. In that case, the {@link #getContainer() container}, - * {@link #getContainerIndex() containerIndex}, and {@link #getContainerKey() containerKey} - * provide additional context. + *

When the method parameter is a container such as a {@link List}, array, + * or {@link java.util.Map}, then a separate {@link ParameterErrors} is created + * for each element that has errors. In that case, the properties + * {@link #getContainer() container}, {@link #getContainerIndex() containerIndex}, + * and {@link #getContainerKey() containerKey} provide additional context. * * @author Rossen Stoyanchev * @since 6.1 diff --git a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterPropertyPathTests.java b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterPropertyPathTests.java new file mode 100644 index 000000000000..04a09463e235 --- /dev/null +++ b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterPropertyPathTests.java @@ -0,0 +1,243 @@ +/* + * Copyright 2002-2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.validation.beanvalidation; + +import java.lang.reflect.Method; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.Size; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import org.springframework.lang.Nullable; +import org.springframework.util.ClassUtils; +import org.springframework.validation.FieldError; +import org.springframework.validation.method.MethodValidationResult; +import org.springframework.validation.method.ParameterErrors; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Test method validation scenarios with cascaded violations on different types + * of method parameters and return values. + * + * @author Rossen Stoyanchev + */ +public class MethodValidationAdapterPropertyPathTests { + + private static final Person validPerson = new Person("John"); + + private static final Person invalidPerson = new Person("Long John Silver"); + + private static final Class[] HINTS = new Class[0]; + + + private final MethodValidationAdapter validationAdapter = new MethodValidationAdapter(); + + + @Nested + class ArgumentTests { + + @Test + void fieldOfObjectPropertyOfBean() { + Method method = getMethod("addCourse"); + Object[] args = {new Course("CS 101", invalidPerson, Collections.emptyList())}; + + MethodValidationResult result = + validationAdapter.validateArguments(new MyService(), method, null, args, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, null, null, null, "professor.name", invalidPerson.name()); + } + + @Test + void fieldOfObjectPropertyOfListElement() { + Method method = getMethod("addCourseList"); + List courses = List.of(new Course("CS 101", invalidPerson, Collections.emptyList())); + + MethodValidationResult result = validationAdapter.validateArguments( + new MyService(), method, null, new Object[] {courses}, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, courses, 0, null, "professor.name", invalidPerson.name()); + } + + @Test + void fieldOfObjectPropertyOfListElements() { + Method method = getMethod("addCourseList"); + List courses = List.of( + new Course("CS 101", invalidPerson, Collections.emptyList()), + new Course("CS 102", invalidPerson, Collections.emptyList())); + + MethodValidationResult result = validationAdapter.validateArguments( + new MyService(), method, null, new Object[] {courses}, HINTS); + + assertThat(result.getAllErrors()).hasSize(2); + for (int i = 0; i < 2; i++) { + ParameterErrors errors = result.getBeanResults().get(i); + assertThat(errors.getContainerIndex()).isEqualTo(i); + assertThat(errors.getFieldError().getField()).isEqualTo("professor.name"); + } + + } + + @Test + void fieldOfObjectPropertyUnderListPropertyOfListElement() { + Method method = getMethod("addCourseList"); + Course cs101 = new Course("CS 101", invalidPerson, Collections.emptyList()); + Course cs201 = new Course("CS 201", validPerson, List.of(cs101)); + Course cs301 = new Course("CS 301", validPerson, List.of(cs201)); + List courses = List.of(cs301); + Object[] args = {courses}; + + MethodValidationResult result = + validationAdapter.validateArguments(new MyService(), method, null, args, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + + assertSingleFieldError(errors, 1, courses, 0, null, + "requiredCourses[0].requiredCourses[0].professor.name", invalidPerson.name()); + } + + @Test + void fieldOfObjectPropertyOfArrayElement() { + Method method = getMethod("addCourseArray"); + Course[] courses = new Course[] {new Course("CS 101", invalidPerson, Collections.emptyList())}; + + MethodValidationResult result = validationAdapter.validateArguments( + new MyService(), method, null, new Object[] {courses}, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, courses, 0, null, "professor.name", invalidPerson.name()); + } + + @Test + void fieldOfObjectPropertyOfMapValue() { + Method method = getMethod("addCourseMap"); + Map courses = Map.of("CS 101", new Course("CS 101", invalidPerson, Collections.emptyList())); + + MethodValidationResult result = validationAdapter.validateArguments( + new MyService(), method, null, new Object[] {courses}, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, courses, null, "CS 101", "professor.name", invalidPerson.name()); + } + + } + + + @Nested + class ReturnValueTests { + + @Test + void fieldOfObjectPropertyOfBean() { + Method method = getMethod("getCourse"); + Course course = new Course("CS 101", invalidPerson, Collections.emptyList()); + + MethodValidationResult result = + validationAdapter.validateReturnValue(new MyService(), method, null, course, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, null, null, null, "professor.name", invalidPerson.name()); + } + + @Test + void fieldOfObjectPropertyOfListElement() { + Method method = getMethod("addCourseList"); + List courses = List.of(new Course("CS 101", invalidPerson, Collections.emptyList())); + + MethodValidationResult result = validationAdapter.validateArguments( + new MyService(), method, null, new Object[] {courses}, HINTS); + + assertThat(result.getAllErrors()).hasSize(1); + ParameterErrors errors = result.getBeanResults().get(0); + assertSingleFieldError(errors, 1, courses, 0, null, "professor.name", invalidPerson.name()); + } + + } + + + private void assertSingleFieldError( + ParameterErrors errors, int errorCount, + @Nullable Object container, @Nullable Integer index, @Nullable Object key, + String field, Object rejectedValue) { + + assertThat(errors.getErrorCount()).isEqualTo(errorCount); + assertThat(errors.getErrorCount()).isEqualTo(1); + assertThat(errors.getContainer()).isEqualTo(container); + assertThat(errors.getContainerIndex()).isEqualTo(index); + assertThat(errors.getContainerKey()).isEqualTo(key); + + FieldError fieldError = errors.getFieldError(); + assertThat(fieldError).isNotNull(); + assertThat(fieldError.getField()).isEqualTo(field); + assertThat(fieldError.getRejectedValue()).isEqualTo(rejectedValue); + } + + + private static Method getMethod(String methodName) { + return ClassUtils.getMethod(MyService.class, methodName, (Class[]) null); + } + + + @SuppressWarnings("unused") + private static class MyService { + + public void addCourse(@Valid Course course) { + } + + public void addCourseList(@Valid List courses) { + } + + public void addCourseArray(@Valid Course[] courses) { + } + + public void addCourseMap(@Valid Map courses) { + } + + @Valid + public Course getCourse(Course course) { + throw new UnsupportedOperationException(); + } + + @Valid + public List getCourseList() { + throw new UnsupportedOperationException(); + } + + } + + + private record Course(@NotBlank String title, @Valid Person professor, @Valid List requiredCourses) { + } + + + @SuppressWarnings("unused") + private record Person(@Size(min = 1, max = 5) String name) { + } + +} diff --git a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterTests.java b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterTests.java index 7f367e2d13b7..db61deb272e1 100644 --- a/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterTests.java +++ b/spring-context/src/test/java/org/springframework/validation/beanvalidation/MethodValidationAdapterTests.java @@ -17,10 +17,8 @@ package org.springframework.validation.beanvalidation; import java.lang.reflect.Method; -import java.util.Collection; import java.util.List; import java.util.Locale; -import java.util.Set; import java.util.function.Consumer; import jakarta.validation.Valid; @@ -197,42 +195,6 @@ void validateListArgument() { }); } - @Test - void validateSetArgument() { - MyService target = new MyService(); - Method method = getMethod(target, "addPeople"); - - testArgs(target, method, new Object[] {Set.of(faustino1234, cayetana6789)}, ex -> { - - assertThat(ex.getAllValidationResults()).hasSize(2); - - int paramIndex = 0; - String objectName = "people"; - List results = ex.getBeanResults(); - - assertThat(results).satisfiesExactlyInAnyOrder( - result -> assertBeanResult(result, paramIndex, objectName, faustino1234, List.of(""" - Field error in object 'people' on field 'name': rejected value [Faustino1234]; \ - codes [Size.people.name,Size.name,Size.java.lang.String,Size]; \ - arguments [org.springframework.context.support.DefaultMessageSourceResolvable: \ - codes [people.name,name]; arguments []; default message [name],10,1]; \ - default message [size must be between 1 and 10]""")), - result -> assertBeanResult(result, paramIndex, objectName, cayetana6789, List.of(""" - Field error in object 'people' on field 'name': rejected value [Cayetana6789]; \ - codes [Size.people.name,Size.name,Size.java.lang.String,Size]; \ - arguments [org.springframework.context.support.DefaultMessageSourceResolvable: \ - codes [people.name,name]; arguments []; default message [name],10,1]; \ - default message [size must be between 1 and 10]""", """ - Field error in object 'people' on field 'hobbies[0]': rejected value [ ]; \ - codes [NotBlank.people.hobbies[0],NotBlank.people.hobbies,NotBlank.hobbies[0],\ - NotBlank.hobbies,NotBlank.java.lang.String,NotBlank]; arguments \ - [org.springframework.context.support.DefaultMessageSourceResolvable: codes \ - [people.hobbies[0],hobbies[0]]; arguments []; default message [hobbies[0]]]; \ - default message [must not be blank]""")) - ); - }); - } - private void testArgs(Object target, Method method, Object[] args, Consumer consumer) { consumer.accept(this.validationAdapter.validateArguments(target, method, null, args, new Class[0])); } @@ -285,7 +247,7 @@ public Person getPerson() { throw new UnsupportedOperationException(); } - public void addPeople(@Valid Collection people) { + public void addPeople(@Valid List people) { } }