HV-1946 Add an empty path node for iterable/multivalued container ele…

…ments
hibernate · May 4, 2023 · a34b15e · a34b15e
1 parent 6a932fe
commit a34b15e
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 10 deletions.
diff --git a/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java b/engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java
@@ -722,9 +722,7 @@ private void doValidate(Object value, String nodeName) {
 					cascadedValueContext.setTypeParameter( cascadingMetaData.getDeclaredContainerClass(), cascadingMetaData.getDeclaredTypeParameterIndex() );
 				}
 
-				if ( nodeName != null ) {
-					cascadedTypeArgumentValueContext.appendTypeParameterNode( nodeName );
-				}
+				cascadedTypeArgumentValueContext.appendTypeParameterNode( nodeName );
 
 				validateCascadedContainerElementsInContext( value, validationContext, cascadedTypeArgumentValueContext, cascadingMetaData, validationOrder );
 			}

diff --git a/engine/src/main/java/org/hibernate/validator/internal/engine/path/PathImpl.java b/engine/src/main/java/org/hibernate/validator/internal/engine/path/PathImpl.java
@@ -134,6 +134,17 @@ public NodeImpl addContainerElementNode(String nodeName) {
 		return currentLeafNode;
 	}
 
+	public boolean needToAddContainerElementNode(String nodeName) {
+		// if the node name is not null that would mean that we need to add it,
+		// but otherwise -- we would want to have an empty node
+		// if a current node is some iterable/multivalued element (E.g. array/list/map etc.).
+		// If we don't add it -- the path would be broken and would lead to a situation
+		// where container elements would be pointed to by a container element node itself
+		// resulting in various node methods like `Node#getIndex()` producing incorrect results.
+		// As an additional side effect of not adding a node it might lead to the path not being correctly copied.
+		return nodeName != null || currentLeafNode.isIterable();
+	}
+
 	public NodeImpl addParameterNode(String nodeName, int index) {
 		requiresWriteableNodeList();
 

diff --git a/engine/src/main/java/org/hibernate/validator/internal/engine/valuecontext/ValueContext.java b/engine/src/main/java/org/hibernate/validator/internal/engine/valuecontext/ValueContext.java
@@ -102,9 +102,11 @@ public final void appendNode(ConstraintLocation location) {
 	}
 
 	public final void appendTypeParameterNode(String nodeName) {
-		PathImpl newPath = PathImpl.createCopy( propertyPath );
-		newPath.addContainerElementNode( nodeName );
-		propertyPath = newPath;
+		if ( propertyPath.needToAddContainerElementNode( nodeName ) ) {
+			PathImpl newPath = PathImpl.createCopy( propertyPath );
+			newPath.addContainerElementNode( nodeName );
+			propertyPath = newPath;
+		}
 	}
 
 	public final void markCurrentPropertyAsIterable() {

diff --git a/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java b/engine/src/main/java/org/hibernate/validator/internal/metadata/core/MetaConstraint.java
@@ -226,10 +226,7 @@ private void doValidate(Object value, String nodeName) {
 				valueContext.setTypeParameter( containerClass, currentValueExtractionPathNode.getTypeParameterIndex() );
 			}
 
-			if ( nodeName != null ) {
-				valueContext.appendTypeParameterNode( nodeName );
-			}
-
+			valueContext.appendTypeParameterNode( nodeName );
 			valueContext.setCurrentValidatedValue( value );
 
 			if ( currentValueExtractionPathNode.hasNext() ) {

diff --git a/engine/src/test/java/org/hibernate/validator/test/internal/engine/path/NodeImplTest.java b/engine/src/test/java/org/hibernate/validator/test/internal/engine/path/NodeImplTest.java
@@ -39,8 +39,10 @@
 import jakarta.validation.Validator;
 import jakarta.validation.constraints.Max;
 import jakarta.validation.constraints.NotNull;
+import jakarta.validation.constraints.Pattern;
 import jakarta.validation.constraints.Size;
 import jakarta.validation.valueextraction.ExtractedValue;
+import jakarta.validation.valueextraction.UnwrapByDefault;
 import jakarta.validation.valueextraction.ValueExtractor;
 
 import org.hibernate.validator.path.ContainerElementNode;
@@ -505,6 +507,46 @@ public void testContainerElementNodeGetValueForNestedContainer() {
 		assertFalse( nodeIterator.hasNext() );
 	}
 
+	@Test
+	@TestForIssue(jiraKey = "HV-1946")
+	public void testIndexedContainerElementMultipleFailuresCorrectPath() {
+		class People {
+			@Pattern( regexp = "[a-z]+")
+			Person[] people;
+
+			public People(Person... people) {
+				this.people = people;
+			}
+		}
+
+		Validator validator = ValidatorUtil.getConfiguration()
+				.addValueExtractor( new PersonArrayValueExtractor() )
+				.buildValidatorFactory()
+				.getValidator();
+
+		Set<ConstraintViolation<People>> constraintViolations = validator.validate(
+				new People( new Person( "name1" ), new Person( "name2" ), new Person( "name3" ) )
+		);
+
+		assertThat( constraintViolations ).containsOnlyViolations(
+				violationOf( Pattern.class )
+						.withPropertyPath( pathWith()
+								.property( "people" )
+								.containerElement( null, true, null, 0, Person[].class, null )
+						),
+				violationOf( Pattern.class )
+						.withPropertyPath( pathWith()
+								.property( "people" )
+								.containerElement( null, true, null, 1, Person[].class, null )
+						),
+				violationOf( Pattern.class )
+						.withPropertyPath( pathWith()
+								.property( "people" )
+								.containerElement( null, true, null, 2, Person[].class, null )
+						)
+		);
+	}
+
 	private void assertConstraintViolationToOneValidation(Set<ConstraintViolation<AWithB>> constraintViolations) {
 		assertThat( constraintViolations ).containsOnlyViolations(
 				violationOf( NotNull.class )
@@ -752,4 +794,15 @@ public void extractValues(CustomContainer<?> originalValue, ValueExtractor.Value
 			}
 		}
 	}
+
+	@UnwrapByDefault
+	private static final class PersonArrayValueExtractor implements ValueExtractor<Person @ExtractedValue(type = String.class) []> {
+
+		@Override
+		public void extractValues(Person[] originalValue, ValueExtractor.ValueReceiver receiver) {
+			for ( int i = 0; i < originalValue.length; i++ ) {
+				receiver.indexedValue( null, i, originalValue[i].name );
+			}
+		}
+	}
 }