[BREAKING] Report malformed JSON responses as load errors - #9205
[BREAKING] Report malformed JSON responses as load errors#9205mvaligursky wants to merge 4 commits into
Conversation
A JSON asset with a malformed or empty body loaded successfully with a null resource. Also deliver one callback per network failure, and stop catching exceptions thrown by the request callback.
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
Automated PR review by Codex (GPT-5).
I traced JSON decoding, duplicate XHR terminal events, retries, concurrency-slot release, and callback exception propagation. The focused Http and AssetRegistry suites pass; changed-file ESLint, diff validation, and all CI checks are green.
One backward-compatibility issue remains: the public Http API now rejects a valid JSON document. Because the new test explicitly codifies this behavior, this would be a permanent API regression rather than an uncovered edge case.
| // delivered as a successful load with a null resource. The one false positive is a body | ||
| // of literal `null`, which is valid JSON but indistinguishable here: the raw text is | ||
| // not readable once the response type is JSON. | ||
| if (xhr.responseType === Http.ResponseType.JSON && response === null) { |
There was a problem hiding this comment.
[P1] Preserve valid JSON null
This condition makes every successful response containing the valid JSON document null fail through the public Http API. Existing callers can legitimately request nullable JSON, and the accompanying test explicitly locks in the breaking behavior. The ambiguity comes from asking XHR to parse the response: please retain/request the raw text and run JSON.parse in the engine (including the .json/octet-stream case), or otherwise detect malformed bodies without redefining valid JSON as an error.
Requesting the raw text and parsing it here distinguishes a parse failure from the valid document `null`. The JSON intent is checked before the content type, so a .json served as application/octet-stream still parses.
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5).
Re-reviewed the changes since commit 8c4682c. The previous finding is resolved: valid JSON null now succeeds while malformed and empty bodies produce parse errors, including the octet-stream case. The focused Http suite (25 tests) and AssetRegistry suite (24 tests) pass; changed-file ESLint, diff validation, and all CI checks are green.
A targeted compatibility probe found one new public-API regression in the raw-text implementation, noted inline.
| xhr.open(method, url, options.async); | ||
| xhr.withCredentials = options.withCredentials !== undefined ? options.withCredentials : this.withCredentials; | ||
| xhr.responseType = options.responseType || this._guessResponseType(url); | ||
| xhr.responseType = xhr._jsonResponse ? Http.ResponseType.TEXT : responseType; |
There was a problem hiding this comment.
[P1] Preserve the returned XHR's JSON contract
Http#get and Http#request publicly return this XHR, so substituting text is observable beyond the callback: a request made with responseType: Http.ResponseType.JSON now immediately reports xhr.responseType === 'text', and after completion xhr.response is the raw JSON string instead of the parsed value. Existing callers that attach their own load handler or inspect the returned request therefore break even though the engine callback still receives parsed data. A targeted probe expecting the previously requested type failed with expected 'text' to equal 'json'. Please keep raw-text parsing behind a loader-specific/internal path or otherwise preserve the documented returned-XHR behavior; if that cannot be preserved, this needs to be treated as an explicit breaking public API change rather than shipped as transparent error handling.
mvaligursky
changed the title
The returned request reports a text response type for JSON requests, so state that on options.responseType and cover it with a test. Also fix the retry test, which asserted a synchronous request count and so could not distinguish one retry from two.
Reading the body as text honours the charset declared in Content-Type, where the JSON response type always decodes as UTF-8, so a server wrongly declaring a charset produced silent mojibake. Override the mime type to force UTF-8. Letting an exception from the callback escape left the request with no completion at all, so a parser throwing after a successful fetch hung the load instead of failing it. Deliver the failure, and log the original error rather than silently replacing it.
|
Closing unmerged. The fix works and is covered by tests, but it changes the behaviour of every JSON load in the engine, and the risks listed in the description — chiefly the unmeasured peak-memory cost on large scene JSON and the untested charset handling — outweigh fixing an edge case that has been benign since 2018. Branch |
1 participant
Fixes #1175.
A JSON asset whose body is malformed or empty loaded "successfully" with
asset.resourceset tonulland no error fired. Asking XHR to parse the body itself makes a parse failure unreportable: it represents a failure and the valid documentnullidentically — as a null response — and the raw text cannot be read back once the response type is JSON.Not merged. See Risks below: the fix is sound and tested, but it changes the behaviour of the most heavily used loading path in the engine, and the risk was judged not worth taking for an edge case that has been benign since 2018.
Changes:
SyntaxError(carrying the character position) while the valid documentnullstays a successful load. Through the asset pipeline a broken body now fireserrorwithError loading JSON resource: <url> [SyntaxError: ...]instead of loading with a null resource. The JSON intent is checked before the content type, so a.jsonserved with a binary content type (1.62 is unable to load JSON files on FB Instant #5264) still parses.overrideMimeType('text/plain; charset=utf-8')forces UTF-8 decoding, since reading text would otherwise honour a declared charset where the JSON response type always decodes as UTF-8.readystatechangebefore firingerror, and the existing guard only covered the retry path — so with retries off (thehttp.getdefault) the callback ran twice with'Network error'.Breaking changes:
Http#get/Http#requestfor a JSON request reportsresponseType === 'text', and itsresponse/responseTexthold the unparsed JSON text rather than the parsed value. The callback — the documented way to receive the data — is unaffected. This is the observable cost of parsing in the engine: reading the raw text is the only way to tell a parse failure apart from the valid documentnull. Documented on theoptions.responseTypeparameter. Nothing insrc/,scripts/orutils/captures the return value ofhttp.get, so this affects external callers only.null. Every valid JSON document is unchanged, includingnull,falseand0.Risks:
config.json. A regression here breaks loading rather than degrading it.overrideMimeTypecall is verified only by hand against a live server — nise's fake XHR does not implement charset-dependent decoding, so no unit test covers it. A future refactor of the response-type handling would not notice it regressing..jsonurl that is not valid JSON used to resolve tonulland may have been silently ignored by an app; it now fails the asset load.test/assets/test-malformed.jsonis deliberately invalid JSON committed to the repo. CI is green today (no JSON lint or formatter gate), but adding one later would fail on it, and the.jsonextension is load-bearing for the test.options.retryingis now redundant with the per-request completion flag, and the JSON branch of the test helper's fake XHR is unreachable.