Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Simplify response model to avoid Converting circular structure to JSON error #5332

Open
Woodz opened this issue Dec 2, 2022 · 9 comments

Comments

@Woodz
Copy link

Woodz commented Dec 2, 2022

Is your feature request related to a problem? Please describe.

It is common practice to log every request and response from an application, using JSON.stringify. However, this doesn't work in Axios because the response model is very complex (it contains a copy of the request and cyclical types). I am not the first person to experience this based on the number of issues and StackOverflow questions raised about this:

It is quite time consuming to research this issue to identify the solution - that only response.data is convertable to JSON so the code should be changed to JSON.stringify(response.data). In addition, this is only a subset of the information needed to be logged (e.g. HTTP response code, etc), so for developers to correctly log the Axios response, it is something like:
JSON.stringify({ data: response.data, status: response.status, statusText: response.statusText, headers: response.headers )

or alternative we could delete the fields within response that don't belong there, namely config and request, but then that means mutating the response, i.e.

delete response.config;
delete response.request;
console.log(JSON.stringify(response))

Describe the solution you'd like

The AxiosResponse should contain only those fields that are part of the response, so I propose that config and request are removed. It is not the responsibility of the AxiosResponse object to contain the corresponding request - if needed, a new object such as AxiosOperation can be created to contain the request, response and config.

In addition, the config and request fields should be examined for circular dependencies and the modelling adjusted to avoid them, since it causes unnecessary confusion to have a circular structure.

Obviously this is a massive breaking change, but given the issues that this causes in many downstream libraries and services that use Axios, the severity of this issue should not be understated.

Describe alternatives you've considered

We could consider creating a custom function to JSONify the response or could try to mitigate this issue via documentation, but I don't believe that the adoption of either option would be easy given the huge number of projects and libraries that use Axios.

Additional context/Screenshots

No response

@Woodz Woodz added the feature label Dec 2, 2022
@DigitalBrainJS
Copy link
Collaborator

I seem to have fixed this in v1.2.0 (#5247). Is this issue still relevant for the latest version?

@deenairn
Copy link

deenairn commented Feb 18, 2023

We're certainly seeing circular reference issues in production, and it's not clear what is causing the problem, only that it happens intermittently, and the fact a JSON.stringify circular reference exception occurs means I am never able to track down the source of the problem. Even in version 1.3.3 I have observed this behaviour, which is the latest axios. Universally, the callstack is always as below, happens on many URLs, always for post. seemingly at random:

Pre-Vue stage failed for <URL>: TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'TLSSocket'
    |     property 'parser' -> object with constructor 'HTTPParser'
    --- property 'socket' closes the circle
    at stringify (<anonymous>)
    at stringifySafely (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:1371:37)
    at Object.transformRequest (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:1434:14)
    at transform (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:1855:15)
    at Object.forEach (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:279:10)
    at Object.transformData (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:1854:9)
    at Axios.dispatchRequest (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:3470:31)
    at Axios.request (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:3831:33)
    at Axios.httpMethod [as post] (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:3870:19)
    at Function.wrap [as post] (/home/site/wwwroot/node_modules/axios/dist/node/axios.cjs:29:15)

For reference, to match line numbers up, here's the 1.3.3 tag: https://github.com/axios/axios/blob/v1.3.3/dist/node/axios.cjs

What I believe is happening, is that in stringifySafely the httpsAgent is being passed in, and it has circular references and it fails. Oddly, this behaviour is inconsistent and although it happens rarely, it happens enough to be noticeable in production, but infrequently enough to be a total nuisance to replicate. I've not found the pattern that causes the problem to occur.

The defect here is that the stingifySafely is not actually safe if the passed parameter is not a string already... unless an encoder is passed. But the origin of the stack trace is from a line from the default implementation of transformRequest as it does not pass in a custom encoder, and JSON.stringify cannot deserialize circular references, and it then fails.

Could JSON.stringify be replaced for library that will handle circular references, or provide a mechanism to substitute the encoder parameter within the defaults? I could replace the default transform myself, but the code would be identical except for the line calling stringifySafely where I would pass another JSON serializer, so it will be a nuisance to maintain.

@spaskhalov
Copy link

Same problem here.
Have got this exception in my jest tests, if axios throw error.

node:internal/child_process/serialization:159
    const string = JSONStringify(message) + '\n';
                   ^

TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'Object'
    |     property 'socket' -> object with constructor 'Object'
    --- property '_httpMessage' closes the circle
    at stringify (<anonymous>)
    at writeChannelMessage (node:internal/child_process/serialization:159:20)
    at process.target._send (node:internal/child_process:847:17)
    at process.target.send (node:internal/child_process:747:19)
    at reportSuccess (/Users/serj/Projects/Vybes/node_modules/jest-worker/build/workers/processChild.js:82:11)
node:internal/child_process/serialization:159
    const string = JSONStringify(message) + '\n';

@nofearOnline
Copy link

@spaskhalov Did you manage to solve it? I hit the same problem.

@lpradovera
Copy link

@nofearOnline same here, if anyone has any pointers, happy to hear them. I only happens on an error.

@spaskhalov
Copy link

@nofearOnline the problem only occurs if the exception is not handled. So I just rewrote my tests so that in cases where I want to check that my methods throw exceptions, I wrap them in a try catch and check that all checks were passed with expect.assertions(number). When no exceptions are thrown, the tests pass normally.

@AdyRock
Copy link

AdyRock commented Jun 12, 2023

I'm also getting the occasional report of a similar error:

TypeError: Converting circular structure to JSON
--> starting at object with constructor 'Object'
| property 'httpsAgent' -> object with constructor 'Agent'
| property 'sockets' -> object with constructor 'Object'
| ...
| property 'errored' -> object with constructor 'Object'
--- property 'config' closes the circle
at stringify ()
at writeChannelMessage (node:internal/child_process/serialization:127:20)
at process.target._send (node:internal/child_process:839:17)
at process.target.send (node:internal/child_process:739:19)

I'm using an https.agent to include a local certificate. Strange that it only occurs a few times out of many successful calls. I try to catch the error with a try/catch but it is still crashing my app.
Does anyone know if there is a solution to this.

@kyle-514
Copy link

kyle-514 commented Aug 8, 2023

Axios error handling documentation references using error.toJSON() to convert error object into a JSON object. Not much else is mentioned. Was this created specifically to address the lack of support in JSON.stringify() for circular references?

It's not clear if toJSON() is used in issues being reported here after the pull request resolving circular reference handling mentioned here was merged, or if these issues are being caused by using JSON.stringify() instead. So does .toJSON() work reliably or not? Thanks.

For the sake of completeness, other recommendations I came across:

  1. install fast-safe-stringify package and use safeStringify() in place of JSON.stringify() when needed.
  2. Use node.js native util.inspect(object) which automatically replaces circular links with "[Circular]" specifically for debugging/logging, with the caveat that the result can't be used with JSON.stringify() as a string is returned. Example: import { inspect } from 'util'; console.log(inspect(error))

facebook-github-bot pushed a commit to facebook/facebook-business-sdk-codegen that referenced this issue Aug 14, 2023
Summary:
[BizSDK][NodeJS] Remove dependency request from package.json

`request` package has long been deprecated and shall be replaced.

Remove `request` and `request-promise` dependencies. Use `axios` as a replacement.

Update `api.js` and `http.js` during migration.

Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332.

Reviewed By: mengxuanzhangz

Differential Revision: D48271770

fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
facebook-github-bot pushed a commit to facebook/facebook-nodejs-business-sdk that referenced this issue Aug 14, 2023
Summary:
[BizSDK][NodeJS] Remove dependency request from package.json

`request` package has long been deprecated and shall be replaced.

Remove `request` and `request-promise` dependencies. Use `axios` as a replacement.

Update `api.js` and `http.js` during migration.

Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332.

Reviewed By: mengxuanzhangz

Differential Revision: D48271770

fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
@jonyfSaiflow
Copy link

Just faced this issue, this proposal helped me find the issue. On a project with NestJS, I started returning an axios result, instead of result.data

MihaelKonjevi added a commit to MihaelKonjevi/facebook-nodejs-business-sdk that referenced this issue Jan 31, 2024
Summary:
[BizSDK][NodeJS] Remove dependency request from package.json

`request` package has long been deprecated and shall be replaced.

Remove `request` and `request-promise` dependencies. Use `axios` as a replacement.

Update `api.js` and `http.js` during migration.

Set `response` to `response.data` due to the return format of Axios response format change. Known issue axios/axios#5332.

Reviewed By: mengxuanzhangz

Differential Revision: D48271770

fbshipit-source-id: f22e5e336cbf9f69cdd597fea62ca08822ce72fd
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

No branches or pull requests

10 participants