feat: Enhance Chat Options

[apappascs]

Key changes include:

• AbstractChatOptions: Introduced an abstract base class, reducing code duplication.
• DefaultChatOptions: A concrete implementation of ChatOptions built on top of
  AbstractChatOptions
• Equals and HashCode: Implemented equals() and hashCode() methods in all ChatOptions classes and the DefaultChatOptions class
• Test Updates: Comprehensive test updates were made across all affected modules to
  verify the new Builder pattern, copy functionality, and the behavior of the
  equals() and hashCode() methods.

Locally tested with:

./mvnw spring-javaformat:apply
./mvnw clean install -DskipTests -Dmaven.javadoc.skip=true
./mvnw verify -Pintegration-tests -pl spring-ai-core 
./mvnw verify -Pintegration-tests -pl models/spring-ai-openai

[apappascs]

@ThomasVitale @tzolov after the changes in this commit:
52198ed#diff-e5cb77ca54425cc7c57b08f1d70da4118eaf2c0377d381b1d184ace96d6f6e89R579

client.createRequest(new Prompt("Test message content"), false) no longer works and throws the following exception:
java.lang.IllegalArgumentException: chatOptions cannot be null

---

In the following scenario using buildRequestPrompt:

var client = new OpenAiChatModel(new OpenAiApi("TEST"),
		OpenAiChatOptions.builder().model("DEFAULT_MODEL").temperature(66.6).build());

var prompt = client.buildRequestPrompt(new Prompt("Test message content"));
var request = client.createRequest(prompt, false);

If we apply the changes from my PR, and use the base class, the resulting request object will be:

ChatCompletionRequest[messages=[ChatCompletionMessage[rawContent=Test message content, role=USER, name=null, toolCallId=null, toolCalls=null, refusal=null, audioOutput=null]], model=null, store=null, metadata=null, frequencyPenalty=null, logitBias=null, logprobs=null, topLogprobs=null, maxTokens=null, maxCompletionTokens=null, n=null, outputModalities=null, audioParameters=null, presencePenalty=null, responseFormat=null, seed=null, serviceTier=null, stop=null, stream=false, streamOptions=null, temperature=null, topP=null, tools=null, toolChoice=null, parallelToolCalls=null, user=null, reasoningEffort=null]

This occurs because the following line now calls ModelOptionsUtils.merge :
https://github.com/spring-projects/spring-ai/blob/main/models/spring-ai-openai/src/main/java/org/springframework/ai/openai/OpenAiChatModel.java#L579

OpenAiChatOptions requestOptions = ModelOptionsUtils.merge(runtimeOptions, this.defaultOptions,
				OpenAiChatOptions.class);

The merge function only returns fields from the concrete class, excluding fields from any abstract parent classes:
https://github.com/spring-projects/spring-ai/blob/main/spring-ai-core/src/main/java/org/springframework/ai/model/ModelOptionsUtils.java#L162

List<String> requestFieldNames = CollectionUtils.isEmpty(acceptedFieldNames)
				? REQUEST_FIELD_NAMES_PER_CLASS.computeIfAbsent(clazz, ModelOptionsUtils::getJsonPropertyValues)
				: acceptedFieldNames;

Therefore, fields defined in the base class are removed:
https://github.com/spring-projects/spring-ai/blob/main/spring-ai-core/src/main/java/org/springframework/ai/model/ModelOptionsUtils.java#L180 :

targetMap = targetMap.entrySet()
			.stream()
			.filter(e -> requestFieldNames.contains(e.getKey()))
			.collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue()));

In contrast, if we use:

request = client.createRequest(new Prompt("Test message content",
				OpenAiChatOptions.builder().model("PROMPT_MODEL").temperature(99.9).build()), true);

all chatOptions are retained.

Is this behavior intended?

If so we could possibly update the getJsonPropertyValues method as follows to also retain fields from the base classes:
https://github.com/spring-projects/spring-ai/blob/main/spring-ai-core/src/main/java/org/springframework/ai/model/ModelOptionsUtils.java#L248C29-L248C50

