HV-1450 Changed which executables are excepted by ExecutableMetaData.Builder

- changed the logic on how executable builder determines if it should accept
a constrained element. Used a most specific sub type from the hierarchy
under inspection to resolve executables and check if they override each
other somewhere in the hierarchy.
- changed the order of metadata providers. Made annotation based as a first
provider. This should build all possible builder delegates after the first
iteration on metadata providers and data from other providers will be just
merged in.

Author: marko-bekhta

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

@@ -127,9 +127,6 @@ public BeanMetaDataManager(ConstraintHelper constraintHelper,
 		this.parameterNameProvider = parameterNameProvider;
 		this.validationOrderGenerator = validationOrderGenerator;
 
-		this.metaDataProviders = newArrayList();
-		this.metaDataProviders.addAll( optionalMetaDataProviders );
-
 		this.methodValidationConfiguration = methodValidationConfiguration;
 
 		this.beanMetaDataCache = new ConcurrentReferenceHashMap<>(
@@ -141,15 +138,19 @@ public BeanMetaDataManager(ConstraintHelper constraintHelper,
 				EnumSet.of( IDENTITY_COMPARISONS )
 		);
 
-		AnnotationProcessingOptions annotationProcessingOptions = getAnnotationProcessingOptionsFromNonDefaultProviders();
+		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 );
 	}
 
 	@SuppressWarnings("unchecked")
