From 062805fcb3012d5bdad6c93c6b9e33b62ffc4312 Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 9 Oct 2018 10:55:18 +0200 Subject: [PATCH] Recursively process DeferredImportSelector properly Previously, if a DeferredImportSelector was identified at a later stage as part of processing the collected set of deferred imports, such selector was processed as a regular import selector. Semantically, it makes sense as we already are in the deferred phase at this point and it doesn't make much sense to bother about the extra contract. However, Spring Framework 5 introduced the notion of Group that a deferred import selector can define. When it does, the container has to honour the contract of the Group rather than calling the simpler ImportSelector parent contract. This commit restructures the processing of such case. When a deferred import selector is detected while processing deferred import selectors, a group is created with only that selector and the group API is invoked. Issue: SPR-17351 --- .../annotation/ConfigurationClassParser.java | 157 +++++++++++------ .../annotation/ImportSelectorTests.java | 160 +++++++++++++++++- 2 files changed, 258 insertions(+), 59 deletions(-) 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 5158867c46f..5547f729947 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 @@ -30,7 +30,6 @@ import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -138,8 +137,7 @@ class ConfigurationClassParser { private final ImportStack importStack = new ImportStack(); - @Nullable - private List deferredImportSelectors; + private final DeferredImportSelectorHandler deferredImportSelectorHandler = new DeferredImportSelectorHandler(); /** @@ -162,8 +160,6 @@ class ConfigurationClassParser { public void parse(Set configCandidates) { - this.deferredImportSelectors = new LinkedList<>(); - for (BeanDefinitionHolder holder : configCandidates) { BeanDefinition bd = holder.getBeanDefinition(); try { @@ -186,7 +182,7 @@ class ConfigurationClassParser { } } - processDeferredImportSelectors(); + this.deferredImportSelectorHandler.process(); } protected final void parse(@Nullable String className, String beanName) throws IOException { @@ -543,53 +539,7 @@ class ConfigurationClassParser { } } - private void processDeferredImportSelectors() { - List deferredImports = this.deferredImportSelectors; - this.deferredImportSelectors = null; - if (deferredImports == null) { - return; - } - - deferredImports.sort(DEFERRED_IMPORT_COMPARATOR); - Map groupings = new LinkedHashMap<>(); - Map configurationClasses = new HashMap<>(); - for (DeferredImportSelectorHolder deferredImport : deferredImports) { - Class group = deferredImport.getImportSelector().getImportGroup(); - DeferredImportSelectorGrouping grouping = groupings.computeIfAbsent( - (group != null ? group : deferredImport), - key -> new DeferredImportSelectorGrouping(createGroup(group))); - grouping.add(deferredImport); - configurationClasses.put(deferredImport.getConfigurationClass().getMetadata(), - deferredImport.getConfigurationClass()); - } - for (DeferredImportSelectorGrouping grouping : groupings.values()) { - grouping.getImports().forEach(entry -> { - ConfigurationClass configurationClass = configurationClasses.get(entry.getMetadata()); - try { - processImports(configurationClass, asSourceClass(configurationClass), - asSourceClasses(entry.getImportClassName()), false); - } - catch (BeanDefinitionStoreException ex) { - throw ex; - } - catch (Throwable ex) { - throw new BeanDefinitionStoreException( - "Failed to process import candidates for configuration class [" + - configurationClass.getMetadata().getClassName() + "]", ex); - } - }); - } - } - private Group createGroup(@Nullable Class type) { - Class effectiveType = (type != null ? type : DefaultDeferredImportSelectorGroup.class); - Group group = BeanUtils.instantiateClass(effectiveType); - ParserStrategyUtils.invokeAwareMethods(group, - ConfigurationClassParser.this.environment, - ConfigurationClassParser.this.resourceLoader, - ConfigurationClassParser.this.registry); - return group; - } private void processImports(ConfigurationClass configClass, SourceClass currentSourceClass, Collection importCandidates, boolean checkForCircularImports) { @@ -611,9 +561,9 @@ class ConfigurationClassParser { ImportSelector selector = BeanUtils.instantiateClass(candidateClass, ImportSelector.class); ParserStrategyUtils.invokeAwareMethods( selector, this.environment, this.resourceLoader, this.registry); - if (this.deferredImportSelectors != null && selector instanceof DeferredImportSelector) { - this.deferredImportSelectors.add( - new DeferredImportSelectorHolder(configClass, (DeferredImportSelector) selector)); + if (selector instanceof DeferredImportSelector) { + this.deferredImportSelectorHandler.handle( + configClass, (DeferredImportSelector) selector); } else { String[] importClassNames = selector.selectImports(currentSourceClass.getMetadata()); @@ -787,6 +737,103 @@ class ConfigurationClassParser { } + private class DeferredImportSelectorHandler { + + @Nullable + private List deferredImportSelectors = new ArrayList<>(); + + /** + * Handle the specified {@link DeferredImportSelector}. If deferred import + * selectors are being collected, this registers this instance to the list. If + * they are being processed, the {@link DeferredImportSelector} is also processed + * immediately according to its {@link DeferredImportSelector.Group}. + * @param configClass the source configuration class + * @param importSelector the selector to handle + */ + public void handle(ConfigurationClass configClass, DeferredImportSelector importSelector) { + DeferredImportSelectorHolder holder = new DeferredImportSelectorHolder( + configClass, importSelector); + if (this.deferredImportSelectors == null) { + DeferredImportSelectorGroupingHandler handler = new DeferredImportSelectorGroupingHandler(); + handler.register(holder); + handler.processGroupImports(); + } + else { + this.deferredImportSelectors.add(holder); + } + } + + public void process() { + List deferredImports = this.deferredImportSelectors; + this.deferredImportSelectors = null; + try { + if (deferredImports != null) { + DeferredImportSelectorGroupingHandler handler = new DeferredImportSelectorGroupingHandler(); + deferredImports.sort(DEFERRED_IMPORT_COMPARATOR); + deferredImports.forEach(handler::register); + handler.processGroupImports(); + } + } + finally { + this.deferredImportSelectors = new ArrayList<>(); + } + } + + } + + + private class DeferredImportSelectorGroupingHandler { + + private final Map groupings = new LinkedHashMap<>(); + + private final Map configurationClasses = new HashMap<>(); + + public void register(DeferredImportSelectorHolder deferredImport) { + Class group = deferredImport.getImportSelector() + .getImportGroup(); + DeferredImportSelectorGrouping grouping = this.groupings.computeIfAbsent( + (group != null ? group : deferredImport), + key -> new DeferredImportSelectorGrouping(createGroup(group))); + grouping.add(deferredImport); + this.configurationClasses.put(deferredImport.getConfigurationClass().getMetadata(), + deferredImport.getConfigurationClass()); + } + + public void processGroupImports() { + for (DeferredImportSelectorGrouping grouping : this.groupings.values()) { + grouping.getImports().forEach(entry -> { + ConfigurationClass configurationClass = this.configurationClasses.get( + entry.getMetadata()); + try { + processImports(configurationClass, asSourceClass(configurationClass), + asSourceClasses(entry.getImportClassName()), false); + } + catch (BeanDefinitionStoreException ex) { + throw ex; + } + catch (Throwable ex) { + throw new BeanDefinitionStoreException( + "Failed to process import candidates for configuration class [" + + configurationClass.getMetadata().getClassName() + "]", ex); + } + }); + } + } + + private Group createGroup(@Nullable Class type) { + Class effectiveType = (type != null ? type + : DefaultDeferredImportSelectorGroup.class); + Group group = BeanUtils.instantiateClass(effectiveType); + ParserStrategyUtils.invokeAwareMethods(group, + ConfigurationClassParser.this.environment, + ConfigurationClassParser.this.resourceLoader, + ConfigurationClassParser.this.registry); + return group; + } + + } + + private static class DeferredImportSelectorHolder { private final ConfigurationClass configurationClass; diff --git a/spring-context/src/test/java/org/springframework/context/annotation/ImportSelectorTests.java b/spring-context/src/test/java/org/springframework/context/annotation/ImportSelectorTests.java index 45abc4afbac..f05116ca663 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/ImportSelectorTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/ImportSelectorTests.java @@ -20,15 +20,19 @@ import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedList; +import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import org.hamcrest.Matcher; +import org.hamcrest.collection.IsIterableContainingInOrder; import org.junit.Before; import org.junit.Test; import org.mockito.InOrder; @@ -49,7 +53,7 @@ import org.springframework.lang.Nullable; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.mockito.Mockito.any; import static org.mockito.Mockito.*; @@ -140,6 +144,47 @@ public class ImportSelectorTests { assertThat(iterator.next().getClassName(), equalTo(GroupedConfig1.class.getName())); } + @Test + public void importSelectorsWithNestedGroup() { + DefaultListableBeanFactory beanFactory = spy(new DefaultListableBeanFactory()); + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(beanFactory); + context.register(ParentConfiguration1.class); + context.refresh(); + InOrder ordered = inOrder(beanFactory); + ordered.verify(beanFactory).registerBeanDefinition(eq("a"), any()); + ordered.verify(beanFactory).registerBeanDefinition(eq("e"), any()); + ordered.verify(beanFactory).registerBeanDefinition(eq("c"), any()); + assertThat(TestImportGroup.instancesCount.get(), equalTo(2)); + assertThat(TestImportGroup.imports.size(), equalTo(2)); + assertThat(TestImportGroup.allImports(), hasEntry( + is(ParentConfiguration1.class.getName()), + IsIterableContainingInOrder.contains(DeferredImportSelector1.class.getName(), + ChildConfiguration1.class.getName()))); + assertThat(TestImportGroup.allImports(), hasEntry( + is(ChildConfiguration1.class.getName()), + IsIterableContainingInOrder.contains(DeferredImportedSelector3.class.getName()))); + } + + @Test + public void importSelectorsWithNestedGroupSameDeferredImport() { + DefaultListableBeanFactory beanFactory = spy(new DefaultListableBeanFactory()); + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(beanFactory); + context.register(ParentConfiguration2.class); + context.refresh(); + InOrder ordered = inOrder(beanFactory); + ordered.verify(beanFactory).registerBeanDefinition(eq("b"), any()); + ordered.verify(beanFactory).registerBeanDefinition(eq("d"), any()); + assertThat(TestImportGroup.instancesCount.get(), equalTo(2)); + assertThat(TestImportGroup.allImports().size(), equalTo(2)); + assertThat(TestImportGroup.allImports(), hasEntry( + is(ParentConfiguration2.class.getName()), + IsIterableContainingInOrder.contains(DeferredImportSelector2.class.getName(), + ChildConfiguration2.class.getName()))); + assertThat(TestImportGroup.allImports(), hasEntry( + is(ChildConfiguration2.class.getName()), + IsIterableContainingInOrder.contains(DeferredImportSelector2.class.getName()))); + } + @Test public void invokeAwareMethodsInImportGroup() { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(GroupedConfig1.class); @@ -297,6 +342,15 @@ public class ImportSelectorTests { } } + @Configuration + public static class DeferredImportedSelector3 { + + @Bean + public String e() { + return "e"; + } + } + @Configuration @Import(IndirectImportSelector.class) @@ -360,6 +414,95 @@ public class ImportSelectorTests { } + @Configuration + @Import({ImportSelector1.class, ParentDeferredImportSelector1.class}) + public static class ParentConfiguration1 { + } + + + public static class ParentDeferredImportSelector1 implements DeferredImportSelector { + + @Override + public String[] selectImports(AnnotationMetadata importingClassMetadata) { + ImportSelectorTests.importFrom.put(getClass(), importingClassMetadata.getClassName()); + return new String[] { DeferredImportSelector1.class.getName(), ChildConfiguration1.class.getName() }; + } + + @Nullable + @Override + public Class getImportGroup() { + return TestImportGroup.class; + } + + } + + @Configuration + @Import({ImportSelector2.class, ParentDeferredImportSelector2.class}) + public static class ParentConfiguration2 { + } + + public static class ParentDeferredImportSelector2 implements DeferredImportSelector { + + @Override + public String[] selectImports(AnnotationMetadata importingClassMetadata) { + ImportSelectorTests.importFrom.put(getClass(), importingClassMetadata.getClassName()); + return new String[] { DeferredImportSelector2.class.getName(), ChildConfiguration2.class.getName() }; + } + + @Nullable + @Override + public Class getImportGroup() { + return TestImportGroup.class; + } + + } + + @Configuration + @Import(ChildDeferredImportSelector1.class) + public static class ChildConfiguration1 { + + } + + + public static class ChildDeferredImportSelector1 implements DeferredImportSelector { + + @Override + public String[] selectImports(AnnotationMetadata importingClassMetadata) { + ImportSelectorTests.importFrom.put(getClass(), importingClassMetadata.getClassName()); + return new String[] { DeferredImportedSelector3.class.getName() }; + } + + @Nullable + @Override + public Class getImportGroup() { + return TestImportGroup.class; + } + + } + + @Configuration + @Import(ChildDeferredImportSelector2.class) + public static class ChildConfiguration2 { + + } + + public static class ChildDeferredImportSelector2 implements DeferredImportSelector { + + @Override + public String[] selectImports(AnnotationMetadata importingClassMetadata) { + ImportSelectorTests.importFrom.put(getClass(), importingClassMetadata.getClassName()); + return new String[] { DeferredImportSelector2.class.getName() }; + } + + @Nullable + @Override + public Class getImportGroup() { + return TestImportGroup.class; + } + + } + + public static class TestImportGroup implements DeferredImportSelector.Group, BeanClassLoaderAware, ResourceLoaderAware, BeanFactoryAware, EnvironmentAware { @@ -384,17 +527,26 @@ public class ImportSelectorTests { TestImportGroup.imports.clear(); } + static Map> allImports() { + return TestImportGroup.imports.entrySet() + .stream() + .collect(Collectors.toMap((entry) -> entry.getKey().getClassName(), + Map.Entry::getValue)); + } + private final List instanceImports = new ArrayList<>(); + @Override public void process(AnnotationMetadata metadata, DeferredImportSelector selector) { + for (String importClassName : selector.selectImports(metadata)) { + this.instanceImports.add(new Entry(metadata, importClassName)); + } TestImportGroup.imports.addAll(metadata, Arrays.asList(selector.selectImports(metadata))); } @Override public Iterable selectImports() { - LinkedList content = new LinkedList<>(); - TestImportGroup.imports.forEach((metadata, values) -> - values.forEach(value -> content.add(new Entry(metadata, value)))); + LinkedList content = new LinkedList<>(this.instanceImports); Collections.reverse(content); return content; }