In some cases we have a dedicated top level object as a builder and in some cases it in an inner class. Also, most options builders do not implement an interface. We should adhere to the design that has been used pretty consistently in spring-framework which has the builder as an inner object and also an interface for the builder.
We should also add tests for the builders.
This came out of the PR discussion #414
Comment From: youngmoneee
I still think that ensuring the immutability of objects is crucial to prevent potential concurrency issues. The reason I initially designed the builder as a separate object outside the option object was not only to facilitate simple inheritance but also to allow future extensions such as fromDefaults() and fromConfig().
However, I now think that separation of concerns and consistency are more important. Based on this, my current considerations are as follows: 1. Operate in a consistent manner with other Spring projects through a simple builder. 2. The object itself must be immutable, and modifications should only be allowed through the builder. 3. Minimize inconvenience from this approach (e.g., by passing existing objects as builder parameters). 4. A separate factory or converter that handles the responsibility of object creation and transformation when relying on Spring Boot. 5. Testing for the implementation of the above points.
Do you have any additional ideas or considerations?
Comment From: markpollack
Hi!
I agree with these points
- Operate in a consistent manner with other Spring projects through a simple builder.
- The object itself must be immutable, and modifications should only be allowed through the builder.
- Minimize inconvenience from this approach (e.g., by passing existing objects as builder parameters).
As for 4. I want to avoid the code duplication (which is quite large and boilerplate, prone to mistakes) between the 'core options' class and what is exposed to boot. We have figured out a way around that, basically anything that requires a @NestedConfigurationProperty annotation will be added to the file META-INF/additional-spring-configuration-metadata.json in the appropriate autoconfig package. See here, for the docs.
I didn't understand the comment
The reason I initially designed the builder as a separate object outside the option object was not only to facilitate simple inheritance but also to allow future extensions such as fromDefaults() and fromConfig().
but perhaps it isn't relevant if we follow the 'simple builder' approach in other spring projects.
One thing that I'm not sure about is to have an interface for the builder. It is good design, but does feel like a bit overkill.
Thoughts? Thanks for your feedback and interest!
Comment From: youngmoneee
@markpollack
Thank you for your input! The link you provided is currently inaccessible 😞
I believe that directly applying the Lombok project or using an annotation processor could significantly reduce boilerplate code.
The fromConfig and fromDefault methods initially conceived deviate from the single responsibility principle. For these functionalities, it would be better to separate them into a new factory instead.
I agree with your suggestions 😊
Comment From: tzolov
Related to the Builder:
We should use a consistent convention for the Builder set methods. So far wit have many builders that use the with prefix (e.g. withModel(...)) and builders that don't use this prefix (e.g. model(...)).
It looks like the Spring way is not to have a prefix.
I will submit a PR that marks the withProperty(...) methods as deprecated and will add the new property(...) one. And perhaps after one release we can remove the deprecated builder methods ?
Comment From: markpollack
We have made many passes on this, oddly harder than it should be. But anyway, it is certainly in a much better shape. I'm going to close it. If you see anything that should be improved @youngmoneee please let us know.