Fix configuration class overriden/overloaded methods issues (SPR-10992, SPR-10988)

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

@@ -395,36 +395,43 @@ public BeanWrapper instantiateUsingFactoryMethod(final String beanName, final Ro
 		}
 
 		if (factoryMethodToUse == null || argsToUse == null) {
-			// Need to determine the factory method...
-			// Try all methods with this name to see if they match the given arguments.
-			factoryClass = ClassUtils.getUserClass(factoryClass);
-			Method[] rawCandidates;
-
-			final Class factoryClazz = factoryClass;
-			if (System.getSecurityManager() != null) {
-				rawCandidates = AccessController.doPrivileged(new PrivilegedAction<Method[]>() {
-					@Override
-					public Method[] run() {
-						return (mbd.isNonPublicAccessAllowed() ?
-								ReflectionUtils.getAllDeclaredMethods(factoryClazz) : factoryClazz.getMethods());
-					}
-				});
+			Method[] candidates;
+
+			if (factoryMethodToUse != null) {
+				candidates = new Method[] { factoryMethodToUse };
 			}
 			else {
-				rawCandidates = (mbd.isNonPublicAccessAllowed() ?
-						ReflectionUtils.getAllDeclaredMethods(factoryClazz) : factoryClazz.getMethods());
-			}
+				// Need to determine the factory method...
+				// Try all methods with this name to see if they match the given arguments.
+				factoryClass = ClassUtils.getUserClass(factoryClass);
+				Method[] rawCandidates;
+
+				final Class factoryClazz = factoryClass;
+				if (System.getSecurityManager() != null) {
+					rawCandidates = AccessController.doPrivileged(new PrivilegedAction<Method[]>() {
+						@Override
+						public Method[] run() {
+							return (mbd.isNonPublicAccessAllowed() ?
+									ReflectionUtils.getAllDeclaredMethods(factoryClazz) : factoryClazz.getMethods());
+						}
+					});
+				}
+				else {
+					rawCandidates = (mbd.isNonPublicAccessAllowed() ?
+							ReflectionUtils.getAllDeclaredMethods(factoryClazz) : factoryClazz.getMethods());
+				}
 
-			List<Method> candidateSet = new ArrayList<Method>();
-			for (Method candidate : rawCandidates) {
-				if (Modifier.isStatic(candidate.getModifiers()) == isStatic &&
-						candidate.getName().equals(mbd.getFactoryMethodName()) &&
-						mbd.isFactoryMethod(candidate)) {
-					candidateSet.add(candidate);
+				List<Method> candidateSet = new ArrayList<Method>();
+				for (Method candidate : rawCandidates) {
+					if (Modifier.isStatic(candidate.getModifiers()) == isStatic &&
+							candidate.getName().equals(mbd.getFactoryMethodName()) &&
+							mbd.isFactoryMethod(candidate)) {
+						candidateSet.add(candidate);
+					}
 				}
+				candidates = candidateSet.toArray(new Method[candidateSet.size()]);
+				AutowireUtils.sortFactoryMethods(candidates);
 			}
-			Method[] candidates = candidateSet.toArray(new Method[candidateSet.size()]);
-			AutowireUtils.sortFactoryMethods(candidates);
 
 			ConstructorArgumentValues resolvedValues = null;
 			boolean autowiring = (mbd.getResolvedAutowireMode() == RootBeanDefinition.AUTOWIRE_CONSTRUCTOR);

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

@@ -244,8 +244,17 @@ public Method getResolvedFactoryMethod() {
 			return (candidate instanceof Method ? (Method) candidate : null);
 		}
 	}
-
-
+
+	/**
+	 * Allow subclasses to set the resolved factory method.
+	 * @param method factory method to use.
+	 */
+	protected void setResolvedFactoryMethod(Method method) {
+		synchronized (this.constructorArgumentLock) {
+			this.resolvedConstructorOrFactoryMethod = method;
+		}
+	}
+
 	public void registerExternallyManagedConfigMember(Member configMember) {
 		this.externallyManagedConfigMembers.add(configMember);
 	}

spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java

@@ -18,6 +18,7 @@
 
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
@@ -172,6 +173,26 @@ public void addBeanMethod(BeanMethod method) {
 	public Set<BeanMethod> getBeanMethods() {
 		return this.beanMethods;
 	}
+
+	/**
+	 * Gets bean methods elegibles for bean definitions
+	 * skipping overriden methods from superclasses.
+	 */
+	public Set<BeanMethod> getBeanMethodsToDefine() {
+		Set<BeanMethod> methods = new LinkedHashSet<BeanMethod>();
+		Set<String> names = new HashSet<String>();
+
+		for (BeanMethod method : beanMethods) {
+			String name = method.getMetadata().getMethodName();
+			if (!names.contains(name)) {
+				methods.add(method);
+				names.add(name);
+			}
+		}
+
+		return methods;
+	}
+
 
 	public void addImportedResource(String importedResource, Class<? extends BeanDefinitionReader> readerClass) {
 		this.importedResources.put(importedResource, readerClass);

spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java

@@ -26,13 +26,13 @@
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition;
 import org.springframework.beans.factory.annotation.AnnotatedGenericBeanDefinition;
 import org.springframework.beans.factory.annotation.Autowire;
 import org.springframework.beans.factory.annotation.RequiredAnnotationBeanPostProcessor;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanDefinitionHolder;
+import org.springframework.beans.factory.config.SetFactoryBean;
 import org.springframework.beans.factory.parsing.Location;
 import org.springframework.beans.factory.parsing.Problem;
 import org.springframework.beans.factory.parsing.ProblemReporter;
@@ -49,6 +49,7 @@
 import org.springframework.core.io.ResourceLoader;
 import org.springframework.core.type.AnnotationMetadata;
 import org.springframework.core.type.MethodMetadata;
+import org.springframework.core.type.StandardMethodMetadata;
 import org.springframework.core.type.classreading.MetadataReaderFactory;
 import org.springframework.util.StringUtils;
 
@@ -132,7 +133,7 @@ private void loadBeanDefinitionsForConfigurationClass(ConfigurationClass configC
 		if (configClass.isImported()) {
 			registerBeanDefinitionForImportedConfigurationClass(configClass);
 		}
-		for (BeanMethod beanMethod : configClass.getBeanMethods()) {
+		for (BeanMethod beanMethod : configClass.getBeanMethodsToDefine()) {
 			loadBeanDefinitionsForBeanMethod(beanMethod);
 		}
 		loadBeanDefinitionsFromImportedResources(configClass.getImportedResources());
@@ -206,6 +207,12 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
 			return;
 		}
 
+		// Sets the factory method now if possible
+		if (beanMethod.getMetadata() instanceof StandardMethodMetadata) {
+			StandardMethodMetadata smm = (StandardMethodMetadata) beanMethod.getMetadata();
+			beanDef.setResolvedFactoryMethod(smm.getIntrospectedMethod());
+		}
+
 		AnnotationConfigUtils.processCommonDefinitionAnnotations(beanDef, metadata);
 
 		Autowire autowire = bean.getEnum("autowire");
@@ -337,11 +344,13 @@ public ConfigurationClassBeanDefinition(ConfigurationClass configClass) {
 		public ConfigurationClassBeanDefinition(RootBeanDefinition original, ConfigurationClass configClass) {
 			super(original);
 			this.annotationMetadata = configClass.getMetadata();
+			setResolvedFactoryMethod(original.getResolvedFactoryMethod());
 		}
 
 		private ConfigurationClassBeanDefinition(ConfigurationClassBeanDefinition original) {
 			super(original);
 			this.annotationMetadata = original.annotationMetadata;
+			setResolvedFactoryMethod(original.getResolvedFactoryMethod());
 		}
 
 		@Override
@@ -358,6 +367,11 @@ public boolean isFactoryMethod(Method candidate) {
 		public ConfigurationClassBeanDefinition cloneBeanDefinition() {
 			return new ConfigurationClassBeanDefinition(this);
 		}
+
+		@Override
+		public void setResolvedFactoryMethod(Method method) {
+			super.setResolvedFactoryMethod(method);
+		}
 	}
 
 

spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java

@@ -79,6 +79,34 @@ public void beanMethodOverloadingWithInheritanceAndList() {
 		// SPR-11025
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfigWithList.class);
 		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+
+	}
+
+	@Test
+	public void beanMethodOverloadingNotGreedyWithInheritance() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(NotGreedySubConfig.class);
+		assertThat(ctx.getBean(String.class), equalTo("overloaded"));
+	}
+	static @Configuration class NotGreedySuperConfig {
+		@Bean Integer anInt() { return 5; }
+		@Bean String aString(Integer dependency) { return "super"; }
+	}
+	static @Configuration class NotGreedySubConfig extends NotGreedySuperConfig {
+
+		@Bean String aString() { return "overloaded"; }
+	}
+
+	@Test
+	public void beanMethodOverloadingNotBestMathcWithInheritance() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(NotBestMatchSubConfig.class);
+		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+	}
+	static @Configuration class NotBestMathcSuperConfig {
+		@Bean String aString(Integer dependency) { return "super"; }
+	}
+	static @Configuration class NotBestMatchSubConfig extends NotBestMathcSuperConfig {
+		@Bean Integer anInt() { return 5; }
+		@Bean String aString(Number dependency) { return "overloaded"+dependency; }
 	}
 
 	/**

spring-context/src/test/java/org/springframework/context/annotation/configuration/ConfigurationClassInheritanceTest.java

@@ -0,0 +1,65 @@
+/*
+ * 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.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.context.annotation.configuration;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.beans.factory.config.BeanDefinition;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.core.type.StandardMethodMetadata;
+
+
+/**
+ * Test for SPR-10992. Check that bean definition is not overridden
+ * by superclass.
+ * 
+ * @author Jose Luis Martin
+ */
+public class ConfigurationClassInheritanceTest {
+
+	@Test
+	public void testBeanDefinitionOverriding() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Config.class);
+		BeanDefinition bd = ctx.getBeanDefinition("bean");
+		StandardMethodMetadata metadata = (StandardMethodMetadata) bd.getSource();
+		Assert.assertEquals(Config.class, metadata.getIntrospectedMethod().getDeclaringClass());
+		ctx.close();
+	}
+
+	@Configuration
+	public static class ParentConfig {
+
+		@Bean
+		public String bean() {
+			return "parentBean";
+		}
+	}
+
+	@Configuration
+	public static class Config extends ParentConfig {
+
+		@Bean
+		public String bean() {
+			return "overridingBean";
+		}
+
+	}
+
+}
+