Skip to content

Commit

Permalink
Make use of bean definition overriding more visible
Browse files Browse the repository at this point in the history 
This commit makes the use of bean definition overriding more visible and
prepare for a deprecation of the feature in the next major release.

As of this commit, use of bean definition overriding logs at INFO level.
The previous log level can be restored by setting the
allowBeanDefinitionOverriding flag explicitly on the BeanFactory (or
via the related ApplicationContext).

A number of tests that are using bean overriding on purpose have been
updated to set this flag, which will make them easier to find once we
actually deprecate the feature.

Closes spring-projectsgh-31288
  • Loading branch information
@snicoll
snicoll committed Apr 2, 2024
1 parent d262654 commit dd62224
Show file tree
Hide file tree
Showing 16 changed files with 170 additions and 71 deletions.
64 changes: 42 additions & 22 deletions ...s/src/main/java/org/springframework/beans/factory/support/DefaultListableBeanFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ public class DefaultListableBeanFactory extends AbstractAutowireCapableBeanFacto
private String serializationId;

/** Whether to allow re-registration of a different definition with the same name. */
private boolean allowBeanDefinitionOverriding = true;
@Nullable
private Boolean allowBeanDefinitionOverriding;

/** Whether to allow eager class loading even for lazy-init beans. */
private boolean allowEagerClassLoading = true;
Expand Down Expand Up @@ -259,7 +260,7 @@ public void setAllowBeanDefinitionOverriding(boolean allowBeanDefinitionOverridi
* @since 4.1.2
*/
public boolean isAllowBeanDefinitionOverriding() {
return this.allowBeanDefinitionOverriding;
return !Boolean.FALSE.equals(this.allowBeanDefinitionOverriding);
}

/**
Expand Down Expand Up @@ -1142,27 +1143,8 @@ public void registerBeanDefinition(String beanName, BeanDefinition beanDefinitio
if (!isBeanDefinitionOverridable(beanName)) {
throw new BeanDefinitionOverrideException(beanName, beanDefinition, existingDefinition);
}
else if (existingDefinition.getRole() < beanDefinition.getRole()) {
// e.g. was ROLE_APPLICATION, now overriding with ROLE_SUPPORT or ROLE_INFRASTRUCTURE
if (logger.isInfoEnabled()) {
logger.info("Overriding user-defined bean definition for bean '" + beanName +
"' with a framework-generated bean definition: replacing [" +
existingDefinition + "] with [" + beanDefinition + "]");
}
}
else if (!beanDefinition.equals(existingDefinition)) {
if (logger.isDebugEnabled()) {
logger.debug("Overriding bean definition for bean '" + beanName +
"' with a different definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
}
else {
if (logger.isTraceEnabled()) {
logger.trace("Overriding bean definition for bean '" + beanName +
"' with an equivalent definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
logBeanDefinitionOverriding(beanName, beanDefinition, existingDefinition);
}
this.beanDefinitionMap.put(beanName, beanDefinition);
}
Expand Down Expand Up @@ -1217,6 +1199,44 @@ else if (isConfigurationFrozen()) {
}
}

private void logBeanDefinitionOverriding(String beanName, BeanDefinition beanDefinition,
BeanDefinition existingDefinition) {

boolean explicitBeanOverride = (this.allowBeanDefinitionOverriding != null);
if (existingDefinition.getRole() < beanDefinition.getRole()) {
// e.g. was ROLE_APPLICATION, now overriding with ROLE_SUPPORT or ROLE_INFRASTRUCTURE
if (logger.isInfoEnabled()) {
logger.info("Overriding user-defined bean definition for bean '" + beanName +
"' with a framework-generated bean definition: replacing [" +
existingDefinition + "] with [" + beanDefinition + "]");
}
}
else if (!beanDefinition.equals(existingDefinition)) {
if (explicitBeanOverride && logger.isInfoEnabled()) {
logger.info("Overriding bean definition for bean '" + beanName +
"' with a different definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
if (logger.isDebugEnabled()) {
logger.debug("Overriding bean definition for bean '" + beanName +
"' with a different definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
}
else {
if (explicitBeanOverride && logger.isInfoEnabled()) {
logger.info("Overriding bean definition for bean '" + beanName +
"' with an equivalent definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
if (logger.isTraceEnabled()) {
logger.trace("Overriding bean definition for bean '" + beanName +
"' with an equivalent definition: replacing [" + existingDefinition +
"] with [" + beanDefinition + "]");
}
}
}

@Override
public void removeBeanDefinition(String beanName) throws NoSuchBeanDefinitionException {
Assert.hasText(beanName, "'beanName' must not be empty");
Expand Down
6 changes: 6 additions & 0 deletions ...eans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,7 @@ void mergedBeanDefinitionChangesRetainedAfterFreezeConfiguration() {

@Test
void aliasCircle() {
lbf.setAllowBeanDefinitionOverriding(true);
lbf.registerAlias("test", "test2");
lbf.registerAlias("test2", "test3");

Expand Down Expand Up @@ -867,6 +868,7 @@ void aliasChaining() {

@Test
void beanDefinitionOverriding() {
lbf.setAllowBeanDefinitionOverriding(true);
lbf.registerBeanDefinition("test", new RootBeanDefinition(TestBean.class));
lbf.registerBeanDefinition("test", new RootBeanDefinition(NestedTestBean.class));
lbf.registerAlias("otherTest", "test2");
Expand Down Expand Up @@ -906,6 +908,7 @@ void beanDefinitionOverridingNotAllowed() {

@Test
void beanDefinitionOverridingWithAlias() {
lbf.setAllowBeanDefinitionOverriding(true);
lbf.registerBeanDefinition("test", new RootBeanDefinition(TestBean.class));
lbf.registerAlias("test", "testAlias");
lbf.registerBeanDefinition("test", new RootBeanDefinition(NestedTestBean.class));
Expand All @@ -917,6 +920,7 @@ void beanDefinitionOverridingWithAlias() {

@Test
void beanDefinitionOverridingWithConstructorArgumentMismatch() {
lbf.setAllowBeanDefinitionOverriding(true);
RootBeanDefinition bd1 = new RootBeanDefinition(NestedTestBean.class);
bd1.getConstructorArgumentValues().addIndexedArgumentValue(1, "value1");
lbf.registerBeanDefinition("test", bd1);
Expand Down Expand Up @@ -1196,6 +1200,7 @@ void registerExistingSingletonWithAlreadyBound() {

@Test
void reregisterBeanDefinition() {
lbf.setAllowBeanDefinitionOverriding(true);
RootBeanDefinition bd1 = new RootBeanDefinition(TestBean.class);
bd1.setScope(BeanDefinition.SCOPE_PROTOTYPE);
lbf.registerBeanDefinition("testBean", bd1);
Expand Down Expand Up @@ -1306,6 +1311,7 @@ void expressionInStringArray() {

@Test
void withOverloadedSetters() {
lbf.setAllowBeanDefinitionOverriding(true);
RootBeanDefinition rbd = new RootBeanDefinition(SetterOverload.class);
rbd.getPropertyValues().add("object", "a String");
lbf.registerBeanDefinition("overloaded", rbd);
Expand Down
46 changes: 27 additions & 19 deletions ...ans/src/test/java/org/springframework/beans/factory/annotation/LookupAnnotationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

package org.springframework.beans.factory.annotation;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.springframework.beans.factory.config.BeanDefinition;
Expand All @@ -33,25 +32,9 @@
*/
class LookupAnnotationTests {

private DefaultListableBeanFactory beanFactory;


@BeforeEach
void setup() {
beanFactory = new DefaultListableBeanFactory();
AutowiredAnnotationBeanPostProcessor aabpp = new AutowiredAnnotationBeanPostProcessor();
aabpp.setBeanFactory(beanFactory);
beanFactory.addBeanPostProcessor(aabpp);
beanFactory.registerBeanDefinition("abstractBean", new RootBeanDefinition(AbstractBean.class));
beanFactory.registerBeanDefinition("beanConsumer", new RootBeanDefinition(BeanConsumer.class));
RootBeanDefinition tbd = new RootBeanDefinition(TestBean.class);
tbd.setScope(BeanDefinition.SCOPE_PROTOTYPE);
beanFactory.registerBeanDefinition("testBean", tbd);
}


@Test
void testWithoutConstructorArg() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = (AbstractBean) beanFactory.getBean("abstractBean");
Object expected = bean.get();
assertThat(expected.getClass()).isEqualTo(TestBean.class);
Expand All @@ -60,6 +43,7 @@ void testWithoutConstructorArg() {

@Test
void testWithOverloadedArg() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = (AbstractBean) beanFactory.getBean("abstractBean");
TestBean expected = bean.get("haha");
assertThat(expected.getClass()).isEqualTo(TestBean.class);
Expand All @@ -69,6 +53,7 @@ void testWithOverloadedArg() {

@Test
void testWithOneConstructorArg() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = (AbstractBean) beanFactory.getBean("abstractBean");
TestBean expected = bean.getOneArgument("haha");
assertThat(expected.getClass()).isEqualTo(TestBean.class);
Expand All @@ -78,6 +63,7 @@ void testWithOneConstructorArg() {

@Test
void testWithTwoConstructorArg() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = (AbstractBean) beanFactory.getBean("abstractBean");
TestBean expected = bean.getTwoArguments("haha", 72);
assertThat(expected.getClass()).isEqualTo(TestBean.class);
Expand All @@ -88,6 +74,7 @@ void testWithTwoConstructorArg() {

@Test
void testWithThreeArgsShouldFail() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = (AbstractBean) beanFactory.getBean("abstractBean");
assertThatExceptionOfType(AbstractMethodError.class).as("TestBean has no three arg constructor").isThrownBy(() ->
bean.getThreeArguments("name", 1, 2));
Expand All @@ -96,6 +83,7 @@ void testWithThreeArgsShouldFail() {

@Test
void testWithEarlyInjection() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
AbstractBean bean = beanFactory.getBean("beanConsumer", BeanConsumer.class).abstractBean;
Object expected = bean.get();
assertThat(expected.getClass()).isEqualTo(TestBean.class);
Expand All @@ -106,7 +94,7 @@ void testWithEarlyInjection() {
public void testWithNullBean() {
RootBeanDefinition tbd = new RootBeanDefinition(TestBean.class, () -> null);
tbd.setScope(BeanDefinition.SCOPE_PROTOTYPE);
beanFactory.registerBeanDefinition("testBean", tbd);
DefaultListableBeanFactory beanFactory = configureBeanFactory(tbd);

AbstractBean bean = beanFactory.getBean("beanConsumer", BeanConsumer.class).abstractBean;
Object expected = bean.get();
Expand All @@ -116,6 +104,7 @@ public void testWithNullBean() {

@Test
void testWithGenericBean() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
beanFactory.registerBeanDefinition("numberBean", new RootBeanDefinition(NumberBean.class));
beanFactory.registerBeanDefinition("doubleStore", new RootBeanDefinition(DoubleStore.class));
beanFactory.registerBeanDefinition("floatStore", new RootBeanDefinition(FloatStore.class));
Expand All @@ -127,6 +116,7 @@ void testWithGenericBean() {

@Test
void testSingletonWithoutMetadataCaching() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
beanFactory.setCacheBeanMetadata(false);

beanFactory.registerBeanDefinition("numberBean", new RootBeanDefinition(NumberBean.class));
Expand All @@ -140,6 +130,7 @@ void testSingletonWithoutMetadataCaching() {

@Test
void testPrototypeWithoutMetadataCaching() {
DefaultListableBeanFactory beanFactory = configureBeanFactory();
beanFactory.setCacheBeanMetadata(false);

beanFactory.registerBeanDefinition("numberBean", new RootBeanDefinition(NumberBean.class, BeanDefinition.SCOPE_PROTOTYPE, null));
Expand All @@ -155,6 +146,23 @@ void testPrototypeWithoutMetadataCaching() {
assertThat(beanFactory.getBean(FloatStore.class)).isSameAs(bean.getFloatStore());
}

private DefaultListableBeanFactory configureBeanFactory(RootBeanDefinition tbd) {
DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory();
AutowiredAnnotationBeanPostProcessor aabpp = new AutowiredAnnotationBeanPostProcessor();
aabpp.setBeanFactory(beanFactory);
beanFactory.addBeanPostProcessor(aabpp);
beanFactory.registerBeanDefinition("abstractBean", new RootBeanDefinition(AbstractBean.class));
beanFactory.registerBeanDefinition("beanConsumer", new RootBeanDefinition(BeanConsumer.class));
beanFactory.registerBeanDefinition("testBean", tbd);
return beanFactory;
}

private DefaultListableBeanFactory configureBeanFactory() {
RootBeanDefinition tbd = new RootBeanDefinition(TestBean.class);
tbd.setScope(BeanDefinition.SCOPE_PROTOTYPE);
return configureBeanFactory(tbd);
}


public abstract static class AbstractBean {

Expand Down
3 changes: 2 additions & 1 deletion spring-beans/src/test/java/org/springframework/beans/factory/xml/DuplicateBeanIdTests.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2023 the original author or authors.
* Copyright 2002-2024 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.
Expand Down Expand Up @@ -52,6 +52,7 @@ void duplicateBeanIdsWithinSameNestingLevelRaisesError() {
@Test
void duplicateBeanIdsAcrossNestingLevels() {
DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
bf.setAllowBeanDefinitionOverriding(true);
XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(bf);
reader.loadBeanDefinitions(new ClassPathResource("DuplicateBeanIdTests-multiLevel-context.xml", this.getClass()));
TestBean testBean = bf.getBean(TestBean.class); // there should be only one
Expand Down
1 change: 1 addition & 0 deletions ...ng-beans/src/test/java/org/springframework/beans/factory/xml/NestedBeansElementTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ void getBean_withActiveProfile() {
env.setActiveProfiles("dev");

DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
bf.setAllowBeanDefinitionOverriding(true);
XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(bf);
reader.setEnvironment(env);
reader.loadBeanDefinitions(XML);
Expand Down
46 changes: 32 additions & 14 deletions ...t/src/test/java/org/springframework/cache/jcache/config/JCacheCustomInterceptorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.springframework.beans.BeansException;
import org.springframework.beans.factory.BeanFactory;
import org.springframework.beans.factory.config.BeanPostProcessor;
import org.springframework.cache.Cache;
import org.springframework.cache.CacheManager;
import org.springframework.cache.annotation.EnableCaching;
Expand All @@ -43,6 +46,8 @@
import static org.assertj.core.api.Assertions.assertThatRuntimeException;

/**
* Tests that use a custom {@link JCacheInterceptor}.
*
* @author Stephane Nicoll
*/
class JCacheCustomInterceptorTests {
Expand All @@ -56,16 +61,19 @@ class JCacheCustomInterceptorTests {

@BeforeEach
void setup() {
ctx = new AnnotationConfigApplicationContext(EnableCachingConfig.class);
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
context.getBeanFactory().addBeanPostProcessor(
new CacheInterceptorBeanPostProcessor(context.getBeanFactory()));
context.register(EnableCachingConfig.class);
context.refresh();
this.ctx = context;
cs = ctx.getBean("service", JCacheableService.class);
exceptionCache = ctx.getBean("exceptionCache", Cache.class);
}

@AfterEach
void tearDown() {
if (ctx != null) {
ctx.close();
}
ctx.close();
}


Expand All @@ -87,8 +95,8 @@ void customInterceptorAppliesWithRuntimeException() {
@Test
void customInterceptorAppliesWithCheckedException() {
assertThatRuntimeException()
.isThrownBy(() -> cs.cacheWithCheckedException("id", true))
.withCauseExactlyInstanceOf(IOException.class);
.isThrownBy(() -> cs.cacheWithCheckedException("id", true))
.withCauseExactlyInstanceOf(IOException.class);
}


Expand Down Expand Up @@ -120,18 +128,28 @@ public Cache exceptionCache() {
return new ConcurrentMapCache("exception");
}

@Bean
public JCacheInterceptor jCacheInterceptor(JCacheOperationSource cacheOperationSource) {
JCacheInterceptor cacheInterceptor = new TestCacheInterceptor();
cacheInterceptor.setCacheOperationSource(cacheOperationSource);
return cacheInterceptor;
}
}


static class CacheInterceptorBeanPostProcessor implements BeanPostProcessor {

private final BeanFactory beanFactory;

CacheInterceptorBeanPostProcessor(BeanFactory beanFactory) {this.beanFactory = beanFactory;}

public Object postProcessBeforeInitialization(Object bean, String beanName) throws BeansException {
if (beanName.equals("jCacheInterceptor")) {
JCacheInterceptor cacheInterceptor = new TestCacheInterceptor();
cacheInterceptor.setCacheOperationSource(beanFactory.getBean(JCacheOperationSource.class));
return cacheInterceptor;
}
return bean;
}

}

/**
* A test {@link org.springframework.cache.interceptor.CacheInterceptor} that handles special exception
* types.
* A test {@link JCacheInterceptor} that handles special exception types.
*/
@SuppressWarnings("serial")
static class TestCacheInterceptor extends JCacheInterceptor {
Expand Down
Loading

0 comments on commit dd62224

Please sign in to comment.