Skip to content

Conversation

@CorgiBoyG
Copy link
Contributor

@CorgiBoyG CorgiBoyG commented Nov 26, 2025

Description

Fixes the issue where extraBody parameters are lost during ModelOptionsUtils.merge() operation due to missing @JsonProperty annotation.

Changes

  • Added @JsonProperty("extra_body") annotation to extraBody parameter in ChatCompletionRequest constructor
  • Added test case testMergeWithExtraBody() to verify extraBody is correctly merged from OpenAiChatOptions to ChatCompletionRequest

Related Issue

Fixes #4966

Checklist

  • Code follows project style
  • Commit messages include DCO sign-off
  • Added unit tests
  • All tests pass locally

Signed-off-by: CorgiBoyG <rongjieguo2001@163.com>
@CorgiBoyG CorgiBoyG force-pushed the fix-the-extraBody-is-not-effective-4966 branch from ed323b9 to 49857bc Compare November 26, 2025 16:25
@quaff
Copy link
Contributor

quaff commented Nov 27, 2025

It's not a correct fix, see #4936

@CorgiBoyG
Copy link
Contributor Author

CorgiBoyG commented Nov 27, 2025

It's not a correct fix, see #4936

@quaff Thanks for the feedback! I appreciate you pointing to PR #4936.

However, I respectfully disagree that this PR is "not a correct fix." Let me explain why both approaches have their merits:

Why PR #4975 (this PR) is a valid fix

1. Root Cause Analysis

The issue occurs because ModelOptionsUtils.merge() filters fields based on @JsonProperty annotations:

private static List<String> getJsonPropertyValues(Class<?> clazz) { return Arrays.stream(clazz.getDeclaredFields()) .map(field -> field.getAnnotation(JsonProperty.class)) // Only @JsonProperty fields .filter(Objects::nonNull) .map(JsonProperty::value) .toList(); }

Problem: ChatCompletionRequest.extraBody parameter lacked @JsonProperty("extra_body"), causing it to be filtered out during merge.
Solution: Add @JsonProperty("extra_body") → merge preserves the field ✅

2. Serialization is NOT affected

@JsonAnyGetter takes precedence over @JsonProperty, so the JSON output remains flat:

@JsonAnyGetter public Map<String, Object> extraBody() { return this.extraBody; }

This ensures backward compatibility - the HTTP request body is still flattened as expected.

Why PR #4936 is also valuable (but different scope)

PR #4936's @MergeableField annotation is an architectural improvement that:

  • ✅ Provides explicit control over merge behavior
  • ✅ Decouples merge logic from serialization annotations
  • ✅ Could benefit future development

However, it:

  • ⚠️ Changes core merge infrastructure
  • ⚠️ Requires more extensive testing
  • ⚠️ Better suited for a feature release (1.2.0) rather than a patch

Proposed Path Forward

I suggest we:

  1. Merge PR Fix extraBody serialization by adding @JsonProperty annotation #4975 into 1.1.1 as an urgent hotfix (users currently cannot use Qwen3's thinking chain feature)
  2. Continue discussing PR Fix extraBody loss during ModelOptionsUtils.merge() #4936 as a separate architectural improvement for 1.2.0+
  3. Both PRs can coexist - Fix extraBody loss during ModelOptionsUtils.merge() #4936 can build upon Fix extraBody serialization by adding @JsonProperty annotation #4975's fix

This way:

@quaff @deejay1 What are your thoughts on this approach? Thanks!

@markpollack
Copy link
Member

Thanks for the very thoughtful discussion. The merging of options is a bit of a non-intuitive area of the code base. I thought I fixed this but forgot about the full lifecycle. This PR and some additional changes on top of it so that merging of the extra body map with 'default options' and 'runtime options' works correctly. there are mock tests to ensure the wireformat is as expected.

merged on 1.1.x 0646d1e
merged on main 264fc3b

I agree there can be a better approach, let's keep discussing on that other PR.

@markpollack markpollack closed this Dec 3, 2025
@markpollack markpollack added this to the 2.0.0.M1 milestone Dec 3, 2025
@markpollack markpollack self-assigned this Dec 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

the extraBody is not effective

4 participants