JavaConfig Bean overriding with addition [SPR-10988]

[spring-projects-issues]

Benoit Lacelle opened SPR-10988 and commented

I consider injection of beans as a List of automatically detected beans: I introduce several beans implementing the same interface and inject all of them as a List in a later bean.

I've not been able to find official documentation related to this feature. My single source is http://www.coderanch.com/t/605509/Spring/Java-config-autowired-List

Consider this feature, I have an issue with Bean overring: I would like to override a bean defined throught a no-arg method with a bean defined with the List of detected beans. However, spring behave like the second bean definition does not exist.

It can be reproduced with the following test:

import java.util.Date;
import java.util.List;

import org.junit.Assert;
import org.junit.Test;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

public class SpringTest {

	@Test
	public void shouldtestSpringDifferentMethodNames() {
		AnnotationConfigApplicationContext ctx2 = new AnnotationConfigApplicationContext(AConfig.class, CConfig.class);
		Assert.assertEquals("overriden", ctx2.getBean("bean"));
	}

	@Configuration
	public static class AConfig {

		@Bean
		public Object bean() {
			return "not overriden";
		}

	}

	@Configuration
	public static class CConfig extends AConfig {

		@Bean
		public Date anotherBean() {
			return new Date();
		}

		@Bean
		public Object bean(List<? extends Date> someDate) {
			return "overriden";
		}

	}

}

If this is an expected behavior, how can I achieve such an overriding?

I asked the same question on SOF: http://stackoverflow.com/questions/19377789/javaconfig-bean-overriding-failing-with-list-injected

Thanks

---

Affects: 3.2.3

Issue Links:

• ObjectProvider iterable/stream access for "beans of type" resolution in @Bean methods [SPR-11419] #16046 ObjectProvider iterable/stream access for "beans of type" resolution in @Bean methods
• ConfigurationClass.validate() should allow for overloading in general or not at all [SPR-11025] #15653 ConfigurationClass.validate() should allow for overloading in general or not at all

Referenced from: commits b093b84, b00c31a, 78c10cd

0 votes, 5 watchers

[spring-projects-issues]

Phil Webb commented

I am not really sure what you are asking here. Could you try to explain in a little more detail. Overriding @Bean methods on @Configuration classes is somewhat unusual and it is not clear to me what you are trying to achieve by doing that.

[spring-projects-issues]

Benoit Lacelle commented

Considering a library providing some @Configuration classes, I need to override one of these beans. The new bean has a constructor dependency, which is the list of beans of a given type. So the bean definitions changed from

@Bean
public Number bean() {
    return new Integer(3);
}

to

@Bean
public Number bean(List<? extends Date> someDate) {
    return new Double(someDate.size());
}

By "Overriding @Bean methods on @Configuration classes is somewhat unusual", do you mean there is an alternative way which is more common? I'm used to XML configuration and I'm trying to achieve the equivalent of bean overriding by re-using the name of an existing bean.

I get the expected behavior if CCOnfig does not extends AConfig. Would this be a moreusual way of overriding? I tend to extends @Configuration classes for overridings as, in some cases, the overriding is enforced through an @Override annotation.

[spring-projects-issues]

Jose Luis Martin commented

I guess that problem is in ConfigurationClass.validate() method.

The Configuration class obtained by inheritance (CConfig) has two overloaded methods, bean() and bean(List date). So validate() should report a BeanMethodOverloadingProblem and throw a BeanDefinitionParsingException.

I tried to fix it replacing the code that checks for overloaded methods in validate() and seems to work, but MethodMetadata interface don't give access to the introspected method and there are dirty access to it casting to StandardMethodMetadata class.

// An @Bean method may only be overloaded through inheritance. No single
// @Configuration class may declare two @Bean methods with the same name.
Map<String, List<BeanMethod>> methods = new HashMap<String, List<BeanMethod>>();
for (BeanMethod beanMethod : this.beanMethods) {
	String methodName = beanMethod.getMetadata().getMethodName();
	List<BeanMethod> methodList;
	if (!methods.containsKey(methodName)) {
		methodList = new ArrayList<BeanMethod>();
		methods.put(methodName, methodList);
	}
	else {
		methodList = methods.get(methodName);
	}
	methodList.add(beanMethod);
}

for (String methodName : methods.keySet()) {
	List<BeanMethod> methodList = methods.get(methodName);
	if (methodList.size() > 1) {
		// Check for overloading
		Class<?>[] firstParameters =
				((StandardMethodMetadata) methodList.get(0).getMetadata())
				.getIntrospectedMethod().getParameterTypes();
			for (BeanMethod method : methodList) {
			Class<?>[] methodParameters = ((StandardMethodMetadata) method.getMetadata())
					.getIntrospectedMethod().getParameterTypes();
				if (!Arrays.equals(firstParameters, methodParameters)) {
				// Overloaded
				problemReporter.error(new BeanMethodOverloadingProblem(methodName, methodList.size()));
			}
		}
	}
}

