Skip to content

Commit 653918d

Browse files
markpollackmarkpollack
authored
Fix OpenAI streaming finish_reason handling for empty string values (#4132)
This commit addresses issues with OpenAI streaming responses where finish_reason is an empty string ("") instead of null, which caused Jackson deserialization failures and inconsistent metadata formatting. Changes: - Configure Jackson coercion in ModelOptionsUtils to handle empty string to enum conversion (empty string -> null) - Fix getFinishReasonJson() method in OpenAiChatModel to return uppercase enum names for backward compatibility - Fix null pointer exceptions in streaming processing by ensuring getFinishReasonJson() returns empty string instead of null - Maintain finish_reason metadata as uppercase enum names ("STOP", "LENGTH") to preserve backward compatibility - Add comprehensive test suite covering all edge cases - Update streaming tests to expect correct uppercase finish reason values Key fixes: 1. Empty string finish_reason now deserializes correctly without errors 2. Finish reason metadata maintains backward compatibility (uppercase enum names) 3. No more NullPointerException in streaming Map.of() calls 4. Proper handling of null, empty string, and valid finish_reason values Jackson coercion configuration in ModelOptionsUtils now converts empty strings to null for ALL enum deserialization across Spring AI. Analysis shows this change is safe and beneficial for other providers: - Anthropic, Ollama use String fields (unaffected by enum coercion) - ZhiPuAI, MiniMax, MistralAI, DeepSeek use enum fields but have no documented empty string issues - Coercion improves robustness by gracefully handling malformed responses - No breaking changes expected for existing provider implementations Testing: - Added OpenAiStreamingFinishReasonTests with 6 comprehensive test cases - Fixed existing metadata tests to maintain backward compatibility - All tests pass, covering JSON deserialization and streaming scenarios - Validates both error handling and correct behavior - Added tests for changes to ModelOptionsUtils using enum coersion This ensures robust handling of OpenAI's empty string finish_reason edge case while maintaining backward compatibility and improving overall system resilience for malformed API responses. Auto-cherry-pick to 1.0.x Fixes #1358 Signed-off-by: Mark Pollack <[email protected]>
1 parent ecc3317 commit 653918d

File tree

4 files changed

+388
-4
lines changed

4 files changed

+388
-4
lines changed

‎models/spring-ai-openai/src/main/java/org/springframework/ai/openai/OpenAiChatModel.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ public ChatResponse internalCall(Prompt prompt, ChatResponse previousChatRespons
218218
"id", chatCompletion.id() != null ? chatCompletion.id() : "",
219219
"role", choice.message().role() != null ? choice.message().role().name() : "",
220220
"index", choice.index() != null ? choice.index() : 0,
221-
"finishReason", choice.finishReason() != null ? choice.finishReason().name() : "",
221+
"finishReason", getFinishReasonJson(choice.finishReason()),
222222
"refusal", StringUtils.hasText(choice.message().refusal()) ? choice.message().refusal() : "",
223223
"annotations", choice.message().annotations() != null ? choice.message().annotations() : List.of(Map.of()));
224224
return buildGeneration(choice, metadata, request);
@@ -317,7 +317,7 @@ public Flux<ChatResponse> internalStream(Prompt prompt, ChatResponse previousCha
317317
"id", id,
318318
"role", roleMap.getOrDefault(id, ""),
319319
"index", choice.index() != null ? choice.index() : 0,
320-
"finishReason", choice.finishReason() != null ? choice.finishReason().name() : "",
320+
"finishReason", getFinishReasonJson(choice.finishReason()),
321321
"refusal", StringUtils.hasText(choice.message().refusal()) ? choice.message().refusal() : "",
322322
"annotations", choice.message().annotations() != null ? choice.message().annotations() : List.of());
323323
return buildGeneration(choice, metadata, request);
@@ -420,8 +420,8 @@ private Generation buildGeneration(Choice choice, Map<String, Object> metadata,
420420
toolCall.function().name(), toolCall.function().arguments()))
421421
.toList();
422422

423-
String finishReason = (choice.finishReason() != null ? choice.finishReason().name() : "");
424-
var generationMetadataBuilder = ChatGenerationMetadata.builder().finishReason(finishReason);
423+
var generationMetadataBuilder = ChatGenerationMetadata.builder()
424+
.finishReason(getFinishReasonJson(choice.finishReason()));
425425

