While working on #8066, I've seen the ServerPropertiesAutoConfiguration class.
It seems its main goal is to allow Boot users to define their own ServerProperties bean, probably to override defaults.
I believe there are more ways now to achieve that and I'd like to remove that class completely. Thoughts?
Comment From: philwebb
I can't find that class, what package is it in?
Comment From: philwebb
Regardless I agree. When we first created @ConfigurationProperties we thought people might also create their own @Bean methods to define them. I don't think that's really been the case so I'd be in favor of keeping configuration purely an Environment concern.
Comment From: bclozel
Sorry, it's actually ServerPropertiesAutoConfiguration.
But you already answered, thanks!
Comment From: snicoll
@philwebb I agree but I guess some users are relying on that (I also remember something where we create a mini ApplicationContext to check the state of a given properties object, @dsyer might remember). Either in this issue or in a separate one we should have an how-to to explain how to do this (for instance with EnvironmentPostProcessor).
Comment From: dsyer
IMO we should allow user-defined beans to set up defaults for framework @ConfigurationProperties. The fact that not many people actually do it is more to do with how hard we make it than the validity of the use case. ServerProperties is a special one because it is needed during server startup which is very tightly coupled with the application context. I believe that's why it had special treatment.
Comment From: snicoll
I disagree that we should allow user-defined beans to set up defaults. If you want to configure the default, you should use the Environment. I agree that EnvironmnentPostProcessor is a bit low level right now but this is something we must improve if we remove that feature that was, as far as I know, never advertized.
Being able to change defaults without impacting the Environment is a huge smell IMO.
Comment From: dsyer
I guess that's a valid point of view. I'd rather not make the Environment a repository of default values though, because it's weakly typed an weakly keyed (there's no way to prevent typos in keys or values at compile time), whereas the beans that you are trying to configure are not, and that makes them more expressive.
Comment From: wilkinsona
I think that trying to allow people to override @ConfigurationProperties beans was an example of us trying to be a bit too clever and creating problems for ourselves. It was poorly specified and, I think, completely undocumented. I'd be surprised if it's used much at all.
If we want to allow people to configure the default values in a type-safe manner, I'd much rather have a well-defined contract than people messing around with the beans. Thinking aloud, perhaps ConfigurationPropertiesBindingPostProcessor could pass each @ConfigurationProperties bean to any implementations of a callback interface that are available so that they can set some custom defaults before the binding takes place?
Comment From: philwebb
I'm not sure we should even rush to add callback support. I've seen very few people actually complain about this.
Comment From: snicoll
Because they can create their own @ConfigurationProperties beans in certain areas. The feature is inconsistent (why do we do it for certain things and not for others?). But if we remove it, we ought to give them something else. That can be the callback or something else. Or we can decide we drop that feature altogether.
Comment From: snicoll
ManagementServerPropertiesAutoConfiguration should go away as well.
Comment From: snicoll
OK so I have some hacking in a branch that breaks because it has been replaced by @EnableConfigurationProperties(ServerProperties) and friends. The post-processor that looks for that annotation only looks in the current registry so when we have a child context, it creates two ServerProperties. And therefore the customizer of the main server is applied again in the child context. When management.port is set, we attempt to start the "main" tomcat twice...
I've created #8187
Comment From: bclozel
Rebased my changes on top of yours and all tests are green. I must say that the changes look good and it's much easier to reason about test infrastructure in many cases.
Now about the introduction of RegistrationStrategy - I'd like to know if that feature would be actually required by some Spring projects. Otherwise we could leave that option out and introduce it in the next milestone if it's useful to the Spring Boot developers.
Comment From: ky2ninh
Not sure I comprehended everything being said here but when I upgraded my spring boot application from 1.3.8 to 2.6.2, I am getting this error below. I am not referencing explicitly this ServerPropertiesAutoConfiguration.class. How do I fix this?
Thanks!
Karl
o.s.b.w.s.c.AnnotationConfigServletWebServerApplicationContext - Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanDefinitionStoreException: Failed to process import candidates for configuration class [com.abc.abc.vms.config.VMSInitializer]; nested exception is java.io.FileNotFoundException: class path resource [org/springframework/boot/autoconfigure/web/ServerPropertiesAutoConfiguration.class] cannot be opened because it does not exist
o.s.boot.SpringApplication - Application run failed org.springframework.beans.factory.BeanDefinitionStoreException: Failed to process import candidates for configuration class [com.abc.abc.vms.config.VMSInitializer]; nested exception is java.io.FileNotFoundException: class path resource [org/springframework/boot/autoconfigure/web/ServerPropertiesAutoConfiguration.class] cannot be opened because it does not exist
Comment From: snicoll
@ky2ninh please don't add the same comment on multiple issues. Brian already replied to you.