Proper matching of raw generic types and generically typed factory methods

Also optimized getTypeForFactoryMethod's implementation for non-generic factory methods, and reduced calls to getResolvedFactoryMethod in order to avoid repeated synchronization.

Issue: SPR-11034

spring-beans/src/main/java/org/springframework/beans/factory/annotation/QualifierAnnotationAutowireCandidateResolver.java

@@ -222,8 +222,9 @@ public class QualifierAnnotationAutowireCandidateResolver extends GenericTypeAwa
 		}
 		if (qualifier == null) {
 			Annotation targetAnnotation = null;
-			if (bd.getResolvedFactoryMethod() != null) {
-				targetAnnotation = AnnotationUtils.getAnnotation(bd.getResolvedFactoryMethod(), type);
+			Method resolvedFactoryMethod = bd.getResolvedFactoryMethod();
+			if (resolvedFactoryMethod != null) {
+				targetAnnotation = AnnotationUtils.getAnnotation(resolvedFactoryMethod, type);
 			}
 			if (targetAnnotation == null) {
 				// look for matching annotation on the target class

spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java

@@ -21,6 +21,7 @@ import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.lang.reflect.TypeVariable;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.security.PrivilegedActionException;
@@ -587,7 +588,7 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac
 	}
 
 	@Override
-	protected Class<?> predictBeanType(String beanName, RootBeanDefinition mbd, Class... typesToMatch) {
+	protected Class<?> predictBeanType(String beanName, RootBeanDefinition mbd, Class<?>... typesToMatch) {
 		Class<?> targetType = mbd.getTargetType();
 		if (targetType == null) {
 			targetType = (mbd.getFactoryMethodName() != null ? getTypeForFactoryMethod(beanName, mbd, typesToMatch) :
@@ -627,7 +628,12 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac
 	 * @return the type for the bean if determinable, or {@code null} else
 	 * @see #createBean
 	 */
-	protected Class<?> getTypeForFactoryMethod(String beanName, RootBeanDefinition mbd, Class[] typesToMatch) {
+	protected Class<?> getTypeForFactoryMethod(String beanName, RootBeanDefinition mbd, Class<?>... typesToMatch) {
+		Class<?> preResolved = mbd.resolvedFactoryMethodReturnType;
+		if (preResolved != null) {
+			return preResolved;
+		}
+
 		Class<?> factoryClass;
 		boolean isStatic = true;
 
@@ -652,6 +658,7 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac
 
 		// If all factory methods have the same return type, return that type.
 		// Can't clearly figure out exact method due to type converting / autowiring!
+		boolean cache = false;
 		int minNrOfArgs = mbd.getConstructorArgumentValues().getArgumentCount();
 		Method[] candidates = ReflectionUtils.getUniqueDeclaredMethods(factoryClass);
 		Set<Class<?>> returnTypes = new HashSet<Class<?>>(1);
@@ -659,38 +666,51 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac
 			if (Modifier.isStatic(factoryMethod.getModifiers()) == isStatic &&
 					factoryMethod.getName().equals(mbd.getFactoryMethodName()) &&
 					factoryMethod.getParameterTypes().length >= minNrOfArgs) {
-				Class<?>[] paramTypes = factoryMethod.getParameterTypes();
-				String[] paramNames = null;
-				ParameterNameDiscoverer pnd = getParameterNameDiscoverer();
-				if (pnd != null) {
-					paramNames = pnd.getParameterNames(factoryMethod);
-				}
-				ConstructorArgumentValues cav = mbd.getConstructorArgumentValues();
-				Set<ConstructorArgumentValues.ValueHolder> usedValueHolders =
-						new HashSet<ConstructorArgumentValues.ValueHolder>(paramTypes.length);
-				Object[] args = new Object[paramTypes.length];
-				for (int i = 0; i < args.length; i++) {
-					ConstructorArgumentValues.ValueHolder valueHolder = cav.getArgumentValue(
-							i, paramTypes[i], (paramNames != null ? paramNames[i] : null), usedValueHolders);
-					if (valueHolder == null) {
-						valueHolder = cav.getGenericArgumentValue(null, null, usedValueHolders);
+				TypeVariable<Method>[] declaredTypeVariables = factoryMethod.getTypeParameters();
+				// No declared type variables to inspect, so just process the standard return type.
+				if (declaredTypeVariables.length > 0) {
+					// Fully resolve parameter names and argument values.
+					Class<?>[] paramTypes = factoryMethod.getParameterTypes();
+					String[] paramNames = null;
+					ParameterNameDiscoverer pnd = getParameterNameDiscoverer();
+					if (pnd != null) {
+						paramNames = pnd.getParameterNames(factoryMethod);
 					}
-					if (valueHolder != null) {
-						args[i] = valueHolder.getValue();
-						usedValueHolders.add(valueHolder);
+					ConstructorArgumentValues cav = mbd.getConstructorArgumentValues();
+					Set<ConstructorArgumentValues.ValueHolder> usedValueHolders =
+							new HashSet<ConstructorArgumentValues.ValueHolder>(paramTypes.length);
+					Object[] args = new Object[paramTypes.length];
+					for (int i = 0; i < args.length; i++) {
+						ConstructorArgumentValues.ValueHolder valueHolder = cav.getArgumentValue(
+								i, paramTypes[i], (paramNames != null ? paramNames[i] : null), usedValueHolders);
+						if (valueHolder == null) {
+							valueHolder = cav.getGenericArgumentValue(null, null, usedValueHolders);
+						}
+						if (valueHolder != null) {
+							args[i] = valueHolder.getValue();
+							usedValueHolders.add(valueHolder);
+						}
+					}
+					Class<?> returnType = AutowireUtils.resolveReturnTypeForFactoryMethod(
+							factoryMethod, args, getBeanClassLoader());
+					if (returnType != null) {
+						cache = true;
+						returnTypes.add(returnType);
 					}
 				}
-				Class<?> returnType = AutowireUtils.resolveReturnTypeForFactoryMethod(
-						factoryMethod, args, getBeanClassLoader());
-				if (returnType != null) {
-					returnTypes.add(returnType);
+				else {
+					returnTypes.add(factoryMethod.getReturnType());
 				}
 			}
 		}
 
 		if (returnTypes.size() == 1) {
 			// Clear return type found: all factory methods return same type.
-			return returnTypes.iterator().next();
+			Class<?> result = returnTypes.iterator().next();
+			if (cache) {
+				mbd.resolvedFactoryMethodReturnType = result;
+			}
+			return result;
 		}
 		else {
 			// Ambiguous return types found: return null to indicate "not determinable".

spring-beans/src/main/java/org/springframework/beans/factory/support/AutowireUtils.java

@@ -195,17 +195,7 @@ abstract class AutowireUtils {
 		TypeVariable<Method>[] declaredTypeVariables = method.getTypeParameters();
 		Type genericReturnType = method.getGenericReturnType();
 		Type[] methodArgumentTypes = method.getGenericParameterTypes();
-
-		// No declared type variables to inspect, so just return the standard return type.
-		if (declaredTypeVariables.length == 0) {
-			return method.getReturnType();
-		}
-
-		// The supplied argument list is too short for the method's signature, so
-		// return null, since such a method invocation would fail.
-		if (args.length < methodArgumentTypes.length) {
-			return null;
-		}
+		Assert.isTrue(args.length == methodArgumentTypes.length, "Argument array does not match parameter count");
 
 		// Ensure that the type variable (e.g., T) is declared directly on the method
 		// itself (e.g., via <T>), not on the enclosing class or interface.

spring-beans/src/main/java/org/springframework/beans/factory/support/GenericTypeAwareAutowireCandidateResolver.java

@@ -16,6 +16,8 @@
 
 package org.springframework.beans.factory.support;
 
+import java.lang.reflect.Method;
+
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.FactoryBean;
@@ -66,7 +68,7 @@ public class GenericTypeAwareAutowireCandidateResolver implements AutowireCandid
 	 */
 	protected boolean checkGenericTypeMatch(BeanDefinitionHolder bdHolder, DependencyDescriptor descriptor) {
 		ResolvableType dependencyType = descriptor.getResolvableType();
-		if (!dependencyType.hasGenerics()) {
+		if (dependencyType.getType() instanceof Class) {
 			// No generic type -> we know it's a Class type-match, so no need to check again.
 			return true;
 		}
@@ -75,10 +77,19 @@ public class GenericTypeAwareAutowireCandidateResolver implements AutowireCandid
 		if (bdHolder.getBeanDefinition() instanceof RootBeanDefinition) {
 			rbd = (RootBeanDefinition) bdHolder.getBeanDefinition();
 		}
-		if (rbd != null && rbd.getResolvedFactoryMethod() != null) {
+		if (rbd != null && rbd.getFactoryMethodName() != null) {
 			// Should typically be set for any kind of factory method, since the BeanFactory
 			// pre-resolves them before reaching out to the AutowireCandidateResolver...
-			targetType = ResolvableType.forMethodReturnType(rbd.getResolvedFactoryMethod());
+			Class<?> preResolved = rbd.resolvedFactoryMethodReturnType;
+			if (preResolved != null) {
+				targetType = ResolvableType.forClass(preResolved);
+			}
+			else {
+				Method resolvedFactoryMethod = rbd.getResolvedFactoryMethod();
+				if (resolvedFactoryMethod != null) {
+					targetType = ResolvableType.forMethodReturnType(resolvedFactoryMethod);
+				}
+			}
 		}
 		if (targetType == null) {
 			// Regular case: straight bean instance, with BeanFactory available.

spring-beans/src/main/java/org/springframework/beans/factory/support/RootBeanDefinition.java

@@ -71,6 +71,9 @@ public class RootBeanDefinition extends AbstractBeanDefinition {
 	/** Package-visible field for caching the resolved constructor or factory method */
 	Object resolvedConstructorOrFactoryMethod;
 
+	/** Package-visible field for caching the return type of a generically typed factory method */
+	volatile Class<?> resolvedFactoryMethodReturnType;
+
 	/** Package-visible field that marks the constructor arguments as resolved */
 	boolean constructorArgumentsResolved = false;
 

spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java

@@ -21,6 +21,9 @@ import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.util.List;
 import java.util.Map;
 
@@ -1209,6 +1212,81 @@ public class AutowiredAnnotationBeanPostProcessorTests {
 		assertSame(ir, bean.integerRepositoryMap.get("integerRepo"));
 	}
 
+	@Test
+	public void testGenericsBasedFieldInjectionWithQualifiers() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		bf.setAutowireCandidateResolver(new QualifierAnnotationAutowireCandidateResolver());
+		AutowiredAnnotationBeanPostProcessor bpp = new AutowiredAnnotationBeanPostProcessor();
+		bpp.setBeanFactory(bf);
+		bf.addBeanPostProcessor(bpp);
+		RootBeanDefinition bd = new RootBeanDefinition(RepositoryFieldInjectionBeanWithQualifiers.class);
+		bd.setScope(RootBeanDefinition.SCOPE_PROTOTYPE);
+		bf.registerBeanDefinition("annotatedBean", bd);
+		StringRepository sr = new StringRepository();
+		bf.registerSingleton("stringRepo", sr);
+		IntegerRepository ir = new IntegerRepository();
+		bf.registerSingleton("integerRepo", ir);
+
+		RepositoryFieldInjectionBeanWithQualifiers bean = (RepositoryFieldInjectionBeanWithQualifiers) bf.getBean("annotatedBean");
+		assertSame(sr, bean.stringRepository);
+		assertSame(ir, bean.integerRepository);
+		assertSame(1, bean.stringRepositoryArray.length);
+		assertSame(1, bean.integerRepositoryArray.length);
+		assertSame(sr, bean.stringRepositoryArray[0]);
+		assertSame(ir, bean.integerRepositoryArray[0]);
+		assertSame(1, bean.stringRepositoryList.size());
+		assertSame(1, bean.integerRepositoryList.size());
+		assertSame(sr, bean.stringRepositoryList.get(0));
+		assertSame(ir, bean.integerRepositoryList.get(0));
+		assertSame(1, bean.stringRepositoryMap.size());
+		assertSame(1, bean.integerRepositoryMap.size());
+		assertSame(sr, bean.stringRepositoryMap.get("stringRepo"));
+		assertSame(ir, bean.integerRepositoryMap.get("integerRepo"));
+	}
+
+	@Test
+	public void testGenericsBasedFieldInjectionWithMocks() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		bf.setAutowireCandidateResolver(new QualifierAnnotationAutowireCandidateResolver());
+		AutowiredAnnotationBeanPostProcessor bpp = new AutowiredAnnotationBeanPostProcessor();
+		bpp.setBeanFactory(bf);
+		bf.addBeanPostProcessor(bpp);
+		RootBeanDefinition bd = new RootBeanDefinition(RepositoryFieldInjectionBeanWithQualifiers.class);
+		bd.setScope(RootBeanDefinition.SCOPE_PROTOTYPE);
+		bf.registerBeanDefinition("annotatedBean", bd);
+
+		RootBeanDefinition rbd = new RootBeanDefinition(MocksControl.class);
+		bf.registerBeanDefinition("mocksControl", rbd);
+		rbd = new RootBeanDefinition();
+		rbd.setFactoryBeanName("mocksControl");
+		rbd.setFactoryMethodName("createMock");
+		rbd.getConstructorArgumentValues().addGenericArgumentValue(Repository.class);
+		bf.registerBeanDefinition("stringRepo", rbd);
+		rbd = new RootBeanDefinition();
+		rbd.setFactoryBeanName("mocksControl");
+		rbd.setFactoryMethodName("createMock");
+		rbd.getConstructorArgumentValues().addGenericArgumentValue(Repository.class);
+		bf.registerBeanDefinition("integerRepo", rbd);
+
+		Repository sr = bf.getBean("stringRepo", Repository.class);
+		Repository ir = bf.getBean("integerRepo", Repository.class);
+		RepositoryFieldInjectionBeanWithQualifiers bean = (RepositoryFieldInjectionBeanWithQualifiers) bf.getBean("annotatedBean");
+		assertSame(sr, bean.stringRepository);
+		assertSame(ir, bean.integerRepository);
+		assertSame(1, bean.stringRepositoryArray.length);
+		assertSame(1, bean.integerRepositoryArray.length);
+		assertSame(sr, bean.stringRepositoryArray[0]);
+		assertSame(ir, bean.integerRepositoryArray[0]);
+		assertSame(1, bean.stringRepositoryList.size());
+		assertSame(1, bean.integerRepositoryList.size());
+		assertSame(sr, bean.stringRepositoryList.get(0));
+		assertSame(ir, bean.integerRepositoryList.get(0));
+		assertSame(1, bean.stringRepositoryMap.size());
+		assertSame(1, bean.integerRepositoryMap.size());
+		assertSame(sr, bean.stringRepositoryMap.get("stringRepo"));
+		assertSame(ir, bean.integerRepositoryMap.get("integerRepo"));
+	}
+
 	@Test
 	public void testGenericsBasedMethodInjection() {
 		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
@@ -2057,6 +2135,34 @@ public class AutowiredAnnotationBeanPostProcessorTests {
 	}
 
 
+	public static class RepositoryFieldInjectionBeanWithQualifiers {
+
+		@Autowired @Qualifier("stringRepo")
+		public Repository<?> stringRepository;
+
+		@Autowired @Qualifier("integerRepo")
+		public Repository integerRepository;
+
+		@Autowired @Qualifier("stringRepo")
+		public Repository<?>[] stringRepositoryArray;
+
+		@Autowired @Qualifier("integerRepo")
+		public Repository[] integerRepositoryArray;
+
+		@Autowired @Qualifier("stringRepo")
+		public List<Repository> stringRepositoryList;
+
+		@Autowired @Qualifier("integerRepo")
+		public List<Repository<?>> integerRepositoryList;
+
+		@Autowired @Qualifier("stringRepo")
+		public Map<String, Repository<?>> stringRepositoryMap;
+
+		@Autowired @Qualifier("integerRepo")
+		public Map<String, Repository> integerRepositoryMap;
+	}
+
+
 	public static class RepositoryMethodInjectionBean {
 
 		public Repository<String> stringRepository;
@@ -2216,4 +2322,22 @@ public class AutowiredAnnotationBeanPostProcessorTests {
 		}
 	}
 
+
+	/**
+	 * Pseudo-implementation of EasyMock's {@code MocksControl} class.
+	 */
+	public static class MocksControl {
+
+		@SuppressWarnings("unchecked")
+		public <T> T createMock(Class<T> toMock) {
+			return (T) Proxy.newProxyInstance(AutowiredAnnotationBeanPostProcessorTests.class.getClassLoader(), new Class<?>[]{toMock},
+					new InvocationHandler() {
+						@Override
+						public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+							throw new UnsupportedOperationException("mocked!");
+						}
+					});
+		}
+	}
+
 }

spring-beans/src/test/java/org/springframework/beans/factory/support/AutowireUtilsTests.java

@@ -49,9 +49,6 @@ public class AutowireUtilsTests {
 
 		Method createNamedProxyWithDifferentTypes = ReflectionUtils.findMethod(MyTypeWithMethods.class, "createNamedProxy",
 				new Class[] { String.class, Object.class });
-		// one argument to few
-		assertNull(
-				AutowireUtils.resolveReturnTypeForFactoryMethod(createNamedProxyWithDifferentTypes, new Object[]{"enigma"}, getClass().getClassLoader()));
 		assertEquals(Long.class,
 				AutowireUtils.resolveReturnTypeForFactoryMethod(createNamedProxyWithDifferentTypes, new Object[] { "enigma", 99L }, getClass().getClassLoader()));
 

spring-beans/src/test/java/org/springframework/beans/factory/support/BeanFactoryGenericsTests.java

@@ -701,7 +701,6 @@ public class BeanFactoryGenericsTests {
 		rbd.setFactoryBeanName("mocksControl");
 		rbd.setFactoryMethodName("createMock");
 		rbd.getConstructorArgumentValues().addGenericArgumentValue(Runnable.class);
-
 		bf.registerBeanDefinition("mock", rbd);
 
 		Map<String, Runnable> beans = bf.getBeansOfType(Runnable.class);
@@ -719,7 +718,6 @@ public class BeanFactoryGenericsTests {
 		rbd.setFactoryBeanName("mocksControl");
 		rbd.setFactoryMethodName("createMock");
 		rbd.getConstructorArgumentValues().addGenericArgumentValue(Runnable.class.getName());
-
 		bf.registerBeanDefinition("mock", rbd);
 
 		Map<String, Runnable> beans = bf.getBeansOfType(Runnable.class);
@@ -737,7 +735,6 @@ public class BeanFactoryGenericsTests {
 		rbd.setFactoryBeanName("mocksControl");
 		rbd.setFactoryMethodName("createMock");
 		rbd.getConstructorArgumentValues().addIndexedArgumentValue(0, Runnable.class);
-
 		bf.registerBeanDefinition("mock", rbd);
 
 		Map<String, Runnable> beans = bf.getBeansOfType(Runnable.class);
@@ -788,6 +785,7 @@ public class BeanFactoryGenericsTests {
 		}
 	}
 
+
 	/**
 	 * Pseudo-implementation of EasyMock's {@code MocksControl} class.
 	 */
@@ -795,14 +793,10 @@ public class BeanFactoryGenericsTests {
 
 		@SuppressWarnings("unchecked")
 		public <T> T createMock(Class<T> toMock) {
-
-			return (T) Proxy.newProxyInstance(
-					BeanFactoryGenericsTests.class.getClassLoader(),
-					new Class[] { toMock }, new InvocationHandler() {
-
+			return (T) Proxy.newProxyInstance(BeanFactoryGenericsTests.class.getClassLoader(), new Class<?>[] {toMock},
+					new InvocationHandler() {
 						@Override
-						public Object invoke(Object proxy, Method method, Object[] args)
-								throws Throwable {
+						public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
 							throw new UnsupportedOperationException("mocked!");
 						}
 					});