ConfigurationClass.validate() should allow for overloading in general or not at all [SPR-11025]

[spring-projects-issues]

Jose Luis Martin opened SPR-11025 and commented

When checking for overloaded methods, ConfigurationClass.validate()
don't take in account overloaded methods from superclasses, so a invalid configuration class with overloaded methods obtained by inheritance bypass the check.

---

Affects: 4.0 M3

Issue Links:

• JavaConfig Bean overriding with addition [SPR-10988] #15616 JavaConfig Bean overriding with addition
• @Bean overriding does not pick up metadata from most specific method [SPR-10992] #15620 @Bean overriding does not pick up metadata from most specific method
• Fix/optimize handling of @Bean method override regression with return type narrowing on JDK 8 [SPR-11718] #16340 Fix/optimize handling of @Bean method override regression with return type narrowing on JDK 8

Referenced from: commits 935bd25

[spring-projects-issues]

Jose Luis Martin commented

I have done a pull request to try to fix the issue.

[spring-projects-issues]

Phil Webb commented

In the process of merging this pull request I discovered BeanMethodPolymorphismTests with actually confirms that #15616 may have been a legitimate issue. I will add some more comments to that report.

[spring-projects-issues]

Jose Luis Martin commented

Hi Phil,

I think that the test BeanMethodPolymorphismTests is broken. It is not actually testing configuration classes overloaded methods as SubConfig doesn't extend SuperConfig
\

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

More in deep, I guess that inheritance in configuration class doesn't matter for overloaded methods. They are resolved as a plain class at all.

The method used to instantiate the bean is ConstructorResolver.instantiateUsingFactoryMethod() which only takes into account inheritance when:
\

• Arguments has the best match.
• Overloaded method in subclass has the same number of arguments.

So I still think that validate() should test for overloaded methods from superclasses (or never test)

I added both test cases to BeanMethodPolymorphismTests in chelu/SPR-11025/BeanMethodPolymorphismTests.java

[spring-projects-issues]

Phil Webb commented

Thanks Jose,

I did notice the error in the tests and fixed it in commit 78c10cd. I will add a comment to #15616 for your other points.

Cheers.

[spring-projects-issues]

Juergen Hoeller commented

I've removed that overloading check for 4.0 RC2 now. In combination with #15620, this means that we're generally supporting @Bean method overloads at any level in Spring 4.

The only problem is where to pick the bean definition metadata from. We need to do this very early, without knowing which factory method is going to be picked for a particular scenario - since this is a dynamic choice and highly dependent on the available dependencies at the time of first creation of an instance.

In an ideal world, we'd like to have exactly one source for each bean's metadata at any configuration class level. We could theoretically put that metadata at the type level, pointing to a factory method by name (similar to the XML bean definition scenario). However, that's not very practical.

So as of #15620, we're always picking the metadata from the most concrete subclass. However, with overloading in mind, there may be several sources of metadata (i.e. several same-named methods) in every subclass. We're technically picking the @Bean metadata from the first declared method of a given name in the subclass now; however, in practice, I would strongly recommend consistent metadata to be repeated for every factory method for the same bean in the same config subclass. We could perform aggressive validation for this but do not do it at this point due to the rather involved processing necessary. Also, it might be a feature to only have to express this once, with the other factory methods of the same name in the same source file implicitly receiving that metadata as well.

So, to be clear for the practical impact, a bean's definition metadata may be overridden in a config subclass along with an overridden or overloaded @Bean method for that bean. It's just within each config class level where the metadata is expected to be consistent across all overloaded methods.

Juergen

[spring-projects-issues]

Jose Luis Martin commented

Great!.
Thanks Phil and Juergen. I'll see you in the next issue :).