Repository navigation
fix: check the HTTP status code for JSON responses - #471
Conversation
All four URLResponseHandler entry points, JSON and raw, with and without an error argument, now take the body from interceptedData. Middleware runs first, a transport error is rethrown unchanged, and throwIfErrorStatus then treats a status of 400 or above as a decoded APIErrorResponse or GeminiAPIErrorResponse, or as OpenAIError.statusError when the body is neither. The body is returned only after that check, and a missing body throws OpenAIError.emptyData; JSON decoding still happens next, so a 2xx payload that is an API or Gemini error document still surfaces that error. unwrap is removed, the changelog records the new JSON error behavior, and tests cover completion-handler, async, and Combine calls for an API error body, an HTML body, relaxed partial payloads, a 200 error document, a transport failure, and Gemini error shapes. Fixes MacPaw#469
…se-http-status # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
🟢 Approved
No active review issues are reported for the changes that check HTTP status before decoding JSON responses.
No open issues 🎉
What changed in this PR
Fixes #469 by checking HTTP status codes before decoding JSON responses, preventing failed requests from surfacing as decoding errors or relaxed-parsing successes.
Changes:
- Routes all four JSON and raw response handlers through shared middleware, transport-error, and HTTP-status handling.
- Adds regression tests across completion-handler, async, and Combine calls, including HTML bodies, relaxed parsing, and Gemini errors.
- Documents the changed JSON error behavior in the changelog.
| File | Description |
|---|---|
Tests/OpenAITests/JSONResponseStatusTests.swift |
Adds regression coverage for HTTP-status and error-decoding behavior. |
Sources/OpenAI/Private/URLResponseHandler.swift |
Centralizes response handling and checks HTTP status before JSON decoding. |
CHANGELOG.md |
Records the JSON error-handling fix and its caller-visible effects. |
Review effort: Balanced
|
Hi @mvanhorn , thanks for the PR! I'll take a loot at it tomorrow. There is one more conflict (similar to the one before), maybe you could fix it in the meantime. Thanks! |
…se-http-status Resolve the CHANGELOG conflict in [Unreleased]: keep upstream's Changed and Fixed entries and add this fix's entry under Fixed.
|
Merged main again in a4d0d75. Same CHANGELOG spot as last time: this fix's entry now sits under the existing |
There was a problem hiding this comment.
Thanks, this is the right fix for #469, and the tests cover the cases I'd worry about. I'm requesting changes for the middleware response point (inline on interceptedData); the rest are nits.
Gemini errors sent as a single object are tracked separately in #484, so testAsyncChatsThrowsStatusErrorForSingleGeminiErrorObject can stay as it is here.
URLResponseHandler.intercept now returns both the response and the data the middlewares return, and the status check reads that response. Before, a middleware's response was dropped and the original status was checked. Adds tests for a middleware that sets an error status and one that clears it.
Moves the UncheckedBox copies in FilesEndpointTests and JSONResponseStatusTests into one helper in Tests/OpenAITests/Mocks.
The helper runs the response middlewares and checks the status, so the name now says both. Also drop the redundant inner try in the two JSON interceptAndDecode overloads.
The JSONResponseStatusTests cases that stub a 200 status now use .successful(with:) instead of .errorResponse(statusCode: 200, ...), so the stub no longer reads as an error response. Cases with a real error status are unchanged.
|
Appreciate the merge, @nezhyborets. JSON responses with a status of 400 or above now surface as a decoded API error instead of passing through, which should save people some confusing debugging. |
What
All four URLResponseHandler entry points, JSON and raw, with and without an error argument, now take the body from interceptedData. Middleware runs first, a transport error is rethrown unchanged, and throwIfErrorStatus then treats a status of 400 or above as a decoded APIErrorResponse or GeminiAPIErrorResponse, or as OpenAIError.statusError when the body is neither. The body is returned only after that check, and a missing body throws OpenAIError.emptyData; JSON decoding still happens next, so a 2xx payload that is an API or Gemini error document still surfaces that error. unwrap is removed, the changelog records the new JSON error behavior, and tests cover completion-handler, async, and Combine calls for an API error body, an HTML body, relaxed partial payloads, a 200 error document, a transport failure, and Gemini error shapes.
JSON calls could turn a failed HTTP response into the wrong outcome. A body that is not an API error, such as an HTML page from a proxy or gateway, reached the caller as a DecodingError on the expected result type, with no status code attached. Under ParsingOptions.relaxed, a status of 400 or above whose body left required fields out could decode into a result filled with defaults and be returned as success. The JSON overloads of URLResponseHandler ran response middleware and then decodeJson, and never called throwIfErrorStatus. That status check existed only on the raw-data overloads. An API error was recognized only when decoding the expected type failed and the JSON decoder fell back to an API error document, which misses an HTML body and never runs when relaxed decoding already accepts the body as the result.
Fixes #469
Why
All four URLResponseHandler entry points, JSON and raw, with and without an error argument, now take the body from interceptedData. Middleware runs first, a transport error is rethrown unchanged, and throwIfErrorStatus then treats a status of 400 or above as a decoded APIErrorResponse or GeminiAPIErrorResponse, or as OpenAIError.statusError when the body is neither. The body is returned only after that check, and a missing body throws OpenAIError.emptyData; JSON decoding still happens next, so a 2xx payload that is an API or Gemini error document still surfaces that error. unwrap is removed, the changelog records the new JSON error behavior, and tests cover completion-handler, async, and Combine calls for an API error body, an HTML body, relaxed partial payloads, a 200 error document, a transport failure, and Gemini error shapes.
Affected Areas
Scoped to the files listed above; no other caller or consumer changes.