426426
List<Media> media = new ArrayList<>();
427427
String textContent = choice.message().content();
@@ -451,6 +451,14 @@ private Generation buildGeneration(Choice choice, Map<String, Object> metadata,
451451
return new Generation(assistantMessage, generationMetadataBuilder.build());
452452
}
453453

454+
private String getFinishReasonJson(OpenAiApi.ChatCompletionFinishReason finishReason) {
455+
if (finishReason == null) {
456+
return "";
457+
}
458+
// Return enum name for backward compatibility
459+
return finishReason.name();
460+
}
461+
454462
private ChatResponseMetadata from(OpenAiApi.ChatCompletion result, RateLimit rateLimit, Usage usage) {
455463
Assert.notNull(result, "OpenAI ChatCompletionResult must not be null");
456464
var builder = ChatResponseMetadata.builder()

‎models/spring-ai-openai/src/test/java/org/springframework/ai/openai/chat/OpenAiStreamingFinishReasonTests.java

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
/*
2+
* Copyright 2023-2025 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.ai.openai.chat;
18+
19+
import java.util.List;
20+
21+
import com.fasterxml.jackson.core.JsonProcessingException;
22+
import com.fasterxml.jackson.databind.ObjectMapper;
23+
import io.micrometer.observation.ObservationRegistry;
24+
import org.junit.jupiter.api.Test;
25+
import org.junit.jupiter.api.extension.ExtendWith;
26+
import org.mockito.Mock;
27+
import org.mockito.junit.jupiter.MockitoExtension;
28+
import reactor.core.publisher.Flux;
29+
30+
import org.springframework.ai.chat.model.ChatResponse;
31+
import org.springframework.ai.chat.prompt.Prompt;
32+
import org.springframework.ai.model.ModelOptionsUtils;
33+
import org.springframework.ai.model.tool.ToolCallingManager;
34+
import org.springframework.ai.openai.OpenAiChatModel;
35+
import org.springframework.ai.openai.OpenAiChatOptions;
36+
import org.springframework.ai.openai.api.OpenAiApi;
37+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionChunk;
38+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionChunk.ChunkChoice;
39+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionFinishReason;
40+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionMessage;
41+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionMessage.Role;
42+
import org.springframework.ai.openai.api.OpenAiApi.ChatCompletionRequest;
43+
import org.springframework.ai.retry.RetryUtils;
44+
import org.springframework.retry.support.RetryTemplate;
45+
46+
import static org.assertj.core.api.Assertions.assertThat;
47+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
48+
import static org.mockito.ArgumentMatchers.any;
49+
import static org.mockito.ArgumentMatchers.isA;
50+
import static org.mockito.BDDMockito.given;
51+
52+
/**
53+
* Tests for OpenAI streaming responses with various finish_reason scenarios, particularly
54+
* focusing on edge cases like empty string finish_reason values.
55+
*
56+
* @author Mark Pollack
57+
* @author Christian Tzolov
58+
*/
59+
@ExtendWith(MockitoExtension.class)
60+
public class OpenAiStreamingFinishReasonTests {
61+
62+
@Mock
63+
private OpenAiApi openAiApi;
64+
65+
private OpenAiChatModel chatModel;
66+
67+
@Test
68+
void testStreamingWithNullFinishReason() {
69+
// Setup
70+
setupChatModel();
71+
72+
var choice = new ChunkChoice(null, 0, new ChatCompletionMessage("Hello", Role.ASSISTANT), null);
73+
ChatCompletionChunk chunk = new ChatCompletionChunk("id", List.of(choice), 666L, "model", null, null,
74+
"chat.completion.chunk", null);
75+
76+
given(this.openAiApi.chatCompletionStream(isA(ChatCompletionRequest.class), any()))
77+
.willReturn(Flux.just(chunk));
78+
79+
// Execute
80+
Flux<ChatResponse> result = this.chatModel.stream(new Prompt("test"));
81+
82+
// Verify
83+
List<ChatResponse> responses = result.collectList().block();
84+
assertThat(responses).hasSize(1);
85+
ChatResponse response = responses.get(0);
86+
assertThat(response).isNotNull();
87+
assertThat(response.getResult().getOutput().getText()).isEqualTo("Hello");
88+
assertThat(response.getResult().getMetadata().getFinishReason()).isEqualTo("");
89+
}
90+
91+
@Test
92+
void testStreamingWithValidFinishReason() {
93+
// Setup
94+
setupChatModel();
95+
96+
var choice = new ChunkChoice(ChatCompletionFinishReason.STOP, 0,
97+
new ChatCompletionMessage("Complete response", Role.ASSISTANT), null);
98+
ChatCompletionChunk chunk = new ChatCompletionChunk("id", List.of(choice), 666L, "model", null, null,
99+
"chat.completion.chunk", null);
100+
101+
given(this.openAiApi.chatCompletionStream(isA(ChatCompletionRequest.class), any()))
102+
.willReturn(Flux.just(chunk));
103+
104+
// Execute
105+
Flux<ChatResponse> result = this.chatModel.stream(new Prompt("test"));
106+
107+
// Verify
108+
List<ChatResponse> responses = result.collectList().block();
109+
assertThat(responses).hasSize(1);
110+
ChatResponse response = responses.get(0);
111+
assertThat(response).isNotNull();
112+
assertThat(response.getResult().getOutput().getText()).isEqualTo("Complete response");
113+
assertThat(response.getResult().getMetadata().getFinishReason()).isEqualTo("STOP");
114+
}
115+
116+
@Test
117+
void testJsonDeserializationWithEmptyStringFinishReason() throws JsonProcessingException {
118+
// Test the specific JSON from the issue report
119+
String problematicJson = """
120+
{
121+
"id": "chatcmpl-msg_bdrk_012bpm3yfa9inEuftTWYQ46F",
122+
"object": "chat.completion.chunk",
123+
"created": 1726239401,
124+
"model": "claude-3-5-sonnet-20240620",
125+
"choices": [{
126+
"index": 0,
127+
"delta": {
128+
"role": "assistant",
129+
"content": ""
130+
},
131+
"finish_reason": ""
132+
}]
133+
}
134+
""";
135+
136+
// This should either work correctly or throw a clear exception
137+
ChatCompletionChunk chunk = ModelOptionsUtils.jsonToObject(problematicJson, ChatCompletionChunk.class);
138+
139+
// If deserialization succeeds, verify the structure
140+
assertThat(chunk).isNotNull();
141+
assertThat(chunk.choices()).hasSize(1);
142+
143+
var choice = chunk.choices().get(0);
144+
assertThat(choice.index()).isEqualTo(0);
145+
assertThat(choice.delta().content()).isEmpty();
146+
147+
// The key test: what happens with empty string finish_reason?
148+
// This might be null if Jackson handles empty string -> enum conversion
149+
// gracefully
150+
assertThat(choice.finishReason()).isNull();
151+
}
152+
153+
@Test
154+
void testJsonDeserializationWithNullFinishReason() throws JsonProcessingException {
155+
// Test with null finish_reason (should work fine)
156+
String validJson = """
157+
{
158+
"id": "chatcmpl-test",
159+
"object": "chat.completion.chunk",
160+
"created": 1726239401,
161+
"model": "gpt-4",
162+
"choices": [{
163+
"index": 0,
164+
"delta": {
165+
"role": "assistant",
166+
"content": "Hello"
167+
},
168+
"finish_reason": null
169+
}]
170+
}
171+
""";
172+
173+
ChatCompletionChunk chunk = ModelOptionsUtils.jsonToObject(validJson, ChatCompletionChunk.class);
174+
175+
assertThat(chunk).isNotNull();
176+
assertThat(chunk.choices()).hasSize(1);
177+
178+
var choice = chunk.choices().get(0);
179+
assertThat(choice.finishReason()).isNull();
180+
assertThat(choice.delta().content()).isEqualTo("Hello");
181+
}
182+
183+
@Test
184+
void testStreamingWithEmptyStringFinishReasonUsingMockWebServer() {
185+
// Setup
186+
setupChatModel();
187+
188+
// Simulate the problematic response by creating a chunk that would result from
189+
// deserializing JSON with empty string finish_reason
190+
try {
191+
// Try to create a chunk with what would happen if empty string was
192+
// deserialized
193+
var choice = new ChunkChoice(null, 0, new ChatCompletionMessage("", Role.ASSISTANT), null);
194+
ChatCompletionChunk chunk = new ChatCompletionChunk("chatcmpl-msg_bdrk_012bpm3yfa9inEuftTWYQ46F",
195+
List.of(choice), 1726239401L, "claude-3-5-sonnet-20240620", null, null, "chat.completion.chunk",
196+
null);
197+
198+
given(this.openAiApi.chatCompletionStream(isA(ChatCompletionRequest.class), any()))
199+
.willReturn(Flux.just(chunk));
200+
201+
// Execute
202+
Flux<ChatResponse> result = this.chatModel.stream(new Prompt("test"));
203+
204+
// Verify that the streaming works even with null finish_reason
205+
List<ChatResponse> responses = result.collectList().block();
206+
assertThat(responses).hasSize(1);
207+
ChatResponse response = responses.get(0);
208+
assertThat(response).isNotNull();
209+
assertThat(response.getResult().getMetadata().getFinishReason()).isEqualTo("");
210+
211+
}
212+
catch (Exception e) {
213+
// If this fails, it indicates the issue exists in our processing
214+
System.out.println("Streaming failed with empty finish_reason: " + e.getMessage());
215+
throw e;
216+
}
217+
}
218+
219+
@Test
220+
void testModelOptionsUtilsJsonToObjectWithEmptyFinishReason() {
221+
// Test the specific method mentioned in the issue
222+
String jsonWithEmptyFinishReason = """
223+
{
224+
"id": "chatcmpl-msg_bdrk_012bpm3yfa9inEuftTWYQ46F",
225+
"object": "chat.completion.chunk",
226+
"created": 1726239401,
227+
"model": "claude-3-5-sonnet-20240620",
228+
"choices": [{
229+
"index": 0,
230+
"delta": {
231+
"role": "assistant",
232+
"content": ""
233+
},
234+
"finish_reason": ""
235+
}]
236+
}
237+
""";
238+
239+
ChatCompletionChunk chunk = ModelOptionsUtils.jsonToObject(jsonWithEmptyFinishReason,
240+
ChatCompletionChunk.class);
241+
242+
assertThat(chunk).isNotNull();
243+
assertThat(chunk.choices()).hasSize(1);
244+
245+
var choice = chunk.choices().get(0);
246+
// The critical test: how does ModelOptionsUtils handle empty string -> enum?
247+
assertThat(choice.finishReason()).isNull();
248+
}
249+
250+
private void setupChatModel() {
251+
RetryTemplate retryTemplate = RetryUtils.DEFAULT_RETRY_TEMPLATE;
252+
ToolCallingManager toolCallingManager = ToolCallingManager.builder().build();
253+
ObservationRegistry observationRegistry = ObservationRegistry.NOOP;
254+
this.chatModel = new OpenAiChatModel(this.openAiApi, OpenAiChatOptions.builder().build(), toolCallingManager,
255+
retryTemplate, observationRegistry);
256+
}
257+
258+
}

‎spring-ai-model/src/main/java/org/springframework/ai/model/ModelOptionsUtils.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
import com.fasterxml.jackson.databind.JsonNode;
3636
import com.fasterxml.jackson.databind.ObjectMapper;
3737
import com.fasterxml.jackson.databind.SerializationFeature;
38+
import com.fasterxml.jackson.databind.cfg.CoercionAction;
39+
import com.fasterxml.jackson.databind.cfg.CoercionInputShape;
3840
import com.fasterxml.jackson.databind.json.JsonMapper;
3941
import com.fasterxml.jackson.databind.node.ArrayNode;
4042
import com.fasterxml.jackson.databind.node.ObjectNode;
@@ -72,6 +74,13 @@ public abstract class ModelOptionsUtils {
7274
.build()
7375
.configure(DeserializationFeature.ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true);
7476

77+
static {
78+
// Configure coercion for empty strings to null for Enum types
79+
// This fixes the issue where empty string finish_reason values cause
80+
// deserialization failures
81+
OBJECT_MAPPER.coercionConfigFor(Enum.class).setCoercion(CoercionInputShape.EmptyString, CoercionAction.AsNull);
82+
}
83+
7584
private static final List<String> BEAN_MERGE_FIELD_EXCISIONS = List.of("class");
7685

7786
private static final ConcurrentHashMap<Class<?>, List<String>> REQUEST_FIELD_NAMES_PER_CLASS = new ConcurrentHashMap<>();

0 commit comments

Comments
 (0)