Skip to content

fix(GH-5922): Preserve OpenAI response metadata#5929

Open
yaner-here wants to merge 1 commit intospring-projects:mainfrom
yaner-here:GH-5922
Open

fix(GH-5922): Preserve OpenAI response metadata#5929
yaner-here wants to merge 1 commit intospring-projects:mainfrom
yaner-here:GH-5922

Conversation

@yaner-here
Copy link
Copy Markdown
Contributor

Fixes #5922. Preserves unmapped root-level OpenAI chat completion response fields in ChatResponseMetadata.

The OpenAI SDK exposes provider-specific root fields through ChatCompletion._additionalProperties(). These fields were previously dropped when Spring AI mapped ChatCompletion into ChatResponseMetadata. This change copies those additional properties into the metadata map.

@ilayaperumalg ilayaperumalg self-assigned this May 5, 2026
@ilayaperumalg ilayaperumalg added this to the 2.0.0-M6 milestone May 5, 2026
Copy link
Copy Markdown
Member

@ilayaperumalg ilayaperumalg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! It's a great catch to preserve these additional properties.

However, there is one significant issue with leaking SDK-specific types into Spring AI's generic abstractions, and one minor refactoring suggestion:

  1. Leaking com.openai.core.JsonValue:
    By doing result._additionalProperties().forEach(metadataBuilder::keyValue);, the map is populated with OpenAI SDK's JsonValue objects. As shown in the added test (metadata.<JsonValue>get(\non_null_field\)), this forces users to cast values to an OpenAI-specific class. This breaks Spring AI's portability—if a user switches to another provider, their code expecting JsonValue will break.

Recommendation: Unwrap the JsonValue into standard Java types (String, Number, Boolean, List, Map) before adding them. You can use Jackson for this:

result._additionalProperties().forEach((key, jsonValue) -> {
    try {
        Object unmarshalled = ModelOptionsUtils.JSON_MAPPER.readValue(jsonValue.toString(), Object.class);
        metadataBuilder.keyValue(key, unmarshalled);
    } catch (Exception e) {
        metadataBuilder.keyValue(key, jsonValue.toString());
    }
});
  1. Minor Map Copying Simplification:
    In from(ChatResponseMetadata chatResponseMetadata, Usage usage), creating a temporary map via Streams is fine, but you could simplify it by iterating directly over the entries:
ChatResponseMetadata.Builder builder = ChatResponseMetadata.builder()
    .id(chatResponseMetadata.getId())
    .usage(usage)
    .model(chatResponseMetadata.getModel());

chatResponseMetadata.entrySet().forEach(e -> builder.keyValue(e.getKey(), e.getValue()));

return builder.build();

@yaner-here yaner-here force-pushed the GH-5922 branch 4 times, most recently from 54748c3 to 3d452d1 Compare May 6, 2026 07:28
@yaner-here
Copy link
Copy Markdown
Contributor Author

yaner-here commented May 6, 2026

Thanks for the review! All suggestions have been addressed.


Slight implementation difference

There's a slight difference. OpenAI's JsonValue.toString() returns non-standard JSON string. For example, JsonValue.from(Map.of("key1", 1, "key2", 2)).toString() actually returns {key2=2, key1=1}, which is expected to be {"key2": 2, "key1": 1}, thus can't be handled by Jackson.

tools.jackson.core.exc.StreamReadException: Unrecognized token 'key2': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); byte offset: #UNKNOWN]
	at tools.jackson.core.JsonParser._constructReadException(JsonParser.java:1856)
	at tools.jackson.core.json.ReaderBasedJsonParser._reportInvalidToken(ReaderBasedJsonParser.java:3130)
	at tools.jackson.core.json.ReaderBasedJsonParser._handleOddValue(ReaderBasedJsonParser.java:2075)
	at tools.jackson.core.json.ReaderBasedJsonParser.nextToken(ReaderBasedJsonParser.java:809)
	at tools.jackson.databind.deser.jdk.UntypedObjectDeserializerNR._deserializeNR(UntypedObjectDeserializerNR.java:261)
	at tools.jackson.databind.deser.jdk.UntypedObjectDeserializerNR.deserialize(UntypedObjectDeserializerNR.java:65)
	at tools.jackson.databind.deser.DeserializationContextExt.readRootValue(DeserializationContextExt.java:266)
	at tools.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:2639)

So I use ObjectMapper to get the valid JSON string.


Another better implementation?

BTW, JsonValue.convert(Object.class) could perfectly converting itself to Java types. For example, JsonValue.from(List.of("abc", 123, Map.of("key", "value"))).convert(Object.class) returns the expected nested instance.
image

Would JsonValue.convert(Object.class) be better? I would appreciate your thoughts on this.

@yaner-here yaner-here force-pushed the GH-5922 branch 4 times, most recently from efdd543 to 64fb781 Compare May 7, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve unmapped root-level provider fields in ChatResponseMetadata.map

2 participants