[NO-REF] Preserve HTTP error details when the response body is not JSON - #267
[NO-REF] Preserve HTTP error details when the response body is not JSON#267pedro-lb wants to merge 1 commit into
Conversation
`throwHttpErrorFromResponse` parsed every non-2XX body as JSON. Error
responses are not always JSON: rate limit and gateway responses are
commonly plain text or HTML. For those, `response.json()` rejected and
the SDK surfaced the parse failure instead of the request failure:
SyntaxError: Unexpected token 'T', "Too many requests" is not valid JSON
at Response.json (node_modules/node-fetch/src/body.js:149:15)
The real status, statusText, url and headers were lost with it, which are
the details the function exists to attach.
Read the body as text and attempt to parse it, falling back to the raw
text as the error message, and to `HTTP <status>` when the body is empty.
`error.message` is now always a string, where previously it was
`undefined` for a JSON body without a `message` field.
All modules route their non-2XX responses through this helper, so this
covers every endpoint. `tracker.js` already handled non-JSON bodies this
way when emitting error events.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s shared HTTP error handling (helpers.throwHttpErrorFromResponse) so non-2XX responses preserve the actual HTTP failure details even when the response body is not JSON (e.g., rate limit / gateway responses that return plain text or HTML).
Changes:
- Reworks
throwHttpErrorFromResponseto read the response body viaresponse.text(), attempt JSON parsing, and fall back to raw text (orHTTP <status>when empty). - Ensures
error.messageis always a string (avoiding downstream failures like calling.toLowerCase()onundefined). - Adds test coverage for JSON body, plain-text body, and empty body scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/utils/helpers.js |
Changes HTTP error extraction to parse-from-text with safe fallback, preserving status/headers/url on non-JSON failures. |
spec/src/utils/helpers.js |
Updates existing test to use text() and adds coverage for non-JSON and empty-body error responses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Code Review
This PR fixes a real bug — throwHttpErrorFromResponse previously called response.json() unconditionally, swallowing the actual HTTP error when the body wasn't JSON — and the fix is correct and well-described.
Inline comments: 4 discussions added
Overall Assessment:
| expect(e.url).to.equal(responseData.url); | ||
| expect(e.headers).to.deep.equal(responseData.headers); | ||
| } | ||
| const error = await throwHttpErrorFromResponse(new Error(), { |
There was a problem hiding this comment.
Suggestion: The test pattern await fn().catch((e) => e) silently swallows any unexpected resolution — if throwHttpErrorFromResponse were accidentally changed to resolve instead of reject, the test would still pass (all the expect assertions would simply run against undefined). A more defensive pattern would be:
let error;
try {
await throwHttpErrorFromResponse(new Error(), { ... });
throw new Error('Expected throwHttpErrorFromResponse to throw');
} catch (e) {
error = e;
}Alternatively, with chai-as-promised: await expect(throwHttpErrorFromResponse(...)).to.be.rejectedWith(Error).
|
oops. |
Summary
throwHttpErrorFromResponsereads the error body as text and attempts to parse it, instead of assuming JSONerror.message, and toHTTP <status>when the body is emptyWhy
Every module routes its non-2XX responses through
helpers.throwHttpErrorFromResponse, which calledresponse.json()unconditionally. Error responses are not always JSON — rate limit and gateway responses are commonly plain text or HTML. For those, the parse rejects and the SDK surfaces the parse failure rather than the request failure:The
status,statusText,urlandheadersgo with it — the details the function exists to attach. A caller has no way to tell a rate limit from a bad gateway from a malformed payload.tracker.jsalready handles non-JSON bodies this way when it emits error events, so this brings the request path in line with it.Behavior change
error.messageis now always a string. Previously a JSON body without amessagefield producederror.message === undefined, which madeerror.message.toLowerCase()throw — there is a call site doing exactly that inspec/src/modules/catalog/catalog-groups-v2.js:223.Testing
npx mocha spec/src/utils/helpers.js— 22 passing, including the threethrowHttpErrorFromResponsecases (JSON body, plain-text body, empty body)npm run lint— clean, only the pre-existingno-consolewarning inspec/src/modules/catalog/catalog-facet-configurations-v2.js:70npm run test:types— passingNote on CI
This makes the failure legible, it does not make the suite pass.
run-tests.ymlruns 991 tests that each make a real API call, inmocha --parallel, against a rate limit the run exceeds. 27 of the last 30 runs on this workflow failed, on every branch including Dependabot's, going back to April. With this change those runs report429 / Too many requestswith the status attached instead of aSyntaxError, which is a prerequisite for fixing the suite but not a fix for it.The remaining failures are
Timeout of 5000ms exceededinbefore/beforeEachhooks. Worth noting--retries 3does not apply to hook failures, so a single rate-limited hook takes out its wholedescribeblock. Happy to open an issue with the details if that's useful.🤖 Generated with Claude Code