@Bean methods are allowed to override existing bean definitions with a role other than ROLE_APPLICATION now (e.g. framework-generated default beans)

Also, DefaultListableBeanFactory logs a warning when overriding an application definition with a framework-generated definition now, which is expected to be an accident.

Issue: SPR-10607

spring-beans/src/main/java/org/springframework/beans/factory/config/BeanDefinition.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2013 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.
@@ -229,11 +229,11 @@ public interface BeanDefinition extends AttributeAccessor, BeanMetadataElement {
 
 	/**
 	 * Get the role hint for this {@code BeanDefinition}. The role hint
-	 * provides tools with an indication of the importance of a particular
+	 * provides the frameworks as well as tools with an indication of
-	 * {@code BeanDefinition}.
+	 * the role and importance of a particular {@code BeanDefinition}.
 	 * @see #ROLE_APPLICATION
-	 * @see #ROLE_INFRASTRUCTURE
 	 * @see #ROLE_SUPPORT
+	 * @see #ROLE_INFRASTRUCTURE
 	 */
 	int getRole();
 

spring-beans/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java

@@ -691,13 +691,21 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto
 		}
 
 		synchronized (this.beanDefinitionMap) {
-			Object oldBeanDefinition = this.beanDefinitionMap.get(beanName);
+			BeanDefinition oldBeanDefinition = this.beanDefinitionMap.get(beanName);
 			if (oldBeanDefinition != null) {
 				if (!this.allowBeanDefinitionOverriding) {
 					throw new BeanDefinitionStoreException(beanDefinition.getResourceDescription(), beanName,
 							"Cannot register bean definition [" + beanDefinition + "] for bean '" + beanName +
 							"': There is already [" + oldBeanDefinition + "] bound.");
 				}
+				else if (oldBeanDefinition.getRole() < beanDefinition.getRole()) {
+					// e.g. was ROLE_APPLICATION, now overriding with ROLE_SUPPORT or ROLE_INFRASTRUCTURE
+					if (this.logger.isWarnEnabled()) {
+						this.logger.warn("Overriding user-defined bean definition for bean '" + beanName +
+								" with a framework-generated bean definition ': replacing [" +
+								oldBeanDefinition + "] with [" + beanDefinition + "]");
+					}
+				}
 				else {
 					if (this.logger.isInfoEnabled()) {
 						this.logger.info("Overriding bean definition for bean '" + beanName +

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

@@ -27,7 +27,6 @@ import java.util.Set;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import org.springframework.beans.factory.NoSuchBeanDefinitionException;
 import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition;
 import org.springframework.beans.factory.annotation.AnnotatedGenericBeanDefinition;
 import org.springframework.beans.factory.annotation.Autowire;
@@ -119,11 +118,12 @@ class ConfigurationClassBeanDefinitionReader {
 	}
 
 	/**
-	 * Read a particular {@link ConfigurationClass}, registering bean definitions for the
+	 * Read a particular {@link ConfigurationClass}, registering bean definitions
-	 * class itself, all its {@link Bean} methods
+	 * for the class itself and all of its {@link Bean} methods.
 	 */
 	private void loadBeanDefinitionsForConfigurationClass(ConfigurationClass configClass,
 			TrackedConditionEvaluator trackedConditionEvaluator) {
+
 		if (trackedConditionEvaluator.shouldSkip(configClass)) {
 			removeBeanDefinition(configClass);
 			return;
@@ -140,12 +140,8 @@ class ConfigurationClassBeanDefinitionReader {
 	}
 
 	private void removeBeanDefinition(ConfigurationClass configClass) {
-		if (StringUtils.hasLength(configClass.getBeanName())) {
+		if (StringUtils.hasLength(configClass.getBeanName()) && this.registry.containsBeanDefinition(configClass.getBeanName())) {
-			try {
+			this.registry.removeBeanDefinition(configClass.getBeanName());
-				this.registry.removeBeanDefinition(configClass.getBeanName());
-			}
-			catch (NoSuchBeanDefinitionException ex) {
-			}
 		}
 	}
 
@@ -196,7 +192,7 @@ class ConfigurationClassBeanDefinitionReader {
 		beanDef.setAutowireMode(RootBeanDefinition.AUTOWIRE_CONSTRUCTOR);
 		beanDef.setAttribute(RequiredAnnotationBeanPostProcessor.SKIP_REQUIRED_CHECK_ATTRIBUTE, Boolean.TRUE);
 
-		// consider name and any aliases
+		// Consider name and any aliases
 		AnnotationAttributes bean = AnnotationConfigUtils.attributesFor(metadata, Bean.class);
 		List<String> names = new ArrayList<String>(Arrays.asList(bean.getStringArray("name")));
 		String beanName = (names.size() > 0 ? names.remove(0) : beanMethod.getMetadata().getMethodName());
@@ -204,19 +200,9 @@ class ConfigurationClassBeanDefinitionReader {
 			this.registry.registerAlias(beanName, alias);
 		}
 
-		// has this already been overridden (e.g. via XML)?
+		// Has this  effectively been overridden before (e.g. via XML)?
-		if (this.registry.containsBeanDefinition(beanName)) {
+		if (isOverriddenByExistingDefinition(beanMethod, beanName)) {
-			BeanDefinition existingBeanDef = registry.getBeanDefinition(beanName);
+			return;
-			// is the existing bean definition one that was created from a configuration class?
-			if (!(existingBeanDef instanceof ConfigurationClassBeanDefinition)) {
-				// no -> then it's an external override, probably XML
-				// overriding is legal, return immediately
-				if (logger.isDebugEnabled()) {
-					logger.debug(String.format("Skipping loading bean definition for %s: a definition for bean " +
-							"'%s' already exists. This is likely due to an override in XML.", beanMethod, beanName));
-				}
-				return;
-			}
 		}
 
 		AnnotationConfigUtils.processCommonDefinitionAnnotations(beanDef, metadata);
@@ -236,7 +222,7 @@ class ConfigurationClassBeanDefinitionReader {
 			beanDef.setDestroyMethodName(destroyMethodName);
 		}
 
-		// consider scoping
+		// Consider scoping
 		ScopedProxyMode proxyMode = ScopedProxyMode.NO;
 		AnnotationAttributes scope = AnnotationConfigUtils.attributesFor(metadata, Scope.class);
 		if (scope != null) {
@@ -247,7 +233,7 @@ class ConfigurationClassBeanDefinitionReader {
 			}
 		}
 
-		// replace the original bean definition with the target one, if necessary
+		// Replace the original bean definition with the target one, if necessary
 		BeanDefinition beanDefToRegister = beanDef;
 		if (proxyMode != ScopedProxyMode.NO) {
 			BeanDefinitionHolder proxyDef = ScopedProxyCreator.createScopedProxy(
@@ -257,12 +243,40 @@ class ConfigurationClassBeanDefinitionReader {
 		}
 
 		if (logger.isDebugEnabled()) {
-			logger.debug(String.format("Registering bean definition for @Bean method %s.%s()", configClass.getMetadata().getClassName(), beanName));
+			logger.debug(String.format("Registering bean definition for @Bean method %s.%s()",
+					configClass.getMetadata().getClassName(), beanName));
 		}
 
-		registry.registerBeanDefinition(beanName, beanDefToRegister);
+		this.registry.registerBeanDefinition(beanName, beanDefToRegister);
 	}
 
+	protected boolean isOverriddenByExistingDefinition(BeanMethod beanMethod, String beanName) {
+		if (!this.registry.containsBeanDefinition(beanName)) {
+			return false;
+		}
+		BeanDefinition existingBeanDef = this.registry.getBeanDefinition(beanName);
+
+		// Is the existing bean definition one that was created from a configuration class?
+		// -> allow the current bean method to override, since both are at second-pass level
+		if (existingBeanDef instanceof ConfigurationClassBeanDefinition) {
+			return false;
+		}
+
+		// Has the existing bean definition bean marked as a framework-generated bean?
+		// -> allow the current bean method to override it, since it is application-level
+		if (existingBeanDef.getRole() > BeanDefinition.ROLE_APPLICATION) {
+			return false;
+		}
+
+		// At this point, it's a top-level override (probably XML), just having been parsed
+		// before configuration class processing kicks in...
+		if (logger.isInfoEnabled()) {
+			logger.info(String.format("Skipping bean definition for %s: a definition for bean '%s' " +
+					"already exists. This top-level bean definition is considered as an override.",
+					beanMethod, beanName));
+		}
+		return true;
+	}
 
 	private void loadBeanDefinitionsFromImportedResources(
 			Map<String, Class<? extends BeanDefinitionReader>> importedResources) {
@@ -294,9 +308,9 @@ class ConfigurationClassBeanDefinitionReader {
 		}
 	}
 
-	private void loadBeanDefinitionsFromRegistrars(
+	private void loadBeanDefinitionsFromRegistrars(AnnotationMetadata importingClassMetadata,
-			AnnotationMetadata importingClassMetadata,
 			Set<ImportBeanDefinitionRegistrar> importBeanDefinitionRegistrars) {
+
 		for (ImportBeanDefinitionRegistrar registrar : importBeanDefinitionRegistrars) {
 			registrar.registerBeanDefinitions(importingClassMetadata, this.registry);
 		}

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

@@ -16,13 +16,15 @@
 
 package org.springframework.context.annotation;
 
-import static org.junit.Assert.*;
 import org.junit.Test;
-import org.springframework.tests.sample.beans.TestBean;
 
 import org.springframework.beans.factory.support.ChildBeanDefinition;
 import org.springframework.beans.factory.support.DefaultListableBeanFactory;
 import org.springframework.beans.factory.support.RootBeanDefinition;
+import org.springframework.core.io.DescriptiveResource;
+import org.springframework.tests.sample.beans.TestBean;
+
+import static org.junit.Assert.*;
 
 /**
  * @author Chris Beams
@@ -82,6 +84,33 @@ public class ConfigurationClassPostProcessorTests {
 		assertSame(foo, bar.foo);
 	}
 
+	@Test
+	public void testPostProcessorOverridesNonApplicationBeanDefinitions() {
+		DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
+		RootBeanDefinition rbd = new RootBeanDefinition(TestBean.class);
+		rbd.setRole(RootBeanDefinition.ROLE_SUPPORT);
+		beanFactory.registerBeanDefinition("bar", rbd);
+		beanFactory.registerBeanDefinition("config", new RootBeanDefinition(SingletonBeanConfig.class));
+		ConfigurationClassPostProcessor pp = new ConfigurationClassPostProcessor();
+		pp.postProcessBeanFactory(beanFactory);
+		Foo foo = beanFactory.getBean("foo", Foo.class);
+		Bar bar = beanFactory.getBean("bar", Bar.class);
+		assertSame(foo, bar.foo);
+	}
+
+	@Test
+	public void testPostProcessorDoesNotOverrideRegularBeanDefinitions() {
+		DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
+		RootBeanDefinition rbd = new RootBeanDefinition(TestBean.class);
+		rbd.setResource(new DescriptiveResource("XML or something"));
+		beanFactory.registerBeanDefinition("bar", rbd);
+		beanFactory.registerBeanDefinition("config", new RootBeanDefinition(SingletonBeanConfig.class));
+		ConfigurationClassPostProcessor pp = new ConfigurationClassPostProcessor();
+		pp.postProcessBeanFactory(beanFactory);
+		Foo foo = beanFactory.getBean("foo", Foo.class);
+		TestBean bar = beanFactory.getBean("bar", TestBean.class);
+	}
+
 	@Test
 	public void testProcessingAllowedOnlyOncePerProcessorRegistryPair() {
 		DefaultListableBeanFactory bf1 = new DefaultListableBeanFactory();
@@ -91,13 +120,15 @@ public class ConfigurationClassPostProcessorTests {
 		try {
 			pp.postProcessBeanFactory(bf1); // second invocation for bf1 -- should throw
 			fail("expected exception");
-		} catch (IllegalStateException ex) {
+		}
+		catch (IllegalStateException ex) {
 		}
 		pp.postProcessBeanFactory(bf2); // first invocation for bf2 -- should succeed
 		try {
 			pp.postProcessBeanFactory(bf2); // second invocation for bf2 -- should throw
 			fail("expected exception");
-		} catch (IllegalStateException ex) {
+		}
+		catch (IllegalStateException ex) {
 		}
 	}