Skip to content

Reject Fetch requests when a 2xx body cannot be parsed - #1846

Open
Upgrade220 wants to merge 2 commits into
acacode:mainfrom
Upgrade220:fix/fetch-2xx-parse-error
Open

Upgrade220 wants to merge 2 commits into
acacode:mainfrom
Upgrade220:fix/fetch-2xx-parse-error

Conversation

@Upgrade220

@Upgrade220 Upgrade220 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1838

Problem

When a 2xx response body can't be read in the declared format, the generated Fetch client resolves with
ok: true, data: null and the SyntaxError/AbortError stored in error. Examples: HTML from a proxy
login page, truncated JSON, or a body stream aborted after the headers. Callers that branch on ok get null
typed as their DTO, and the failure surfaces later, far from where it happened. With unwrapResponseData the call
resolves to null and the error is lost entirely.

The same parse path also turned legitimate no-body responses into errors: 204 No Content, and an empty 200
from 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:

  • New parseResponseBody method. 204 and 205 resolve to null without reading the body. For
    format: "json", the client reads the body as text first. An empty or whitespace-only body gives null;
    anything else goes through JSON.parse. Other formats are unchanged.
  • Read/parse failures on 2xx responses now reject. The client throws the HttpResponse with the parse
    error in error, the same shape non-2xx rejections already use, so existing catch handlers work. With
    disableThrowOnError, the response still resolves as before.

I chose to reject rather than add a new field such as parseError, because rejecting reuses the existing error
path and adds no public API. The downside is that error can still hold a SyntaxError rather than E. See
the follow-ups below.

Response Before After
2xx, valid JSON resolves, data set unchanged
2xx, empty or whitespace-only body (format: "json") resolves, error: SyntaxError resolves, data: null, error: null
204 / 205 resolves, error: SyntaxError resolves, data: null, error: null
2xx, literal null body resolves, data: null unchanged (valid JSON)
2xx, HTML / plain text when JSON is declared resolves, error: SyntaxError rejects, error: SyntaxError
2xx, truncated JSON resolves, error: SyntaxError rejects
2xx, body stream aborted after headers resolves, error: AbortError rejects (aborts before headers already reject)
non-2xx, empty body rejects, error: SyntaxError rejects, error: null
non-2xx, non-JSON body rejects, error: SyntaxError unchanged
any rejecting case with unwrapResponseData resolved to null rejects
any case with disableThrowOnError resolves resolves (same fields as above)

This 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 minor change.

The Axios client is unchanged: its default transitional.silentJSONParsing returns the raw string instead of
throwing, so it doesn't have this problem in the same form.

Verification

  • New tests/fetch-http-client-response-parsing.test.ts generates the Fetch client (default,
    unwrapResponseData, and disableThrowOnError variants), imports it, and drives it through customFetch
    with each case from the table. 7 of its 9 tests fail on main and all pass with this change. As far as I
    can tell, it is the first test in the repo that runs a generated client rather than checking its text.
  • Generated clients for all three variants pass tsc --strict --noUnusedLocals with the // @ts-nocheck
    header 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 in
    every Fetch fixture.

To reproduce manually:

const client = new HttpClient({
  customFetch: async () => new Response("<html>login</html>", { status: 200 }),
});
await client.request({ path: "/x", method: "GET", format: "json" });
// before: resolves { ok: true, data: null, error: SyntaxError }
// after:  rejects with that HttpResponse

🤖 Generated with Claude Code

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-bot

changeset-bot Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 27005e4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
swagger-typescript-api Major

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

@Upgrade220

Copy link
Copy Markdown
Contributor Author

Two ideas for the maintainers. Neither is implemented here; I'd like your opinion before adding public API.

1. Make strict parsing opt-in

This PR rejects 2xx responses with unreadable bodies by default. Plenty of real APIs declare
application/json but send unquoted OK, an HTML page, or other non-JSON on success. For those users,
upgrading turns calls that used to resolve into rejections.

The only escape hatch today is disableThrowOnError. It's set at generation time and also stops rejections
for 4xx/5xx, so one badly specified endpoint would force a user to give up error rejections for the whole API.

The alternative is a generation option, e.g. strictResponseParsing / --strict-response-parsing, off by default:

  • off: 2xx parse failures resolve as today. The 204/205/empty-body handling from this PR still
    applies, since it's a fix either way.
  • on: 2xx parse failures reject (the behavior in this PR).

The default could then flip in the next major. I'm happy to rework the PR this way if you prefer a
non-breaking release.

2. A processResponse hook in ApiConfig

This was suggested in #1838, and it would work well with either default:

export interface ApiConfig<SecurityDataType = unknown> {
  // ...existing fields
  processResponse?: (
    response: HttpResponse<unknown, unknown>,
    params: FullRequestParams,
  ) => HttpResponse<unknown, unknown> | Promise<HttpResponse<unknown, unknown>>;
}

