Repository navigation
Reject Fetch requests when a 2xx body cannot be parsed - #1846
Upgrade220 wants to merge 2 commits into
Conversation
Treat 204/205 and empty JSON bodies as no data instead of a parse error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 27005e4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Two ideas for the maintainers. Neither is implemented here; I'd like your opinion before adding public API. 1. Make strict parsing opt-inThis PR rejects 2xx responses with unreadable bodies by default. Plenty of real APIs declare The only escape hatch today is The alternative is a generation option, e.g.
The default could then flip in the next major. I'm happy to rework the PR this way if you prefer a 2. A
|
There was a problem hiding this comment.
All reported issues were addressed across 50 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Drop the write-only flag that broke noUnusedLocals with disableThrowOnError, cover 205 in tests, and mark the change as major. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
One more follow-up, about typing, since it's related to the proposals above. On a parse failure,
I'd prefer widening to Happy to add this to this PR or do it as a follow-up, whichever you prefer. |
|
One more topic. It's bigger than this PR, so it may deserve its own issue, but it came up while working on this one: the shape of Today every response has both export type HttpResponse<D, E> = Response & (
| { outcome: "success"; data: D }
| { outcome: "httpError"; error: E }
| { outcome: "parseError"; error: Error }
);Some notes from checking how feasible this is:
This would be a breaking redesign, so I'm not proposing it for this PR. Is it a direction you'd consider, and do you have a preference for the discriminant's name? If so, I can open a separate issue with a fuller design. |
yCheems
left a comment
There was a problem hiding this comment.
Hi! thanks for the PR
Main thing for us: empty body / 204 on a JSON endpoint now becomes data: null with no error, so we can't detect it anymore. Left details in the comments.
| return response[format](); | ||
| } | ||
| const text = await response.text(); | ||
| return text.trim() === "" ? null : JSON.parse(text); |
There was a problem hiding this comment.
The generator emits format: "json" (and takes T) from the first 2xx response of the operation, so this branch runs only for operations whose spec promises a JSON body. If that operation doesn't also declare a no-content response, an empty body here means the server broke the contract, just like HTML or truncated JSON. Now it resolves as success:
// spec: only 200 -> { id: string }; server: 200 with an empty body
const res = await api.getItem();
res.ok; // true
res.data; // null, typed as { id: string }
res.error; // null; before this PR it was SyntaxErrorThe changeset documents the null result, but not what it costs: callers could detect this case before (ok && error != null, the only option with disableThrowOnError), and now they can't. Could empty JSON bodies reject like other parse failures, unless the spec declares a no-content response (see the comment on line 189)? If that's too strict for APIs that misdeclare their responses, it seems to fit the opt-in strictness flag you proposed better than an unconditional default.
| } | ||
|
|
||
| protected parseResponseBody = async (response: Response, format: ResponseFormat): Promise<unknown> => { | ||
| if (response.status === 204 || response.status === 205) { |
There was a problem hiding this comment.
The 204/205 short-circuit fixes a real bug for operations that declare a no-content response next to a body:
responses:
"200": { content: { application/json: { schema: Item } } }
"204": { description: no content }This operation gets format: "json", and a legitimate 204 used to end up as a SyntaxError.
But the template can't tell that apart from an operation that declares only 200: Item and gets a 204 anyway. There the 204 is a contract violation, and it now resolves as success with data: null. The generator knows the declared statuses per operation. Could it emit the short-circuit only for no-content statuses the spec actually declares, and treat an undeclared 204 like any other unparseable body?
Related: when a 204 is declared, data is still typed Item (unless the schema is nullable), so the null is untyped either way.
Minor: the check runs before the format !== "json" branch, so it also applies to blob (generated for image/*) and formData. A 204 there now resolves to null instead of an empty Blob or a TypeError. Probably fine, but worth a line in the changeset.
|
|
||
| <% if (!config.disableThrowOnError) { %> | ||
| if (!response.ok) throw data; | ||
| if (!response.ok || r.error !== null) throw data; |
There was a problem hiding this comment.
Two notes on the rejection:
- The rejected
datais typedHttpResponse<T, E>, but on this patherroris aSyntaxError/DOMException/TypeError, notE. Acatchthat casts toHttpResponse<T, E>and reads fields of the API's error model getsundefined. This PR is alreadymajor, so I'd widenerrortoE | Error(your option 1 in the follow-up comment) in the same release rather than ship a second major for it. r.error !== nullworks as a "body read failed" signal only because the success path never assignserroron 2xx. That's correct today, but it's an implicit invariant. A short comment here, or a local emitted only inside the!disableThrowOnErrorblock so it doesn't tripnoUnusedLocals, would keep a future change from breaking it silently.
| expect(res.error).toBeNull(); | ||
| }); | ||
|
|
||
| test("treats an empty 200 body as no data, not an error", async () => { |
There was a problem hiding this comment.
This test pins the behavior discussed on the template. Whatever the decision, a few cases would make the contract explicit:
- an operation that declares both
200(JSON) and204, to pin that a declared204resolves withdata: null— and, if you go with declared statuses, that an undeclared one doesn't; - a JSON body of literally
nullon a200(currentlyok,data: null,error: null, the same observable result as an empty body); unwrapResponseDatawith an empty200(resolves tonullwith no signal, both before and after this PR);- optionally, a non-2xx response with an HTML body (e.g. a
502from a proxy) to pin that it rejects with theSyntaxErrorinerror.
Fixes #1838
Problem
When a 2xx response body can't be read in the declared
format, the generated Fetch client resolves withok: true,data: nulland theSyntaxError/AbortErrorstored inerror. Examples: HTML from a proxylogin page, truncated JSON, or a body stream aborted after the headers. Callers that branch on
okgetnulltyped as their DTO, and the failure surfaces later, far from where it happened. With
unwrapResponseDatathe callresolves to
nulland the error is lost entirely.The same parse path also turned legitimate no-body responses into errors:
204 No Content, and an empty200from an endpoint that declares a JSON body, both ended up with
error: SyntaxError.Solution
All changes are in
templates/base/http-clients/fetch-http-client.ejs:parseResponseBodymethod.204and205resolve tonullwithout reading the body. Forformat: "json", the client reads the body as text first. An empty or whitespace-only body givesnull;anything else goes through
JSON.parse. Other formats are unchanged.HttpResponsewith the parseerror in
error, the same shape non-2xx rejections already use, so existingcatchhandlers work. WithdisableThrowOnError, the response still resolves as before.I chose to reject rather than add a new field such as
parseError, because rejecting reuses the existing errorpath and adds no public API. The downside is that
errorcan still hold aSyntaxErrorrather thanE. Seethe follow-ups below.
datasetformat: "json")error: SyntaxErrordata: null,error: null204/205error: SyntaxErrordata: null,error: nullnullbodydata: nullerror: SyntaxErrorerror: SyntaxErrorerror: SyntaxErrorerror: AbortErrorerror: SyntaxErrorerror: nullerror: SyntaxErrorunwrapResponseDatanulldisableThrowOnErrorThis is a behavior change. APIs whose contracts declare JSON but whose servers send something else will
now reject, so the changeset is marked
major. I've left a comment below proposing an opt-in alternative,which would make this a
minorchange.The Axios client is unchanged: its default
transitional.silentJSONParsingreturns the raw string instead ofthrowing, so it doesn't have this problem in the same form.
Verification
tests/fetch-http-client-response-parsing.test.tsgenerates the Fetch client (default,unwrapResponseData, anddisableThrowOnErrorvariants), imports it, and drives it throughcustomFetchwith each case from the table. 7 of its 9 tests fail on
mainand all pass with this change. As far as Ican tell, it is the first test in the repo that runs a generated client rather than checking its text.
tsc --strict --noUnusedLocalswith the// @ts-nocheckheader removed. Generated files include that header by default, so without removing it the check proves nothing.
bun run test: 319/319 passing. The 141 updated snapshots contain only this template change, repeated inevery Fetch fixture.
To reproduce manually:
🤖 Generated with Claude Code