Skip to content

AuthenticationConfiguration respects primary beans #6035

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

Merged
merged 1 commit into from
Nov 14, 2018
Merged

AuthenticationConfiguration respects primary beans #6035

merged 1 commit into from
Nov 14, 2018

Conversation

kagof
Copy link
Contributor

@kagof kagof commented Nov 1, 2018

Resolves #3912 .

I know that @shazin created #4136 as well, however it is now nearly two years old and full of conflicts.

Assert.isTrue(primaryBeanNames.size() == 1, () -> "Found " + primaryBeanNames.size()
+ " beans for type " + interfaceName + " marked as primary");
beanName = primaryBeanNames.get(0);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails if the result is 0 bean names

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is checked at line 152 - if there are 0 bean names, we short circuit and return null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you are correct.

lazyTargetSource.setTargetBeanName(beanNamesForType[0]);
String beanName;
if (beanNamesForType.length > 1) {
List<String> primaryBeanNames = Arrays.stream(beanNamesForType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be able to be simplified to something like:

applicationContext.getAutowireCapableBeanFactory().resolveNamedBean(Foo.class);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rwinch, would that replace whole BeanFactoryUtils.beanNamesForTypeIncludingAncestors(applicationContext, interfaceName); part? If I understand correctly, since the implementation of resolveNamedBean handles primary, this method could just become something along the lines of

LazyInitTargetSource lazyTargetSource = new LazyInitTargetSource();
String beanName;
try {
	beanName = applicationContext.getAutowireCapableBeanFactory().resolveNamedBean(interfaceName).getBeanName();
} catch (NoSuchBeanDefinitionException ex) {
	return null;
}
lazyTargetSource.setTargetBeanName(beanName);
lazyTargetSource.setBeanFactory(applicationContext);
ProxyFactoryBean proxyFactory = new ProxyFactoryBean();
proxyFactory = objectPostProcessor.postProcess(proxyFactory);
proxyFactory.setTargetSource(lazyTargetSource);
return (T) proxyFactory.getObject();

However, is this method still lazy if we do that? It looks to me like resolveNamedBean will init an instance of the bean.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out. I missed that it did create the bean too.

Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Please see my inline comments for requested changes.

@rwinch rwinch merged commit db5e542 into spring-projects:master Nov 14, 2018
@rwinch rwinch self-assigned this Nov 14, 2018
@rwinch rwinch changed the title #3912 lazyBean method respects @Primary annotation AuthenticationConfiguration respects primary beans Nov 14, 2018
@rwinch rwinch added in: config An issue in spring-security-config type: enhancement A general enhancement labels Nov 14, 2018
@rwinch rwinch added this to the 5.2.0.M1 milestone Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants