Reduce reflection and annotation usage in ConfigData infrastructure

[dsyer]

I hope there are many places we can improve this. Let's start with a simple one.

ConfigDataProperties is a value object, and is constructed reflectively via Binder.bind() which is wasteful. As far as I can tell it is only ever used to look up the value of spring.config.import (the env var upper case version is never used because it would be bound already to the "initial" imports). And "import" doesn't have any kebab case variants, so you don't need a Binder. I think we can simply look in the Environment and if the property is there construct a ConfigDataProperties instance directly by calling the constructor or a factor method.

It also uses an annotation @Name to rename the property bound to its constructor, which also seems unnecessarily indirect (we control both the property name and the constructor source code). We probably can't get rid of the annotation processing if we need to use Binder.bind(), but if we can skip that then even better.

[dsyer]

Here's another idea: Spring Boot can trace back all Binder results to a config property name and value, I believe. Maybe we could use that information to short cut the process - bind once and index the result and re-use it. I'd be happy to look into doing that at build time (fits with the general agenda and constraints of spring-init). We would need to be able to override the existing factories more easily probably - at runtime you would want to consult the record of what is known about at build time so you can avoid doing it again.

[dsyer]

Also FWIW here's an interface in Spring Init for bypassing Binder in @ConfigurationProperties:

public interface PropertiesBinder<T> {

	T bind(T bean, Environment environment);

}

Then there's a global utility PropertiesBinders that you can use to register instances of PropertiesBinder. Something like that would work here - a hook to replace the default Binder based Environment bindings.

[philwebb]

As far as I can tell it is only ever used to look up the value of spring.config.import

It also looks up spring.config.activate.on-cloud-platform and spring.config.activate.on-profile. Both of those are enums and both can be used in the main application.properties in a relaxed form. For example spring.profile.activate.oncloudplatform=Kubernetes is valid.

I think we can simply look in the Environment and if the property is there construct a ConfigDataProperties instance directly by calling the constructor or a factory method.

There are a number of edge cases that cause problems if we try and obtain values directly. We've been bitten with this quite a bit in the past, so when we started working on the new class we intentionally decided to keep the Binder central to the new code. One common problem would be lists where YAML files get expanded to the [N] syntax. We'd need to deal with those which would mean repeating a lot of the logic from IndexedElementsBinder.

It also uses an annotation @name to rename the property bound to its constructor, which also seems unnecessarily indirect (we control both the property name and the constructor source code).

We'd need a different way to get the parameter name in ValueObjectBinder. Currently PARAMETER_NAME_DISCOVERER is hard coded. I went with the annotation because it meant we could update ValueObjectBinder without needing to increase the surface area of the API.

[philwebb]

Maybe we could use that information to short cut the process - bind once and index the result and re-use it.

Are we assuming that you can't use environment or system variables to override properties?

[dsyer]

Are we assuming that you can't use environment or system variables to override properties?

That's not going to be true in general, but I think for building native images and containers people are willing to accept that kind of compromise. We just have to be explicit that they will only get the souped up performance in a "strict closed world" (no changes to class path and environment at runtime).

Anyway, it's probably not necessary to restrict to literally no changes in the environment, just to only changes in the values of environment properties.

[dsyer]

It also looks up spring.config.activate.on-cloud-platform and spring.config.activate.on-profile. Both of those are enums and both can be used in the main application.properties in a relaxed form. For example spring.profile.activate.oncloudplatform=Kubernetes is valid.

I still don't think that a fully-leaded Binder.bind() is warranted here (and in other places). It's really hard to debug/read/understand apart from anything else. Could we maybe separate the "reflection", "relaxed property names" and "type conversion" concerns a bit more, so that it could be written more imperatively?

E.g.

Collection<String> imports = StringUtils.commaDelimitedListToSet(Something.getRelaxedProperty(environment, "spring.config.import", ""));

The property name conventions could be encapsulated in the Environment even. Then you could do it all without reading any annotation metadata and without calling any methods reflectively:

Collection<String> imports = StringUtils.commaDelimitedListToSet(environment, "spring.config.import", "");
String[] profiles = StringUtils.commaDelimitedListToStringArray(environment, "spring.config.activate.on-profile", "");
CloudPlatform platform = CloudPlatform.valueOf(environment, "spring.config.activate.on-cloud-platform", "NONE").toUpperCase());
ConfigDataProperties data = new ConfigDataProperties(imports, new Activate(platform, profiles));

ConfigDataProperties only has 3 inputs (hence it is a sensible value object) and they are all primitive-ish (easy type conversion from String).

[philwebb]

I totally agree that the Binder could do with some improvements to help with debugging and understanding the code. I don't, however, agree that the answer is to not use it to process ConfigData. One of the reasons that the binder is so complicated is that it attempts to deal with all the subtle binding issues that can occur. We've had issues in the past where people are confused why certain features can't be used at certain times.

For example, if we switch to StringUtils.commaDelimitedListToSet(Something.getRelaxedProperty(environment, "spring.config.import", "")) we're immediately restricting the way that spring.config.import can be used.

This would be a valid application.yml:

spring.config.import: "classpath:foo.yaml,classpath:bar.yaml"

This one would not:

spring.config.import: ["classpath:foo.yaml", "classpath:bar.yaml"]

We'd have to document that restriction and explain why. We'd also be adding the limitation that a comma can never be part of the location string.

On top of that, the new code intentionally doesn't modify the Environment until all processing has completed. So we'd need a new Environment instance just for use during the ConfigData processing.

I did recently make a change to Bindable that allows it to carry attributes. I think we could use this to remove the reflective @Name annotation check. I think we should start with that and looking at what else we can do to improve the Binder code.

[dsyer]

if we switch to StringUtils.commaDelimitedListToSet()

So maybe let's not do that. We'd have to change the implementation of the binding so it doesn't expect a String value. That's maybe included in the "type conversion" part. The goal is just to make it more imperative and less obscure. You can still encapsulate as much as you want potentially.

Not sure I follow the bit about a new Environment instance.

[philwebb]

Looking at all the options I don't think we can remove the use of Binder, certainly not without causing a lot of instability. I've opened #23713 to look at the binder in general so see how we might be able to reduce the use of reflection in 3.x.