HV-1450 Some minor editorialization

Author: gsmet

engine/src/main/java/org/hibernate/validator/internal/metadata/BeanMetaDataManager.java

@@ -11,6 +11,7 @@
 import static org.hibernate.validator.internal.util.ConcurrentReferenceHashMap.ReferenceType.SOFT;
 import static org.hibernate.validator.internal.util.logging.Messages.MESSAGES;
 
+import java.util.ArrayList;
 import java.util.EnumSet;
 import java.util.List;
 
@@ -28,12 +29,14 @@
 import org.hibernate.validator.internal.metadata.provider.AnnotationMetaDataProvider;
 import org.hibernate.validator.internal.metadata.provider.MetaDataProvider;
 import org.hibernate.validator.internal.metadata.raw.BeanConfiguration;
+import org.hibernate.validator.internal.util.CollectionHelper;
 import org.hibernate.validator.internal.util.ConcurrentReferenceHashMap;
 import org.hibernate.validator.internal.util.Contracts;
 import org.hibernate.validator.internal.util.ExecutableHelper;
 import org.hibernate.validator.internal.util.ExecutableParameterNameProvider;
 import org.hibernate.validator.internal.util.TypeResolutionHelper;
 import org.hibernate.validator.internal.util.classhierarchy.ClassHierarchyHelper;
+import org.hibernate.validator.internal.util.stereotypes.Immutable;
 
 /**
  * This manager is in charge of providing all constraint related meta data
@@ -74,6 +77,7 @@ public class BeanMetaDataManager {
 	 * Additional metadata providers used for meta data retrieval if
 	 * the XML and/or programmatic configuration is used.
 	 */
+	@Immutable
 	private final List<MetaDataProvider> metaDataProviders;
 
 	/**
@@ -140,17 +144,20 @@ public BeanMetaDataManager(ConstraintHelper constraintHelper,
 
 		AnnotationProcessingOptions annotationProcessingOptions = getAnnotationProcessingOptionsFromNonDefaultProviders( optionalMetaDataProviders );
 		AnnotationMetaDataProvider defaultProvider = new AnnotationMetaDataProvider(
-					constraintHelper,
-					typeResolutionHelper,
-					valueExtractorManager,
-					annotationProcessingOptions
-			);
-		this.metaDataProviders = newArrayList();
-		// default annotation based metadata provider should go first in the list of providers to prevent possible issues
-		// similar to HV-1450. This way after the first run we will have all possible constrained elements in the list and
-		// should only merge in additional constraint information from other providers.
-		this.metaDataProviders.add( defaultProvider );
-		this.metaDataProviders.addAll( optionalMetaDataProviders );
+				constraintHelper,
+				typeResolutionHelper,
+				valueExtractorManager,
+				annotationProcessingOptions
+		);
+		List<MetaDataProvider> tmpMetaDataProviders = new ArrayList<>( optionalMetaDataProviders.size() + 1 );
+		// We add the annotation based metadata provider at the first position so that the entire metadata model is assembled
+		// first.
+		// The other optional metadata providers will then contribute their additional metadata to the preexisting model.
+		// This helps to mitigate issues like HV-1450.
+		tmpMetaDataProviders.add( defaultProvider );
+		tmpMetaDataProviders.addAll( optionalMetaDataProviders );
+
+		this.metaDataProviders = CollectionHelper.toImmutableList( tmpMetaDataProviders );
 	}
 
 	@SuppressWarnings("unchecked")

engine/src/main/java/org/hibernate/validator/internal/metadata/aggregated/ExecutableMetaData.java

@@ -310,15 +310,15 @@ public boolean accepts(ConstrainedElement constrainedElement) {
 
 			//are the locations equal (created by different builders) or
 			//does one of the executables override the other one?
-			return resolveToSameMethodInHierarchy( executable, candidate );
+			return isResolvedToSameMethodInHierarchy( executable, candidate );
 		}
 
-		private boolean resolveToSameMethodInHierarchy(Executable first, Executable other) {
+		private boolean isResolvedToSameMethodInHierarchy(Executable first, Executable other) {
 			if ( first instanceof Constructor || other instanceof Constructor ) {
 				return first.equals( other );
 			}
 
-			return executableHelper.resolveToSameMethodInHierarchy( getBeanClass(), (Method) first, (Method) other );
+			return executableHelper.isResolvedToSameMethodInHierarchy( getBeanClass(), (Method) first, (Method) other );
 		}
 
 		private boolean overrides(Executable first, Executable other) {

engine/src/main/java/org/hibernate/validator/internal/util/ExecutableHelper.java

@@ -91,7 +91,7 @@ public boolean overrides(Method subTypeMethod, Method superTypeMethod) {
 			return false;
 		}
 
-		if ( !isOneMethodVisibleToAnother( subTypeMethod, superTypeMethod ) ) {
+		if ( !isMethodVisibleTo( superTypeMethod, subTypeMethod ) ) {
 			return false;
 		}
 
@@ -110,7 +110,7 @@ public boolean overrides(Method subTypeMethod, Method superTypeMethod) {
 	 * 		override another one in the class hierarchy with {@code mainSubType} at the bottom,
 	 * 		{@code false} otherwise.
 	 */
