Support to inherit common properties for configuring multiple beans

[quaff]

It's used for configuring multiple beans (#15732), we could extract common or use primary properties as parent now.

Take org.springframework.boot.autoconfigure.data.redis.RedisProperties for example, given:

# primary
spring.data.redis:
  host: 127.0.0.1
  port: 6379

# additional
additional.data.redis:
  port: 6380

Then effective properties:

additional.data.redis:
  host: 127.0.0.1
  port: 6380

should be bound to additionalRedisProperties:

	@Bean(autowireCandidate = false) // do not back off autoconfigured one
	@ConfigurationProperties(prefix = "additional.data.redis", inheritedPrefix = "spring.data.redis")
	RedisProperties additionalRedisProperties() {
		return new RedisProperties();
	}

[quaff]

Not implemented yet, it seems require design work from team member.

[wilkinsona]

Thanks, @quaff, but I'm afraid we're really not ready to review pull requests for #15732 at this stage. It's a very broad and complex topic and it's going to require a significant amount of design work. If you have suggestions that will contribute to that design work, please create them in a branch in your fork and then post a comment to #15732 rather than opening a PR.

[quaff]

Thanks, @quaff, but I'm afraid we're really not ready to review pull requests for #15732 at this stage. It's a very broad and complex topic and it's going to require a significant amount of design work. If you have suggestions that will contribute to that design work, please create them in a branch in your fork and then post a comment to #15732 rather than opening a PR.

@wilkinsona Could you reopen it for a while, I think I found an elegant way, I force-pushed my branch.

[quaff]

The goal of this PR is not to resolve #15732 but related to that, it's not duplicate but improvement of @ConfigurationProperties, please consider keeping it open to let others discuss, or should I create separate issue? @wilkinsona

[wilkinsona]

I don’t think we should re-open this and please do not open a new issue.

We don’t want to do anything related to #15732 until we’ve decided on the approach that we want to take. If we head in the direction you have proposed here and it proves to be the wrong direction, it will make harder to get things right and may prove confusing for users where we have to replace one approach with another.

[quaff]

I don’t think we should re-open this and please do not open a new issue.

We don’t want to do anything related to #15732 until we’ve decided on the approach that we want to take. If we head in the direction you have proposed here and it proves to be the wrong direction, it will make harder to get things right and may prove confusing for users where we have to replace one approach with another.

@wilkinsona I understand your concerns, take a step back, could Spring Boot provide extendibility to allow application extending ConfigurationPropertiesBinder to create specific Binder for ConfigurationPropertiesBean base on custom annotation along side with @ConfigurationProperties?

Currently I achieve the use case I shared with an ugly way.

	@ConfigurationProperties(prefix = DefaultRedisProperties.PREFIX)
	public class DefaultRedisProperties extends RedisProperties {

		public static final String PREFIX = "spring.data.redis";

	}

	@ConfigurationProperties(prefix = GlobalRedisProperties.PREFIX)
	public class GlobalRedisProperties extends RedisProperties {

		public static final String PREFIX = "global.data.redis";

		@Autowired
		public GlobalRedisProperties(DefaultRedisProperties defaultRedisProperties) {
			BeanUtils.copyProperties(defaultRedisProperties, this);
		}

	}

It has obvious drawbacks:

1. subclass is required per service.
2. constructor binding is not supported.

EDIT: WIP prototype https://github.com/spring-projects/spring-boot/compare/main...quaff:patch-83?expand=1

[wilkinsona]

Anything's possible but, given that the motivation for it is to support auto-configuration of multiple beans, I don't think we can consider adding it until #15732 has moved forwards.

[quaff]

Anything's possible but, given that the motivation for it is to support auto-configuration of multiple beans, I don't think we can consider adding it until #15732 has moved forwards.

Please take a look at my proposed changes, all tests passed on my machine, but I think it's mainly for #42361, and application can use it for part of #15732.

[wilkinsona]

all tests passed on my machine

It's not about whether or not the changes break anything, but about the surface area of the project that needs to be maintained and supported. Adding anything to that surface area has a cost that needs to be justified.

I think it's mainly for #42361, and application can use it for part of #15732.

We've yet to fully investigate #42361, but it appears to be a performance issue. Ideally, we'd address it with internal changes to the implementation but we don't yet know if that's possible. Until we've had a chance to investigate further, it's far too soon to be using #42361 as a justification for adding public API.

[quaff]

It's not about whether or not the changes break anything

Just for letting you know it's a workable prototype.

but about the surface area of the project that needs to be maintained and supported. Adding anything to that surface area has a cost that needs to be justified.

I admit it's a trade-off at new features and cost to maintain, thanks for your attention.

[quaff]

One more thing, as you can see the commit improve ConfigurationPropertiesBean.findAnnotations() to retain all annotations instead of @ConfigurationProperties and @Validated, It may surprise developers at future point that ConfigurationPropertiesBean.asBindTarget().getAnnotation(OtherAnnotation.class) returning null, I would like to submit PR if you think is worthy to improve it now.

[quaff]

One more thing, as you can see the commit improve ConfigurationPropertiesBean.findAnnotations() to retain all annotations instead of @ConfigurationProperties and @Validated, It may surprise developers at future point that ConfigurationPropertiesBean.asBindTarget().getAnnotation(OtherAnnotation.class) returning null, I would like to submit PR if you think is worthy to improve it now.

@philwebb WDYT?

[philwebb]

Thanks for the prod @quaff, my initial thoughts are that we should probably leave that code alone unless there's an actual need to access other annotations. Currently things are quite targeted and we're not collecting or synthesizing annotations unnecessarily.