fix: don't throw SyntaxError on non-JSON error response bodies#605
Open
spokodev wants to merge 2 commits into
Open
fix: don't throw SyntaxError on non-JSON error response bodies#605spokodev wants to merge 2 commits into
spokodev wants to merge 2 commits into
Conversation
When a response has no Content-Type header, detectResponseType defaults to "json", so the response body is run through native JSON.parse. That parse ran before the status check and outside any try/catch, so a plain-text 4xx/5xx body (common from nginx/HAProxy/CDN error pages) threw a bare SyntaxError instead of a FetchError. That error carried no status/data, silently skipped retry for retryable codes such as 503, and bypassed ignoreResponseError/onResponseError. Wrap the parse and fall back to the raw text when it is not valid JSON. Valid JSON still parses to an object; error and non-JSON bodies resolve to a FetchError and go through the normal retry and error-handling paths. This restores the non-throwing behavior from before the switch to native JSON.parse, without re-adding the destr dependency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates ChangesNon-JSON Response Handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fetch.ts`:
- Around line 220-227: Scope the JSON parse fallback in fetch.ts so it only
applies to error responses, not all responses. Update the try/catch around
parseFunction(data) to preserve the current raw-text fallback only when the
response status indicates an error (for example, in the FetchResponse handling
path that builds context.response._data), and let malformed JSON on 2xx
responses surface normally. Use the existing parseFunction,
context.response._data, and FetchError/ignoreResponseError flow to locate and
adjust the logic.
In `@test/index.test.ts`:
- Around line 67-75: The `/flaky-503` test path in `test/index.test.ts` is still
returning a plain object on success, which lets h3 auto-add `Content-Type:
application/json` and defeats the missing-header scenario. Update the
`all("/flaky-503", ...)` handler so both the retry failure and the eventual
success branch return a `Response`, and explicitly delete the `content-type`
header in the success branch as well. Use the `flaky503Attempts` logic and the
`/flaky-503` handler to keep the test behavior focused on the missing-header
case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27c6b238-170f-44e3-b17e-cb48d7eedb27
📒 Files selected for processing (2)
src/fetch.tstest/index.test.ts
Comment on lines
+67
to
+75
| .all("/flaky-503", () => { | ||
| flaky503Attempts++; | ||
| if (flaky503Attempts < 2) { | ||
| const response = new Response("Service Unavailable", { status: 503 }); | ||
| response.headers.delete("content-type"); | ||
| return response; | ||
| } | ||
| return { ok: true }; | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files containing flaky-503 / relevant assertion ==\n'
rg -n --hidden --no-ignore-vcs 'flaky-503|valid JSON without content-type|content-type' test/index.test.ts
printf '\n== package files mentioning h3 ==\n'
rg -n --hidden --no-ignore-vcs '"h3"|h3@|from '\''h3'\''' package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null || true
printf '\n== test/index.test.ts excerpt ==\n'
sed -n '55,90p' test/index.test.ts
printf '\n---\n'
sed -n '296,315p' test/index.test.ts
printf '\n== repository files with h3 docs/source references ==\n'
rg -n --hidden --no-ignore-vcs 'JSON serializable|content-type|application/json|h3 docs' . 2>/dev/null || trueRepository: unjs/ofetch
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== h3 package files ==\n'
fd -a '^$' node_modules/.pnpm/h3@2.0.1-rc.5 -t f | head -n 200
printf '\n== h3 serialization/content-type references ==\n'
rg -n --hidden --no-ignore-vcs 'content-type|application/json|JSON serializable|stringified|toWebResponse|Response\(' node_modules/.pnpm/h3@2.0.1-rc.5 -g '!**/*.map' | head -n 200
printf '\n== likely entry files ==\n'
for f in \
node_modules/.pnpm/h3@2.0.1-rc.5/node_modules/h3/dist/index.mjs \
node_modules/.pnpm/h3@2.0.1-rc.5/node_modules/h3/dist/index.cjs \
node_modules/.pnpm/h3@2.0.1-rc.5/node_modules/h3/package.json
do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,260p' "$f"; }
doneRepository: unjs/ofetch
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f=node_modules/.pnpm/h3@2.0.1-rc.5/node_modules/h3/dist/h3.mjs
printf '\n== prepareResponse / toResponse excerpt ==\n'
sed -n '180,310p' "$f"
printf '\n== nearby helper for JSON response ==\n'
sed -n '250,285p' "$f"
printf '\n== exact lines around plain-object handling ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("node_modules/.pnpm/h3@2.0.1-rc.5/node_modules/h3/dist/h3.mjs")
lines = p.read_text().splitlines()
for i in range(180, 311):
if "jsonHeaders" in lines[i-1] or "typeof val" in lines[i-1] or "prepareResponse" in lines[i-1] or "FastResponse(JSON.stringify" in lines[i-1]:
print(f"{i}: {lines[i-1]}")
PYRepository: unjs/ofetch
Length of output: 7367
/flaky-503 still sets Content-Type on the success path.
return { ok: true } is auto-serialized by h3 as JSON with Content-Type: application/json, so this test doesn’t exercise the missing-header case it describes. Use a Response and delete the header there too.
Fix
.all("/flaky-503", () => {
flaky503Attempts++;
if (flaky503Attempts < 2) {
const response = new Response("Service Unavailable", { status: 503 });
response.headers.delete("content-type");
return response;
}
- return { ok: true };
+ const response = new Response(JSON.stringify({ ok: true }));
+ response.headers.delete("content-type");
+ return response;
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .all("/flaky-503", () => { | |
| flaky503Attempts++; | |
| if (flaky503Attempts < 2) { | |
| const response = new Response("Service Unavailable", { status: 503 }); | |
| response.headers.delete("content-type"); | |
| return response; | |
| } | |
| return { ok: true }; | |
| }) | |
| .all("/flaky-503", () => { | |
| flaky503Attempts++; | |
| if (flaky503Attempts < 2) { | |
| const response = new Response("Service Unavailable", { status: 503 }); | |
| response.headers.delete("content-type"); | |
| return response; | |
| } | |
| const response = new Response(JSON.stringify({ ok: true })); | |
| response.headers.delete("content-type"); | |
| return response; | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/index.test.ts` around lines 67 - 75, The `/flaky-503` test path in
`test/index.test.ts` is still returning a plain object on success, which lets h3
auto-add `Content-Type: application/json` and defeats the missing-header
scenario. Update the `all("/flaky-503", ...)` handler so both the retry failure
and the eventual success branch return a `Response`, and explicitly delete the
`content-type` header in the success branch as well. Use the `flaky503Attempts`
logic and the `/flaky-503` handler to keep the test behavior focused on the
missing-header case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A non-JSON error response with no
Content-Typeheader throws a bareSyntaxErrorthat bypasses retry and error handling.Repro
A local server returns a plain-text
503with noContent-Typeheader, like a bare error page from nginx/HAProxy/a CDN:Fetching it:
Actual: a
SyntaxError("Unexpected token 'S'...") with no.status,.data, or.response. Because it is not aFetchError, the retry path is skipped for the retryable503, andignoreResponseError/onResponseErrornever run.Expected: a
FetchErrorwith.status === 503and.data === "Service Unavailable", retried perretryStatusCodes, and swallowed byignoreResponseError.Root cause
The response body is parsed before the status check and outside any
try/catch. When there is noContent-Type,detectResponseType("")defaults tojson, so the raw body runs through nativeJSON.parse. A non-JSON body throws, and the throw escapes before the>= 400handling inonError, so it never becomes aFetchErrorand never reaches retry or the response-error hooks.Fix
Wrap the parse and fall back to the raw text when the body is not valid JSON:
Valid JSON still parses to an object via native
JSON.parse. Error and non-JSON bodies now resolve to aFetchErrorcarrying the status and the raw text, so they flow through the normal retry and error-handling paths. This restores the non-throwing behavior from before #520 switched the success path to nativeJSON.parse, without re-adding thedestrdependency.Tests
Added regression tests in
test/index.test.tsusing the existing local H3 server: a plain-text503withoutContent-Typeresolves to aFetchError(status503, data equal to the text) instead of aSyntaxError, that503is retried perretryStatusCodes,ignoreResponseErrorswallows it, and valid JSON still parses to an object.Verified red before the change (
SyntaxError, no retry) and green after. Full suite: 32/32 passing, lint and prettier clean.Summary by CodeRabbit
Bug Fixes
Content-Type, the original plain text body is preserved instead of failing during parsing.Tests
Content-Type, malformed JSON cases, and verification of retry and ignore-error behavior.