Prevent duplicate @import processing

Refactor ConfigurationClassParser to recursively find values from
all @import annotations, combining them into a single unique set.

This change prevents ImportBeanDefinitionRegistrars from being
invoked twice.

Issue: SPR-9925

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

@@ -17,14 +17,13 @@
 package org.springframework.context.annotation;
 
 import java.io.IOException;
-import java.lang.annotation.Annotation;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.Stack;
@@ -222,10 +221,9 @@ protected AnnotationMetadata doProcessConfigurationClass(
 		}
 
 		// process any @Import annotations
-		List<AnnotationAttributes> imports =
-			findAllAnnotationAttributes(Import.class, metadata.getClassName(), true);
-		for (AnnotationAttributes importAnno : imports) {
-			processImport(configClass, importAnno.getStringArray("value"), true);
+		Set<String> imports = getImports(metadata.getClassName(), null, new HashSet<String>());
+		if (imports != null && !imports.isEmpty()) {
+			processImport(configClass, imports.toArray(new String[imports.size()]), true);
 		}
 
 		// process any @ImportResource annotations
@@ -265,45 +263,36 @@ protected AnnotationMetadata doProcessConfigurationClass(
 	}
 
 	/**
-	 * Return a list of attribute maps for all declarations of the given annotation
-	 * on the given annotated class using the given MetadataReaderFactory to introspect
-	 * annotation metadata. Meta-annotations are ordered first in the list, and if the
-	 * target annotation is declared directly on the class, its map of attributes will be
-	 * ordered last in the list.
-	 * @param targetAnnotation the annotation to search for, both locally and as a meta-annotation
-	 * @param annotatedClassName the class to inspect
-	 * @param classValuesAsString whether class attributes should be returned as strings
+	 * Recursively collect all declared {@code @Import} values. Unlike most
+	 * meta-annotations it is valid to have several {@code @Import}s declared with
+	 * different values, the usual process or returning values from the first
+	 * meta-annotation on a class is not sufficient.
+	 * <p>For example, it is common for a {@code @Configuration} class to declare direct
+	 * {@code @Import}s in addition to meta-imports originating from an {@code @Enable}
+	 * annotation.
+	 * @param className the class name to search
+	 * @param imports the imports collected so far or {@code null}
+	 * @param visited used to track visited classes to prevent infinite recursion (must not be null)
+	 * @return a set of all {@link Import#value() import values} or {@code null}
+	 * @throws IOException if there is any problem reading metadata from the named class
 	 */
-	private List<AnnotationAttributes> findAllAnnotationAttributes(
-			Class<? extends Annotation> targetAnnotation, String annotatedClassName,
-			boolean classValuesAsString) throws IOException {
-
-		List<AnnotationAttributes> allAttribs = new ArrayList<AnnotationAttributes>();
-
-		MetadataReader reader = this.metadataReaderFactory.getMetadataReader(annotatedClassName);
-		AnnotationMetadata metadata = reader.getAnnotationMetadata();
-		String targetAnnotationType = targetAnnotation.getName();
-
-		for (String annotationType : metadata.getAnnotationTypes()) {
-			if (annotationType.equals(targetAnnotationType)) {
-				continue;
+	private Set<String> getImports(String className, Set<String> imports,
+			Set<String> visited) throws IOException {
+		if (visited.add(className)) {
+			AnnotationMetadata metadata = metadataReaderFactory.getMetadataReader(className).getAnnotationMetadata();
+			Map<String, Object> attributes = metadata.getAnnotationAttributes(Import.class.getName(), true);
+			if (attributes != null) {
+				String[] value = (String[]) attributes.get("value");
+				if (value != null && value.length > 0) {
+					imports = (imports == null ? new LinkedHashSet<String>() : imports);
+					imports.addAll(Arrays.asList(value));
+				}
 			}
-			AnnotationMetadata metaAnnotations =
-					this.metadataReaderFactory.getMetadataReader(annotationType).getAnnotationMetadata();
-			AnnotationAttributes targetAttribs =
-					AnnotationAttributes.fromMap(metaAnnotations.getAnnotationAttributes(targetAnnotationType, classValuesAsString));
-			if (targetAttribs != null) {
-				allAttribs.add(targetAttribs);
+			for (String annotationType : metadata.getAnnotationTypes()) {
+				getImports(annotationType, imports, visited);
 			}
 		}
-
-		AnnotationAttributes localAttribs =
-				AnnotationAttributes.fromMap(metadata.getAnnotationAttributes(targetAnnotationType, classValuesAsString));
-		if (localAttribs != null) {
-			allAttribs.add(localAttribs);
-		}
-
-		return allAttribs;
+		return imports;
 	}
 
 	private void processImport(ConfigurationClass configClass, String[] classesToImport, boolean checkForCircularImports) throws IOException {
@@ -440,5 +429,4 @@ public CircularImportProblem(ConfigurationClass attemptedImport, Stack<Configura
 					new Location(importStack.peek().getResource(), metadata));
 		}
 	}
-
 }

spring-context/src/test/java/org/springframework/context/annotation/ImportAwareTests.java

@@ -25,10 +25,14 @@
 import org.springframework.beans.BeansException;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
+import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanPostProcessor;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.beans.factory.support.GenericBeanDefinition;
 import org.springframework.core.annotation.AnnotationAttributes;
 import org.springframework.core.type.AnnotationMetadata;
 import org.springframework.scheduling.annotation.AsyncAnnotationBeanPostProcessor;
+import org.springframework.util.Assert;
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
@@ -77,6 +81,24 @@ public void indirectlyAnnotatedWithImport() {
 		assertThat(foo, is("xyz"));
 	}
 
+	@Test
+	public void importRegistrar() throws Exception {
+		ImportedRegistrar.called = false;
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ImportingRegistrarConfig.class);
+		ctx.refresh();
+		assertNotNull(ctx.getBean("registrarImportedBean"));
+	}
+
+	@Test
+	public void importRegistrarWithImport() throws Exception {
+		ImportedRegistrar.called = false;
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(ImportingRegistrarConfigWithImport.class);
+		ctx.refresh();
+		assertNotNull(ctx.getBean("registrarImportedBean"));
+		assertNotNull(ctx.getBean(ImportedConfig.class));
+	}
 
 	@Configuration
 	@Import(ImportedConfig.class)
@@ -131,4 +153,35 @@ public Object postProcessAfterInitialization(Object bean, String beanName) throw
 		public void setBeanFactory(BeanFactory beanFactory) throws BeansException {
 		}
 	}
+
+	@Configuration
+	@EnableImportRegistrar
+	static class ImportingRegistrarConfig {
+	}
+
+	@Configuration
+	@EnableImportRegistrar
+	@Import(ImportedConfig.class)
+	static class ImportingRegistrarConfigWithImport {
+	}
+
+	@Target(ElementType.TYPE)
+	@Retention(RetentionPolicy.RUNTIME)
+	@Import(ImportedRegistrar.class)
+	public @interface EnableImportRegistrar {
+	}
+
+	static class ImportedRegistrar implements ImportBeanDefinitionRegistrar {
+
+		static boolean called;
+
+		public void registerBeanDefinitions(AnnotationMetadata importingClassMetadata,
+				BeanDefinitionRegistry registry) {
+			BeanDefinition beanDefinition = new GenericBeanDefinition();
+			beanDefinition.setBeanClassName(String.class.getName());
+			registry.registerBeanDefinition("registrarImportedBean", beanDefinition );
+			Assert.state(called == false, "ImportedRegistrar called twice");
+			called = true;
+		}
+	}
 }