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

Consistent non-exposure of null beans in the BeanFactory API [SPR-17034] #21572

Closed
spring-projects-issues opened this issue Jul 12, 2018 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jul 12, 2018

Brian Clozel opened SPR-17034 and commented

Currently the core container allows to create null beans like this:

@Bean
public HandlerMapping myHandlerMapping() {
  return null;
}

This can be quite useful to back off and consume resources if that bean is not required (or a no-op), given the application configuration.

A recent change in the Spring MVC infrastructure setup uncovered several issues, since in many places we're asking for those infrastructure beans without guarding against null values.

Given the previous myHandlerMapping bean declaration, asking BeanFactory for beans can then return null instances in several cases:

// the map will contain "myHandlerMapping", null
Map<String, HandlerMapping> matchingBeans =
					BeanFactoryUtils.beansOfTypeIncludingAncestors(context, HandlerMapping.class, true, false);

// this works
assertThat(context.getBean("resourceHandlerMapping")).isEqualTo(null);

/*
 * this throws org.springframework.beans.factory.BeanNotOfRequiredTypeException: 
 * Bean named 'resourceHandlerMapping' is expected to be of type 
 * 'org.springframework.web.servlet.HandlerMapping' but was actually of type 
 * 'org.springframework.beans.factory.support.NullBean'
*/
assertThat(context.getBean("resourceHandlerMapping", HandlerMapping.class)).isEqualTo(null);

Looking at the previous examples, we could improve the consistency of the behavior, or document a bit more the null case and what it means.


Affects: 5.0.7

Reference URL: a40d25a

Attachments:

Issue Links:

Referenced from: commits 77d72f1, 680afa7, def6fbb

0 votes, 6 watchers

@spring-projects-issues
Copy link
Collaborator Author

Joe Grandja commented

The Spring Security 5.1.0-BUILD-SNAPSHOT is currently failing as a result of this failing test.

The main issue is that a NPE is happening because there are null entries in the List of DispatcherServlet.handlerMappings.

In 5.0.7, this was handled with WebMvcConfigurationSupport.EmptyHandlerMapping instances but in 5.1.0-BUILD-SNAPSHOT WebMvcConfigurationSupport.EmptyHandlerMapping has been removed.

Attached are 2 debug screenshots to show this.

 

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

To elaborate on what Joe reported. There are enough tests failing in Spring Security that it is impractical for us to ignore them. This is making it difficult for us to accept changes into master since we require Spring Framework 5.1.0.BUILD-SNAPSHOT+. If a solution cannot be found soon, can we revert this code until a long term solution can be made? This will allow other projects' tests to pass until the long term solution is found.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

I've pushed a change that excludes null beans from ListableBeanFactory.getBeansOfType... which generally makes sense there next to 5.0's getBean behavior. This should fix the immediate HandlerMapping problem (as per a unit test at least) and is likely to be part of the eventual arrangement in any case.

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

Thanks Juergen Hoeller! This has fixed the Spring Security build

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Multi-element injection points (i.e. injected Map / Collection instances aggregating several target beans) do not include null bean entries anymore either now, consistent with the recent getBeansOfType revision.

However, such bean definitions do not disappear completely. They still show up on early type match checks (through isTypeMatch or getBeanNamesForType) and still evaluate late for non-singleton beans, potentially being null in one case but not the other.

For regular injection points that might not resolve uniquely due to an eventally-null bean definition, it is still recommended to use @Primary on the common target bean or to mark the null definition as autowireCandidate=false in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants