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

Revisit @Configuration(proxyBeanMethods = false) with qualified injection points #23887

Closed
snicoll opened this issue Oct 29, 2019 · 1 comment
Assignees
Labels
type: regression A bug that is also a regression
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Oct 29, 2019

This relates to #23839

We've switched DelegatingWebMvcConfiguration to disable method proxying and there are a number of side effects that aren't currently covered.

requestMappingHandlerAdapter takes a mvcContentNegotiationManager, mvcConversionService and mvcValidator. These are "qualified" in the aim of calling the related method on the same class.

The problem with this approach is that if any of those types are already exposed with a @Primary bean, the context will not even attempt to look for a candidate (and therefore will not invoke the dedicated bean factory method if necessary).

A concrete illustration of this problem for mvcConverter can be found in spring-projects/spring-boot#18672. A fix is to add a @Qualifier to teach the context the qualified bean is actually required and the context should attempt to look it up.

This seems to be working as expected (although with more metadata than it should be ideally) but given that no tests broke, I can only assume this use case isn't covered by tests at the moment. The purpose of this issue is to revisit this support, add missing tests and eventually change our mind with regards to this decision.

@snicoll snicoll added the type: regression A bug that is also a regression label Oct 29, 2019
@snicoll snicoll added this to the 5.2.1 milestone Oct 29, 2019
@snicoll snicoll self-assigned this Oct 29, 2019
@snicoll snicoll changed the title MVC configuration may not use expected infrastructure Revisit @Configuration(proxyBeanMethods = false) with qualified injection points Oct 30, 2019
@snicoll
Copy link
Member Author

snicoll commented Oct 30, 2019

This turned out to be a larger affair. Checking our use of @Configuration(proxyBeanMethods = false) when the injection point expect a qualified injection point. Production code seems rather limited at the moment:

  • ProxyTransactionManagementConfiguration
  • DelegatingWebMvcConfiguration
  • DelegatingWebFluxConfiguration

snicoll added a commit to spring-projects/spring-boot that referenced this issue Oct 30, 2019
This commit is a follow-up of a change in Spring Framework[1] to make
sure injection points that are expecting a specific bean by name use
a qualifier.

As a result of this change, MVC uses the dedicated MVC validator again
rather than the general one auto-configured by Spring Boot.

[1] spring-projects/spring-framework#23887

Closes gh-18672
pull bot pushed a commit to scope-demo/spring-framework that referenced this issue Oct 30, 2019
Previously, the infrastructure provided by WebMvcConfigurationSupport
and WebFluxConfigurationSupport can lead to unexpected results due to
the lack of qualifier for certain dependencies. Those configuration
classes refer to very specific beans, yet their injection points do not
define such qualifiers. As a result, if a candidate exists for the
requested type, the context will inject the existing bean and will
ignore a most specific one as such constraint it not defined. This can
be easily reproduced by having a primary Validator whereas a dedicated
"mvcValidator" is expected. Note that a parameter name is in no way a
constraint as the name is only used as a fallback when a single
candidate cannot be determined.

This commit provides explicit @qualifier metadata for such injection
points, renaming the parameter name in the process to clarify that it
isn't relevant for the proper bean to be resolved by the context.

Closes spring-projectsgh-23887
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

1 participant