fix(error): add toJSON to APIError so response headers serialize correctly#1848
fix(error): add toJSON to APIError so response headers serialize correctly#1848anishesg wants to merge 1 commit intoopenai:masterfrom
Conversation
…ectly - [ ] I understand that this repository is auto-generated and my pull request may not be merged Signed-off-by: anish k <ak8686@princeton.edu>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d003bf731
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| status: this.status, | ||
| headers: this.headers ? Object.fromEntries(this.headers.entries()) : undefined, | ||
| error: this.error, | ||
| code: this.code, |
There was a problem hiding this comment.
toJSON() now returns a fixed object literal, so JSON.stringify() no longer includes enumerable fields added by subclasses (for example OAuthError.error_code) or by runtime assignment (for example APIConnectionError.cause). Before this change those properties were serialized automatically, so this is a regression for log pipelines or cross-process error transport that depend on those fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah good point, I'll switch it to use Object.assign to pull in the subclass properties instead of hardcoding the fields.
There was a problem hiding this comment.
Yeah good point, I'll switch it to use Object.assign to pull in the subclass properties instead of hardcoding the fields.
| code: this.code, | ||
| param: this.param, | ||
| type: this.type, | ||
| request_id: this.requestID, |
There was a problem hiding this comment.
The serializer emits request_id, but the public error property is requestID; prior to toJSON() the serialized key was requestID because it was an enumerable own property. This silently changes the JSON contract and can break consumers that parse serialized errors to read the request ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point, I'll rename the property to request_id to match the serialized output.
There was a problem hiding this comment.
Good point, I'll rename the property to request_id to match the serialized output.
1 participant
Changes being requested
The native
Headersclass in Node.js does not enumerate its entries when passed toJSON.stringify, so callingJSON.stringify(error)on anyAPIError(includingRateLimitError) produces"headers": {}— hiding every response header includingx-ratelimit-reset-requests,x-ratelimit-remaining-tokens, and the other rate-limit headers users need for retry logic.The fix adds a
toJSON()method toAPIErrorinsrc/core/error.tsthat convertsthis.headersto a plain key/value object viaObject.fromEntries(this.headers.entries()).this.headersitself still holds the nativeHeadersinstance so callers using.headers.get(...)are unaffected; the change only improves serialization.Additional context & links
Headersdoes not implementtoJSON, andJSON.stringifyonly walks own enumerable properties, so the internal slot holding headers entries is invisible to it.error.headers.get('x-ratelimit-reset-requests')directly will see no change in behavior.Fixes #1477