Properly evaluate @Conditional in case of multiple imports for same config class

Issue: SPR-11788

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -83,13 +83,13 @@ class ConditionEvaluator {
 
 		for (String[] conditionClasses : getConditionClasses(metadata)) {
 			for (String conditionClass : conditionClasses) {
-				Condition condition = getCondition(conditionClass, context.getClassLoader());
+				Condition condition = getCondition(conditionClass, this.context.getClassLoader());
 				ConfigurationPhase requiredPhase = null;
 				if (condition instanceof ConfigurationCondition) {
 					requiredPhase = ((ConfigurationCondition) condition).getConfigurationPhase();
 				}
 				if (requiredPhase == null || requiredPhase == phase) {
-					if (!condition.matches(context, metadata)) {
+					if (!condition.matches(this.context, metadata)) {
 						return true;
 					}
 				}

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -53,7 +53,7 @@ final class ConfigurationClass {
 
 	private String beanName;
 
-	private final ConfigurationClass importedBy;
+	private final Set<ConfigurationClass> importedBy = new LinkedHashSet<ConfigurationClass>(1);
 
 	private final Set<BeanMethod> beanMethods = new LinkedHashSet<BeanMethod>();
 
@@ -76,7 +76,6 @@ final class ConfigurationClass {
 		this.metadata = metadataReader.getAnnotationMetadata();
 		this.resource = metadataReader.getResource();
 		this.beanName = beanName;
-		this.importedBy = null;
 	}
 
 	/**
@@ -90,7 +89,7 @@ final class ConfigurationClass {
 	public ConfigurationClass(MetadataReader metadataReader, ConfigurationClass importedBy) {
 		this.metadata = metadataReader.getAnnotationMetadata();
 		this.resource = metadataReader.getResource();
-		this.importedBy = importedBy;
+		this.importedBy.add(importedBy);
 	}
 
 	/**
@@ -105,7 +104,6 @@ final class ConfigurationClass {
 		this.metadata = new StandardAnnotationMetadata(clazz, true);
 		this.resource = new DescriptiveResource(clazz.toString());
 		this.beanName = beanName;
-		this.importedBy = null;
 	}
 
 	/**
@@ -119,7 +117,7 @@ final class ConfigurationClass {
 	public ConfigurationClass(Class<?> clazz, ConfigurationClass importedBy) {
 		this.metadata = new StandardAnnotationMetadata(clazz, true);
 		this.resource = new DescriptiveResource(clazz.toString());
-		this.importedBy = importedBy;
+		this.importedBy.add(importedBy);
 	}
 
 
@@ -150,16 +148,24 @@ final class ConfigurationClass {
 	 * @see #getImportedBy()
 	 */
 	public boolean isImported() {
-		return (this.importedBy != null);
+		return !this.importedBy.isEmpty();
 	}
 
 	/**
-	 * Return the configuration class that imported this class,
-	 * or {@code null} if this configuration was not imported.
-	 * @since 4.0
+	 * Merge the imported-by declarations from the given configuration class into this one.
+	 * @since 4.0.5
+	 */
+	public void mergeImportedBy(ConfigurationClass otherConfigClass) {
+		this.importedBy.addAll(otherConfigClass.importedBy);
+	}
+
+	/**
+	 * Return the configuration classes that imported this class,
+	 * or an empty Set if this configuration was not imported.
+	 * @since 4.0.5
 	 * @see #isImported()
 	 */
-	public ConfigurationClass getImportedBy() {
+	public Set<ConfigurationClass> getImportedBy() {
 		return this.importedBy;
 	}
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -391,14 +391,19 @@ class ConfigurationClassBeanDefinitionReader {
 			Boolean skip = this.skipped.get(configClass);
 			if (skip == null) {
 				if (configClass.isImported()) {
-					if (shouldSkip(configClass.getImportedBy())) {
-						// The config that imported this one was skipped, therefore we are skipped
+					boolean allSkipped = true;
+					for (ConfigurationClass importedBy : configClass.getImportedBy()) {
+						if (!shouldSkip(importedBy)) {
+							allSkipped = false;
+						}
+					}
+					if (allSkipped) {
+						// The config classes that imported this one were all skipped, therefore we are skipped...
 						skip = true;
 					}
 				}
 				if (skip == null) {
-					skip = conditionEvaluator.shouldSkip(configClass.getMetadata(),
-							ConfigurationPhase.REGISTER_BEAN);
+					skip = conditionEvaluator.shouldSkip(configClass.getMetadata(), ConfigurationPhase.REGISTER_BEAN);
 				}
 				this.skipped.put(configClass, skip);
 			}

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

@@ -24,6 +24,7 @@ import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -112,7 +113,8 @@ class ConfigurationClassParser {
 
 	private final ComponentScanAnnotationParser componentScanParser;
 
-	private final Set<ConfigurationClass> configurationClasses = new LinkedHashSet<ConfigurationClass>();
+	private final Map<ConfigurationClass, ConfigurationClass> configurationClasses =
+			new LinkedHashMap<ConfigurationClass, ConfigurationClass>();
 
 	private final Map<String, ConfigurationClass> knownSuperclasses = new HashMap<String, ConfigurationClass>();
 
@@ -189,13 +191,23 @@ class ConfigurationClassParser {
 			return;
 		}
 
-		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();
+		ConfigurationClass existingClass = this.configurationClasses.get(configClass);
+		if (existingClass != null) {
+			if (configClass.isImported()) {
+				if (existingClass.isImported()) {
+					existingClass.mergeImportedBy(configClass);
+				}
+				// Otherwise ignore new imported config class; existing non-imported class overrides it.
+				return;
+			}
+			else {
+				// 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();
+					}
 				}
 			}
 		}
@@ -207,7 +219,7 @@ class ConfigurationClassParser {
 		}
 		while (sourceClass != null);
 
-		this.configurationClasses.add(configClass);
+		this.configurationClasses.put(configClass, configClass);
 	}
 
 	/**
@@ -466,13 +478,13 @@ class ConfigurationClassParser {
 	 * @see ConfigurationClass#validate
 	 */
 	public void validate() {
-		for (ConfigurationClass configClass : this.configurationClasses) {
+		for (ConfigurationClass configClass : this.configurationClasses.keySet()) {
 			configClass.validate(this.problemReporter);
 		}
 	}
 
 	public Set<ConfigurationClass> getConfigurationClasses() {
-		return this.configurationClasses;
+		return this.configurationClasses.keySet();
 	}
 
 	public List<PropertySource<?>> getPropertySources() {

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

@@ -0,0 +1,115 @@
+/*
+ * Copyright 2012-2014 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.Test;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.AnnotationConfigApplicationContext;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.ConditionContext;
+import org.springframework.context.annotation.Conditional;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.ConfigurationCondition;
+import org.springframework.context.annotation.Import;
+import org.springframework.core.type.AnnotatedTypeMetadata;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author Andy Wilkinson
+ */
+public class ImportWithConditionTests {
+
+	private AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
+
+	@Test
+	public void conditionalThenUnconditional() throws Exception {
+		this.context.register(ConditionalThenUnconditional.class);
+		this.context.refresh();
+		assertFalse(this.context.containsBean("beanTwo"));
+		assertTrue(this.context.containsBean("beanOne"));
+	}
+
+	@Test
+	public void unconditionalThenConditional() throws Exception {
+		this.context.register(UnconditionalThenConditional.class);
+		this.context.refresh();
+		assertFalse(this.context.containsBean("beanTwo"));
+		assertTrue(this.context.containsBean("beanOne"));
+	}
+
+
+	@Configuration
+	@Import({ConditionalConfiguration.class, UnconditionalConfiguration.class})
+	protected static class ConditionalThenUnconditional {
+
+		@Autowired
+		private BeanOne beanOne;
+	}
+
+
+	@Configuration
+	@Import({UnconditionalConfiguration.class, ConditionalConfiguration.class})
+	protected static class UnconditionalThenConditional {
+
+		@Autowired
+		private BeanOne beanOne;
+	}
+
+
+	@Configuration
+	@Import(BeanProvidingConfiguration.class)
+	protected static class UnconditionalConfiguration {
+	}
+
+
+	@Configuration
+	@Conditional(NeverMatchingCondition.class)
+	@Import(BeanProvidingConfiguration.class)
+	protected static class ConditionalConfiguration {
+	}
+
+
+	@Configuration
+	protected static class BeanProvidingConfiguration {
+
+		@Bean
+		BeanOne beanOne() {
+			return new BeanOne();
+		}
+	}
+
+
+	private static final class BeanOne {
+	}
+
+
+	private static final class NeverMatchingCondition implements ConfigurationCondition {
+
+		@Override
+		public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) {
+			return false;
+		}
+
+		@Override
+		public ConfigurationPhase getConfigurationPhase() {
+			return ConfigurationPhase.REGISTER_BEAN;
+		}
+	}
+
+}