RESOLVED - issue SPR-6779: imported @Configuration classes do not get enhanced and fail to satisfy scoping requirements

refactoring, polishing.

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2010 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.
@@ -16,6 +16,7 @@
 
 package org.springframework.context.annotation;
 
+import java.io.IOException;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -26,22 +27,31 @@ import java.util.Set;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
-
 import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition;
 import org.springframework.beans.factory.annotation.Autowire;
 import org.springframework.beans.factory.annotation.RequiredAnnotationBeanPostProcessor;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanDefinitionHolder;
+import org.springframework.beans.factory.parsing.Location;
+import org.springframework.beans.factory.parsing.Problem;
+import org.springframework.beans.factory.parsing.ProblemReporter;
 import org.springframework.beans.factory.parsing.SourceExtractor;
+import org.springframework.beans.factory.support.AbstractBeanDefinition;
 import org.springframework.beans.factory.support.BeanDefinitionReader;
 import org.springframework.beans.factory.support.BeanDefinitionReaderUtils;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.beans.factory.support.GenericBeanDefinition;
 import org.springframework.beans.factory.support.RootBeanDefinition;
+import org.springframework.core.Conventions;
 import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.core.io.Resource;
 import org.springframework.core.type.AnnotationMetadata;
 import org.springframework.core.type.MethodMetadata;
