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

Propose new spring.autoconfigure.exclusion property binding as Map to support multiple property sources #41669

Open
tmoschou opened this issue Aug 1, 2024 · 6 comments
Labels
status: on-hold We can't start working on this issue yet type: enhancement A general enhancement
Milestone

Comments

@tmoschou
Copy link

tmoschou commented Aug 1, 2024

Currently the spring.autoconfigure.exclude property is bound as a List<Class<?>>. This is a problem as collections cannot be merged across different property sources or individual elements removed. Additionally it requires property sources with higher precedence to have global knowledge of all preexisting exclusion from other sources in order to append a new exclusion.

This is a frequent pain-point for us and others. See #27414 - Consider merging spring.autoconfigure.exclude from multiple profiles and #9137

Understandably merging / overriding logic for collections is difficult - refer to #12444 (comment)

I propose a new configuration property (E.g. spring.autoconfigure.exclusions or perhaps some other property if too similarly named) that is bound as a Map<Class<?>, Boolean>. Binding to a map will allow merging from different property sources and profiles. E.g.

---
spring:
  config.activate.on-profile: nosecurity
  autoconfigure:
    exclusions: 
      org.springframework.boot.autoconfigure.security.servlet.SecurityAutoConfiguration: true
      org.springframework.boot.actuate.autoconfigure.security.servlet.ManagementWebSecurityAutoConfiguration: true
 
---
spring:
  config.activate.on-profile: noredis
  autoconfigure:
    exclusions: 
      org.springframework.boot.autoconfigure.data.redis.RedisAutoConfiguration: true

Including the class name in the key is not too different from the logging.level.<class> properties and similar to management.health.<name>.enabled.

I have a custom EnvironmentPostProcessor that I think achieves my goal by rebinding the spring.autoconfigure.exclude property from my custom spring.autoconfigure.exclusions property? But it would be nice if spring-boot had first class support for this. For posterity here it is

@Order(Ordered.LOWEST_PRECEDENCE)
public class AutoConfigureExcludeEnvironmentPostProcessor implements EnvironmentPostProcessor {

    private static final String SPRING_AUTOCONFIGURE_EXCLUDE = "spring.autoconfigure.exclude";
    private static final String SPRING_AUTOCONFIGURE_EXCLUDEMAP = "spring.autoconfigure.exclusion";

    @Override
    public void postProcessEnvironment(ConfigurableEnvironment environment, SpringApplication application) {
        Binder binder = Binder.get(environment);
        Set<String> disabled = new HashSet<>();
        binder.bind(SPRING_AUTOCONFIGURE_EXCLUDE, Bindable.listOf(String.class)).ifBound(disabled::addAll);
        Map<String, Boolean> excludeMap =
            binder.bind(SPRING_AUTOCONFIGURE_EXCLUDEMAP, Bindable.mapOf(String.class, Boolean.class))
                .orElseGet(Collections::emptyMap);

        for (Map.Entry<String, Boolean> entry : excludeMap.entrySet()) {
            if (entry.getValue()) {
                disabled.add(entry.getKey());
            } else {
                // override if class is present in 'spring.autoconfigure.exclude' property
                disabled.remove(entry.getKey());
            }
        }

        if (!disabled.isEmpty() || environment.containsProperty(SPRING_AUTOCONFIGURE_EXCLUDE)) {
            environment.getPropertySources().addFirst(new MapPropertySource(
                AutoConfigureExcludeEnvironmentPostProcessor.class.getSimpleName(),
                Map.of(SPRING_AUTOCONFIGURE_EXCLUDE, disabled)
            ));
        }
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 1, 2024
@quaff
Copy link
Contributor

quaff commented Aug 5, 2024

It's a good idea, It would be great if Spring Boot introduce the new spring.autoconfigure.exclusion and deprecate the painful spring.autoconfigure.exclude.

@mhalbritter mhalbritter added the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 5, 2024
@philwebb philwebb added type: enhancement A general enhancement status: on-hold We can't start working on this issue yet and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 21, 2024
@philwebb philwebb added this to the 3.x milestone Aug 21, 2024
@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Aug 21, 2024
@philwebb
Copy link
Member

We'd like to investigate #41830 to see if we can offer a general purpose solution for merging lists. I'll put this one on hold until we've done that.

@quaff
Copy link
Contributor

quaff commented Aug 22, 2024

We'd like to investigate #41830 to see if we can offer a general purpose solution for merging lists. I'll put this one on hold until we've done that.

This proposal follows the way of Spring Boot, could you elaborate the disadvantage of this proposal?
I'm not against #41830, but I think this proposal is better for autoconfigure exclusions, that one introduce new magic makes thing more complex and looks strange.

@philwebb
Copy link
Member

I like the new proposal as well but I think it's prudent to investigate if a general purpose solution is viable before we offer too many ways of doing the same thing.

One concern I have with the new proposal is the confusion that might arise by having two very similar properties that work in slightly different ways. We'd need spring.autoconfigure.exclude for the List, and spring.autoconfigure.excludes for the Map. That's a very subtle distinction and one that's likely to cause some confusion I think. Perhaps we can come up with a better property name, or deprecate spring.autoconfigure.exclude. The problem with deprecation is that we then force folks that don't need to merge exclusions into using a more verbose syntax.

@quaff
Copy link
Contributor

quaff commented Aug 22, 2024

Perhaps we can come up with a better property name.

I'm vote for spring.autoconfigure.exclusions.

deprecate spring.autoconfigure.exclude.

I suggest deprecating it in next minor release and remove it next major release with a dedicated FailureAnalyzer to guide folks to migrate.

@tmoschou
Copy link
Author

tmoschou commented Aug 22, 2024

Thanks for the update!

That's a very subtle distinction and one that's likely to cause some confusion I think

I agree could be confusing, I think I initially had spring.autoconfigure.exclusions-map when prototyping with my EnvironmentPostProcessor and added resources/META-INF/additional-spring-configuration-metadata.json with

{
  "properties": [
    {
      "name": "spring.autoconfigure.exclusions-map",
      "type": "java.util.Map<java.lang.Class, java.lang.Boolean>",
      "description": "Auto-configuration classes to exclude."
    }
  ]
}

for type hints, which give IDE assistance (IntelliJ)

Screenshot 2024-08-22 at 5 13 16 PM Screenshot 2024-08-22 at 5 12 28 PM

I did consider whether should deprecate too, I don't have any overly strong opinions either way should the new property be ultimately agreed too. However if the old is not deprecated, then both should be supported simultaneously (not mutually exclusive) IMO, as my env post processor does.

Also not against, #41830, but I am interested how this might work with collections of complex types that themselves might have nested collections - I'll repost that last question on that ticket too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: on-hold We can't start working on this issue yet type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants