From f5397d64269feb484a33317b1652e43832f3e413 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 21 Feb 2024 18:36:03 +0100 Subject: [PATCH] Consistent processing of overridden bean methods Closes gh-28286 --- .../context/annotation/BeanMethod.java | 28 ++++++--- .../annotation/ConfigurationClass.java | 13 ++--- ...onfigurationClassBeanDefinitionReader.java | 9 ++- .../BeanMethodPolymorphismTests.java | 58 ++++++++++++++++++- .../ConfigurationClassProcessingTests.java | 14 +++++ 5 files changed, 103 insertions(+), 19 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/context/annotation/BeanMethod.java b/spring-context/src/main/java/org/springframework/context/annotation/BeanMethod.java index ea09841aee31..b1570ddad5a7 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/BeanMethod.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/BeanMethod.java @@ -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. @@ -16,6 +16,8 @@ package org.springframework.context.annotation; +import java.util.Map; + import org.springframework.beans.factory.parsing.Problem; import org.springframework.beans.factory.parsing.ProblemReporter; import org.springframework.core.type.MethodMetadata; @@ -52,22 +54,24 @@ public void validate(ProblemReporter problemReporter) { return; } - if (this.configurationClass.getMetadata().isAnnotated(Configuration.class.getName())) { - if (!getMetadata().isOverridable()) { - // instance @Bean methods within @Configuration classes must be overridable to accommodate CGLIB - problemReporter.error(new NonOverridableMethodError()); - } + Map attributes = + getConfigurationClass().getMetadata().getAnnotationAttributes(Configuration.class.getName()); + if (attributes != null && (Boolean) attributes.get("proxyBeanMethods") && !getMetadata().isOverridable()) { + // instance @Bean methods within @Configuration classes must be overridable to accommodate CGLIB + problemReporter.error(new NonOverridableMethodError()); } } @Override public boolean equals(@Nullable Object other) { - return (this == other || (other instanceof BeanMethod that && this.metadata.equals(that.metadata))); + return (this == other || (other instanceof BeanMethod that && + this.configurationClass.equals(that.configurationClass) && + getLocalMethodIdentifier(this.metadata).equals(getLocalMethodIdentifier(that.metadata)))); } @Override public int hashCode() { - return this.metadata.hashCode(); + return this.configurationClass.hashCode() * 31 + getLocalMethodIdentifier(this.metadata).hashCode(); } @Override @@ -76,6 +80,14 @@ public String toString() { } + private static String getLocalMethodIdentifier(MethodMetadata metadata) { + String metadataString = metadata.toString(); + int index = metadataString.indexOf(metadata.getDeclaringClassName()); + return (index >= 0 ? metadataString.substring(index + metadata.getDeclaringClassName().length()) : + metadataString); + } + + private class VoidDeclaredMethodError extends Problem { VoidDeclaredMethodError() { diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java index 0882e7ca41e4..dfb3c4045bb5 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java @@ -223,13 +223,12 @@ void validate(ProblemReporter problemReporter) { Map attributes = this.metadata.getAnnotationAttributes(Configuration.class.getName()); // A configuration class may not be final (CGLIB limitation) unless it declares proxyBeanMethods=false - if (attributes != null && (Boolean) attributes.get("proxyBeanMethods")) { - if (this.metadata.isFinal()) { - problemReporter.error(new FinalConfigurationProblem()); - } - for (BeanMethod beanMethod : this.beanMethods) { - beanMethod.validate(problemReporter); - } + if (attributes != null && (Boolean) attributes.get("proxyBeanMethods") && this.metadata.isFinal()) { + problemReporter.error(new FinalConfigurationProblem()); + } + + for (BeanMethod beanMethod : this.beanMethods) { + beanMethod.validate(problemReporter); } // A configuration class may not contain overloaded bean methods unless it declares enforceUniqueMethods=false diff --git a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index e29443838e00..df5888469c20 100644 --- a/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/spring-context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -53,6 +53,7 @@ import org.springframework.core.type.StandardMethodMetadata; import org.springframework.lang.NonNull; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.StringUtils; /** @@ -229,8 +230,12 @@ private void loadBeanDefinitionsForBeanMethod(BeanMethod beanMethod) { beanDef.setUniqueFactoryMethodName(methodName); } - if (metadata instanceof StandardMethodMetadata sam) { - beanDef.setResolvedFactoryMethod(sam.getIntrospectedMethod()); + if (metadata instanceof StandardMethodMetadata smm && + configClass.getMetadata() instanceof StandardAnnotationMetadata sam) { + Method method = ClassUtils.getMostSpecificMethod(smm.getIntrospectedMethod(), sam.getIntrospectedClass()); + if (method == smm.getIntrospectedMethod()) { + beanDef.setResolvedFactoryMethod(method); + } } beanDef.setAutowireMode(AbstractBeanDefinition.AUTOWIRE_CONSTRUCTOR); diff --git a/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java b/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java index 773b2345fdd9..9a9ce4c6394d 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java @@ -29,6 +29,7 @@ /** * Tests regarding overloading and overriding of bean methods. + * *

Related to SPR-6618. * * @author Chris Beams @@ -41,6 +42,7 @@ public class BeanMethodPolymorphismTests { @Test void beanMethodDetectedOnSuperClass() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Config.class); + assertThat(ctx.getBean("testBean", BaseTestBean.class)).isNotNull(); } @@ -50,6 +52,7 @@ void beanMethodOverriding() { ctx.register(OverridingConfig.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); assertThat(ctx.getBean("testBean", BaseTestBean.class).toString()).isEqualTo("overridden"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isTrue(); @@ -61,17 +64,45 @@ void beanMethodOverridingOnASM() { ctx.registerBeanDefinition("config", new RootBeanDefinition(OverridingConfig.class.getName())); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); assertThat(ctx.getBean("testBean", BaseTestBean.class).toString()).isEqualTo("overridden"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isTrue(); } + @Test + void beanMethodOverridingWithDifferentBeanName() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); + ctx.register(OverridingConfigWithDifferentBeanName.class); + ctx.setAllowBeanDefinitionOverriding(false); + ctx.refresh(); + + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("myTestBean")).isFalse(); + assertThat(ctx.getBean("myTestBean", BaseTestBean.class).toString()).isEqualTo("overridden"); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("myTestBean")).isTrue(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); + } + + @Test + void beanMethodOverridingWithDifferentBeanNameOnASM() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); + ctx.registerBeanDefinition("config", new RootBeanDefinition(OverridingConfigWithDifferentBeanName.class.getName())); + ctx.setAllowBeanDefinitionOverriding(false); + ctx.refresh(); + + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("myTestBean")).isFalse(); + assertThat(ctx.getBean("myTestBean", BaseTestBean.class).toString()).isEqualTo("overridden"); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("myTestBean")).isTrue(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); + } + @Test void beanMethodOverridingWithNarrowedReturnType() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); ctx.register(NarrowedOverridingConfig.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); assertThat(ctx.getBean("testBean", BaseTestBean.class).toString()).isEqualTo("overridden"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isTrue(); @@ -83,6 +114,7 @@ void beanMethodOverridingWithNarrowedReturnTypeOnASM() { ctx.registerBeanDefinition("config", new RootBeanDefinition(NarrowedOverridingConfig.class.getName())); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isFalse(); assertThat(ctx.getBean("testBean", BaseTestBean.class).toString()).isEqualTo("overridden"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("testBean")).isTrue(); @@ -94,6 +126,7 @@ void beanMethodOverloadingWithoutInheritance() { ctx.register(ConfigWithOverloading.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getBean(String.class)).isEqualTo("regular"); } @@ -104,6 +137,7 @@ void beanMethodOverloadingWithoutInheritanceAndExtraDependency() { ctx.getDefaultListableBeanFactory().registerSingleton("anInt", 5); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getBean(String.class)).isEqualTo("overloaded5"); } @@ -113,6 +147,7 @@ void beanMethodOverloadingWithAdditionalMetadata() { ctx.register(ConfigWithOverloadingAndAdditionalMetadata.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isFalse(); assertThat(ctx.getBean(String.class)).isEqualTo("regular"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isTrue(); @@ -125,6 +160,7 @@ void beanMethodOverloadingWithAdditionalMetadataButOtherMethodExecuted() { ctx.getDefaultListableBeanFactory().registerSingleton("anInt", 5); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isFalse(); assertThat(ctx.getBean(String.class)).isEqualTo("overloaded5"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isTrue(); @@ -136,18 +172,19 @@ void beanMethodOverloadingWithInheritance() { ctx.register(SubConfig.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isFalse(); assertThat(ctx.getBean(String.class)).isEqualTo("overloaded5"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isTrue(); } - // SPR-11025 - @Test + @Test // SPR-11025 void beanMethodOverloadingWithInheritanceAndList() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); ctx.register(SubConfigWithList.class); ctx.setAllowBeanDefinitionOverriding(false); ctx.refresh(); + assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isFalse(); assertThat(ctx.getBean(String.class)).isEqualTo("overloaded5"); assertThat(ctx.getDefaultListableBeanFactory().containsSingleton("aString")).isTrue(); @@ -161,6 +198,7 @@ void beanMethodOverloadingWithInheritanceAndList() { @Test void beanMethodShadowing() { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ShadowConfig.class); + assertThat(ctx.getBean(String.class)).isEqualTo("shadow"); } @@ -214,6 +252,22 @@ public String toString() { } + @Configuration + static class OverridingConfigWithDifferentBeanName extends BaseConfig { + + @Bean("myTestBean") @Lazy + @Override + public BaseTestBean testBean() { + return new BaseTestBean() { + @Override + public String toString() { + return "overridden"; + } + }; + } + } + + @Configuration static class NarrowedOverridingConfig extends BaseConfig { diff --git a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ConfigurationClassProcessingTests.java b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ConfigurationClassProcessingTests.java index f0e66428d32a..7a860fb1739b 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/configuration/ConfigurationClassProcessingTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/configuration/ConfigurationClassProcessingTests.java @@ -143,6 +143,11 @@ void finalBeanMethod() { initBeanFactory(ConfigWithFinalBean.class)); } + @Test + void finalBeanMethodWithoutProxy() { + initBeanFactory(ConfigWithFinalBeanWithoutProxy.class); + } + @Test // gh-31007 void voidBeanMethod() { assertThatExceptionOfType(BeanDefinitionParsingException.class).isThrownBy(() -> @@ -438,6 +443,15 @@ static class ConfigWithFinalBean { } + @Configuration(proxyBeanMethods = false) + static class ConfigWithFinalBeanWithoutProxy { + + @Bean public final TestBean testBean() { + return new TestBean(); + } + } + + @Configuration static class ConfigWithVoidBean {