Full commit is in chelu@c4e6895

[spring-projects-issues]

Phil Webb commented

Benoit Lacelle,

Thanks for the information. That makes much more sense now. When I said:

Overriding @Bean methods on @Configuration classes is somewhat unusual

I was thinking in the context of a single application, as you are talking about reuse of a common configuration provided by a shared library I can see why you would do this. In fact we do this ourselves with classes such as WebMvcConfigurationSupport.

As Jose Luis Martin points out, the restriction on overloaded methods is intentional.

One option that might work for you is to leave the @Bean method as-is and autowire the detected beans to a private member:

@Configuration
public static class CConfig extends AConfig {

	@Autowired
	private List<Date> dateBeans;

	@Bean
	public Date anotherBean() {
		return new Date();
	}

	@Bean
	public Object bean() {
		// can access this.dateBeans here
		return "overriden";
	}
}

You could also make CConfig implement ApplicationContextAware then use the methods from BeanFactoryUtils to get what you need.

[spring-projects-issues]

Benoit Lacelle commented

Thanks for the details Phil.

I'm not sure to understand one point: is it legit for a @Configuration to override a bean from a parent @Configuration? I understand it is legit given WebMvcConfigurationSupport but not legit as "the restriction on overloaded methods is intentional". Is it legit only in some cases?

Anyway, if the override describe in this ticket is not legit, a fail-fast fix like the one from Jose Luis Martin would be much helpful.

[spring-projects-issues]

Phil Webb commented

It is legal to override @Bean methods but you cannot overload them. So you can override the method, but not create a new version with additional arguments.

[spring-projects-issues]

Jose Luis Martin commented

Hi Phil

I agree that work as designed but ConfigurationClass.validate() still is accepting a invalid configuration.

As this bug is now Resolved, should I report a new bug for that?

[spring-projects-issues]

Phil Webb commented

Jose Luis Martin, I think this one is getting a little cluttered so yes please, a new bug would be appreciated.

[spring-projects-issues]

Benoit Lacelle commented

Thanks for your help.

I was convinced bean overriding through method overloading was legal. Fact is the following overload works:

public class SpringTest {

	@Test
	public void shouldtestSpringDifferentMethodNames() {
		AnnotationConfigApplicationContext ctx2 = new AnnotationConfigApplicationContext(AConfig.class, CConfig.class);
		Assert.assertEquals("overriden", ctx2.getBean("bean"));
	}

	@Configuration
	public static class AConfig {

		@Bean
		public Object bean() {
			return "not overriden";
		}

	}

	@Configuration
	public static class CConfig extends AConfig {

		@Bean
		public Date anotherBean() {
			return new Date();
		}

		@Bean
		public Object bean(Date someDate) {
			return "overriden";
		}

	}

}

This is why I was confused about which bean overridings are legal and which ones are not.

Jose,
I believe your fix will make fail the test described in this comment. Then, I tend to believe it shall break existing configurations.

Should we take in account @Bean(override=true) to allow bean overloading?

[spring-projects-issues]

Phil Webb commented

On further investigation is appears that overloaded bean methods are intended to work the way that you describe. BeanMethodPolymorphismTests shows an example. It appears that your specific issue relates to the fact that you are injecting a List of beans, as opposed to a single item.

It seems that the real problem may lie in the ConstructorResolver class (and specifically the getTypeDifferenceWeight method) which is weighing the no-arg method above the version that accepts a List.

[spring-projects-issues]

Benoit Lacelle commented

Great Phil. Would you consider porting the fix in the 3.2 branch?

[spring-projects-issues]

Phil Webb commented

Unfortunately I don't yet have fix. I am going to assign this one to Juergen Hoeller to take a look at.

[spring-projects-issues]

Jose Luis Martin commented

I see... though I never would expect that a class will work differently depending on whether it was obtained through inheritance or not. (!)

The List argument is not the only problem. As methods are ordered using AutowireUtils.sortConstructors(), overloading a bean method with a less arguments one will fail too.

I think that this issue is related to #15620 because overloaded bean definitions are overriden by superclass too.

[spring-projects-issues]

Jose Luis Martin commented

I'm a bit confused here because overloaded/overwriten methods are supposed to be supported by configuration classes but I have not found any code that handle it in the framework.

I have done a new pull request trying add support for both features following the indications of javadoc of BeanMethodPolymorphismTests, ie choosing the most specifc method in the hierarchy.

