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

Make use of bean definition overriding more visible #31288

Closed
snicoll opened this issue Sep 21, 2023 · 7 comments
Closed

Make use of bean definition overriding more visible #31288

snicoll opened this issue Sep 21, 2023 · 7 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Sep 21, 2023

With errors that are hard to track, and with XML bean definitions taking always precedence over Java config (which can be surprising), we'd like to deprecate bean overriding wholesale with the idea of removing the feature at some point.

Spring Boot has already paved the way by disabling it by default and introducing a property to opt-in to the previous behavior. Us deprecating the feature in the core framework should send a strong signal to developers that they should not rely on that feature anymore.

@snicoll snicoll added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Sep 21, 2023
@snicoll snicoll added this to the 6.x Backlog milestone Sep 21, 2023
@jhoeller jhoeller changed the title Deprecate bean overidding Deprecate bean overriding Sep 21, 2023
@dearoneesama
Copy link

It scares me off, but It looks like we are not talking about overriding @Bean-decorataed methods right?

@snicoll
Copy link
Member Author

snicoll commented Sep 22, 2023

registering twice a bean definition for the same key (i.e. bean name).

@mmoayyed
Copy link
Contributor

mmoayyed commented Jan 23, 2024

Us deprecating the feature in the core framework should send a strong signal to developers that they should not rely on that feature anymore.

Could you please elaborate on how one might go about doing the same sort of thing when the particular bean is not marked as conditional? For example, what is the right strategy for overriding this bean?

@Bean
public MessageSource messageSource(MessageSourceProperties properties) {}
  • Should we exclude the configuration class?
  • Should we examine the conditions that trigger the creation of this bean and rework the app to disable those conditions? (In this case, rename the language bundle?)
  • Could we start sending pull requests that mark such beans with a @ConditionalOnMissingBean and family?

@snicoll
Copy link
Member Author

snicoll commented Feb 12, 2024

So we're going to change this particular issue to log a warning that indicates why it is being logged and that a next release will make it more noisy. We will also update the reference guide to indicate that users should move away from that feature and how. Once that's done we will advocate the Spring Boot property to be deprecated.

Deprecation of the feature proper is now scheduled for Spring Framework 7 to give enough room for users to notice and plan accordingly.

@mmoayyed If you want to override it in your configuration, it won't be possible and the mitigation will happen on a case by case basis. For tests, we are working to give users a first class support for overriding specific beans.

@snicoll snicoll changed the title Deprecate bean definition overriding Log a warning when bean definition overriding is used Feb 12, 2024
@snicoll
Copy link
Member Author

snicoll commented Mar 11, 2024

We probably need to rephrase the note in https://docs.spring.io/spring-framework/reference/6.2-SNAPSHOT/testing/annotations/integration-spring/annotation-beanoverriding.html#spring-testing-annotation-beanoverriding-mockitobean.

In the meantime support for bean overriding in tests has landed in 6.2.0-SNAPSHOT. The doc above provides an overview if you want to give the snapshots a try.

@snicoll
Copy link
Member Author

snicoll commented Mar 26, 2024

Giving this a try on our own projects revealed quite a number of issues. If we pursue with this, we should probably introduce a flag that we can use to opt-in for an exception as the warning alone makes it harder to validate that things are working as expected.

@snicoll
Copy link
Member Author

snicoll commented Mar 27, 2024

This is where we are with my testing https://ge.spring.io/s/rx7ixhsvowv4m

@snicoll snicoll changed the title Log a warning when bean definition overriding is used Make use of bean definition overriding more visible Apr 2, 2024
snicoll added a commit to snicoll/spring-framework that referenced this issue Apr 2, 2024
This commit makes the use of bean definition overriding more visible and
prepare for a deprecation of the feature in the next major release.

As of this commit, use of bean definition overriding logs at INFO level.
The previous log level can be restored by setting the
allowBeanDefinitionOverriding flag explicitly on the BeanFactory (or
via the related ApplicationContext).

A number of tests that are using bean overriding on purpose have been
updated to set this flag, which will make them easier to find once we
actually deprecate the feature.

Closes spring-projectsgh-31288
@snicoll snicoll closed this as completed in 7a74e45 Apr 2, 2024
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

4 participants