feat: handle circular structure serialization

[leandro-costa-oliveira]

Problem

res.json in lib/response.js calls stringify which fails if a object with circular structure is passed.

This is pretty common when you have calls using axios lib, the axios errors include the response object which have circular structure. Since a bunch a libs rely on axios, this is a pretty common cause of issues.

Example of Stack Trace

Doing a soap call with routing-controllers on top of express, fails to parse the error.

TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Agent'

--- property 'agent' closes the circle
    at JSON.stringify (<anonymous>)
    at stringify (/app/node_modules/express/lib/response.js:1034:12)
    at ServerResponse.json (/app/node_modules/express/lib/response.js:245:14)
    at ExpressDriver.handleError (/app/node_modules/routing-controllers/cjs/driver/express/ExpressDriver.js:351:26)
    at /app/node_modules/routing-controllers/cjs/RoutingControllers.js:115:36
    at process.processTicksAndRejections (node:internal/process/task_queues:104:5)

Tests

Added tests in test/res.json and test/res.jsonp simulating the circular structure.

[notadev99]

Real problem — axios-error objects with circular refs hitting res.json is common. A few things to work through before this changes a core contract:

Default behavior change. Today res.json throws on a circular structure; this makes it silently return 200 with "[Circular]" substituted. Some apps rely on that throw to surface the bug rather than ship malformed data to a client. Is silent recovery the right default, or should it be opt-in (an app setting / option) so existing throw behavior is preserved by default? That feels like a maintainer call to settle before the implementation details.

Error detection is string-based. isCircularJsonError matches /circular structure/i on the message, which is tied to the current V8 wording — if that string ever changes it silently stops catching and falls back to a raw throw. Not fatal on Node, but fragile; a comment noting the dependency would help.

Reimplemented array-replacer semantics. hasReplacerKey and the array branch re-derive what JSON.stringify already does natively with an array replacer, and matching its exact semantics (nested keys, the always-included top-level "") is easy to get subtly wrong. Is the array-replacer path needed for the fix, or could the fallback stay narrower?

Tests. The single case covers a top-level self-reference. Since the custom replacer also handles user replacer functions, array replacers, and nesting, it would be worth a case for each — especially a user-supplied replacer plus circular data, which is the trickiest path.

[leandro-costa-oliveira]

Real problem — axios-error objects with circular refs hitting res.json is common. A few things to work through before this changes a core contract:

Default behavior change. Today res.json throws on a circular structure; this makes it silently return 200 with "[Circular]" substituted. Some apps rely on that throw to surface the bug rather than ship malformed data to a client. Is silent recovery the right default, or should it be opt-in (an app setting / option) so existing throw behavior is preserved by default? That feels like a maintainer call to settle before the implementation details.

Totally agree, changed to be opt-in, keeping the default behavior unchaged, to avoid breaking peoples projetcs.

Error detection is string-based. isCircularJsonError matches /circular structure/i on the message, which is tied to the current V8 wording — if that string ever changes it silently stops catching and falls back to a raw throw. Not fatal on Node, but fragile; a comment noting the dependency would help.

Added a comment, mentioning that is dependent on v8 engine. Also the tests should cover it if there's some change on the copy.

Reimplemented array-replacer semantics. hasReplacerKey and the array branch re-derive what JSON.stringify already does natively with an array replacer, and matching its exact semantics (nested keys, the always-included top-level "") is easy to get subtly wrong. Is the array-replacer path needed for the fix, or could the fallback stay narrower?

Removed.

Tests. The single case covers a top-level self-reference. Since the custom replacer also handles user replacer functions, array replacers, and nesting, it would be worth a case for each — especially a user-supplied replacer plus circular data, which is the trickiest path.

Added more test cases, having coverage for default behavior that should throw the error and the new opt-in behavior handling the circular dependency.

[notadev99]

Thanks for turning this around so fast — this addresses everything cleanly:

• Opt-in via json circular with the default left as-throw is the right call; existing apps keep their current behavior.
• Handling the array-replacer case by throwing rather than reimplementing its semantics is a nice simplification — it keeps native JSON.stringify behavior intact on that path.
• The test matrix now covers the paths that matter: default-throws, opt-in basic, nested, and function-replacer + circular. The default-throws case in particular pins the contract so it can't silently regress later.

One non-blocking follow-up: json circular is a new app setting, so it'll want a History.md/changelog entry and a docs line alongside json escape / json replacer / json spaces so people can actually discover it. Otherwise this looks good from my side.

[leandro-costa-oliveira]

Thanks for turning this around so fast — this addresses everything cleanly:

• Opt-in via json circular with the default left as-throw is the right call; existing apps keep their current behavior.
• Handling the array-replacer case by throwing rather than reimplementing its semantics is a nice simplification — it keeps native JSON.stringify behavior intact on that path.
• The test matrix now covers the paths that matter: default-throws, opt-in basic, nested, and function-replacer + circular. The default-throws case in particular pins the contract so it can't silently regress later.

One non-blocking follow-up: json circular is a new app setting, so it'll want a History.md/changelog entry and a docs line alongside json escape / json replacer / json spaces so people can actually discover it. Otherwise this looks good from my side.

Done, log entry on History.md added. :)

[notadev99]

Looks good — the History.md line is clear and correctly scopes it to both res.json and res.jsonp. You've addressed everything I raised across both rounds; nothing further from me. Thanks for the quick turnarounds.