@@ -195,9 +196,9 @@ private <T> BeanMetaDataImpl<T> createBeanMetaData(Class<T> clazz) {
 	/**
 	 * @return returns the annotation ignores from the non annotation based meta data providers
 	 */
-	private AnnotationProcessingOptions getAnnotationProcessingOptionsFromNonDefaultProviders() {
+	private AnnotationProcessingOptions getAnnotationProcessingOptionsFromNonDefaultProviders(List<MetaDataProvider> optionalMetaDataProviders) {
 		AnnotationProcessingOptions options = new AnnotationProcessingOptionsImpl();
-		for ( MetaDataProvider metaDataProvider : metaDataProviders ) {
+		for ( MetaDataProvider metaDataProvider : optionalMetaDataProviders ) {
 			options.merge( metaDataProvider.getAnnotationProcessingOptions() );
 		}
 

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

@@ -310,9 +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 executable.equals( candidate ) ||
-					overrides( executable, candidate ) ||
-					overrides( candidate, executable );
+			return resolveToSameMethodInHierarchy( executable, candidate );
+		}
+
+		private boolean resolveToSameMethodInHierarchy(Executable first, Executable other) {
+			if ( first instanceof Constructor || other instanceof Constructor ) {
+				return first.equals( other );
+			}
+
+			return executableHelper.resolveToSameMethodInHierarchy( getBeanClass(), (Method) first, (Method) other );
 		}
 
 		private boolean overrides(Executable first, Executable other) {

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

@@ -17,6 +17,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.hibernate.validator.internal.util.classhierarchy.Filters;
 import org.hibernate.validator.internal.util.logging.Log;
 import org.hibernate.validator.internal.util.logging.LoggerFactory;
 import org.hibernate.validator.internal.util.privilegedactions.GetResolvedMemberMethods;
@@ -90,14 +91,83 @@ public boolean overrides(Method subTypeMethod, Method superTypeMethod) {
 			return false;
 		}
 
-		if ( !Modifier.isPublic( superTypeMethod.getModifiers() ) && !Modifier.isProtected( superTypeMethod.getModifiers() )
-				&& !superTypeMethod.getDeclaringClass().getPackage().equals( subTypeMethod.getDeclaringClass().getPackage() ) ) {
+		if ( !isOneMethodVisibleToAnother( subTypeMethod, superTypeMethod ) ) {
 			return false;
 		}
 
 		return instanceMethodParametersResolveToSameTypes( subTypeMethod, superTypeMethod );
 	}
 
+	/**
+	 * Checks if a pair of given methods ({@code left} and {@code right}) are resolved to the same
+	 * method based on the {@code mainSubType} type.
+	 *
+	 * @param mainSubType a type at the bottom of class hierarchy to be used to lookup the methods.
+	 * @param left one of the methods to check
+	 * @param right another of the methods to check
+	 *
+	 * @return {@code true} if a pair of methods are equal {@code left == right}, or one of the methods
+	 * 		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) {
+		Contracts.assertValueNotNull( mainSubType, "mainSubType" );
+		Contracts.assertValueNotNull( left, "left" );
+		Contracts.assertValueNotNull( right, "right" );
+
+		if ( left.equals( right ) ) {
+			return true;
+		}
+
+		if ( !left.getName().equals( right.getName() ) ) {
+			return false;
+		}
+
+		// methods with same name in the same class should be different
+		if ( left.getDeclaringClass().equals( right.getDeclaringClass() ) ) {
+			return false;
+		}
+
+		if ( left.getParameterTypes().length != right.getParameterTypes().length ) {
+			return false;
+		}
+
+		// if at least one method from a pair is static - they are different methods
+		if ( Modifier.isStatic( right.getModifiers() ) || Modifier.isStatic( left.getModifiers() ) ) {
+			return false;
+		}
+
+		// HV-861 Bridge method should be ignored. Classmates type/member resolution will take care of proper
+		// override detection without considering bridge methods
+		if ( left.isBridge() || right.isBridge() ) {
+			return false;
+		}
+
+		// if one of the methods is private - methods are different
+		if ( Modifier.isPrivate( left.getModifiers() ) || Modifier.isPrivate( right.getModifiers() ) ) {
+			return false;
+		}
+
+		if ( !isOneMethodVisibleToAnother( left, right ) || !isOneMethodVisibleToAnother( right, left ) ) {
+			return false;
+		}
+
+		// We need to check if the passed mainSubType is not a Weld proxy. In case of proxy we need to get
+		// a class that was proxied otherwise we can use the class itself. This is due to the issue that
+		// call to Class#getGenericInterfaces() on a Weld proxy returns raw types instead of parametrized
+		// generics and methods will not be resolved correctly.
+		return instanceMethodParametersResolveToSameTypes(
+				Filters.excludeProxies().accepts( mainSubType ) ? mainSubType : mainSubType.getSuperclass(),
+				left,
+				right
+		);
+	}
+
+	private static boolean isOneMethodVisibleToAnother(Method left, Method right) {
+		return Modifier.isPublic( right.getModifiers() ) || Modifier.isProtected( right.getModifiers() )
+				|| right.getDeclaringClass().getPackage().equals( left.getDeclaringClass().getPackage() );
+	}
+
 	public static String getSimpleName(Executable executable) {
 		return executable instanceof Constructor ? executable.getDeclaringClass().getSimpleName() : executable.getName();
 	}
@@ -140,14 +210,18 @@ public static ElementType getElementType(Executable executable) {
 	 * @return {@code true} if the parameters of the two methods resolve to the same types, {@code false otherwise}.
 	 */
 	private boolean instanceMethodParametersResolveToSameTypes(Method subTypeMethod, Method superTypeMethod) {
-		if ( subTypeMethod.getParameterTypes().length == 0 ) {
+		return instanceMethodParametersResolveToSameTypes( subTypeMethod.getDeclaringClass(), subTypeMethod, superTypeMethod );
+	}
+
+	private boolean instanceMethodParametersResolveToSameTypes(Class<?> mainSubType, Method left, Method right) {
+		if ( left.getParameterTypes().length == 0 ) {
 			return true;
 		}
 
-		ResolvedType resolvedSubType = typeResolver.resolve( subTypeMethod.getDeclaringClass() );
+		ResolvedType resolvedSubType = typeResolver.resolve( mainSubType );
 
 		MemberResolver memberResolver = new MemberResolver( typeResolver );
-		memberResolver.setMethodFilter( new SimpleMethodFilter( subTypeMethod, superTypeMethod ) );
+		memberResolver.setMethodFilter( new SimpleMethodFilter( left, right ) );
 		ResolvedTypeWithMembers typeWithMembers = memberResolver.resolve(
 				resolvedSubType,
 				null,
@@ -181,9 +255,9 @@ private boolean instanceMethodParametersResolveToSameTypes(Method subTypeMethod,
 		catch (ArrayIndexOutOfBoundsException e) {
 			LOG.debug(
 					"Error in ExecutableHelper#instanceMethodParametersResolveToSameTypes comparing "
-							+ subTypeMethod
+							+ left
 							+ " with "
-							+ superTypeMethod
+							+ right
 			);
 		}
 

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

@@ -0,0 +1,71 @@
+/*
+ * Hibernate Validator, declare and validate application constraints
+ *
+ * License: Apache License, Version 2.0
+ * See the license.txt file in the root directory or <http://www.apache.org/licenses/LICENSE-2.0>.
+ */
+package org.hibernate.validator.test.internal.engine.methodvalidation;
+
+import static org.hibernate.validator.testutil.ConstraintViolationAssert.assertThat;
+import static org.hibernate.validator.testutil.ConstraintViolationAssert.violationOf;
+
+import java.lang.reflect.Method;
+import java.util.Set;
+
+import javax.validation.ConstraintViolation;
+import javax.validation.Validation;
+import javax.validation.Validator;
+import javax.validation.constraints.NotNull;
+
+import org.hibernate.validator.testutil.TestForIssue;
+
+import org.testng.annotations.Test;
+
+/**
+ * @author Marko Bekhta
+ */
+public class MethodParameterConstraintsInParallelHierarchyTest {
+
+	/**
+	 * NOTE: prior to the changes where this test was added it would fail randomly.
+	 */
+	@Test
+	@TestForIssue(jiraKey = "HV-1450")
+	public void testDeepParallelHierarchyIsProcessedCorrectly() throws Exception {
+
+		WebServiceImpl service = new WebServiceImpl();
+		Method method = WebServiceImpl.class.getMethod( "getEntityVersion", Long.class );
+		Object[] params = new Object[] { null };
+
+		Validator validator = Validation.byDefaultProvider().configure()
+				.buildValidatorFactory().getValidator();
+
+		Set<ConstraintViolation<WebServiceImpl>> violations =
+				validator.forExecutables().validateParameters( service, method, params );
+
+		assertThat( violations ).containsOnlyViolations( violationOf( NotNull.class ) );
+	}
+
+	private class WebServiceImpl extends AbstractWebService implements ExtendedWebService {
+
+	}
+
+	private abstract class AbstractWebService implements WebService {
+
+		@Override
+		public int getEntityVersion(Long id) {
+			return id.intValue();
+		}
+	}
+
+	private interface ExtendedWebService extends WebService {
+
+		@Override
+		int getEntityVersion(Long id);
+	}
+
+	private interface WebService {
+
+		int getEntityVersion(@NotNull Long id);
+	}
+}