It would run after the body is parsed and before the throw decision. Why it's useful:

  • Per-endpoint leniency. A user with one badly specified endpoint can clear error (or fill in data) for
    that path only, instead of disabling throwing globally. That addresses the main downside of strict-by-default.
  • Body validation. It can reject a literal null (or any shape mismatch) where a DTO was expected. Neither
    this PR nor the client can tell that apart from a void endpoint.
  • Access to format and the other request params. customFetch never receives format because it is
    stripped before the call, so fetch-level wrappers can't tell whether a body was expected.
  • Only the Fetch client needs it. Axios already has interceptors.

One design point needs agreement: Response.ok is a read-only getter, so the hook can't flip success into
failure by changing a field. The simplest contract is probably "throw inside the hook to reject, return to
resolve". The client would then base its parse-failure rejection on whether error is still set after the hook runs.

Let me know which direction you'd like, and whether the hook should go in this PR or a separate one.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@Upgrade220

Upgrade220 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor Author

One more follow-up, about typing, since it's related to the proposals above.

On a parse failure, error holds a SyntaxError, DOMException (abort) or TypeError, while HttpResponse<D, E> declares error: E. This PR doesn't change that. There are two ways to make the type honest:

  • Widen to error: E | Error. All errors stay in one field, so callers have one place to check. Code that reads fields from the API's error type (e.g. res.error.code) would need to check instanceof Error first. That is a compile-time break, but it describes what can actually happen at runtime.
  • Add a separate parseError?: Error field. Existing code would keep compiling, but anyone handling errors in general would have to check two fields. That repeats the "response says two things at once" problem from Fetch client: 2xx responses with an invalid body resolve as success with data: null and error: SyntaxError #1838.

I'd prefer widening to E | Error. This PR is already a major change, so the type change can ship in the same release instead of needing another major later. One caveat: with throwing on by default, catch (e) receives unknown, so the wider type mostly helps disableThrowOnError users and code that casts the caught value to HttpResponse<D, E>.

Happy to add this to this PR or do it as a follow-up, whichever you prefer.

@Upgrade220

Copy link
Copy Markdown
Contributor Author

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 HttpResponse.

Today every response has both data: D and error: E, and one of them is always null at runtime. Callers can't tell which from the type, and #1838 is a direct consequence: ok: true together with an error. TypeScript can express this precisely with a discriminated union:

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:

  • ok can't be the discriminant. It's Response's read-only getter (status 200–299), so a 2xx response with an unparseable body has ok: true and can't fall into a failure branch without Object.defineProperty hacks or no longer returning the Response itself. A separate field avoids both problems and gives distinct error kinds.
  • Naming the discriminant. status and type are already Response properties, so they're out. I'd suggest outcome because it says what it is: how the request ended. Other options are kind (generic), state (sounds like a lifecycle stage, e.g. pending/loading) and result (awkward as result.result). I'm not attached to the name or to the camelCase values, so tell me what you prefer.
  • With throwing on (the default), the resolved type could be just the success branch. Errors reject, and the value catch receives is unknown, so the full union mainly benefits disableThrowOnError users and anyone who casts the caught value.
  • The success branch can't be precise from inside the client. HttpClient only receives T and E per route. For a route with 200: Pet, 204, 404: NotFound and 422: Invalid, the generator emits request<Pet, NotFound | Invalid>: the 204 is dropped (only the first success status is used), and errors lose their status codes. The route parser already keeps per-status types (responseInfos in schema-routes.ts), so the generator could emit { status: 200; data: Pet } | { status: 204; data: null }, and similar for errors. A catch-all branch is still needed for statuses the spec doesn't list.

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 yCheems left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SyntaxError

The 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) {

@yCheems yCheems Nov 6, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two notes on the rejection:

  • The rejected data is typed HttpResponse<T, E>, but on this path error is a SyntaxError/DOMException/TypeError, not E. A catch that casts to HttpResponse<T, E> and reads fields of the API's error model gets undefined. This PR is already major, so I'd widen error to E | Error (your option 1 in the follow-up comment) in the same release rather than ship a second major for it.
  • r.error !== null works as a "body read failed" signal only because the success path never assigns error on 2xx. That's correct today, but it's an implicit invariant. A short comment here, or a local emitted only inside the !disableThrowOnError block so it doesn't trip noUnusedLocals, 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 () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and 204, to pin that a declared 204 resolves with data: null — and, if you go with declared statuses, that an undeclared one doesn't;
  • a JSON body of literally null on a 200 (currently ok, data: null, error: null, the same observable result as an empty body);
  • unwrapResponseData with an empty 200 (resolves to null with no signal, both before and after this PR);
  • optionally, a non-2xx response with an HTML body (e.g. a 502 from a proxy) to pin that it rejects with the SyntaxError in error.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fetch client: 2xx responses with an invalid body resolve as success with data: null and error: SyntaxError

2 participants