Fix: more vigorous handling of syntax for spring.autoconfigure.exclude

[anenviousguest]

This fixes #2329

Pull request checklist

• Please read our contributor guide
• Consider creating a discussion on the discussion forum
  first
• Make sure the PR doesn't introduce backward compatibility issues
• Make sure to have sufficient test cases

Pull Request type

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Changes in this PR

In a nutshell - this PR makes ExcludeAutoConfigurationsEnvironmentPostProcessor defer the retrieval of spring.autoconfigure.exclude to Spring's binder instead of manually looping through available properties source and doing the guess work wrt syntax which was used for a property.

As far as the Test Properties are concerned - there was a dedicated code in the ExcludeAutoConfigurationsEnvironmentPostProcessor which was trying to ensure that those takes precedence - but yet again, this is rather the job of SpringBoot itself and not of DGS to make this happen. I checked #2032 which was describing the original issue with test properties not taking precedence and added a dedicated SpringBootTest to illustrate that it should work without special handling on the side of DGS.

[anenviousguest]

basically, that's a replacement of the test which i removed from ExcludeAutoConfigurationsEnvironmentPostProcessorTest (see below).

Here, we have test property source, with our expectation that this will take priority over the same property defined via application-<profile>.properties which I also added

[anenviousguest]

thanks @paulbakker - I pushed one more commit to fix linting errors.

[jjacobs44]

Hey @anenviousguest, first off thanks for the report and contribution!

It seems that while this does fix your initial bug report, it also changes the underlying behavior of the exclude mechanism more broadly. As we have it implemented now, we join the value of spring.autoconfigure.exclude across all property sources. With the implementation here, it now only resolves to the the value of spring.autoconfigure.exclude for the single highest priority property source. You might argue that's more "correct", but in any case that's a breaking change.

Any opposition to just using a comma separated list instead of yaml array syntax? IIRC there are many pitfalls around using yaml array syntax for a variety of spring properties and comma separated lists avoid the headache.

[anenviousguest]

Thanks @jjacobs44 , this is a fair point.

Ultimately, it would be your call as code owners, but please consider this:

• spring.autoconfigure.exclude is a property maintained by SpringBoot, not by DGS

• ergo, it is liable to the rules outlined in Externalized Configuration documentation:

  Spring Boot uses a very particular PropertySource order that is designed to allow sensible overriding of values. Later property sources can override the values defined in earlier ones.

  Sources are considered in the following order:
  ...

  There follows a list of 14 bullet points whose order is important.

  And further down on the same page - under Merging Complex Types section, where it says:

  When lists are configured in more than one place, overriding works by replacing the entire list.

  This is what all SpringBoot developers are relying upon.

• The documentation of DGS (https://netflix.github.io/dgs/) does not seem to mention anywhere that DGS fiddles around with spring.autoconfigure.exclude, let alone any particular algorithm of how exactly it does that. Neither did I manage to find an occurrence of GraphQlObservationAutoConfiguration or GraphQlWebMvcSecurityAutoConfiguration in the DGS documentation. Taking that into account, it's probably unfair to treat internals of the class we're modifying in this PR as the contract DGS has to stick to, especially given that the property itself is coined by SpringBoot which outlines the contract for merging properties in another way.

Also I looked once again at #2032 - even there, the author complained that:

Setting the spring.autoconfigure.exclude e.g. in unit test by the @TestProperties appends the value to the configuration property already defined e.g. in application.yaml file. Does not override it.

So, the expectation was based on SpringBoot contract - that @TestProperties would override what was defined in application-<...>.yaml (or .properties, for that matter), and not that they would be concatenated from multiple sources.

WDYT?

PS: to this one:

Any opposition to just using a comma separated list instead of yaml array syntax?

That's the work-around we are currently forced to use, but it's far from perfect because from ergonomics standpoint, arrays are easier to maintain. This particular property holds an array of fully qualified class names, so keeping it in a single string drastically reduces readability and makes it harder to grok the diff.

Thanks.