Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
Fix: Handle circular references in Http.request JSON processing#10744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Fix: Handle circular references in Http.request JSON processing#10744
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Introduces a `safeJsonStringify` utility to prevent crashes when `JSON.stringify` encounters circular references. This issue was particularly noted in Android when Http.request responses or debug information containing circular structures (e.g., from CSS-like objects) were processed.The `safeJsonStringify` utility replaces circular references with `"[Circular]"`.This commit applies `safeJsonStringify` to:- The `content.toJSON()` method in `packages/core/http/http-request/index.android.ts` to safely parse HTTP response bodies.- The debug data generation in `packages/core/http/http-request/index.android.ts` for `requestWillBeSent` and `responseReceived` events, ensuring headers and FormData content are safely stringified for the debugger.Additionally, unit tests for `safeJsonStringify` have been added in `packages/core/utils/safe-json-stringify.spec.ts` to cover various scenarios, including direct and nested circular references, and handling of `toJSON` methods.
…JSON methodEliminates unnecessary performance overhead by returning parseJSON(str) directly instead of converting the already-parsed JavaScript object to a JSON string and then parsing it back. HTTP response data doesn't contain circular references that would require safeJsonStringify processing.Signed-off-by: alvidofaisal <alvidofaisal@gmail.com>
coderabbitaibot commentedJun 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughA new utility, Changes
Sequence Diagram(s)sequenceDiagram participant Caller participant HTTPRequest participant safeJsonStringify Caller->>HTTPRequest: Initiate HTTP request HTTPRequest->>safeJsonStringify: Serialize headers/content for debugging safeJsonStringify-->>HTTPRequest: Return safe JSON string HTTPRequest-->>Caller: Complete request with debug infoPoem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/core/http/http-request/index.android.ts (2)
97-105:Double parse / stringify is wasteful
JSON.parse(safeJsonStringify(headers))incurs two passes over the same object just to deep-clone & de-circularise.
A shallow header object rarely needs deep traversal and never holds circular refs by spec.-const safeDebugHeaders = JSON.parse(safeJsonStringify(headers));+const safeDebugHeaders = { ...headers }; // headers are already plain key-value pairsIf genuine circularity is still possible, keep the safer version but gate it behind a cheap check:
constsafeDebugHeaders=Object.values(headers).some(v=>typeofv==='object') ?JSON.parse(safeJsonStringify(headers)) :{ ...headers};
280-304:Lost request body fidelity
FormDataandArrayBufferare reduced to either an almost empty object ({}after stringify) or the placeholder"[ArrayBuffer]".
This makes network-debugger output far less useful.Suggestion:
-} else if (options.content instanceof FormData) {- (request as any).postData = safeJsonStringify(options.content);+} else if (options.content instanceof FormData) {+ // Iterate entries to build a readable representation+ const fd: Record<string, unknown[]> = {};+ options.content.forEach((v, k) => {+ (fd[k] ||= []).push(typeof v === 'object' ? '[Object]' : v);+ });+ (request as any).postData = safeJsonStringify(fd); ...-} else if (options.content instanceof ArrayBuffer) {- (request as any).postData = '[ArrayBuffer]';+} else if (options.content instanceof ArrayBuffer) {+ (request as any).postData = `ArrayBuffer(${options.content.byteLength} bytes)`; }This preserves safety while giving developers actionable insight.
packages/core/utils/safe-json-stringify.spec.ts (2)
46-51:Spec comment block can be removed or turned intoit.todoLines 47-73 contain in-test commentary explaining potential mismatches; leaving this prose in the production test adds noise.
Convert to separate
describe/itblocks or delete after deciding desired behaviour.
74-75:Assertion may mask real behaviourIf
safeJsonStringify(undefined)is changed (see utility review) this expectation will fail.
Either split into its ownitwith explicit rationale or assert usingtoBeUndefined()to track intended semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/http/http-request/index.android.ts(3 hunks)packages/core/utils/safe-json-stringify.spec.ts(1 hunks)packages/core/utils/safe-json-stringify.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/http/http-request/index.android.ts (2)
packages/core/utils/safe-json-stringify.ts (1)
safeJsonStringify(1-15)packages/core/http/http-request/index.ios.ts (1)
request(75-233)
packages/core/utils/safe-json-stringify.spec.ts (1)
packages/core/utils/safe-json-stringify.ts (1)
safeJsonStringify(1-15)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
To be frank, I don't see why core needs this handling. Also, are there any http libraries that do this to begin with? |
I figured since .toJSON() is already there in core, making it tougher against these circular reference crashes would be a good thing. It directly tackles that crash in issue#10712. Totally agree, you can always grab the raw text. This just makes the helper itself a bit safer for folks who use it, so it doesn't unexpectedly break the app. Just trying to make it a bit more solid for that common case. |
CatchABus commentedJun 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I see your point here but I believe there's a benefit of leaving the handling of that case to developers. Personally, I always avoid circular refs and if I really need them then I try things like defining them as non-enumerable properties in my json objects. |
Uh oh!
There was an error while loading.Please reload this page.
PR Checklist
What is the current behavior?
JSON.stringifycrashes when encountering circular references in Android Http.request responses or debug information containing circular structures (e.g., from CSS-like objects). This causes app crashes when processing HTTP responses that contain objects with circular references.Additionally, the
toJSON()method in HTTP responses had unnecessary performance overhead by converting already-parsed JavaScript objects to JSON strings and parsing them back.What is the new behavior?
safeJsonStringifyutility that safely handles circular references by replacing them with"[Circular]"placeholderssafeJsonStringifyto HTTP response debug headers and request debug data to prevent crashescontent.toJSON()method to return parsed objects directly without redundant stringify-parse cyclestoJSONmethod edge casesFixes/Implements/Closes #[Issue Number].
#10712
Summary by CodeRabbit