[integrations][openai] Let provider exceptions reach the caller unwrapped - #989
[integrations][openai] Let provider exceptions reach the caller unwrapped#989weiqingy wants to merge 2 commits into
Conversation
wenjin272
left a comment
There was a problem hiding this comment.
The new tests only exercise a request-building IllegalArgumentException, not the provider/SDK exception propagation that is the main contract of this PR. They guard against restoring the exact broad catch blocks, but would still pass if a future change caught and wrapped only OpenAI SDK exceptions. The Azure path has no new regression coverage at all, and its IllegalArgumentException already propagated before this PR, so the current suite cannot distinguish its provider-error behavior. Could we use a local fake HTTP endpoint returning a 4xx for Chat Completions, Responses, and Azure, then assert the concrete SDK exception type plus its status/error payload? That would verify the behavior at the level where the issue occurs and cover all three changed paths.
|
@wenjin272 Thanks for review. You were right, and it wasn't hypothetical. I wrapped only Added Two caveats:
The two |
…pped The three OpenAI-family connections caught every exception from a chat call and rethrew it as a generic RuntimeException. The SDK exceptions already carry the HTTP status and the provider's error payload, so wrapping them forced callers to unwrap a cause to see what went wrong, and it diverged from the Python connections, which propagate the SDK exception as-is. The catch also covered request building, so a local validation failure such as a tool message missing its externalId surfaced as "Failed to call OpenAI chat completions API" rather than as the IllegalArgumentException it is. Generated-by: Claude Code 2.1.226
…endpoint The existing tests only exercised a request-building IllegalArgumentException, so they would still pass if a future change caught and wrapped only OpenAI SDK exceptions, and the Azure path had no coverage of the change at all. FakeOpenAIErrorEndpoint serves an OpenAI error envelope over loopback, letting each connection's error path run without a live API call. One test per connection asserts the concrete BadRequestException and the code it carries from the provider payload. A 400 is used because the SDK does not retry it, which keeps the exchange to a single request. Generated-by: Claude Code 2.1.226
b32dac3 to
0a00294
Compare
Linked issue: #936
Purpose of change
Case 3 of #936. The three OpenAI-family connections caught every exception from a chat call and rethrew it as a generic
RuntimeException, while the Python connections let the SDK exception propagate unchanged. A caller writing against both languages saw different failure types for the same provider condition.The SDK exceptions already carry the HTTP status and the provider's error payload, so the wrapper only forced callers to inspect a cause. This removes it from
OpenAICompletionsConnection,AzureOpenAIChatModelConnectionandOpenAIResponsesModelConnection, which aligns Java with Python. Local validation failures continue to raiseIllegalArgumentException.There is a second effect worth calling out. Each
tryalso covered request building, so a local validation failure was rewrapped too. A tool message missing itsexternalIdreached the caller asRuntimeException("Failed to call OpenAI chat completions API.")rather than as theIllegalArgumentExceptionthatOpenAIChatCompletionsUtilsactually threw. Azure already had acatch (IllegalArgumentException e) { throw e; }guarding against exactly this, which is now unnecessary and is removed with the rest.Out of scope: the wrappers around tool schema and tool argument JSON handling stay. Those wrap a checked
JsonProcessingExceptionfrom local work rather than a provider error, so they cannot propagate unwrapped.Tests
OpenAICompletionsConnectionTest.testRequestBuildingFailurePropagatesUnwrappedand the same test in a newOpenAIResponsesModelConnectionTestcallchatwith a tool message that has noexternalIdand assert theIllegalArgumentExceptionreaches the caller with its own type and message. Request building throws before the client is touched, so neither test needs a live API call.I verified both tests discriminate rather than pass by construction: restoring the old
catchblock failstestRequestBuildingFailurePropagatesUnwrappedon the assertion, since the wrapper produces a plainRuntimeExceptionthat is not anIllegalArgumentException.Azure gets no new test. Its
IllegalArgumentExceptionalready propagated, so the only behavior that changed there is what happens to a provider SDK exception, and reaching that needs a live call through the finalOpenAIClient.mvn -pl integrations/chat-models/openai testpasses, 72 tests. Spotless and RAT are clean.API
No public API change.
chatkeeps its signature; only the exception type a caller observes on failure changes, fromRuntimeExceptionwrapping the SDK exception to the SDK exception itself.Documentation
doc-neededdoc-not-neededdoc-includedWas this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.226