Properly handle knownSuperclasses in case of an overridden ConfigurationClass

Issue: SPR-10546

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

@@ -36,8 +36,8 @@ import org.springframework.util.ClassUtils;
 
 /**
  * Represents a user-defined {@link Configuration @Configuration} class.
- * Includes a set of {@link Bean} methods, including all such methods defined in the
+ * Includes a set of {@link Bean} methods, including all such methods
- * ancestry of the class, in a 'flattened-out' manner.
+ * defined in the ancestry of the class, in a 'flattened-out' manner.
  *
  * @author Chris Beams
  * @author Juergen Hoeller
@@ -51,15 +51,15 @@ final class ConfigurationClass {
 
 	private final Resource resource;
 
-	private final Map<String, Class<? extends BeanDefinitionReader>> importedResources =
-			new LinkedHashMap<String, Class<? extends BeanDefinitionReader>>();
-
-	private final Set<BeanMethod> beanMethods = new LinkedHashSet<BeanMethod>();
-
 	private String beanName;
 
 	private final boolean imported;
 
+	private final Set<BeanMethod> beanMethods = new LinkedHashSet<BeanMethod>();
+
+	private final Map<String, Class<? extends BeanDefinitionReader>> importedResources =
+			new LinkedHashMap<String, Class<? extends BeanDefinitionReader>>();
+
 
 	/**
 	 * Create a new {@link ConfigurationClass} with the given name.
@@ -98,7 +98,7 @@ final class ConfigurationClass {
 	 * @see ConfigurationClass#ConfigurationClass(Class, boolean)
 	 */
 	public ConfigurationClass(Class<?> clazz, String beanName) {
-		Assert.hasText(beanName, "bean name must not be null");
+		Assert.hasText(beanName, "Bean name must not be null");
 		this.metadata = new StandardAnnotationMetadata(clazz, true);
 		this.resource = new DescriptiveResource(clazz.toString());
 		this.beanName = beanName;
@@ -132,6 +132,14 @@ final class ConfigurationClass {
 		return ClassUtils.getShortName(getMetadata().getClassName());
 	}
 
+	public void setBeanName(String beanName) {
+		this.beanName = beanName;
+	}
+
+	public String getBeanName() {
+		return this.beanName;
+	}
+
 	/**
 	 * Return whether this configuration class was registered via @{@link Import} or
 	 * automatically registered due to being nested within another configuration class.
@@ -141,14 +149,6 @@ final class ConfigurationClass {
 		return this.imported;
 	}
 
-	public void setBeanName(String beanName) {
-		this.beanName = beanName;
-	}
-
-	public String getBeanName() {
-		return this.beanName;
-	}
-
 	public void addBeanMethod(BeanMethod method) {
 		this.beanMethods.add(method);
 	}
@@ -157,8 +157,7 @@ final class ConfigurationClass {
 		return this.beanMethods;
 	}
 
-	public void addImportedResource(
+	public void addImportedResource(String importedResource, Class<? extends BeanDefinitionReader> readerClass) {
-			String importedResource, Class<? extends BeanDefinitionReader> readerClass) {
 		this.importedResources.put(importedResource, readerClass);
 	}
 
@@ -166,28 +165,8 @@ final class ConfigurationClass {
 		return this.importedResources;
 	}
 
+
 	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.
-		final char hashDelim = '#';
-		Map<String, Integer> methodNameCounts = new HashMap<String, Integer>();
-		for (BeanMethod beanMethod : beanMethods) {
-			String dClassName = beanMethod.getMetadata().getDeclaringClassName();
-			String methodName = beanMethod.getMetadata().getMethodName();
-			String fqMethodName = dClassName + hashDelim + methodName;
-			Integer currentCount = methodNameCounts.get(fqMethodName);
-			int newCount = currentCount != null ? currentCount + 1 : 1;
-			methodNameCounts.put(fqMethodName, newCount);
-		}
-
-		for (String methodName : methodNameCounts.keySet()) {
-			int count = methodNameCounts.get(methodName);
-			if (count > 1) {
-				String shortMethodName = methodName.substring(methodName.indexOf(hashDelim)+1);
-				problemReporter.error(new BeanMethodOverloadingProblem(shortMethodName, count));
-			}
-		}
-
 		// A configuration class may not be final (CGLIB limitation)
 		if (getMetadata().isAnnotated(Configuration.class.getName())) {
 			if (getMetadata().isFinal()) {
@@ -195,6 +174,23 @@ final class ConfigurationClass {
 			}
 		}
 
+		// 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);
 		}

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

@@ -23,12 +23,9 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Iterator;
-import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.Stack;
 
@@ -79,7 +76,6 @@ import static org.springframework.context.annotation.MetadataUtils.*;
  *
  * @author Chris Beams
  * @author Juergen Hoeller
- * @author Rob Winch
  * @since 3.0
  * @see ConfigurationClassBeanDefinitionReader
  */
@@ -91,14 +87,6 @@ class ConfigurationClassParser {
 
 	private final ImportStack importStack = new ImportStack();
 
-	private final Set<String> knownSuperclasses = new LinkedHashSet<String>();
-
-	private final Map<ConfigurationClass,ConfigurationClass> configurationClasses =
-		new LinkedHashMap<ConfigurationClass,ConfigurationClass>();
-
-	private final Stack<PropertySource<?>> propertySources =
-		new Stack<PropertySource<?>>();
-
 	private final Environment environment;
 
 	private final ResourceLoader resourceLoader;
@@ -107,6 +95,12 @@ class ConfigurationClassParser {
 
 	private final ComponentScanAnnotationParser componentScanParser;
 
+	private final Set<ConfigurationClass> configurationClasses = new LinkedHashSet<ConfigurationClass>();
+
+	private final Map<String, ConfigurationClass> knownSuperclasses = new HashMap<String, ConfigurationClass>();
+
+	private final Stack<PropertySource<?>> propertySources = new Stack<PropertySource<?>>();
+
 
 	/**
 	 * Create a new {@link ConfigurationClassParser} instance that will be used
@@ -155,70 +149,28 @@ class ConfigurationClassParser {
 			}
 		}
 
-		// recursively process the configuration class and its superclass hierarchy
+		if (this.configurationClasses.contains(configClass) && configClass.getBeanName() != null) {
+			// Explicit bean definition found, probably replacing an import.
+			// Let's remove the old one and go with the new one.
+			this.configurationClasses.remove(configClass);
+			for (Iterator<ConfigurationClass> it = this.knownSuperclasses.values().iterator(); it.hasNext();) {
+				if (configClass.equals(it.next())) {
+					it.remove();
+				}
+			}
+		}
+
+		// Recursively process the configuration class and its superclass hierarchy.
 		do {
 			metadata = doProcessConfigurationClass(configClass, metadata);
 		}
 		while (metadata != null);
 
-		if (getConfigurationClasses().contains(configClass) && configClass.getBeanName() != null) {
+		this.configurationClasses.add(configClass);
-			// Explicit bean definition found, probably replacing an import.
-			// Let's remove the old one and go with the new one.
-			ConfigurationClass originalConfigClass = removeConfigurationClass(configClass);
-
-			mergeFromOriginalConfig(originalConfigClass,configClass);
-		}
-
-		addConfigurationClass(configClass);
-	}
-
-
-	/**
-	 * Merges from the original {@link ConfigurationClass} to the new
-	 * {@link ConfigurationClass}. This is necessary if parent classes have already been
-	 * processed.
-	 *
-	 * @param originalConfigClass the original {@link ConfigurationClass} that may have
-	 *        additional metadata
-	 * @param configClass the new {@link ConfigurationClass} that will have metadata added
-	 *        to it if necessary
-	 */
-	private void mergeFromOriginalConfig(ConfigurationClass originalConfigClass,
-			ConfigurationClass configClass) {
-
-		Set<String> beanMethodNames = new HashSet<String>();
-		for(BeanMethod beanMethod : configClass.getBeanMethods()) {
-			beanMethodNames.add(createBeanMethodName(beanMethod));
-		}
-
-		for(BeanMethod originalBeanMethod : originalConfigClass.getBeanMethods()) {
-			String originalBeanMethodName = createBeanMethodName(originalBeanMethod);
-			if(!beanMethodNames.contains(originalBeanMethodName)) {
-				configClass.addBeanMethod(new BeanMethod(originalBeanMethod.getMetadata(), configClass));
-			}
-		}
-		for(Entry<String, Class<? extends BeanDefinitionReader>> originalImportedEntry : originalConfigClass.getImportedResources().entrySet()) {
-			if(!configClass.getImportedResources().containsKey(originalImportedEntry.getKey())) {
-				configClass.addImportedResource(originalImportedEntry.getKey(), originalImportedEntry.getValue());
-			}
-		}
 	}
 
 	/**
-	 * Converts a {@link BeanMethod} into the fully qualified name of the Method
+	 * @return annotation metadata of superclass, {@code null} if none found or previously processed
-	 *
-	 * @param beanMethod
-	 * @return fully qualified name of the {@link BeanMethod}
-	 */
-	private String createBeanMethodName(BeanMethod beanMethod) {
-		String hashDelim = "#";
-		String dClassName = beanMethod.getMetadata().getDeclaringClassName();
-		String methodName = beanMethod.getMetadata().getMethodName();
-		return dClassName + hashDelim + methodName;
-	}
-
-	/**
-	 * @return annotation metadata of superclass, null if none found or previously processed
 	 */
 	protected AnnotationMetadata doProcessConfigurationClass(
 			ConfigurationClass configClass, AnnotationMetadata metadata) throws IOException {
@@ -232,7 +184,7 @@ class ConfigurationClassParser {
 			processPropertySource(propertySource);
 		}
 
-		// process any @ComponentScan annotions
+		// process any @ComponentScan annotations
 		AnnotationAttributes componentScan = attributesFor(metadata, ComponentScan.class);
 		if (componentScan != null) {
 			// the config class is annotated with @ComponentScan -> perform the scan immediately
@@ -248,7 +200,6 @@ class ConfigurationClassParser {
 		}
 
 		// process any @Import annotations
-
 		Set<Object> imports = new LinkedHashSet<Object>();
 		Set<Object> visited = new LinkedHashSet<Object>();
 		collectImports(metadata, imports, visited);
@@ -275,7 +226,8 @@ class ConfigurationClassParser {
 		// process superclass, if any
 		if (metadata.hasSuperClass()) {
 			String superclass = metadata.getSuperClassName();
-			if (this.knownSuperclasses.add(superclass)) {
+			if (!this.knownSuperclasses.containsKey(superclass)) {
+				this.knownSuperclasses.put(superclass, configClass);
 				// superclass found, return its annotation metadata and recurse
 				if (metadata instanceof StandardAnnotationMetadata) {
 					Class<?> clazz = ((StandardAnnotationMetadata) metadata).getIntrospectedClass();
@@ -302,14 +254,6 @@ class ConfigurationClassParser {
 		return null;
 	}
 
-	private void addConfigurationClass(ConfigurationClass configClass) {
-		this.configurationClasses.put(configClass,configClass);
-	}
-
-	private ConfigurationClass removeConfigurationClass(ConfigurationClass configClass) {
-		return this.configurationClasses.remove(configClass);
-	}
-
 	/**
 	 * Register member (nested) classes that happen to be configuration classes themselves.
 	 * @param metadata the metadata representation of the containing class
@@ -500,13 +444,13 @@ class ConfigurationClassParser {
 	 * @see ConfigurationClass#validate
 	 */
 	public void validate() {
-		for (ConfigurationClass configClass : getConfigurationClasses()) {
+		for (ConfigurationClass configClass : this.configurationClasses) {
 			configClass.validate(this.problemReporter);
 		}
 	}
 
 	public Set<ConfigurationClass> getConfigurationClasses() {
-		return this.configurationClasses.keySet();
+		return this.configurationClasses;
 	}
 
 	public Stack<PropertySource<?>> getPropertySources() {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2011 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.
@@ -36,6 +36,7 @@ abstract class ConfigurationMethod {
 		this.configurationClass = configurationClass;
 	}
 
+
 	public MethodMetadata getMetadata() {
 		return this.metadata;
 	}
@@ -48,13 +49,22 @@ abstract class ConfigurationMethod {
 		return new Location(this.configurationClass.getResource(), this.metadata);
 	}
 
+	String getFullyQualifiedMethodName() {
+		return this.metadata.getDeclaringClassName() + "#" + this.metadata.getMethodName();
+	}
+
+	static String getShortMethodName(String fullyQualifiedMethodName) {
+		return fullyQualifiedMethodName.substring(fullyQualifiedMethodName.indexOf('#') + 1);
+	}
+
 	public void validate(ProblemReporter problemReporter) {
 	}
 
+
 	@Override
 	public String toString() {
 		return String.format("[%s:name=%s,declaringClass=%s]",
-				this.getClass().getSimpleName(), this.getMetadata().getMethodName(), this.getMetadata().getDeclaringClassName());
+				getClass().getSimpleName(), getMetadata().getMethodName(), getMetadata().getDeclaringClassName());
 	}
 
 }