Properly handle knownSuperclasses in case of an overridden ConfigurationClass

Issue: SPR-10546
(cherry picked from commit 6e4317e)

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
- * ancestry of the class, in a 'flattened-out' manner.
+ * Includes a set of {@link Bean} methods, including all such methods
+ * defined in the ancestry of the class, in a 'flattened-out' manner.
  *
  * @author Chris Beams
  * @author Juergen Hoeller
@@ -52,15 +52,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 ConfigurationClass importedBy;
 
+	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.
@@ -99,7 +99,7 @@ final class ConfigurationClass {
 	 * @see ConfigurationClass#ConfigurationClass(Class, ConfigurationClass)
 	 */
 	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;
@@ -133,6 +133,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.
@@ -153,14 +161,6 @@ final class ConfigurationClass {
 		return importedBy;
 	}
 
-	public void setBeanName(String beanName) {
-		this.beanName = beanName;
-	}
-
-	public String getBeanName() {
-		return this.beanName;
-	}
-
 	public void addBeanMethod(BeanMethod method) {
 		this.beanMethods.add(method);
 	}
@@ -169,8 +169,7 @@ final class ConfigurationClass {
 		return this.beanMethods;
 	}
 
-	public void addImportedResource(
-			String importedResource, Class<? extends BeanDefinitionReader> readerClass) {
+	public void addImportedResource(String importedResource, Class<? extends BeanDefinitionReader> readerClass) {
 		this.importedResources.put(importedResource, readerClass);
 	}
 
@@ -178,35 +177,32 @@ final class ConfigurationClass {
 		return this.importedResources;
 	}
 
+
 	public void validate(ProblemReporter problemReporter) {
+		// A configuration class may not be final (CGLIB limitation)
+		if (getMetadata().isAnnotated(Configuration.class.getName())) {
+			if (getMetadata().isFinal()) {
+				problemReporter.error(new FinalConfigurationProblem());
+			}
+		}
+
 		// 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;
+		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 methodName : methodNameCounts.keySet()) {
-			int count = methodNameCounts.get(methodName);
+		for (String fqMethodName : methodNameCounts.keySet()) {
+			int count = methodNameCounts.get(fqMethodName);
 			if (count > 1) {
-				String shortMethodName = methodName.substring(methodName.indexOf(hashDelim)+1);
+				String shortMethodName = ConfigurationMethod.getShortMethodName(fqMethodName);
 				problemReporter.error(new BeanMethodOverloadingProblem(shortMethodName, count));
 			}
 		}
 
-		// A configuration class may not be final (CGLIB limitation)
-		if (getMetadata().isAnnotated(Configuration.class.getName())) {
-			if (getMetadata().isFinal()) {
-				problemReporter.error(new FinalConfigurationProblem());
-			}
-		}
-
 		for (BeanMethod beanMethod : this.beanMethods) {
 			beanMethod.validate(problemReporter);
 		}

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

@@ -90,40 +90,37 @@ class ConfigurationClassParser {
 
 	private static final Comparator<DeferredImportSelectorHolder> DEFERRED_IMPORT_COMPARATOR =
 			new Comparator<ConfigurationClassParser.DeferredImportSelectorHolder>() {
-		@Override
-		public int compare(DeferredImportSelectorHolder o1,
-				DeferredImportSelectorHolder o2) {
-			return AnnotationAwareOrderComparator.INSTANCE.compare(
-					o1.getImportSelector(), o2.getImportSelector());
-		}
-	};
+				@Override
+				public int compare(DeferredImportSelectorHolder o1, DeferredImportSelectorHolder o2) {
+					return AnnotationAwareOrderComparator.INSTANCE.compare(o1.getImportSelector(), o2.getImportSelector());
+				}
+			};
+
 
 	private final MetadataReaderFactory metadataReaderFactory;
 
 	private final ProblemReporter problemReporter;
 
-	private final ImportStack importStack = new ImportStack();
-
-	private final Set<String> knownSuperclasses = new LinkedHashSet<String>();
-
-	private final Set<ConfigurationClass> configurationClasses =
-		new LinkedHashSet<ConfigurationClass>();
-
-	private final Stack<PropertySource<?>> propertySources =
-		new Stack<PropertySource<?>>();
-
 	private final Environment environment;
 
 	private final ResourceLoader resourceLoader;
 
 	private final BeanDefinitionRegistry registry;
 
+	private final BeanNameGenerator beanNameGenerator;
+
 	private final ComponentScanAnnotationParser componentScanParser;
 
-	private final BeanNameGenerator beanNameGenerator;
+	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<?>>();
+
+	private final ImportStack importStack = new ImportStack();
+
+	private final List<DeferredImportSelectorHolder> deferredImportSelectors = new LinkedList<DeferredImportSelectorHolder>();
 
-	private final List<DeferredImportSelectorHolder> deferredImportSelectors =
-			new LinkedList<DeferredImportSelectorHolder>();
 
 	/**
 	 * Create a new {@link ConfigurationClassParser} instance that will be used
@@ -143,6 +140,7 @@ class ConfigurationClassParser {
 				resourceLoader, environment, componentScanBeanNameGenerator, registry);
 	}
 
+
 	public void parse(Set<BeanDefinitionHolder> configCandidates) {
 		for (BeanDefinitionHolder holder : configCandidates) {
 			BeanDefinition bd = holder.getBeanDefinition();
@@ -177,35 +175,39 @@ class ConfigurationClassParser {
 	 * @param clazz the Class to parse
 	 * @param beanName must not be null (as of Spring 3.1.1)
 	 */
-	protected void parse(Class<?> clazz, String beanName) throws IOException {
+	public void parse(Class<?> clazz, String beanName) throws IOException {
 		processConfigurationClass(new ConfigurationClass(clazz, beanName));
 	}
 
-	protected void processConfigurationClass(ConfigurationClass configClass) throws IOException {
-		AnnotationMetadata metadata = configClass.getMetadata();
 
-		if (ConditionalAnnotationHelper.shouldSkip(configClass, this.registry,
-				this.environment, this.beanNameGenerator)) {
+	protected void processConfigurationClass(ConfigurationClass configClass) throws IOException {
+		if (ConditionalAnnotationHelper.shouldSkip(configClass, this.registry, this.environment, this.beanNameGenerator)) {
 			return;
 		}
 
-		// recursively process the configuration class and its superclass hierarchy
-		do {
-			metadata = doProcessConfigurationClass(configClass, metadata);
-		}
-		while (metadata != null);
-
 		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.
+		AnnotationMetadata metadata = configClass.getMetadata();
+		do {
+			metadata = doProcessConfigurationClass(configClass, metadata);
+		}
+		while (metadata != null);
+
 		this.configurationClasses.add(configClass);
 	}
 
 	/**
-	 * @return annotation metadata of superclass, null if none found or previously processed
+	 * @return annotation metadata of superclass, {@code null} if none found or previously processed
 	 */
 	protected AnnotationMetadata doProcessConfigurationClass(
 			ConfigurationClass configClass, AnnotationMetadata metadata) throws IOException {
@@ -219,7 +221,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
@@ -235,7 +237,6 @@ class ConfigurationClassParser {
 		}
 
 		// process any @Import annotations
-
 		Set<Object> imports = new LinkedHashSet<Object>();
 		Set<Object> visited = new LinkedHashSet<Object>();
 		collectImports(metadata, imports, visited);
@@ -262,7 +263,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();
@@ -416,7 +418,7 @@ class ConfigurationClassParser {
 				throw new BeanDefinitionStoreException("Failed to load bean class: ", ex);
 			}
 		}
-		deferredImportSelectors.clear();
+		this.deferredImportSelectors.clear();
 	}
 
 	private void processImport(ConfigurationClass configClass, Collection<?> classesToImport, boolean checkForCircularImports) throws IOException {
@@ -435,9 +437,9 @@ class ConfigurationClassParser {
 						ImportSelector selector = BeanUtils.instantiateClass(candidateClass, ImportSelector.class);
 						invokeAwareMethods(selector);
 						if(selector instanceof DeferredImportSelector) {
-							this.deferredImportSelectors.add(new DeferredImportSelectorHolder(
-									configClass, (DeferredImportSelector) selector));
-						} else {
+							this.deferredImportSelectors.add(new DeferredImportSelectorHolder(configClass, (DeferredImportSelector) selector));
+						}
+						else {
 							processImport(configClass, Arrays.asList(selector.selectImports(importingClassMetadata)), false);
 						}
 					}
@@ -516,7 +518,7 @@ class ConfigurationClassParser {
 		return this.propertySources;
 	}
 
-	public ImportRegistry getImportRegistry() {
+	ImportRegistry getImportRegistry() {
 		return this.importStack;
 	}
 
@@ -582,26 +584,11 @@ class ConfigurationClassParser {
 	}
 
 
-	/**
-	 * {@link Problem} registered upon detection of a circular {@link Import}.
-	 */
-	private static class CircularImportProblem extends Problem {
-
-		public CircularImportProblem(ConfigurationClass attemptedImport, Stack<ConfigurationClass> importStack, AnnotationMetadata metadata) {
-			super(String.format("A circular @Import has been detected: " +
-					"Illegal attempt by @Configuration class '%s' to import class '%s' as '%s' is " +
-					"already present in the current import stack [%s]", importStack.peek().getSimpleName(),
-					attemptedImport.getSimpleName(), attemptedImport.getSimpleName(), importStack),
-					new Location(importStack.peek().getResource(), metadata));
-		}
-	}
-
-
 	private static class DeferredImportSelectorHolder {
 
-		private ConfigurationClass configurationClass;
+		private final ConfigurationClass configurationClass;
 
-		private DeferredImportSelector importSelector;
+		private final DeferredImportSelector importSelector;
 
 		public DeferredImportSelectorHolder(ConfigurationClass configurationClass, DeferredImportSelector importSelector) {
 			this.configurationClass = configurationClass;
@@ -609,11 +596,27 @@ class ConfigurationClassParser {
 		}
 
 		public ConfigurationClass getConfigurationClass() {
-			return configurationClass;
+			return this.configurationClass;
 		}
 
 		public DeferredImportSelector getImportSelector() {
-			return importSelector;
+			return this.importSelector;
 		}
 	}
+
+
+	/**
+	 * {@link Problem} registered upon detection of a circular {@link Import}.
+	 */
+	private static class CircularImportProblem extends Problem {
+
+		public CircularImportProblem(ConfigurationClass attemptedImport, Stack<ConfigurationClass> importStack, AnnotationMetadata metadata) {
+			super(String.format("A circular @Import has been detected: " +
+					"Illegal attempt by @Configuration class '%s' to import class '%s' as '%s' is " +
+					"already present in the current import stack [%s]", importStack.peek().getSimpleName(),
+					attemptedImport.getSimpleName(), attemptedImport.getSimpleName(), importStack),
+					new Location(importStack.peek().getResource(), metadata));
+		}
+	}
+
 }

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());
 	}
 
 }