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

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<DeferredImportSelectorHolder> deferredImportSelectors;
+	private final DeferredImportSelectorHandler deferredImportSelectorHandler = new DeferredImportSelectorHandler();
 
 
 	/**
@@ -162,8 +160,6 @@ class ConfigurationClassParser {
 
 
 	public void parse(Set<BeanDefinitionHolder> 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<DeferredImportSelectorHolder> deferredImports = this.deferredImportSelectors;
-		this.deferredImportSelectors = null;
-		if (deferredImports == null) {
-			return;
-		}
-
-		deferredImports.sort(DEFERRED_IMPORT_COMPARATOR);
-		Map<Object, DeferredImportSelectorGrouping> groupings = new LinkedHashMap<>();
-		Map<AnnotationMetadata, ConfigurationClass> configurationClasses = new HashMap<>();
-		for (DeferredImportSelectorHolder deferredImport : deferredImports) {
-			Class<? extends Group> 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<? extends Group> type) {
-		Class<? extends Group> 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<SourceClass> 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<DeferredImportSelectorHolder> 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<DeferredImportSelectorHolder> 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<Object, DeferredImportSelectorGrouping> groupings = new LinkedHashMap<>();
+
+		private final Map<AnnotationMetadata, ConfigurationClass> configurationClasses = new HashMap<>();
+
+		public void register(DeferredImportSelectorHolder deferredImport) {
+			Class<? extends Group> 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<? extends Group> type) {
+			Class<? extends Group> 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;

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<? extends DeferredImportSelector.Group> 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<? extends DeferredImportSelector.Group> 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<? extends DeferredImportSelector.Group> 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<? extends DeferredImportSelector.Group> 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<String, List<String>> allImports() {
+			return TestImportGroup.imports.entrySet()
+					.stream()
+					.collect(Collectors.toMap((entry) -> entry.getKey().getClassName(),
+							Map.Entry::getValue));
+		}
+		private final List<Entry> 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<Entry> selectImports() {
-			LinkedList<Entry> content = new LinkedList<>();
-			TestImportGroup.imports.forEach((metadata, values) ->
-					values.forEach(value ->  content.add(new Entry(metadata, value))));
+			LinkedList<Entry> content = new LinkedList<>(this.instanceImports);
 			Collections.reverse(content);
 			return content;
 		}