spring-projects / spring-ai

An Application Framework for AI Engineering
https://docs.spring.io/spring-ai/reference/index.html
Apache License 2.0
3.15k stars 783 forks source link

Consistent design of Options builder classes #1592

Open markpollack opened 22 hours ago

markpollack commented 22 hours ago

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

youngmoneee commented 7 hours ago

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?

markpollack commented 52 minutes ago

Hi!

I agree with these points

  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).

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!