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; + } + }