public static List<String> getJsonPropertyValues(Class<?> clazz) {
		List<String> jsonPropertyValues = new ArrayList<>();
		while (clazz != null) {
			for (Field field : clazz.getDeclaredFields()) {
				JsonProperty jsonProperty = field.getAnnotation(JsonProperty.class);
				if (jsonProperty != null) {
					jsonPropertyValues.add(jsonProperty.value());
				}
			}
			clazz = clazz.getSuperclass();
		}
		return jsonPropertyValues;
	}

I would appreciate your feedback on this.

[ThomasVitale]

@apappascs thanks for raising this issue! Could you share some details about how you're using createRequest()? For an unfortunate implementation, that method is not marked as private to simplify testing (https://github.com/spring-projects/spring-ai/blob/main/models/spring-ai-openai/src/main/java/org/springframework/ai/openai/OpenAiChatModel.java#L617), but it's designed to be private, so it's not supposed to be used outside the ChatModel class, not even from possible subclasses. We should probably make it private, together with the new client.buildRequestPrompt() which followed the same approach, but might be problematic. In any case, those methods are package protected and ensure certain conditions apply when calling them. I'll elaborate in a separate comment.

[ThomasVitale]

I see now where the problem arises. All the logic merging options and building the model provider-specific requests objects is tailored to work with concrete objects. So, when we finally merge runtime and default options, they are both of the same concrete type (e.g. OpenAiChatOptions). The buildRequestPrompt() takes care of that.

The createRequest() method takes a Prompt as input, and that Prompt will always have a non-null ChatOptions object with the concrete type (e.g. OpenAiChatOptions). So it is by design that when calling createRequest() outside this workflow you get an exception if the options are not provided in the prompt.

After the changes introduced in this PR, some of those conditions are not true anymore because of the introduction of the abstract class and the move of some of the options upstream. Hence, the errors you mentioned.

My suggestion is to keep the current API and not introduce an abstract class. The ChatOptions API captures common options already without enforcing any requirement on how to store that information. For example,ChatOptions has a getStopSequences() API that can be implemented across multiple providers, even though some of them store that information with a different name (e.g. OpenAI stores that info in a field named stop whereas Anthropic stores that in a field named stopSequences). And that's fine, because the abstraction is via the getStopSequences() API and not at the field-level.

Moving some of the fields (and the JSON configuration for their naming) up one level introduces an intermediate abstraction, resulting in having to maintain two sets of abstractions: one abstraction for common getters, and one abstraction for common fields, with the risk of additional complexity, fragility, and maintenance cost. For example, in the PR, the stopSequences field has been moved to the abstract class together with a getter (which is JSON-ignored). That creates issues in Anthropic, where the getter should not be ignored. Instead, in OpenAI, it needs to be ignored, because OpenAI uses a stop field instead, yet a stopSequences field is also present because its comes from the abstract class.

I think the ChatOptions API could use some improvements, especially around the merge logic, but I'm not sure about the introduction of this abstract class.

[ThomasVitale]

I like the improvement to ensure the consistent usage of equals() and hashCode() across all implementations, and the additional verification of copy() and builder() methods. Would it make sense to have those changes in a separate PR to simplify the review process?

[apappascs]

@ThomasVitale Thank you for the detailed feedback. I appreciate your careful consideration of the API design and potential long-term maintenance implications.

During the refactoring, I noticed that stopSequences is the only field with a naming difference. Most models use stop, making the interface's use of stopSequences inconsistent.

Additionally, the current level of duplicated code (essentially 100% in some areas) presents a significant maintenance challenge.

This duplication extends to the tool-related methods (e.g., setToolCallbacks, getToolNames, isInternalToolExecutionEnabled, getFunctionCallbacks, getFunctions), which are also copied across all models.

Furthermore, the *Model*Api classes exhibit considerable code duplication in the chatCompletionEntity and chatCompletionStream methods. While the URLs, headers, and request/response objects are provider-specific, the core logic is largely identical. The Template Method pattern could be a good solution here, allowing us to maintain and update the common logic in one place, rather than across all LLM model API classes.

I'll put some more thought into this and follow up with a separate comment outlining a revised approach. Hopefully, @tzolov can also provide some input here.

[ThomasVitale]

@apappascs thanks! There's indeed lots of duplication that would be nice to remove, spanning options, models, and apis.

[apappascs]

@ThomasVitale Updated the code to keep only changes for equals, hashCode and tests for now and removed the abstract class as you recommended and we could check if this is an option on another PR.