Ensure Parent ConfigurationClass loaded on overrides

Previously ConfigurationClassParser could override a nested
@Configuration without consideration of @Bean's defined in parent
classes.

This commit ensures that if the original ConfigurationClass contains
additional bean definitions it is processed again.

Issue: SPR-10546

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

@@ -23,9 +23,12 @@ 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;
 
@@ -76,6 +79,7 @@ import static org.springframework.context.annotation.MetadataUtils.*;
  *
  * @author Chris Beams
  * @author Juergen Hoeller
+ * @author Rob Winch
  * @since 3.0
  * @see ConfigurationClassBeanDefinitionReader
  */
@@ -89,8 +93,8 @@ class ConfigurationClassParser {
 
 	private final Set<String> knownSuperclasses = new LinkedHashSet<String>();
 
-	private final Set<ConfigurationClass> configurationClasses =
-		new LinkedHashSet<ConfigurationClass>();
+	private final Map<ConfigurationClass,ConfigurationClass> configurationClasses =
+		new LinkedHashMap<ConfigurationClass,ConfigurationClass>();
 
 	private final Stack<PropertySource<?>> propertySources =
 		new Stack<PropertySource<?>>();
@@ -157,13 +161,60 @@ class ConfigurationClassParser {
 		}
 		while (metadata != null);
 
-		if (this.configurationClasses.contains(configClass) && configClass.getBeanName() != null) {
+		if (getConfigurationClasses().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);
+			ConfigurationClass originalConfigClass = removeConfigurationClass(configClass);
+
+			mergeFromOriginalConfig(originalConfigClass,configClass);
 		}
 
-		this.configurationClasses.add(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
+	 *
+	 * @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;
 	}
 
 	/**
@@ -251,6 +302,14 @@ 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
@@ -441,13 +500,13 @@ class ConfigurationClassParser {
 	 * @see ConfigurationClass#validate
 	 */
 	public void validate() {
-		for (ConfigurationClass configClass : this.configurationClasses) {
+		for (ConfigurationClass configClass : getConfigurationClasses()) {
 			configClass.validate(this.problemReporter);
 		}
 	}
 
 	public Set<ConfigurationClass> getConfigurationClasses() {
-		return this.configurationClasses;
+		return this.configurationClasses.keySet();
 	}
 
 	public Stack<PropertySource<?>> getPropertySources() {

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ImportedConfig.java

@@ -0,0 +1,33 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+public class ImportedConfig {
+	@Bean
+	public String myBean() {
+		return "myBean";
+	}
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ParentConfig.java

@@ -0,0 +1,34 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+public class ParentConfig {
+	@Bean
+	public String myBean() {
+		return "myBean";
+	}
+}
+

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ParentWithComponentScanConfig.java

@@ -0,0 +1,32 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.ComponentScan;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.spr10546.scanpackage.AEnclosingConfig;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+@ComponentScan(basePackageClasses=AEnclosingConfig.class)
+public class ParentWithComponentScanConfig {
+
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ParentWithImportConfig.java

@@ -0,0 +1,31 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+@Import(ImportedConfig.class)
+public class ParentWithImportConfig {
+
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ParentWithImportResourceConfig.java

@@ -0,0 +1,31 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.ImportResource;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+@ImportResource("classpath:org/springframework/context/annotation/spr10546/importedResource.xml")
+public class ParentWithImportResourceConfig {
+
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/ParentWithParentConfig.java

@@ -0,0 +1,29 @@
+/*
+ * 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.spr10546;
+
+import org.springframework.context.annotation.Configuration;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+@Configuration
+public class ParentWithParentConfig extends ParentConfig {
+
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/Spr10546Tests.java

@@ -0,0 +1,149 @@
+/*
+ * 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.spr10546;
+
+import org.junit.After;
+import org.junit.Test;
+import org.springframework.context.ConfigurableApplicationContext;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Import;
+import org.springframework.context.annotation.spr10546.scanpackage.AEnclosingConfig;
+
+import static org.hamcrest.CoreMatchers.*;
+import static org.junit.Assert.*;
+
+
+/**
+ *
+ * @author Rob Winch
+ */
+public class Spr10546Tests {
+	private ConfigurableApplicationContext context;
+
+	@After
+	public void closeContext() {
+		if(context != null) {
+			context.close();
+		}
+	}
+
+	// These fail prior to fixing SPR-10546
+
+	@Test
+	public void enclosingConfigFirstParentDefinesBean() {
+		assertLoadsMyBean(AEnclosingConfig.class,AEnclosingConfig.ChildConfig.class);
+	}
+
+	/**
+	 * Prior to fixing SPR-10546 this might have succeeded depending on the ordering the
+	 * classes were picked up. If they are picked up in the same order as
+	 * {@link #enclosingConfigFirstParentDefinesBean()} then it would fail. This test is
+	 * mostly for illustration purposes, but doesn't hurt to continue using it.
+	 *
+	 * <p>We purposely use the {@link AEnclosingConfig} to make it alphabetically prior to the
+	 * {@link AEnclosingConfig.ChildConfig} which encourages this to occur with the
+	 * classpath scanning implementation being used by the author of this test.
+	 */
+	@Test
+	public void enclosingConfigFirstParentDefinesBeanWithScanning() {
+		AnnotationConfigApplicationContext ctx= new AnnotationConfigApplicationContext();
+		context = ctx;
+		ctx.scan(AEnclosingConfig.class.getPackage().getName());
+		ctx.refresh();
+		assertThat(context.getBean("myBean",String.class), equalTo("myBean"));
+	}
+
+	@Test
+	public void enclosingConfigFirstParentDefinesBeanWithImportResource() {
+		assertLoadsMyBean(AEnclosingWithImportResourceConfig.class,AEnclosingWithImportResourceConfig.ChildConfig.class);
+	}
+
+	@Configuration
+	static class AEnclosingWithImportResourceConfig {
+		@Configuration
+		public static class ChildConfig extends ParentWithImportResourceConfig {}
+	}
+
+	@Test
+	public void enclosingConfigFirstParentDefinesBeanWithComponentScan() {
+		assertLoadsMyBean(AEnclosingWithComponentScanConfig.class,AEnclosingWithComponentScanConfig.ChildConfig.class);
+	}
+
+	@Configuration
+	static class AEnclosingWithComponentScanConfig {
+		@Configuration
+		public static class ChildConfig extends ParentWithComponentScanConfig {}
+	}
+
+	@Test
+	public void enclosingConfigFirstParentWithParentDefinesBean() {
+		assertLoadsMyBean(AEnclosingWithGrandparentConfig.class,AEnclosingWithGrandparentConfig.ChildConfig.class);
+	}
+
+	@Configuration
+	static class AEnclosingWithGrandparentConfig {
+		@Configuration
+		public static class ChildConfig extends ParentWithParentConfig {}
+	}
+
+	@Test
+	public void importChildConfigThenChildConfig() {
+		assertLoadsMyBean(ImportChildConfig.class,ChildConfig.class);
+	}
+
+	@Configuration
+	static class ChildConfig extends ParentConfig {}
+
+	@Configuration
+	@Import(ChildConfig.class)
+	static class ImportChildConfig {}
+
+
+	// These worked prior, but validating they continue to work
+
+	@Test
+	public void enclosingConfigFirstParentDefinesBeanWithImport() {
+		assertLoadsMyBean(AEnclosingWithImportConfig.class,AEnclosingWithImportConfig.ChildConfig.class);
+	}
+
+	@Configuration
+	static class AEnclosingWithImportConfig {
+		@Configuration
+		public static class ChildConfig extends ParentWithImportConfig {}
+	}
+
+	@Test
+	public void childConfigFirst() {
+		assertLoadsMyBean(AEnclosingConfig.ChildConfig.class, AEnclosingConfig.class);
+	}
+
+	@Test
+	public void enclosingConfigOnly() {
+		assertLoadsMyBean(AEnclosingConfig.class);
+	}
+
+	@Test
+	public void childConfigOnly() {
+		assertLoadsMyBean(AEnclosingConfig.ChildConfig.class);
+	}
+
+	private void assertLoadsMyBean(Class<?>... annotatedClasses) {
+		context = new AnnotationConfigApplicationContext(annotatedClasses);
+		assertThat(context.getBean("myBean",String.class), equalTo("myBean"));
+	}
+}

spring-context/src/test/java/org/springframework/context/annotation/spr10546/importedResource.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xmlns:c="http://www.springframework.org/schema/c"
+	xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">
+
+	<bean id="myBean" class="java.lang.String" c:_0="myBean"/>
+</beans>

spring-context/src/test/java/org/springframework/context/annotation/spr10546/scanpackage/AEnclosingConfig.java

@@ -0,0 +1,34 @@
+/*
+ * 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.spr10546.scanpackage;
+
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.spr10546.ParentConfig;
+
+
+/**
+ * Note the name of {@link AEnclosingConfig} is chosen to help ensure scanning picks up
+ * the enclosing configuration prior to {@link ChildConfig} to demonstrate this can happen
+ * with classpath scanning.
+ *
+ * @author Rob Winch
+ */
+@Configuration
+public class AEnclosingConfig {
+	@Configuration
+	public static class ChildConfig extends ParentConfig {}
+}