Consistent processing of overridden bean methods

Closes gh-28286
spring-projects · Feb 21, 2024 · f5397d6 · f5397d6
1 parent 2a1b30d
commit f5397d6
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 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
@@ -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<String, Object> 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
@@ -223,13 +223,12 @@ void validate(ProblemReporter problemReporter) {
 		Map<String, Object> 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/...n/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/...n/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/...ext/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java b/...ext/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java
@@ -29,6 +29,7 @@
 
 /**
  * Tests regarding overloading and overriding of bean methods.
+ *
  * <p>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/...g/springframework/context/annotation/configuration/ConfigurationClassProcessingTests.java b/...g/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 {