Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify @Bean return type recommendation in case of multiple interfaces [SPR-15973] #20524

Closed
spring-issuemaster opened this issue Sep 18, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Sep 18, 2017

Michal Domagala opened SPR-15973 and commented

I discovered than bean graph may depend on order of bean declaration. I always thought that assembly algorithm analyze injections and determine order of beans creation.

However, example below shows that bean graph depend on order of declaration.

public static void main(String[] args) {
		SpringApplication.run(MyApp.class, args);
	}
	
	public static interface A {}
	
	public static interface B {}
	
	public static interface C {}
	
	public static class ABC implements A,B,C {}
	
	@Bean Object checkB(@Autowired(required = false) B b) {
		System.out.println("B is " + b);
		return new Object();
	}
	
	@Bean A a() {
		System.out.println("abc");
		return new ABC();
	}
	
	@Bean Object checkC(@Autowired(required = false) C c) {
		System.out.println("C is " + c);
		return new Object();
	}

Actual:

B is null
abc
C is com.sixdegreeshq.MyApp$ABC@17f7cd29

Expected:
Rather

B is null
abc
C is null

but without a doubt B and C should be symmetric


Affects: 4.3.11

Referenced from: commits ffe80ff, 963dd3f

Backported to: 4.3.12

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2017

Juergen Hoeller commented

I'm afraid this is a known limitation in type prediction: Since your a() method only declares A as a return type, the container's type prediction doesn't know about any other implemented type in advance (as long as that bean hasn't been created and the actual instance type isn't known yet). We generally recommend the strongest return type possible for @Bean methods, i.e. ABC in your case, for consistent type matching results. This should also produce a consistent outcome in your scenario.

Be aware: Generally speaking, declaration order does matter. The container needs to start creating beans somewhere, and it does so in registration (in this case: declaration) order. Once a singleton-scoped bean has been created, its full instance type is taken into account, so you may get more matches than against its predicted type. BTW you can enforce a certain creation order by using @DependsOn, e.g. @DependsOn("a") on your checkB method which enforces the availability of "a"'s instance upfront... but I'd really recommend declaring the most specific return type on a() instead.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2017

Michal Domagala commented

Clear.
I didn't know the recommendation "strongest return type possible", it is against typical pattern that return type should be as much general as possible.

Moreover, respectable internet pages seems be not familiar with the recommendation, for example

https://docs.spring.io/spring/docs/current/spring-framework-reference/htmlsingle/#beans-java-declaring-a-bean :)

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 19, 2017

Juergen Hoeller commented

Good catch: More specifically, the return type should be as strong as needed for type matching purposes (which it supposedly is for that example in the reference docs). It doesn't have to be the full impl type, rather just expose all relevant interfaces that some other component might use to refer to it. As a simple rule, it doesn't hurt to expose the full impl type. You're not the first to bump into this, though...

I'll repurpose this issue towards a strong and clear note about declared return types in the reference docs. Might even make sense to add such a note to several places there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.