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

Avoid calling other bean methods in web config #22596

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

bclozel
Copy link
Member

@bclozel bclozel commented Mar 14, 2019

This commit changes the main configuration classes for Spring MVC and
Spring WebFlux to not call other bean methods when setting up the web
infrastructure. This allows the DelegatingWebFluxConfiguration and
DelegatingWebMvcConfiguration to opt-in the lite-mode, as introduced
in gh-22461.

@bclozel bclozel requested review from jhoeller and rstoyanchev March 14, 2019 17:59
@bclozel
Copy link
Member Author

bclozel commented Mar 14, 2019

In this PR, I'm trying to check what it would take to leverage lite-mode in the web configurations.
This changes a few public methods, but arguably those are @Bean methods and not meant to be called directly. This shows the exact dependencies required by each bean, which was not necessarily obvious in the previous version.

@jhoeller , @rstoyanchev I'd like to get your opinion on that.
Thanks!

@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Mar 14, 2019
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

The configuration support classes are documented as the "advanced option" for configuring Spring MVC or Spring WebFlux, so this will disrupt some applications using or overriding the protected methods. Fortunately those are few.

What I'm more concerned is that proxyBeanMethods is false by default. Applications extending these classes for advanced config would need to be careful not to call bean methods directly which is easy to overlook, and would be surprising. Classes may even run into that by simply upgrading to 5.2 if they already do that. It could also be an optimization that's not actually needed.

Would it make more sense to go with proxied bean methods by default and only set it to false when you're ready and willing to opt in. Boot can certainly turn off the proxying since it is in control of this configuration and guarantee it's done right.

@bclozel
Copy link
Member Author

bclozel commented Mar 20, 2019

@rstoyanchev you're right - I've changed my PR accordingly. We can still add that proxyBeanMethods attribute in Spring Boot.

@sdeleuze
Copy link
Contributor

👍 for that change!

@bclozel bclozel self-assigned this Apr 2, 2019
@bclozel bclozel added this to the 5.2 M1 milestone Apr 2, 2019
This commit changes the main configuration classes for Spring MVC and
Spring WebFlux to not call other bean methods when setting up the web
infrastructure. This allows configuration classes extending
`DelegatingWebFluxConfiguration` and `DelegatingWebMvcConfiguration`
to opt-in the lite-mode, as introduced in spring-projectsgh-22461.
@bclozel bclozel force-pushed the proxyBeanMethods branch from ee0b04f to 47c8d1d Compare April 3, 2019 08:17
@bclozel bclozel merged commit 47c8d1d into spring-projects:master Apr 3, 2019
@bclozel bclozel deleted the proxyBeanMethods branch April 3, 2019 09:26
bclozel added a commit to spring-projects/spring-boot that referenced this pull request Apr 3, 2019
This commit applies changes similar to what's been done in gh-9068, for
MVC and WebFlux configurations. This is now possible thanks to the
changes done in Spring Framework in
spring-projects/spring-framework#22596

Fixes gh-16427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants