Skip to content

Consider providing recommendation for override/replace beans usecase #18228

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

Closed
ttddyy opened this issue Sep 13, 2019 · 7 comments
Closed

Consider providing recommendation for override/replace beans usecase #18228

ttddyy opened this issue Sep 13, 2019 · 7 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@ttddyy
Copy link
Contributor

ttddyy commented Sep 13, 2019

Since spring-boot 2.1, bean overriding is disabled by default.
I think this is very reasonable and protects from unknowns by preventing hidden bean overriding.

However, time to time, there is a need to override beans.
It would be great to have the best practice/guideline for handling such use cases.

Background:

I write a library which is used by multiple service applications. The library is written on top of spring-boot and provides @Enable... annotations.
Typically we create @Enable... annotations rather than creating auto configurations.
This is a decision that we want to explicitly turn on/off features in service rather than auto-magically happen via auto configuration.

The issue is that sometimes services want to override some beans defined in library.
If we provide auto configuration, we can use @ConditionalOn... annotations since it is guaranteed to be evaluated after all @Configuration classes are processed by AutoConfigurationImportSelector.

However, since we use @EnableX, there is no distinction for @Configuration classes whether they are from library's or from application's while processing configuration classes.
Therefore, due to the component scan is not deterministic to process configuration classes, we cannot reliably use @ConditionalOnX annotations because the configuration class processing order might change.

One approach is I can write a custom import-selector that makes sure all library configurations are processed after application's configurations but before auto configurations. This could allow library configurations to use @ConditionalOn annotations. However, it is a bit of too much in custom framework and even writing a custom import-selector, we also need to @EnableAutoConfiguration equivalent to bring in the custom import-selector to be in effect on application.

We also want to avoid using @Primary defined in application configuration in order to override beans in library. This is because @Primary bean may have impact writing tests in application.

Instead, an approach I chose was using BeanFactoryPostProcessor and unregister bean definitions that I want to override.

public class UnregisterBeanDefinitionBeanFactoryPostProcessor implements BeanFactoryPostProcessor, Ordered {
  private boolean lenient = false;
  private List<String> beanNames = new ArrayList<>();  // bean names to unregister

  @Override
  public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException {
    BeanDefinitionRegistry registry = (BeanDefinitionRegistry) beanFactory;
    for (String beanName : this.beanNames) {
        try {
            registry.removeBeanDefinition(beanName);
        } catch (NoSuchBeanDefinitionException e) {
            if (!this.lenient) {
                throw e;
            }
        }
    }
  }

  // ...
}

This is so far working good(with limited usage), since the post processor is called after all configurations are processed(including auto configuration) but before instantiating beans. The drawback is bean-name based injection will have a problem but typically beans are injected by type.

I have seen people use @ConditionalOn[Bean|Class] on non autoconfiguration classes(with component scan). Mostly, people don't realize the fragility of @ConditionalOnX annotations.
Simply, it is working in their current environment, so that they think it is ok to use.

From library/framework development perspective, one suggestion is that spring-boot to provide a mechanism to guarantee certain configuration classes to be processed after normal configurations but before auto configurations. For example, provide a means to list up such configuration classes in spring.factories with certain key(which is pretty similar to what autoconfiguration do), then defer the configuration processing right before autoconfigurations.

Another way would be making allow-bean-definition-overriding flag to be per bean definition. Currently, this is a beanFactory level switch. However, if it is down to per bean definition level, it can fine control what beans to be overridable. (maybe too much granularity)

It's my feedback about bean overriding usecase.

It is common that people create own framework/library on top of spring/spring-boot in order to share their architecture/infrastructure to applications. So it is nice to have some level of support in spring-boot for developing library on top.

Also, it would be great if you could provide some clear guideline for bean override dos and donts, blog post, documentation, some support in framework(spring or spring-boot), etc.

Thanks,

References:
spring-projects/spring-framework#15434
spring-projects/spring-framework#19229

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 13, 2019
@philwebb
Copy link
Member

I'm not sure we have clear guidelines ourselves when it comes to these more complex scenarios. Generally speaking @Enable... annotations are part of user configuration and so are always processed before Spring Boot auto-configuration. Mostly if you register a bean in an @Enable... annotation, our auto-configuration should back off.