Instead of dealing with ConstructorResolver, I chose to assign the factory method to use at parsing time.

The pull request has merged the changes that I proposed to #15620 because overloaded bean definitions are overriden by superclasses too.

[spring-projects-issues]

Phil Webb commented

See also this comment from #15653.

[spring-projects-issues]

Juergen Hoeller commented

As of 4.0 RC2, we use Spring's non-lenient constructor resolution mode for @Bean methods now. To be backported to 3.2.5 as well.

Since @Bean methods are never used with externally specified constructor argument values but rather just with autowiring, the non-lenient constructor resolution mode is appropriate in case of an overloaded @Bean method, not performing any type difference weight checks. This change includes a refinement of Spring's existing non-lenient constructor resolution (which needs to be explicitly turned on and is therefore not well tested), narrowing the conditions for the ambiguity check (only in case of the same number of arguments and not for overridden methods).

Juergen

[spring-projects-issues]

Benoit Lacelle commented

Thanks Juergen!

[spring-projects-issues]

Jose Luis Martin commented

Hi Juergen,

I think that the fix is still ignoring inheritance rule pointed by javadoc in BeanMethodPolymorphismTests

In the case of inheritance, the most specific subclass bean method will always be the one that is invoked.

The non-lenient constructor resolution mode will fail or pick the wrong method when overloading a bean method with the same or more number of arguments (already pointed here).

For example,
\

@Configuration
	static class SuperConfig {
		
		@Bean Long aLong() {
			return 4l;
		}
		
		@Bean Date aDate() {
			return new Date();
		}

		@Bean
		String aString(Long l, Date date) {
			return "super";
		}
	}


	@Configuration
	static class SubConfig extends SuperConfig {

		@Bean
		Integer anInt() {
			return 5;
		}

		@Bean
		String aString(Integer dependency) {
			return "overloaded" + dependency;
		}
	}

will pick SuperConfig.aString().

Please, consider if the two test cases added to chelu/../BeanMethodPolymorphismTest should fail or not.

[spring-projects-issues]

Juergen Hoeller commented

In my opinion, neither of the two new test cases should pass. Fundamentally, what we're designing here are regular Java classes, just with a special kind of factory method semantic. Overriding and overloading should work as usual, with the inheritance position of a newly overloaded method not being significant. In other words, except for the Java language override case where obviously the subclass position of a method is significant, it shouldn't matter whether a method is placed on a superclass or a subclass. A method invocation from a Java source file picks the best parameter type fit in an overloading scenario, no matter where the method is defined in the inheritance hierarchy; that's exactly what we're doing here as well.

Note that this is substantially different from the scenario where a separate configuration class is being registered, overriding a formerly registered bean definition. In that case, the parameter signature of the factory methods involved is not significant since we're not within a Java class hierarchy. We are rather defining an overriding order for several indiviudally registered configuration classes, just like in the case of several XML bean definition files containing a bean with the same name.

So a split into separate configuration classes without inheritance may be one way out for a scenario like yours. Another one would be a superclass method signature that accepts all potentially needed arguments, with the superclass variant itself not making full use of it. Or, as suggested above, access to certain dependencies via @Autowired fields on the configuration class, not affecting the factory method signature. Finally, the instances returned from a factory method may also use dependency injection themselves, that is, have @Autowired fields or methods to be processed after the factory method returns - again, not involving the factory method signature in the first place.

One more note: This is about as far as we can go for 3.2.x in any case. Any far-reaching changes to configuration class semantics would have to be a 4.x feature only. However, as noted above, I do not actually believe that we need to bend those semantics any further; the closeness to regular Java method processing is a key feature in its own right.

Juergen

[spring-projects-issues]

Jose Luis Martin commented

Hi Juergen.

Thanks for your attention.

Overriding and overloading should work as usual, with the inheritance position of a newly overloaded method not being significant.

Wonderful!, That was my first point of view, but note that the framework is not actually following it. (see my comments on #15653).

Although I totally agree with your explanation above, I'm still not sure that current fix solve the problem because to be consistent, for overloaded methods, the bean definition registered at parsing time and the factory method picked at instantiation time should match. (Please, see #15620).

[spring-projects-issues]

Juergen Hoeller commented

Indeed, that validation step is inaccurate then... It reflects a sort of half-baked guidance that we might be better off just removing.

As for the bean definition metadata, indeed, that's not necessarily picked from the actual method used at runtime. I'm going to review what we can do there. We could also insist on all same-named @Bean methods to define the same metadata, and throw a validation exception otherwise.

This work will probably only happen for 4.0, though. With 3.2.5, we're really just trying to fix the obvious - like a subclass @Bean method not being picked, which is even wrong in the old model - and not go much further than that.

Juergen