From 52f44b340e03de4fa092f543346c1eb384e78e8d Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Fri, 16 May 2014 15:06:22 +0200 Subject: [PATCH] Properly evaluate @Conditional in case of multiple imports for same config class Issue: SPR-11788 --- .../annotation/ConditionEvaluator.java | 6 +- .../annotation/ConfigurationClass.java | 28 +++-- ...onfigurationClassBeanDefinitionReader.java | 15 ++- .../annotation/ConfigurationClassParser.java | 34 ++++-- .../ImportWithConditionTests.java | 115 ++++++++++++++++++ 5 files changed, 168 insertions(+), 30 deletions(-) create mode 100644 spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportWithConditionTests.java diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConditionEvaluator.java b/spring-context/src/main/java/org/springframework/context/annotation/ConditionEvaluator.java index cc98804ea88..fe33694a0a8 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConditionEvaluator.java +++ b/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; } } diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java index de0318722cc..5087c1fd2c3 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java +++ b/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 importedBy = new LinkedHashSet(1); private final Set beanMethods = new LinkedHashSet(); @@ -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 getImportedBy() { return this.importedBy; } diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index 3a9d7408d90..eadbbf05ad1 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/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); } diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java index 1438a2aa217..8c99d1c385f 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java +++ b/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 configurationClasses = new LinkedHashSet(); + private final Map configurationClasses = + new LinkedHashMap(); private final Map knownSuperclasses = new HashMap(); @@ -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 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 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 getConfigurationClasses() { - return this.configurationClasses; + return this.configurationClasses.keySet(); } public List> getPropertySources() { diff --git a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportWithConditionTests.java b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ImportWithConditionTests.java new file mode 100644 index 00000000000..d72ef56088b --- /dev/null +++ b/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; + } + } + +}