-	public boolean resolveToSameMethodInHierarchy(Class<?> mainSubType, Method left, Method right) {
+	public boolean isResolvedToSameMethodInHierarchy(Class<?> mainSubType, Method left, Method right) {
 		Contracts.assertValueNotNull( mainSubType, "mainSubType" );
 		Contracts.assertValueNotNull( left, "left" );
 		Contracts.assertValueNotNull( right, "right" );
@@ -148,7 +148,7 @@ public boolean resolveToSameMethodInHierarchy(Class<?> mainSubType, Method left,
 			return false;
 		}
 
-		if ( !isOneMethodVisibleToAnother( left, right ) || !isOneMethodVisibleToAnother( right, left ) ) {
+		if ( !isMethodVisibleTo( right, left ) || !isMethodVisibleTo( left, right ) ) {
 			return false;
 		}
 
@@ -163,9 +163,9 @@ public boolean resolveToSameMethodInHierarchy(Class<?> mainSubType, Method left,
 		);
 	}
 
-	private static boolean isOneMethodVisibleToAnother(Method left, Method right) {
-		return Modifier.isPublic( right.getModifiers() ) || Modifier.isProtected( right.getModifiers() )
-				|| right.getDeclaringClass().getPackage().equals( left.getDeclaringClass().getPackage() );
+	private static boolean isMethodVisibleTo(Method visibleMethod, Method otherMethod) {
+		return Modifier.isPublic( visibleMethod.getModifiers() ) || Modifier.isProtected( visibleMethod.getModifiers() )
+				|| visibleMethod.getDeclaringClass().getPackage().equals( otherMethod.getDeclaringClass().getPackage() );
 	}
 
 	public static String getSimpleName(Executable executable) {

engine/src/test/java/org/hibernate/validator/test/internal/engine/methodvalidation/MethodParameterConstraintsInParallelHierarchyTest.java

@@ -27,7 +27,7 @@
 public class MethodParameterConstraintsInParallelHierarchyTest {
 
 	/**
-	 * NOTE: prior to the changes where this test was added it would fail randomly.
+	 * NOTE: prior to the changes made for HV-1450, this test was failing randomly.
 	 */
 	@Test
 	@TestForIssue(jiraKey = "HV-1450")
@@ -37,13 +37,14 @@ public void testDeepParallelHierarchyIsProcessedCorrectly() throws Exception {
 		Method method = WebServiceImpl.class.getMethod( "getEntityVersion", Long.class );
 		Object[] params = new Object[] { null };
 
-		Validator validator = Validation.byDefaultProvider().configure()
-				.buildValidatorFactory().getValidator();
+		for ( int i = 0; i < 100; i++ ) {
+			Validator validator = Validation.byDefaultProvider().configure()
+					.buildValidatorFactory().getValidator();
 
-		Set<ConstraintViolation<WebServiceImpl>> violations =
-				validator.forExecutables().validateParameters( service, method, params );
+			Set<ConstraintViolation<WebServiceImpl>> violations = validator.forExecutables().validateParameters( service, method, params );
 
-		assertThat( violations ).containsOnlyViolations( violationOf( NotNull.class ) );
+			assertThat( violations ).containsOnlyViolations( violationOf( NotNull.class ) );
+		}
 	}
 
 	private class WebServiceImpl extends AbstractWebService implements ExtendedWebService {