From 2887b74448f127a1f55d51faf46823d2b463b2c0 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Wed, 30 Dec 2009 19:42:12 +0000 Subject: [PATCH] Resolved SPR-6618. Restrictions were too tight on overloaded bean methods and were preventing it altogether. Overloading is now allowed, as long as there is no ambiguity at runtime which bean method should be invoked. git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@2745 50f2f4bb-b051-0410-bef5-90022cba6387 --- .../annotation/ConfigurationClass.java | 58 +++++++-------- ...onfigurationClassBeanDefinitionReader.java | 10 +-- .../annotation/ConfigurationClassMethod.java | 24 ++++--- .../annotation/ConfigurationClassParser.java | 6 +- .../BeanMethodPolymorphismTests.java | 71 +++++++++++++++++++ .../core/type/MethodMetadata.java | 6 ++ .../core/type/StandardMethodMetadata.java | 7 +- .../AnnotationMetadataReadingVisitor.java | 2 +- .../MethodMetadataReadingVisitor.java | 9 ++- 9 files changed, 146 insertions(+), 47 deletions(-) create mode 100644 org.springframework.context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java index eb25db05977..4aefbf5497c 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClass.java @@ -16,6 +16,7 @@ package org.springframework.context.annotation; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.Map; @@ -48,12 +49,10 @@ final class ConfigurationClass { private final Resource resource; - private final Map importedResources = new LinkedHashMap(); + private final Map> importedResources = new LinkedHashMap>(); private final Set methods = new LinkedHashSet(); - private final Map overloadedMethodMap = new LinkedHashMap(); - private String beanName; @@ -90,42 +89,46 @@ final class ConfigurationClass { return this.beanName; } - public ConfigurationClass addMethod(ConfigurationClassMethod method) { + public void addMethod(ConfigurationClassMethod method) { this.methods.add(method); - String name = method.getMetadata().getMethodName(); - Integer count = this.overloadedMethodMap.get(name); - if (count != null) { - this.overloadedMethodMap.put(name, count + 1); - } - else { - this.overloadedMethodMap.put(name, 1); - } - return this; } public Set getMethods() { return this.methods; } - public void addImportedResource(String importedResource, Class readerClass) { + public void addImportedResource(String importedResource, Class readerClass) { this.importedResources.put(importedResource, readerClass); } - public Map getImportedResources() { + public Map> getImportedResources() { return this.importedResources; } public void validate(ProblemReporter problemReporter) { - // No overloading of factory methods allowed - for (Map.Entry entry : this.overloadedMethodMap.entrySet()) { - String methodName = entry.getKey(); - int count = entry.getValue(); + + // a @Bean method may only be overloaded through inheritance. No single + // @Configuration class may declare two @Bean methods with the same name. + final char hashDelim = '#'; + Map methodNameCounts = new HashMap(); + for (ConfigurationClassMethod method : methods) { + String dClassName = method.getMetadata().getDeclaringClassName(); + String methodName = method.getMetadata().getMethodName(); + String fqMethodName = dClassName + hashDelim + methodName; + Integer currentCount = methodNameCounts.get(fqMethodName); + int newCount = currentCount != null ? currentCount + 1 : 1; + methodNameCounts.put(fqMethodName, newCount); + } + + for (String methodName : methodNameCounts.keySet()) { + int count = methodNameCounts.get(methodName); if (count > 1) { - problemReporter.error(new OverloadedMethodProblem(methodName, count)); + String shortMethodName = methodName.substring(methodName.indexOf(hashDelim)+1); + problemReporter.error(new BeanMethodOverloadingProblem(shortMethodName, count)); } } - + // A configuration class may not be final (CGLIB limitation) if (getMetadata().isAnnotated(Configuration.class.getName())) { if (getMetadata().isFinal()) { @@ -149,7 +152,6 @@ final class ConfigurationClass { return getMetadata().getClassName().hashCode(); } - /** Configuration classes must be non-final to accommodate CGLIB subclassing. */ private class FinalConfigurationProblem extends Problem { @@ -159,15 +161,15 @@ final class ConfigurationClass { } } + /** Bean methods on configuration classes may only be overloaded through inheritance. */ + private class BeanMethodOverloadingProblem extends Problem { - /** Factory methods on configuration classes must not be overloaded. */ - private class OverloadedMethodProblem extends Problem { - - public OverloadedMethodProblem(String methodName, int count) { - super(String.format("@Configuration class '%s' has %s overloaded factory methods of name '%s'. " + - "Only one factory method of the same name allowed.", + public BeanMethodOverloadingProblem(String methodName, int count) { + super(String.format("@Configuration class '%s' has %s overloaded @Bean methods named '%s'. " + + "Only one @Bean method of a given name is allowed within each @Configuration class.", getSimpleName(), count, methodName), new Location(getResource(), getMetadata())); } } + } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java index e0578224af4..7bf76877538 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassBeanDefinitionReader.java @@ -119,7 +119,7 @@ class ConfigurationClassBeanDefinitionReader { * the BeanDefinitionRegistry based on its contents. */ private void loadBeanDefinitionsForModelMethod(ConfigurationClassMethod method) { - ConfigurationClass configClass = method.getDeclaringClass(); + ConfigurationClass configClass = method.getConfigurationClass(); MethodMetadata metadata = method.getMetadata(); RootBeanDefinition beanDef = new ConfigurationClassBeanDefinition(configClass); @@ -213,11 +213,11 @@ class ConfigurationClassBeanDefinitionReader { registry.registerBeanDefinition(beanName, beanDefToRegister); } - private void loadBeanDefinitionsFromImportedResources(Map importedResources) { - Map readerInstanceCache = new HashMap(); - for (Map.Entry entry : importedResources.entrySet()) { + private void loadBeanDefinitionsFromImportedResources(Map> importedResources) { + Map, BeanDefinitionReader> readerInstanceCache = new HashMap, BeanDefinitionReader>(); + for (Map.Entry> entry : importedResources.entrySet()) { String resource = entry.getKey(); - Class readerClass = entry.getValue(); + Class readerClass = entry.getValue(); if (!readerInstanceCache.containsKey(readerClass)) { try { BeanDefinitionReader readerInstance = (BeanDefinitionReader) diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassMethod.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassMethod.java index c1379e44963..c6b96ce7e05 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassMethod.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassMethod.java @@ -16,6 +16,8 @@ package org.springframework.context.annotation; +import static java.lang.String.format; + import org.springframework.beans.factory.parsing.Location; import org.springframework.beans.factory.parsing.Problem; import org.springframework.beans.factory.parsing.ProblemReporter; @@ -35,32 +37,37 @@ final class ConfigurationClassMethod { private final MethodMetadata metadata; - private final ConfigurationClass declaringClass; + private final ConfigurationClass configurationClass; - public ConfigurationClassMethod(MethodMetadata metadata, ConfigurationClass declaringClass) { + public ConfigurationClassMethod(MethodMetadata metadata, ConfigurationClass configurationClass) { this.metadata = metadata; - this.declaringClass = declaringClass; + this.configurationClass = configurationClass; } - public MethodMetadata getMetadata() { return this.metadata; } - public ConfigurationClass getDeclaringClass() { - return this.declaringClass; + public ConfigurationClass getConfigurationClass() { + return this.configurationClass; } public Location getResourceLocation() { - return new Location(this.declaringClass.getResource(), metadata); + return new Location(this.configurationClass.getResource(), metadata); } public void validate(ProblemReporter problemReporter) { - if (this.declaringClass.getMetadata().isAnnotated(Configuration.class.getName()) && !getMetadata().isOverridable()) { + if (this.configurationClass.getMetadata().isAnnotated(Configuration.class.getName()) && !getMetadata().isOverridable()) { problemReporter.error(new NonOverridableMethodError()); } } + + @Override + public String toString() { + return format("[%s:name=%s,declaringClass=%s]", + this.getClass().getSimpleName(), this.getMetadata().getMethodName(), this.getMetadata().getDeclaringClassName()); + } /** @@ -74,4 +81,5 @@ final class ConfigurationClassMethod { } } + } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java index 5cdfade8394..6b3771d59e6 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java +++ b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java @@ -131,7 +131,7 @@ class ConfigurationClassParser { } if (metadata.isAnnotated(ImportResource.class.getName())) { String[] resources = (String[]) metadata.getAnnotationAttributes(ImportResource.class.getName()).get("value"); - Class readerClass = (Class) metadata.getAnnotationAttributes(ImportResource.class.getName()).get("reader"); + Class readerClass = (Class) metadata.getAnnotationAttributes(ImportResource.class.getName()).get("reader"); if (readerClass == null) { throw new IllegalStateException("No reader class associated with imported resources: " + StringUtils.arrayToCommaDelimitedString(resources)); @@ -140,8 +140,8 @@ class ConfigurationClassParser { configClass.addImportedResource(resource, readerClass); } } - Set methods = metadata.getAnnotatedMethods(Bean.class.getName()); - for (MethodMetadata methodMetadata : methods) { + Set beanMethods = metadata.getAnnotatedMethods(Bean.class.getName()); + for (MethodMetadata methodMetadata : beanMethods) { configClass.addMethod(new ConfigurationClassMethod(methodMetadata, configClass)); } } diff --git a/org.springframework.context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java b/org.springframework.context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java new file mode 100644 index 00000000000..be7e4e9ae4a --- /dev/null +++ b/org.springframework.context/src/test/java/org/springframework/context/annotation/BeanMethodPolymorphismTests.java @@ -0,0 +1,71 @@ +package org.springframework.context.annotation; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.junit.Test; +import org.springframework.beans.factory.parsing.BeanDefinitionParsingException; + +/** + * Tests regarding overloading and overriding of bean methods. + * Related to SPR-6618. + * + * Bean-annotated methods should be able to be overridden, just as any regular + * method. This is straightforward. + * + * Bean-annotated methods should be able to be overloaded, though supporting this + * is more subtle. Essentially, it must be unambiguous to the container which bean + * method to call. A simple way to think about this is that no one Configuration + * class may declare two bean methods with the same name. In the case of inheritance, + * the most specific subclass bean method will always be the one that is invoked. + * + * @author Chris Beams + */ +public class BeanMethodPolymorphismTests { + + @Test + public void beanMethodOverloadingWithoutInheritance() { + @SuppressWarnings("unused") + @Configuration class Config { + @Bean String aString() { return "na"; } + @Bean String aString(Integer dependency) { return "na"; } + } + try { + new AnnotationConfigApplicationContext(Config.class); + fail("expected bean method overloading exception"); + } catch (BeanDefinitionParsingException ex) { + assertTrue(ex.getMessage(), ex.getMessage().contains("2 overloaded @Bean methods named 'aString'")); + } + } + + @Test + public void beanMethodOverloadingWithInheritance() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(SubConfig.class); + assertThat(ctx.getBean(String.class), equalTo("overloaded5")); + } + static @Configuration class SuperConfig { + @Bean String aString() { return "super"; } + } + static @Configuration class SubConfig { + @Bean Integer anInt() { return 5; } + @Bean String aString(Integer dependency) { return "overloaded"+dependency; } + } + + /** + * When inheritance is not involved, it is still possible to override a bean method from + * the container's point of view. This is not strictly 'overloading' of a method per se, + * so it's referred to here as 'shadowing' to distinguish the difference. + */ + @Test + public void beanMethodShadowing() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ShadowConfig.class); + assertThat(ctx.getBean(String.class), equalTo("shadow")); + } + @Import(SubConfig.class) + static @Configuration class ShadowConfig { + @Bean String aString() { return "shadow"; } + } + +} diff --git a/org.springframework.core/src/main/java/org/springframework/core/type/MethodMetadata.java b/org.springframework.core/src/main/java/org/springframework/core/type/MethodMetadata.java index ae32ce6e457..b9417e8873d 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/type/MethodMetadata.java +++ b/org.springframework.core/src/main/java/org/springframework/core/type/MethodMetadata.java @@ -24,6 +24,7 @@ import java.util.Map; * * @author Juergen Hoeller * @author Mark Pollack + * @author Chris Beams * @since 3.0 * @see StandardMethodMetadata * @see AnnotationMetadata#getAnnotatedMethods @@ -35,6 +36,11 @@ public interface MethodMetadata { */ String getMethodName(); + /** + * Return the fully-qualified name of the class that declares this method. + */ + public String getDeclaringClassName(); + /** * Return whether the underlying method is declared as 'static'. */ diff --git a/org.springframework.core/src/main/java/org/springframework/core/type/StandardMethodMetadata.java b/org.springframework.core/src/main/java/org/springframework/core/type/StandardMethodMetadata.java index 9aaf20af72a..c6c8f28f80e 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/type/StandardMethodMetadata.java +++ b/org.springframework.core/src/main/java/org/springframework/core/type/StandardMethodMetadata.java @@ -30,6 +30,7 @@ import org.springframework.util.Assert; * * @author Juergen Hoeller * @author Mark Pollack + * @author Chris Beams * @since 3.0 */ public class StandardMethodMetadata implements MethodMetadata { @@ -53,10 +54,14 @@ public class StandardMethodMetadata implements MethodMetadata { return this.introspectedMethod; } - + public String getMethodName() { return this.introspectedMethod.getName(); } + + public String getDeclaringClassName() { + return this.introspectedMethod.getDeclaringClass().getName(); + } public boolean isStatic() { return Modifier.isStatic(this.introspectedMethod.getModifiers()); diff --git a/org.springframework.core/src/main/java/org/springframework/core/type/classreading/AnnotationMetadataReadingVisitor.java b/org.springframework.core/src/main/java/org/springframework/core/type/classreading/AnnotationMetadataReadingVisitor.java index 2b59804c321..77c304ad520 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/type/classreading/AnnotationMetadataReadingVisitor.java +++ b/org.springframework.core/src/main/java/org/springframework/core/type/classreading/AnnotationMetadataReadingVisitor.java @@ -57,7 +57,7 @@ final class AnnotationMetadataReadingVisitor extends ClassMetadataReadingVisitor @Override public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { - MethodMetadataReadingVisitor mm = new MethodMetadataReadingVisitor(name, access, this.classLoader); + MethodMetadataReadingVisitor mm = new MethodMetadataReadingVisitor(name, access, this.getClassName(), this.classLoader); this.methodMetadataSet.add(mm); return mm; } diff --git a/org.springframework.core/src/main/java/org/springframework/core/type/classreading/MethodMetadataReadingVisitor.java b/org.springframework.core/src/main/java/org/springframework/core/type/classreading/MethodMetadataReadingVisitor.java index e311508b78a..4852ab7e7cd 100644 --- a/org.springframework.core/src/main/java/org/springframework/core/type/classreading/MethodMetadataReadingVisitor.java +++ b/org.springframework.core/src/main/java/org/springframework/core/type/classreading/MethodMetadataReadingVisitor.java @@ -41,15 +41,18 @@ final class MethodMetadataReadingVisitor extends MethodAdapter implements Method private final int access; + private String declaringClassName; + private final ClassLoader classLoader; private final Map> attributeMap = new LinkedHashMap>(); - public MethodMetadataReadingVisitor(String name, int access, ClassLoader classLoader) { + public MethodMetadataReadingVisitor(String name, int access, String declaringClassName, ClassLoader classLoader) { super(new EmptyVisitor()); this.name = name; this.access = access; + this.declaringClassName = declaringClassName; this.classLoader = classLoader; } @@ -85,4 +88,8 @@ final class MethodMetadataReadingVisitor extends MethodAdapter implements Method return this.attributeMap.get(annotationType); } + public String getDeclaringClassName() { + return this.declaringClassName; + } + }