Without knowing the details of your specific use-case, it's very hard to offer general advice. It also feels like something that's quite unusual for a typical Spring Boot user to do (so we don't want to add a section to the reference documentation).

We've tried to document that @Conditional... annotations should only be used with auto-configuration classes, but unfortunately some users still use them incorrectly.

Your BeanFactoryPostProcessor seems like a logical approach, but I'm a little confused why you'd need to replace bean definitions registered by another library. Perhaps you can share some code that shows concretely what you are trying to do? We can then decide if additional documentation is necessary.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Sep 27, 2019
@ttddyy
Copy link
Contributor Author

ttddyy commented Sep 30, 2019

We create a library that multiple service applications use.
Let's call the library "common-library".

The "common-library" provides implementation classes and configurations, such as followings:

  • @EnableCommonAuth - integrate with our auth infrastructure
  • @EnableCommonMetrics - integrate with our monitoring infra
  • @EnableCommonCaching - setup CacheManager to our caching infra
  • @EnableCommonJpaCaching - setup JPA manager with caching infra
  • @EnableCommonApiVersioning - setup web layer to handle API versioning strategy
  • @EnableCommonWebErrorHandling - error handling configurations
  • @EnableCommonReadinessLiveness - readiness/liveness endpoints setup
  • @EnableCommonEncryption - setup encryption related services
  • and more...
    (@EnableCommonX is a composed annotation that just imports corresponding @Configuration classes)

Each service application team can use common-library to their applications, and choose @EnableCommonX configurations to share common setup/logic based on what each application needs.

Here is our usecase for bean override.

For example, @EnableCommonCaching creates a HealthIndicator bean by default.
If service team has different requirement for checking health of the cache, then they want to use their own implementation rather than the one that common-library provides.
In such case, they will replace the HealthIndicator bean with their implementation bean.

Another example is in @EnableCommonAuth, common-library provides an implementation that translate our permission model to a spring-security's Authentication implementation. Majority of applications are happy to use the auth implementation provided by common-library; but some services need to use different representation of the auth model.
In this case, the service application replaces the bean that is responsible for converting the roles to Authentication to their own converter implementation.
This way, other part of the auth related logic, such as accessing the auth provider, oauth flow, etc, will still use the same setup(beans) provided by common-library.

In addition to bean override, ideally, I would like to use @ConditionalOnMissingBean etc, in common-library. However, since our configurations are not auto-configurations(we want to enable them explicitly), when application runs, there will be no distinction between @Configurations from common-library and ones from application's own.
Therefore, ordering of processing configuration classes becomes issue for @ConditionalOnMissingBean.

While writing this, an idea I came up is to make common-library configurations to be auto configurations and disable them by default(by some way). When @EnableCommonX is specified, then enable the corresponding configuration(if that is possible).
In this way, we can safely use @ConditionalOnX in common-library; though, it feels a bit of circumventing to use auto-configuration infrastructure.

I believe writing such library is not something special; rather it is very typical especially in this microservice era. There are many applications within organization that share common code/infra/setup.

From such library perspective, I think here is the things would be nice to have:

  • Fine grained bean override control (spring.main.allow-bean-definition-overriding is all or none)
    • Maybe declare overriding beans explicitly; so that, it is obvious which beans application is intended to override.
  • A way to disable specified auto configurations by defaults (by library)
    • So that having jar does not activate the specified auto configurations by default
    • Maybe an entry in spring.factories to disable by default
  • A way to explicitly enable auto configuration (when disabled by default)
    • If such exists, @EnableCommonX can be a composed annotation to enable corresponding auto configurations.

Thanks,

@philwebb
Copy link
Member

Closing following the discussion in #19400 (comment)

@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jan 29, 2020
@nightswimmings
Copy link

I'm not sure we have clear guidelines ourselves when it comes to these more complex scenarios. Generally speaking @Enable... annotations are part of user configuration and so are always processed before Spring Boot auto-configuration. Mostly if you register a bean in an @Enable... annotation, our auto-configuration should back off.

Without knowing the details of your specific use-case, it's very hard to offer general advice. It also feels like something that's quite unusual for a typical Spring Boot user to do (so we don't want to add a section to the reference documentation).

We've tried to document that @Conditional... annotations should only be used with auto-configuration classes, but unfortunately some users still use them incorrectly.

Your BeanFactoryPostProcessor seems like a logical approach, but I'm a little confused why you'd need to replace bean definitions registered by another library. Perhaps you can share some code that shows concretely what you are trying to do? We can then decide if additional documentation is necessary.

@philwebb Regarding that "@conditional...` annotations should only be used with auto-configuration classes, but unfortunately some users still use them incorrectly." Is this still true in 2024? I'm trying to find any mention in the documentation but I am failing to do so

@philwebb
Copy link
Member

@nightswimmings There are some notes in the javadoc. For example

* The condition can only match the bean definitions that have been processed by the
* application context so far and, as such, it is strongly recommended to use this
* condition on auto-configuration classes only. If a candidate bean may be created by
* another auto-configuration, make sure that the one using this condition runs after.

@nightswimmings
Copy link

nightswimmings commented Jun 19, 2024

@philwebb Thanks!! So is the consideration something that applies only to particular conditions that work especially then?
For instance is it legit using a @conditionalonproperty over a regular @configuration class?

@philwebb
Copy link
Member

@nightswimmings As a general rule, we recommend not using any @Conditional... annotation on regular configuration classes. They're really not designed to be used that way.

Having said that, conditions such as @ConditionalOnClass and @ConditionalOnProperty that will technically work because the things that they test won't change. I'd still don't think they should be used, the lower-level org.springframework.context.annotation.Conditional annotation is probably a better fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants