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();
}
Comment From: quaff
Not implemented yet, it seems require design work from team member.
Comment From: 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.
Comment From: 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.
Comment From: 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
Comment From: 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.
Comment From: 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
Comment From: 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.
Comment From: 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.
Comment From: 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 https://github.com/spring-projects/spring-boot/issues/42361, and application can use it for part of https://github.com/spring-projects/spring-boot/issues/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.
Comment From: 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.
Comment From: 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.
Comment From: quaff
One more thing, as you can see the commit improve
ConfigurationPropertiesBean.findAnnotations()to retain all annotations instead of@ConfigurationPropertiesand@Validated, It may surprise developers at future point thatConfigurationPropertiesBean.asBindTarget().getAnnotation(OtherAnnotation.class)returning null, I would like to submit PR if you think is worthy to improve it now.
@philwebb WDYT?
Comment From: 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.