+import org.springframework.core.type.StandardAnnotationMetadata;
+import org.springframework.core.type.classreading.CachingMetadataReaderFactory;
+import org.springframework.core.type.classreading.MetadataReader;
+import org.springframework.core.type.classreading.MetadataReaderFactory;
+import org.springframework.stereotype.Component;
 import org.springframework.util.StringUtils;
 
 /**
@@ -58,20 +68,37 @@ import org.springframework.util.StringUtils;
  */
 class ConfigurationClassBeanDefinitionReader {
 
-	private static final Log logger = LogFactory.getLog(ConfigurationClassBeanDefinitionReader.class);
+	static final String CONFIGURATION_CLASS_FULL = "full";
+
+	static final String CONFIGURATION_CLASS_LITE = "lite";
 
+	static final String CONFIGURATION_CLASS_ATTRIBUTE =
+		Conventions.getQualifiedAttributeName(ConfigurationClassPostProcessor.class, "configurationClass");
+
+	private static final Log logger = LogFactory.getLog(ConfigurationClassBeanDefinitionReader.class);
+	
 	private final BeanDefinitionRegistry registry;
 
 	private final SourceExtractor sourceExtractor;
 
+	private final ProblemReporter problemReporter;
+
+	private final MetadataReaderFactory metadataReaderFactory;
+
+
 
 	/**
 	 * Create a new {@link ConfigurationClassBeanDefinitionReader} instance that will be used
 	 * to populate the given {@link BeanDefinitionRegistry}.
+	 * @param problemReporter 
+	 * @param metadataReaderFactory 
 	 */
-	public ConfigurationClassBeanDefinitionReader(BeanDefinitionRegistry registry, SourceExtractor sourceExtractor) {
+	public ConfigurationClassBeanDefinitionReader(BeanDefinitionRegistry registry, SourceExtractor sourceExtractor,
+			ProblemReporter problemReporter, MetadataReaderFactory metadataReaderFactory) {
 		this.registry = registry;
 		this.sourceExtractor = sourceExtractor;
+		this.problemReporter = problemReporter;
+		this.metadataReaderFactory = metadataReaderFactory;
 	}
 
 
@@ -90,7 +117,7 @@ class ConfigurationClassBeanDefinitionReader {
 	 * class itself, all its {@link Bean} methods
 	 */
 	private void loadBeanDefinitionsForConfigurationClass(ConfigurationClass configClass) {
-		doLoadBeanDefinitionForConfigurationClass(configClass);
+		doLoadBeanDefinitionForConfigurationClassIfNecessary(configClass);
 		
 		for (ConfigurationClassMethod method : configClass.getMethods()) {
 			loadBeanDefinitionsForModelMethod(method);
@@ -102,16 +129,31 @@ class ConfigurationClassBeanDefinitionReader {
 	/**
 	 * Registers the {@link Configuration} class itself as a bean definition.
 	 */
-	private void doLoadBeanDefinitionForConfigurationClass(ConfigurationClass configClass) {
-		if (configClass.getBeanName() == null) {
-			GenericBeanDefinition configBeanDef = new GenericBeanDefinition();
-			configBeanDef.setBeanClassName(configClass.getMetadata().getClassName());
-			new ConfigurationClassPostProcessor().checkConfigurationClassCandidate(configBeanDef);
+	private void doLoadBeanDefinitionForConfigurationClassIfNecessary(ConfigurationClass configClass) {
+		if (configClass.getBeanName() != null) {
+			// a bean definition already exists for this configuration class -> nothing to do
+			return;
+		}
+
+		// no bean definition exists yet -> this must be an imported configuration class (@Import).
+		GenericBeanDefinition configBeanDef = new GenericBeanDefinition();
+		String className = configClass.getMetadata().getClassName();
+		configBeanDef.setBeanClassName(className);
+		if(checkConfigurationClassCandidate(configBeanDef, this.metadataReaderFactory)) {
 			String configBeanName = BeanDefinitionReaderUtils.registerWithGeneratedName(configBeanDef, this.registry);
 			configClass.setBeanName(configBeanName);
 			if (logger.isDebugEnabled()) {
 				logger.debug(String.format("Registered bean definition for imported @Configuration class %s", configBeanName));
 			}
+		} else {
+			try {
+				MetadataReader reader = this.metadataReaderFactory.getMetadataReader(className);
+				AnnotationMetadata metadata = reader.getAnnotationMetadata();
+				this.problemReporter.error(
+						new InvalidConfigurationImportProblem(className, reader.getResource(), metadata));
+			} catch (IOException ex) {
+				throw new IllegalStateException("Could not create MetadataReader for class " + className);
+			}
 		}
 	}
 
@@ -235,6 +277,51 @@ class ConfigurationClassBeanDefinitionReader {
 		}
 	}
 
+	/**
+	 * Check whether the given bean definition is a candidate for a configuration class,
+	 * and mark it accordingly.
+	 * @param beanDef the bean definition to check
+	 * @param metadataReaderFactory the current factory in use by the caller
+	 * @return whether the candidate qualifies as (any kind of) configuration class
+	 */
+	static boolean checkConfigurationClassCandidate(BeanDefinition beanDef, MetadataReaderFactory metadataReaderFactory) {
+		AnnotationMetadata metadata = null;
+	
+		// Check already loaded Class if present...
+		// since we possibly can't even load the class file for this Class.
+		if (beanDef instanceof AbstractBeanDefinition && ((AbstractBeanDefinition) beanDef).hasBeanClass()) {
+			metadata = new StandardAnnotationMetadata(((AbstractBeanDefinition) beanDef).getBeanClass());
+		}
+		else {
+			String className = beanDef.getBeanClassName();
+			if (className != null) {
+				try {
+					MetadataReader metadataReader = metadataReaderFactory.getMetadataReader(className);
+					metadata = metadataReader.getAnnotationMetadata();
+				}
+				catch (IOException ex) {
+					if (logger.isDebugEnabled()) {
+						logger.debug("Could not find class file for introspecting factory methods: " + className, ex);
+					}
+					return false;
+				}
+			}
+		}
+	
+		if (metadata != null) {
+			if (metadata.isAnnotated(Configuration.class.getName())) {
+				beanDef.setAttribute(CONFIGURATION_CLASS_ATTRIBUTE, CONFIGURATION_CLASS_FULL);
+				return true;
+			}
+			else if (metadata.isAnnotated(Component.class.getName()) ||
+					metadata.hasAnnotatedMethods(Bean.class.getName())) {
+				beanDef.setAttribute(CONFIGURATION_CLASS_ATTRIBUTE, CONFIGURATION_CLASS_LITE);
+				return true;
+			}
+		}
+		return false;
+	}
+
 	/**
 	 * {@link RootBeanDefinition} marker subclass used to signify that a bean definition
 	 * created from a configuration class as opposed to any other configuration source.
@@ -270,4 +357,20 @@ class ConfigurationClassBeanDefinitionReader {
 		}
 	}
 
+	
+	/**
+	 * Configuration classes must be annotated with {@link Configuration @Configuration} or
+	 * declare at least one {@link Bean @Bean} method.
+	 */
+	private static class InvalidConfigurationImportProblem extends Problem {
+	
+		public InvalidConfigurationImportProblem(String className, Resource resource, AnnotationMetadata metadata) {
+			super(String.format("%s was imported as a Configuration class but is not annotated " +
+					"with @Configuration nor does it declare any @Bean methods. Update the class to " +
+					"meet either of these requirements or do not attempt to import it.", className),
+					new Location(resource, metadata));
+		}
+	
+	}
+
 }

org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassEnhancer.java

@@ -86,9 +86,6 @@ class ConfigurationClassEnhancer {
 	 * @return fully-qualified name of the enhanced subclass
 	 */
 	public Class<?> enhance(Class<?> configClass) {
-		if (logger.isDebugEnabled()) {
-			logger.debug("Enhancing " + configClass.getName());
-		}
 		Class<?> enhancedClass = createClass(newEnhancer(configClass));
 		if (logger.isDebugEnabled()) {
 			logger.debug(String.format("Successfully enhanced %s; enhanced class name is: %s",

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2010 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.
@@ -24,11 +24,9 @@ import java.util.LinkedHashSet;
 import java.util.Set;
 import java.util.Stack;
 
-import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.parsing.Location;
 import org.springframework.beans.factory.parsing.Problem;
 import org.springframework.beans.factory.parsing.ProblemReporter;
-import org.springframework.core.io.Resource;
 import org.springframework.core.type.AnnotationMetadata;
 import org.springframework.core.type.MethodMetadata;
 import org.springframework.core.type.StandardAnnotationMetadata;
@@ -43,7 +41,7 @@ import org.springframework.util.StringUtils;
  * another using the {@link Import} annotation).
  *
  * <p>This class helps separate the concern of parsing the structure of a Configuration
- * class from the concern of registering {@link BeanDefinition} objects based on the
+ * class from the concern of registering BeanDefinition objects based on the
  * content of that model.
  *
  * <p>This ASM-based implementation avoids reflection and eager class loading in order to
@@ -153,24 +151,13 @@ class ConfigurationClassParser {
 		else {
 			this.importStack.push(configClass);
 			for (String classToImport : classesToImport) {
-				processClassToImport(classToImport);
+				MetadataReader reader = this.metadataReaderFactory.getMetadataReader(classToImport);
+				processConfigurationClass(new ConfigurationClass(reader, null));
 			}
 			this.importStack.pop();
 		}
 	}
 
-	private void processClassToImport(String classToImport) throws IOException {
-		MetadataReader reader = this.metadataReaderFactory.getMetadataReader(classToImport);
-		AnnotationMetadata metadata = reader.getAnnotationMetadata();
-		if (!metadata.isAnnotated(Configuration.class.getName())) {
-			this.problemReporter.error(
-					new NonAnnotatedConfigurationProblem(metadata.getClassName(), reader.getResource(), metadata));
-		}
-		else {
-			processConfigurationClass(new ConfigurationClass(reader, null));
-		}
-	}
-
 	/**
 	 * Validate each {@link ConfigurationClass} object.
 	 * @see ConfigurationClass#validate
@@ -229,20 +216,6 @@ class ConfigurationClassParser {
 	}
 
 
-	/**
-	 * Configuration classes must be annotated with {@link Configuration @Configuration}.
-	 */
-	private static class NonAnnotatedConfigurationProblem extends Problem {
-
-		public NonAnnotatedConfigurationProblem(String className, Resource resource, AnnotationMetadata metadata) {
-			super(String.format("%s was imported as a @Configuration class but was not actually annotated " +
-					"with @Configuration. Annotate the class or do not attempt to process it.", className),
-					new Location(resource, metadata));
-		}
-
-	}
-
-
 	/**
 	 * {@link Problem} registered upon detection of a circular {@link Import}.
 	 */

org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassPostProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2009 the original author or authors.
+ * Copyright 2002-2010 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.
@@ -16,6 +16,9 @@
 
 package org.springframework.context.annotation;
 
+import static org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.CONFIGURATION_CLASS_ATTRIBUTE;
+import static org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.CONFIGURATION_CLASS_FULL;
+
 import java.io.IOException;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
@@ -37,14 +40,9 @@ import org.springframework.beans.factory.parsing.ProblemReporter;
 import org.springframework.beans.factory.parsing.SourceExtractor;
 import org.springframework.beans.factory.support.AbstractBeanDefinition;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
-import org.springframework.core.Conventions;
 import org.springframework.core.Ordered;
-import org.springframework.core.type.AnnotationMetadata;
-import org.springframework.core.type.StandardAnnotationMetadata;
 import org.springframework.core.type.classreading.CachingMetadataReaderFactory;
-import org.springframework.core.type.classreading.MetadataReader;
 import org.springframework.core.type.classreading.MetadataReaderFactory;
-import org.springframework.stereotype.Component;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
 
@@ -67,14 +65,6 @@ import org.springframework.util.ClassUtils;
  */
 public class ConfigurationClassPostProcessor implements BeanFactoryPostProcessor, BeanClassLoaderAware {
 
-	private static final String CONFIGURATION_CLASS_ATTRIBUTE =
-			Conventions.getQualifiedAttributeName(ConfigurationClassPostProcessor.class, "configurationClass");
-
-	private static final String CONFIGURATION_CLASS_FULL = "full";
-
-	private static final String CONFIGURATION_CLASS_LITE = "lite";
-
-
 	/** Whether the CGLIB2 library is present on the classpath */
 	private static final boolean cglibAvailable = ClassUtils.isPresent(
 			"net.sf.cglib.proxy.Enhancer", ConfigurationClassPostProcessor.class.getClassLoader());
@@ -155,7 +145,7 @@ public class ConfigurationClassPostProcessor implements BeanFactoryPostProcessor
 		Set<BeanDefinitionHolder> configCandidates = new LinkedHashSet<BeanDefinitionHolder>();
 		for (String beanName : registry.getBeanDefinitionNames()) {
 			BeanDefinition beanDef = registry.getBeanDefinition(beanName);
-			if (checkConfigurationClassCandidate(beanDef)) {
+			if (ConfigurationClassBeanDefinitionReader.checkConfigurationClassCandidate(beanDef, this.metadataReaderFactory)) {
 				configCandidates.add(new BeanDefinitionHolder(beanDef, beanName));
 			}
 		}
@@ -184,51 +174,9 @@ public class ConfigurationClassPostProcessor implements BeanFactoryPostProcessor
 		parser.validate();
 
 		// Read the model and create bean definitions based on its content
-		new ConfigurationClassBeanDefinitionReader(registry, this.sourceExtractor).loadBeanDefinitions(parser.getConfigurationClasses());
-	}
-
-	/**
-	 * Check whether the given bean definition is a candidate for a configuration class,
-	 * and mark it accordingly.
-	 * @param beanDef the bean definition to check
-	 * @return whether the candidate qualifies as (any kind of) configuration class
-	 */
-	protected boolean checkConfigurationClassCandidate(BeanDefinition beanDef) {
-		AnnotationMetadata metadata = null;
-
-		// Check already loaded Class if present...
-		// since we possibly can't even load the class file for this Class.
-		if (beanDef instanceof AbstractBeanDefinition && ((AbstractBeanDefinition) beanDef).hasBeanClass()) {
-			metadata = new StandardAnnotationMetadata(((AbstractBeanDefinition) beanDef).getBeanClass());
-		}
-		else {
-			String className = beanDef.getBeanClassName();
-			if (className != null) {
-				try {
-					MetadataReader metadataReader = this.metadataReaderFactory.getMetadataReader(className);
-					metadata = metadataReader.getAnnotationMetadata();
-				}
-				catch (IOException ex) {
-					if (logger.isDebugEnabled()) {
-						logger.debug("Could not find class file for introspecting factory methods: " + className, ex);
-					}
-					return false;
-				}
-			}
-		}
-
-		if (metadata != null) {
-			if (metadata.isAnnotated(Configuration.class.getName())) {
-				beanDef.setAttribute(CONFIGURATION_CLASS_ATTRIBUTE, CONFIGURATION_CLASS_FULL);
-				return true;
-			}
-			else if (metadata.isAnnotated(Component.class.getName()) ||
-					metadata.hasAnnotatedMethods(Bean.class.getName())) {
-				beanDef.setAttribute(CONFIGURATION_CLASS_ATTRIBUTE, CONFIGURATION_CLASS_LITE);
-				return true;
-			}
-		}
-		return false;
+		ConfigurationClassBeanDefinitionReader reader =
+			new ConfigurationClassBeanDefinitionReader(registry, this.sourceExtractor, this.problemReporter, this.metadataReaderFactory);
+		reader.loadBeanDefinitions(parser.getConfigurationClasses());
 	}
 
 	/**
@@ -262,8 +210,8 @@ public class ConfigurationClassPostProcessor implements BeanFactoryPostProcessor
 		for (Map.Entry<String, AbstractBeanDefinition> entry : configBeanDefs.entrySet()) {
 			AbstractBeanDefinition beanDef = entry.getValue();
 			try {
-				Class configClass = beanDef.resolveBeanClass(this.beanClassLoader);
-				Class enhancedClass = enhancer.enhance(configClass);
+				Class<?> configClass = beanDef.resolveBeanClass(this.beanClassLoader);
+				Class<?> enhancedClass = enhancer.enhance(configClass);
 				if (logger.isDebugEnabled()) {
 					logger.debug(String.format("Replacing bean definition '%s' existing class name '%s' " +
 							"with enhanced class name '%s'", entry.getKey(), configClass.getName(), enhancedClass.getName()));

org.springframework.context/src/test/java/org/springframework/context/annotation/configuration/ImportedConfigurationClassEnhancementTests.java

@@ -1,3 +1,19 @@
+/*
+ * Copyright 2002-2010 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 static org.hamcrest.CoreMatchers.sameInstance;
@@ -7,6 +23,7 @@ import static org.junit.Assert.assertTrue;
 import org.junit.Test;
 import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.parsing.BeanDefinitionParsingException;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
@@ -59,7 +76,12 @@ public class ImportedConfigurationClassEnhancementTests {
 		assertThat("got two distinct instances of testBean when singleton scoping was expected",
 				testBean1, sameInstance(testBean2));
 	}
-	
+
+
+	@Test(expected=BeanDefinitionParsingException.class)
+	public void importingAnNonConfigurationClassCausesIllegalArgumentException() {
+		new AnnotationConfigApplicationContext(ConfigThatImportsNonConfigClass.class);
+	}
 }
 
 @Configuration
@@ -79,3 +101,7 @@ class ConfigThatDoesImport extends Config { }
 
 @Configuration
 class ConfigThatDoesNotImport extends Config { }
+
+@Configuration
+@Import(Void.class)
+class ConfigThatImportsNonConfigClass { }

org.springframework.context/src/test/resources/log4j.xml

@@ -11,9 +11,9 @@
 		</layout>
 	</appender>
 
-	<logger name="org.springframework.context">
-		<level value="debug" />
-	</logger>
+<!--	<logger name="org.springframework.mapping">-->
+<!--		<level value="debug" />-->
+<!--	</logger>-->
 
 	<!-- Root Logger -->
 	<root>