Revised @bean processing rules

@bean method metadata is always being picked from the most concrete subclass; @bean method overloads are allowed within the same config class as well; and @bean overrides and overloads work with 'allowBeanDefinitionOverriding'=false now.

Issue: SPR-10992
Issue: SPR-11025

Author: jhoeller

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

@@ -17,7 +17,6 @@
 package org.springframework.context.annotation;
 
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
@@ -156,8 +155,8 @@ public boolean isImported() {
 	}
 
 	/**
-	 * Returns the configuration class that imported this class or {@code null} if
-	 * this configuration was not imported.
+	 * Return the configuration class that imported this class,
+	 * or {@code null} if this configuration was not imported.
 	 * @since 4.0
 	 * @see #isImported()
 	 */
@@ -182,7 +181,7 @@ public void addImportBeanDefinitionRegistrar(ImportBeanDefinitionRegistrar regis
 	}
 
 	public Set<ImportBeanDefinitionRegistrar> getImportBeanDefinitionRegistrars() {
-		return Collections.unmodifiableSet(importBeanDefinitionRegistrars);
+		return Collections.unmodifiableSet(this.importBeanDefinitionRegistrars);
 	}
 
 	public Map<String, Class<? extends BeanDefinitionReader>> getImportedResources() {
@@ -197,23 +196,6 @@ public void validate(ProblemReporter problemReporter) {
 			}
 		}
 
-		// An @Bean method may only be overloaded through inheritance. No single
-		// @Configuration class may declare two @Bean methods with the same name.
-		Map<String, Integer> methodNameCounts = new HashMap<String, Integer>();
-		for (BeanMethod beanMethod : this.beanMethods) {
-			String fqMethodName = beanMethod.getFullyQualifiedMethodName();
-			Integer currentCount = methodNameCounts.get(fqMethodName);
-			int newCount = (currentCount != null ? currentCount + 1 : 1);
-			methodNameCounts.put(fqMethodName, newCount);
-		}
-		for (String fqMethodName : methodNameCounts.keySet()) {
-			int count = methodNameCounts.get(fqMethodName);
-			if (count > 1) {
-				String shortMethodName = ConfigurationMethod.getShortMethodName(fqMethodName);
-				problemReporter.error(new BeanMethodOverloadingProblem(shortMethodName, count));
-			}
-		}
-
 		for (BeanMethod beanMethod : this.beanMethods) {
 			beanMethod.validate(problemReporter);
 		}
@@ -232,7 +214,7 @@ public int hashCode() {
 
 	@Override
 	public String toString() {
-		return String.format("[ConfigurationClass:beanName=%s,resource=%s]", this.beanName, this.resource);
+		return "ConfigurationClass:beanName=" + this.beanName + ",resource=" + this.resource;
 	}
 
 
@@ -247,17 +229,4 @@ public FinalConfigurationProblem() {
 		}
 	}
 
-
-	/**
-	 * Bean methods on configuration classes may only be overloaded through inheritance.
-	 */
-	private class BeanMethodOverloadingProblem extends Problem {
-
-		public BeanMethodOverloadingProblem(String methodName, int count) {
-			super(String.format("@Configuration class '%s' has %s overloaded @Bean methods named '%s'. " +
-					"Only one @Bean method of a given name is allowed within each @Configuration class.",
-					getSimpleName(), count, methodName), new Location(getResource(), getMetadata()));
-		}
-	}
-
 }

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

@@ -174,6 +174,7 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
 		if (this.conditionEvaluator.shouldSkip(beanMethod.getMetadata(), ConfigurationPhase.REGISTER_BEAN)) {
 			return;
 		}
+
 		ConfigurationClass configClass = beanMethod.getConfigurationClass();
 		MethodMetadata metadata = beanMethod.getMetadata();
 
