Skip to content

Commit fe8dec9

Browse files
jhoellerjhoeller
committed
@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
1 parent 4b2847d commit fe8dec9

File tree

4 files changed

+91
-38
lines changed

4 files changed

+91
-38
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2012 the original author or authors.
2+
* Copyright 2002-2013 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -229,11 +229,11 @@ public interface BeanDefinition extends AttributeAccessor, BeanMetadataElement {
229229

230230
/**
231231
* Get the role hint for this {@code BeanDefinition}. The role hint
232-
* provides tools with an indication of the importance of a particular
233-
* {@code BeanDefinition}.
232+
* provides the frameworks as well as tools with an indication of
233+
* the role and importance of a particular {@code BeanDefinition}.
234234
* @see #ROLE_APPLICATION
235-
* @see #ROLE_INFRASTRUCTURE
236235
* @see #ROLE_SUPPORT
236+
* @see #ROLE_INFRASTRUCTURE
237237
*/
238238
int getRole();
239239

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -691,13 +691,21 @@ public void registerBeanDefinition(String beanName, BeanDefinition beanDefinitio
691691
}
692692

693693
synchronized (this.beanDefinitionMap) {
694-
Object oldBeanDefinition = this.beanDefinitionMap.get(beanName);
694+
BeanDefinition oldBeanDefinition = this.beanDefinitionMap.get(beanName);
695695
if (oldBeanDefinition != null) {
696696
if (!this.allowBeanDefinitionOverriding) {
697697
throw new BeanDefinitionStoreException(beanDefinition.getResourceDescription(), beanName,
698698
"Cannot register bean definition [" + beanDefinition + "] for bean '" + beanName +
699699
"': There is already [" + oldBeanDefinition + "] bound.");
700700
}
701+
else if (oldBeanDefinition.getRole() < beanDefinition.getRole()) {
702+
// e.g. was ROLE_APPLICATION, now overriding with ROLE_SUPPORT or ROLE_INFRASTRUCTURE
703+
if (this.logger.isWarnEnabled()) {
704+
this.logger.warn("Overriding user-defined bean definition for bean '" + beanName +
705+
" with a framework-generated bean definition ': replacing [" +
706+
oldBeanDefinition + "] with [" + beanDefinition + "]");
707+
}
708+
}
701709
else {
702710
if (this.logger.isInfoEnabled()) {
703711
this.logger.info("Overriding bean definition for bean '" + beanName +

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

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@
2727
import org.apache.commons.logging.Log;
2828
import org.apache.commons.logging.LogFactory;
2929

30-
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
3130
import org.springframework.beans.factory.annotation.AnnotatedBeanDefinition;
3231
import org.springframework.beans.factory.annotation.AnnotatedGenericBeanDefinition;
3332
import org.springframework.beans.factory.annotation.Autowire;
@@ -119,11 +118,12 @@ public void loadBeanDefinitions(Set<ConfigurationClass> configurationModel) {
119118
}
120119

121120
/**
122-
* Read a particular {@link ConfigurationClass}, registering bean definitions for the
123-
* class itself, all its {@link Bean} methods
121+
* Read a particular {@link ConfigurationClass}, registering bean definitions
122+
* for the class itself and all of its {@link Bean} methods.
124123
*/
125124
private void loadBeanDefinitionsForConfigurationClass(ConfigurationClass configClass,
126125
TrackedConditionEvaluator trackedConditionEvaluator) {
126+
127127
if (trackedConditionEvaluator.shouldSkip(configClass)) {
128128
removeBeanDefinition(configClass);
129129
return;
@@ -140,12 +140,8 @@ private void loadBeanDefinitionsForConfigurationClass(ConfigurationClass configC
140140
}
141141

142142
private void removeBeanDefinition(ConfigurationClass configClass) {
143-
if (StringUtils.hasLength(configClass.getBeanName())) {
144-
try {
145-
this.registry.removeBeanDefinition(configClass.getBeanName());
146-
}
147-
catch (NoSuchBeanDefinitionException ex) {
148-
}
143+
if (StringUtils.hasLength(configClass.getBeanName()) && this.registry.containsBeanDefinition(configClass.getBeanName())) {
144+
this.registry.removeBeanDefinition(configClass.getBeanName());
149145
}
150146
}
151147

@@ -196,27 +192,17 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
196192
beanDef.setAutowireMode(RootBeanDefinition.AUTOWIRE_CONSTRUCTOR);
197193
beanDef.setAttribute(RequiredAnnotationBeanPostProcessor.SKIP_REQUIRED_CHECK_ATTRIBUTE, Boolean.TRUE);
198194

199-
// consider name and any aliases
195+
// Consider name and any aliases
200196
AnnotationAttributes bean = AnnotationConfigUtils.attributesFor(metadata, Bean.class);
201197
List<String> names = new ArrayList<String>(Arrays.asList(bean.getStringArray("name")));
202198
String beanName = (names.size() > 0 ? names.remove(0) : beanMethod.getMetadata().getMethodName());
203199
for (String alias : names) {
204200
this.registry.registerAlias(beanName, alias);
205201
}
206202

207-
// has this already been overridden (e.g. via XML)?
208-
if (this.registry.containsBeanDefinition(beanName)) {
209-
BeanDefinition existingBeanDef = registry.getBeanDefinition(beanName);
210-
// is the existing bean definition one that was created from a configuration class?
211-
if (!(existingBeanDef instanceof ConfigurationClassBeanDefinition)) {
212-
// no -> then it's an external override, probably XML
213-
// overriding is legal, return immediately
214-
if (logger.isDebugEnabled()) {
215-
logger.debug(String.format("Skipping loading bean definition for %s: a definition for bean " +
216-
"'%s' already exists. This is likely due to an override in XML.", beanMethod, beanName));
217-
}
218-
return;
219-
}
203+
// Has this effectively been overridden before (e.g. via XML)?
204+
if (isOverriddenByExistingDefinition(beanMethod, beanName)) {
205+
return;
220206
}
221207

222208
AnnotationConfigUtils.processCommonDefinitionAnnotations(beanDef, metadata);
@@ -236,7 +222,7 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
236222
beanDef.setDestroyMethodName(destroyMethodName);
237223
}
238224

239-
// consider scoping
225+
// Consider scoping
240226
ScopedProxyMode proxyMode = ScopedProxyMode.NO;
241227
AnnotationAttributes scope = AnnotationConfigUtils.attributesFor(metadata, Scope.class);
242228
if (scope != null) {
@@ -247,7 +233,7 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
247233
}
248234
}
249235

250-
// replace the original bean definition with the target one, if necessary
236+
// Replace the original bean definition with the target one, if necessary
251237
BeanDefinition beanDefToRegister = beanDef;
252238
if (proxyMode != ScopedProxyMode.NO) {
253239
BeanDefinitionHolder proxyDef = ScopedProxyCreator.createScopedProxy(
@@ -257,12 +243,40 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) {
257243
}
258244

259245
if (logger.isDebugEnabled()) {
260-
logger.debug(String.format("Registering bean definition for @Bean method %s.%s()", configClass.getMetadata().getClassName(), beanName));
246+
logger.debug(String.format("Registering bean definition for @Bean method %s.%s()",
247+
configClass.getMetadata().getClassName(), beanName));
261248
}
262249

263-
registry.registerBeanDefinition(beanName, beanDefToRegister);
250+
this.registry.registerBeanDefinition(beanName, beanDefToRegister);
264251
}
265252

253+
protected boolean isOverriddenByExistingDefinition(BeanMethod beanMethod, String beanName) {
254+
if (!this.registry.containsBeanDefinition(beanName)) {
255+
return false;
256+
}
257+
BeanDefinition existingBeanDef = this.registry.getBeanDefinition(beanName);
258+
259+
// Is the existing bean definition one that was created from a configuration class?
260+
// -> allow the current bean method to override, since both are at second-pass level
261+
if (existingBeanDef instanceof ConfigurationClassBeanDefinition) {
262+
return false;
263+
}
264+
265+
// Has the existing bean definition bean marked as a framework-generated bean?
266+
// -> allow the current bean method to override it, since it is application-level
267+
if (existingBeanDef.getRole() > BeanDefinition.ROLE_APPLICATION) {
268+
return false;
269+
}
270+
271+
// At this point, it's a top-level override (probably XML), just having been parsed
272+
// before configuration class processing kicks in...
273+
if (logger.isInfoEnabled()) {
274+
logger.info(String.format("Skipping bean definition for %s: a definition for bean '%s' " +
275+
"already exists. This top-level bean definition is considered as an override.",
276+
beanMethod, beanName));
277+
}
278+
return true;
279+
}
266280

267281
private void loadBeanDefinitionsFromImportedResources(
268282
Map<String, Class<? extends BeanDefinitionReader>> importedResources) {
@@ -294,9 +308,9 @@ private void loadBeanDefinitionsFromImportedResources(
294308
}
295309
}
296310

297-
private void loadBeanDefinitionsFromRegistrars(
298-
AnnotationMetadata importingClassMetadata,
311+
private void loadBeanDefinitionsFromRegistrars(AnnotationMetadata importingClassMetadata,
299312
Set<ImportBeanDefinitionRegistrar> importBeanDefinitionRegistrars) {
313+
300314
for (ImportBeanDefinitionRegistrar registrar : importBeanDefinitionRegistrars) {
301315
registrar.registerBeanDefinitions(importingClassMetadata, this.registry);
302316
}

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

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,15 @@
1616

1717
package org.springframework.context.annotation;
1818

19-
import static org.junit.Assert.*;
2019
import org.junit.Test;
21-
import org.springframework.tests.sample.beans.TestBean;
2220

2321
import org.springframework.beans.factory.support.ChildBeanDefinition;
2422
import org.springframework.beans.factory.support.DefaultListableBeanFactory;
2523
import org.springframework.beans.factory.support.RootBeanDefinition;
24+
import org.springframework.core.io.DescriptiveResource;
25+
import org.springframework.tests.sample.beans.TestBean;
26+
27+
import static org.junit.Assert.*;
2628

2729
/**
2830
* @author Chris Beams
@@ -82,6 +84,33 @@ public void testPostProcessorIntrospectsInheritedDefinitionsCorrectly() {
8284
assertSame(foo, bar.foo);
8385
}
8486

87+
@Test
88+
public void testPostProcessorOverridesNonApplicationBeanDefinitions() {
89+
DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
90+
RootBeanDefinition rbd = new RootBeanDefinition(TestBean.class);
91+
rbd.setRole(RootBeanDefinition.ROLE_SUPPORT);
92+
beanFactory.registerBeanDefinition("bar", rbd);
93+
beanFactory.registerBeanDefinition("config", new RootBeanDefinition(SingletonBeanConfig.class));
94+
ConfigurationClassPostProcessor pp = new ConfigurationClassPostProcessor();
95+
pp.postProcessBeanFactory(beanFactory);
96+
Foo foo = beanFactory.getBean("foo", Foo.class);
97+
Bar bar = beanFactory.getBean("bar", Bar.class);
98+
assertSame(foo, bar.foo);
99+
}
100+
101+
@Test
102+
public void testPostProcessorDoesNotOverrideRegularBeanDefinitions() {
103+
DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
104+
RootBeanDefinition rbd = new RootBeanDefinition(TestBean.class);
105+
rbd.setResource(new DescriptiveResource("XML or something"));
106+
beanFactory.registerBeanDefinition("bar", rbd);
107+
beanFactory.registerBeanDefinition("config", new RootBeanDefinition(SingletonBeanConfig.class));
108+
ConfigurationClassPostProcessor pp = new ConfigurationClassPostProcessor();
109+
pp.postProcessBeanFactory(beanFactory);
110+
Foo foo = beanFactory.getBean("foo", Foo.class);
111+
TestBean bar = beanFactory.getBean("bar", TestBean.class);
112+
}
113+
85114
@Test
86115
public void testProcessingAllowedOnlyOncePerProcessorRegistryPair() {
87116
DefaultListableBeanFactory bf1 = new DefaultListableBeanFactory();
@@ -91,13 +120,15 @@ public void testProcessingAllowedOnlyOncePerProcessorRegistryPair() {
91120
try {
92121
pp.postProcessBeanFactory(bf1); // second invocation for bf1 -- should throw
93122
fail("expected exception");
94-
} catch (IllegalStateException ex) {
123+
}
124+
catch (IllegalStateException ex) {
95125
}
96126
pp.postProcessBeanFactory(bf2); // first invocation for bf2 -- should succeed
97127
try {
98128
pp.postProcessBeanFactory(bf2); // second invocation for bf2 -- should throw
99129
fail("expected exception");
100-
} catch (IllegalStateException ex) {
130+
}
131+
catch (IllegalStateException ex) {
101132
}
102133
}
103134

0 commit comments

Comments
 (0)