From b00c31a6204ee05230c5fecbeaf3a4de73a1215e Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 4 Nov 2013 00:19:55 +0100 Subject: [PATCH] Use non-lenient constructor resolution mode for @Bean methods Since @Bean methods are never used with externally specified constructor argument values but rather just with autowiring, the non-lenient constructor resolution mode is appropriate in case of an overloaded @Bean method, not performing any type difference weight checks. This change includes a refinement of Spring's existing non-lenient constructor resolution (which needs to be explicitly turned on and is therefore not well tested), narrowing the conditions for the ambiguity check (only in case of the same number of arguments and not for overridden methods). Issue: SPR-10988 (cherry picked from commit b093b84) --- .../factory/support/ConstructorResolver.java | 32 +++--- ...onfigurationClassBeanDefinitionReader.java | 1 + .../BeanMethodPolymorphismTests.java | 97 +++++++++++++++---- .../springframework/util/MethodInvoker.java | 21 ++-- 4 files changed, 109 insertions(+), 42 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java index 384866ffdd..f290d51b18 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -151,7 +151,7 @@ class ConstructorResolver { // Take specified constructors, if any. Constructor[] candidates = chosenCtors; if (candidates == null) { - Class beanClass = mbd.getBeanClass(); + Class beanClass = mbd.getBeanClass(); try { candidates = (mbd.isNonPublicAccessAllowed() ? beanClass.getDeclaredConstructors() : beanClass.getConstructors()); @@ -169,7 +169,7 @@ class ConstructorResolver { for (int i = 0; i < candidates.length; i++) { Constructor candidate = candidates[i]; - Class[] paramTypes = candidate.getParameterTypes(); + Class[] paramTypes = candidate.getParameterTypes(); if (constructorToUse != null && argsToUse.length > paramTypes.length) { // Already found greedy constructor that can be satisfied -> @@ -341,7 +341,7 @@ class ConstructorResolver { this.beanFactory.initBeanWrapper(bw); Object factoryBean; - Class factoryClass; + Class factoryClass; boolean isStatic; String factoryBeanName = mbd.getFactoryBeanName(); @@ -399,7 +399,7 @@ class ConstructorResolver { factoryClass = ClassUtils.getUserClass(factoryClass); Method[] rawCandidates; - final Class factoryClazz = factoryClass; + final Class factoryClazz = factoryClass; if (System.getSecurityManager() != null) { rawCandidates = AccessController.doPrivileged(new PrivilegedAction() { public Method[] run() { @@ -445,7 +445,7 @@ class ConstructorResolver { for (int i = 0; i < candidates.length; i++) { Method candidate = candidates[i]; - Class[] paramTypes = candidate.getParameterTypes(); + Class[] paramTypes = candidate.getParameterTypes(); if (paramTypes.length >= minNrOfArgs) { ArgumentsHolder argsHolder; @@ -503,7 +503,15 @@ class ConstructorResolver { minTypeDiffWeight = typeDiffWeight; ambiguousFactoryMethods = null; } - else if (factoryMethodToUse != null && typeDiffWeight == minTypeDiffWeight) { + // Find out about ambiguity: In case of the same type difference weight + // for methods with the same number of parameters, collect such candidates + // and eventually raise an ambiguity exception. + // However, only perform that check in non-lenient constructor resolution mode, + // and explicitly ignore overridden methods (with the same parameter signature). + else if (factoryMethodToUse != null && typeDiffWeight == minTypeDiffWeight && + !mbd.isLenientConstructorResolution() && + paramTypes.length == factoryMethodToUse.getParameterTypes().length && + !Arrays.equals(paramTypes, factoryMethodToUse.getParameterTypes())) { if (ambiguousFactoryMethods == null) { ambiguousFactoryMethods = new LinkedHashSet(); ambiguousFactoryMethods.add(factoryMethodToUse); @@ -540,7 +548,7 @@ class ConstructorResolver { "Invalid factory method '" + mbd.getFactoryMethodName() + "': needs to have a non-void return type!"); } - else if (ambiguousFactoryMethods != null && !mbd.isLenientConstructorResolution()) { + else if (ambiguousFactoryMethods != null) { throw new BeanCreationException(mbd.getResourceDescription(), beanName, "Ambiguous factory method matches found in bean '" + beanName + "' " + "(hint: specify index/type/name arguments for simple parameters to avoid type ambiguities): " + @@ -644,7 +652,7 @@ class ConstructorResolver { */ private ArgumentsHolder createArgumentArray( String beanName, RootBeanDefinition mbd, ConstructorArgumentValues resolvedValues, - BeanWrapper bw, Class[] paramTypes, String[] paramNames, Object methodOrCtor, + BeanWrapper bw, Class[] paramTypes, String[] paramNames, Object methodOrCtor, boolean autowiring) throws UnsatisfiedDependencyException { String methodType = (methodOrCtor instanceof Constructor ? "constructor" : "factory method"); @@ -750,7 +758,7 @@ class ConstructorResolver { private Object[] resolvePreparedArguments( String beanName, RootBeanDefinition mbd, BeanWrapper bw, Member methodOrCtor, Object[] argsToResolve) { - Class[] paramTypes = (methodOrCtor instanceof Method ? + Class[] paramTypes = (methodOrCtor instanceof Method ? ((Method) methodOrCtor).getParameterTypes() : ((Constructor) methodOrCtor).getParameterTypes()); TypeConverter converter = (this.beanFactory.getCustomTypeConverter() != null ? this.beanFactory.getCustomTypeConverter() : bw); @@ -822,7 +830,7 @@ class ConstructorResolver { this.preparedArguments = args; } - public int getTypeDifferenceWeight(Class[] paramTypes) { + public int getTypeDifferenceWeight(Class[] paramTypes) { // If valid arguments found, determine type difference weight. // Try type difference weight on both the converted arguments and // the raw arguments. If the raw weight is better, use it. @@ -832,7 +840,7 @@ class ConstructorResolver { return (rawTypeDiffWeight < typeDiffWeight ? rawTypeDiffWeight : typeDiffWeight); } - public int getAssignabilityWeight(Class[] paramTypes) { + public int getAssignabilityWeight(Class[] paramTypes) { for (int i = 0; i < paramTypes.length; i++) { if (!ClassUtils.isAssignableValue(paramTypes[i], this.arguments[i])) { return Integer.MAX_VALUE; diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 27ae171955..c5b07237cf 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -310,6 +310,7 @@ class ConfigurationClassBeanDefinitionReader { public ConfigurationClassBeanDefinition(ConfigurationClass configClass) { this.annotationMetadata = configClass.getMetadata(); + setLenientConstructorResolution(false); } public ConfigurationClassBeanDefinition(RootBeanDefinition original, ConfigurationClass configClass) { diff --git a/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java b/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java index 1d573540f8..b666f8be92 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java @@ -16,18 +16,20 @@ package org.springframework.context.annotation; -import static org.hamcrest.CoreMatchers.equalTo; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; - import java.lang.annotation.Inherited; +import java.util.List; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; + import org.springframework.beans.factory.parsing.BeanDefinitionParsingException; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.RootBeanDefinition; +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.*; + /** * Tests regarding overloading and overriding of bean methods. * Related to SPR-6618. @@ -42,22 +44,27 @@ import org.springframework.beans.factory.support.RootBeanDefinition; * the most specific subclass bean method will always be the one that is invoked. * * @author Chris Beams + * @author Phillip Webb */ +@SuppressWarnings("resource") public class BeanMethodPolymorphismTests { + @Rule + public ExpectedException thrown = ExpectedException.none(); + + @Test public void beanMethodOverloadingWithoutInheritance() { + @SuppressWarnings({ "hiding" }) @Configuration class Config { @Bean String aString() { return "na"; } @Bean String aString(Integer dependency) { return "na"; } } - try { - new AnnotationConfigApplicationContext(Config.class); - fail("expected bean method overloading exception"); - } catch (BeanDefinitionParsingException ex) { - assertTrue(ex.getMessage(), ex.getMessage().contains("2 overloaded @Bean methods named 'aString'")); - } + + this.thrown.expect(BeanDefinitionParsingException.class); + this.thrown.expectMessage("overloaded @Bean methods named 'aString'"); + new AnnotationConfigApplicationContext(Config.class); } @Test @@ -65,12 +72,12 @@ public class BeanMethodPolymorphismTests { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfig.class); assertThat(ctx.getBean(String.class), equalTo("overloaded5")); } - static @Configuration class SuperConfig { - @Bean String aString() { return "super"; } - } - static @Configuration class SubConfig { - @Bean Integer anInt() { return 5; } - @Bean String aString(Integer dependency) { return "overloaded"+dependency; } + + @Test + public void beanMethodOverloadingWithInheritanceAndList() { + // SPR-11025 + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfigWithList.class); + assertThat(ctx.getBean(String.class), equalTo("overloaded5")); } /** @@ -83,10 +90,6 @@ public class BeanMethodPolymorphismTests { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ShadowConfig.class); assertThat(ctx.getBean(String.class), equalTo("shadow")); } - @Import(SubConfig.class) - static @Configuration class ShadowConfig { - @Bean String aString() { return "shadow"; } - } /** * Tests that polymorphic Configuration classes need not explicitly redeclare the @@ -110,6 +113,7 @@ public class BeanMethodPolymorphismTests { public TestBean testBean() { return new TestBean(); } + } @@ -117,4 +121,55 @@ public class BeanMethodPolymorphismTests { static class Config extends BaseConfig { } + + @Configuration + static class SuperConfig { + + @Bean + String aString() { + return "super"; + } + } + + + @Configuration + static class SubConfig extends SuperConfig { + + @Bean + Integer anInt() { + return 5; + } + + @Bean + String aString(Integer dependency) { + return "overloaded" + dependency; + } + } + + + @Configuration + static class SubConfigWithList extends SuperConfig { + + @Bean + Integer anInt() { + return 5; + } + + @Bean + String aString(List dependency) { + return "overloaded" + dependency.get(0); + } + } + + + @Configuration + @Import(SubConfig.class) + static class ShadowConfig { + + @Bean + String aString() { + return "shadow"; + } + } + } diff --git a/spring-core/src/main/java/org/springframework/util/MethodInvoker.java b/spring-core/src/main/java/org/springframework/util/MethodInvoker.java index 565a68c696..b1bd04ccb3 100644 --- a/spring-core/src/main/java/org/springframework/util/MethodInvoker.java +++ b/spring-core/src/main/java/org/springframework/util/MethodInvoker.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2013 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. @@ -40,7 +40,7 @@ import java.lang.reflect.Modifier; */ public class MethodInvoker { - private Class targetClass; + private Class targetClass; private Object targetObject; @@ -61,14 +61,14 @@ public class MethodInvoker { * @see #setTargetObject * @see #setTargetMethod */ - public void setTargetClass(Class targetClass) { + public void setTargetClass(Class targetClass) { this.targetClass = targetClass; } /** * Return the target class on which to call the target method. */ - public Class getTargetClass() { + public Class getTargetClass() { return this.targetClass; } @@ -158,7 +158,7 @@ public class MethodInvoker { this.targetMethod = methodName; } - Class targetClass = getTargetClass(); + Class targetClass = getTargetClass(); String targetMethod = getTargetMethod(); if (targetClass == null) { throw new IllegalArgumentException("Either 'targetClass' or 'targetObject' is required"); @@ -194,7 +194,7 @@ public class MethodInvoker { * @return the resolved Class * @throws ClassNotFoundException if the class name was invalid */ - protected Class resolveClassName(String className) throws ClassNotFoundException { + protected Class resolveClassName(String className) throws ClassNotFoundException { return ClassUtils.forName(className, ClassUtils.getDefaultClassLoader()); } @@ -287,19 +287,22 @@ public class MethodInvoker { * Therefore, with an arg of type Integer, a constructor (Integer) would be preferred to a * constructor (Number) which would in turn be preferred to a constructor (Object). * All argument weights get accumulated. + *

Note: This is the algorithm used by MethodInvoker itself and also the algorithm + * used for constructor and factory method selection in Spring's bean container (in case + * of lenient constructor resolution which is the default for regular bean definitions). * @param paramTypes the parameter types to match * @param args the arguments to match * @return the accumulated weight for all arguments */ - public static int getTypeDifferenceWeight(Class[] paramTypes, Object[] args) { + public static int getTypeDifferenceWeight(Class[] paramTypes, Object[] args) { int result = 0; for (int i = 0; i < paramTypes.length; i++) { if (!ClassUtils.isAssignableValue(paramTypes[i], args[i])) { return Integer.MAX_VALUE; } if (args[i] != null) { - Class paramType = paramTypes[i]; - Class superClass = args[i].getClass().getSuperclass(); + Class paramType = paramTypes[i]; + Class superClass = args[i].getClass().getSuperclass(); while (superClass != null) { if (paramType.equals(superClass)) { result = result + 2;