@@ -258,9 +259,12 @@ protected boolean isOverriddenByExistingDefinition(BeanMethod beanMethod, String
 		BeanDefinition existingBeanDef = this.registry.getBeanDefinition(beanName);
 
 		// Is the existing bean definition one that was created from a configuration class?
-		// -> allow the current bean method to override, since both are at second-pass level
+		// -> allow the current bean method to override, since both are at second-pass level.
+		// However, if the bean method is an overloaded case on the same configuration class,
+		// preserve the existing bean definition.
 		if (existingBeanDef instanceof ConfigurationClassBeanDefinition) {
-			return false;
+			ConfigurationClassBeanDefinition ccbd = (ConfigurationClassBeanDefinition) existingBeanDef;
+			return (ccbd.getMetadata().getClassName().equals(beanMethod.getConfigurationClass().getMetadata().getClassName()));
 		}
 
 		// Has the existing bean definition bean marked as a framework-generated bean?

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

@@ -16,14 +16,10 @@
 
 package org.springframework.context.annotation;
 
-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;
 
@@ -34,50 +30,75 @@
  * Tests regarding overloading and overriding of bean methods.
  * Related to SPR-6618.
  *
- * Bean-annotated methods should be able to be overridden, just as any regular
- * method. This is straightforward.
- *
- * Bean-annotated methods should be able to be overloaded, though supporting this
- * is more subtle. Essentially, it must be unambiguous to the container which bean
- * method to call.  A simple way to think about this is that no one Configuration
- * class may declare two bean methods with the same name.  In the case of inheritance,
- * the most specific subclass bean method will always be the one that is invoked.
- *
  * @author Chris Beams
  * @author Phillip Webb
+ * @author Juergen Hoeller
  */
-@SuppressWarnings("resource")
 public class BeanMethodPolymorphismTests {
 
-	@Rule
-	public ExpectedException thrown = ExpectedException.none();
-
-
 	@Test
 	public void beanMethodOverloadingWithoutInheritance() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ConfigWithOverloading.class);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertThat(ctx.getBean(String.class), equalTo("regular"));
+	}
 
-		@SuppressWarnings({ "hiding" })
-		@Configuration class Config {
-			@Bean String aString() { return "na"; }
-			@Bean String aString(Integer dependency) { return "na"; }
-		}
+	@Test
+	public void beanMethodOverloadingWithoutInheritanceAndExtraDependency() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ConfigWithOverloading.class);
+		ctx.getDefaultListableBeanFactory().registerSingleton("anInt", 5);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+	}
+
+	@Test
+	public void beanMethodOverloadingWithAdditionalMetadata() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ConfigWithOverloadingAndAdditionalMetadata.class);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertFalse(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
+		assertThat(ctx.getBean(String.class), equalTo("regular"));
+		assertTrue(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
+	}
 
-		this.thrown.expect(BeanDefinitionParsingException.class);
-		this.thrown.expectMessage("overloaded @Bean methods named 'aString'");
-		new AnnotationConfigApplicationContext(Config.class);
+	@Test
+	public void beanMethodOverloadingWithAdditionalMetadataButOtherMethodExecuted() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ConfigWithOverloadingAndAdditionalMetadata.class);
+		ctx.getDefaultListableBeanFactory().registerSingleton("anInt", 5);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertFalse(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
+		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+		assertTrue(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
 	}
 
 	@Test
 	public void beanMethodOverloadingWithInheritance() {
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfig.class);
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(SubConfig.class);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertFalse(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
 		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+		assertTrue(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
 	}
 
+	// SPR-11025
 	@Test
 	public void beanMethodOverloadingWithInheritanceAndList() {
-		// SPR-11025
-		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfigWithList.class);
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(SubConfigWithList.class);
+		ctx.setAllowBeanDefinitionOverriding(false);
+		ctx.refresh();
+		assertFalse(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
 		assertThat(ctx.getBean(String.class), equalTo("overloaded5"));
+		assertTrue(ctx.getDefaultListableBeanFactory().containsSingleton("aString"));
 	}
 
 	/**
@@ -91,11 +112,6 @@ public void beanMethodShadowing() {
 		assertThat(ctx.getBean(String.class), equalTo("shadow"));
 	}
 
-	/**
-	 * Tests that polymorphic Configuration classes need not explicitly redeclare the
-	 * {@link Configuration} annotation. This respects the {@link Inherited} nature
-	 * of the Configuration annotation, even though it's being detected via ASM.
-	 */
 	@Test
 	public void beanMethodsDetectedOnSuperClass() {
 		DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
@@ -122,6 +138,36 @@ static class Config extends BaseConfig {
 	}
 
 
+	@Configuration
+	static class ConfigWithOverloading {
+
+		@Bean
+		String aString() {
+			return "regular";
+		}
+
+		@Bean
+		String aString(Integer dependency) {
+			return "overloaded" + dependency;
+		}
+	}
+
+
+	@Configuration
+	static class ConfigWithOverloadingAndAdditionalMetadata {
+
+		@Bean @Lazy
+		String aString() {
+			return "regular";
+		}
+
+		@Bean
+		String aString(Integer dependency) {
+			return "overloaded" + dependency;
+		}
+	}
+
+
 	@Configuration
 	static class SuperConfig {
 
@@ -140,7 +186,7 @@ Integer anInt() {
 			return 5;
 		}
 
-		@Bean
+		@Bean @Lazy
 		String aString(Integer dependency) {
 			return "overloaded" + dependency;
 		}
@@ -155,7 +201,7 @@ Integer anInt() {
 			return 5;
 		}
 
-		@Bean
+		@Bean @Lazy
 		String aString(List<Integer> dependency) {
 			return "overloaded" + dependency.get(0);
 		}