Apollo-server: Throwing circular object in resolver breaks apollo-server (500 HTTP error)

[ashtonsix]

OK, so the title isn't 100% accurate. Putting this code in a resolver:

await axios.zeit.post('/v2/domains/salamander.ai/records', {
  name: subdomain,
  type: 'A',
  value: instanceUrl
})

Lead to this error when the request failed (printed in console & returned to client):

TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
    at prettyJSONStringify (/home/ashton/code/6labs/packages/salamander-apollo/node_modules/apollo-server-core/dist/runHttpQuery.js:19:17)
    at Object.<anonymous> (/home/ashton/code/6labs/packages/salamander-apollo/node_modules/apollo-server-core/dist/runHttpQuery.js:297:33)
    at Generator.next (<anonymous>)
    at fulfilled (/home/ashton/code/6labs/packages/salamander-apollo/node_modules/apollo-server-core/dist/runHttpQuery.js:4:58)
    at process._tickCallback (internal/process/next_tick.js:68:7)

I wanted to simplify a bit so came up with this, but it worked fine (good error with message === "[Object object]"):

const a = {}
const b = {a}
a.b = b
throw a

Not sure why the first thing is bad but second is OK.

FYI I'm using [email protected]

[twelve17]

Workaround: use the formatResponse option function to catch the circular reference, re-serialize with a safe stringify function, before handing off to apollo:

import stringify from 'json-stringify-safe';
import { get } from 'lodash';

const logger = ...;

export const formatResponse = res => {
  const errors = get(res, 'errors', []);
  try {
    JSON.stringify(errors);
  } catch (err) {
    logger.debug(
      'formatResponse: circular reference(s) detected, removing them'
    );
    res.errors = JSON.parse(stringify(res.errors));
  }
  return res;
};

[goldo]

just came across this error. Shouldn't it be handle by apollo itself ?

[mdlavin]

I ran into this last week and it felt like fixing it in Apollo was a more scalable approach than having every consuming project adopt the workaround. I hope the fix is appropriate

[abernix]

@mdlavin Based on your comments on #2490, this seems pretty important to you. I hope we can help figure out the right solution, but could you — or anyone else experiencing this — please put together a runnable reproduction with a real use-case where you've encountered this? In theory, I do support the idea of being defensive and limiting the need for every developer to guard against such behavior. That said, I'm currently not sold on #2490 being the best solution, so I hope you'll hear me out.

Another solution here might be to limit the depth of properties on the error that we actually serialize, rather than introducing another package and a nested [Circular] message that someone might want to customize further. (I'm also not even sure if anyone would actually see this nested [Circular] string, but a reproduction would help with that understanding.)

Ultimately, maybe we're doing a lot more (deep) serialization here than we need to, which could be costly from a performance perspective. Let's make sure we're getting the relevant information that someone might need from an error, but only deep serialize the object if absolutely necessary?

[rocketraman]

@abernix In my case (and I believe the OPs case), I saw this happen any time calls to a backend via Axios in a resolver threw an error. The errors from Axios do contain these circular refs.

What I did is to wrap all my Axios calls in a function that catches errors and just extracts out the relevant info, and then throws a new error, so the workaround is pretty simple. But when I first encountered it it was problematic because all I was seeing from Apollo was the circular ref error, and not the original error, so it took a while just to figure out what was going on.

Therefore, I'd do two things here:

1) Limit the depth of the serialization, and

2) recover more gracefully from errors in the error handling code itself, so that similar problems in the future still give the dev some useful info.

[mdlavin]

Axios is a great example of errors with circular references. Another case
seems to be AWS X-Ray integration. Something about the way they augment the
thrown errors with extra context is circular. I'm happy to build a working
example of the failure based on Axios if you still need that.

The limited depth serialization seems like a fine alternative. Hopefully
this code is only hit during error conditions so, while performance is
always nice, it shouldn't be a critical path. Can you say more about the
easy solution for using an existing serializer? It seems quick, safe, and
only used when things fail. The idea of customization of Circular messages
seems unlikely, and edge case of an edge case.

On Thu, May 16, 2019 at 3:52 PM Raman Gupta [e-mail]
wrote:

@abernix https://github.com/abernix In my case (and I believe the OPs
case), I saw this happen any time calls to a backend via Axios in a
resolver threw an error. The errors from Axios do contain these circular
refs.

What I did is to wrap all my Axios calls in a function that catches errors
and just extracts out the relevant info, and then throws a new error, so
the workaround is pretty simple. But when I first encountered it it was
problematic because all I was seeing from Apollo was the circular ref
error, and not the original error, so it took a while just to figure
out what was going on.

Therefore, I'd do two things here:

1.

Limit the depth of the serialization, and
2.

recover more gracefully from errors in the error handling code itself,
so that similar problems in the future still give the dev some useful info.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apollographql/apollo-server/issues/1433?email_source=notifications&email_token=AAA2DF6PKPZPXWA3PMMPATLPVW3RTA5CNFSM4FMK7OAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVS4JZQ#issuecomment-493208806,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA2DF3QFYN3RUYPVEQFSETPVW3RTANCNFSM4FMK7OAA
.

[rlarcher]

I have faced this error when my resolvers throw errors that come directly from Axios on any error status code. For me, I was able to fix this by having my catch block on the upstream call do throw new Error(err) rather than throw err

[koenpunt]

Axios adds a toJSON on errors, which results in a "safe" to serialize error. Maybe the implementation could check for that method. This could of course also be done in the formatError method.

[koenpunt]

Sentry for example actually checks for such a method before logging the error; https://github.com/getsentry/sentry-javascript/commit/7e081b527e5c8f56d655239461134e2f12753ad8#diff-a88b07b3d334ad11919cdcaf8b58d2b1R211

[abernix]

@mdlavin

I'm happy to build a working example of the failure based on Axios if you still need that.

That's what I was looking for, yes.

Mostly because it would be super helpful to know if the Axios (which is clearly the common thread here) error has implemented a custom Error.prototype.toString and to better understand what sort of actual properties its attaching to the error, which seems likely to only happen with a reproduction.

Can you say more about the easy solution for using an existing serializer?

A straw-person proposal might be as simple as providing a custom serializer to JSON.stringify's second argument:

const a = {};
const b = {a};
a.b = b;
JSON.stringify(
  { prop: 1, crop: 2, top: b, mop: { stop: b }, err: new Error("womp") },
  (key, value) => (key ? "" + value : value)
);

// '{"prop":"1","crop":"2","top":"[object Object]","mop":"[object Object]","err":"Error: womp"}'

[mdlavin]

I wish I had more time, but I won't be able to build an example server
until Monday =(. I'll make sure to tackle this as soon as I can.

On Thu, May 16, 2019 at 6:27 PM Jesse Rosenberger [e-mail]
wrote:

@mdlavin https://github.com/mdlavin

I'm happy to build a working example of the failure based on Axios if you
still need that.

That's what I was looking for, yes.

Mostly because it would be super helpful to know if the Axios (which is
clearly the common thread here) error has implemented a custom
Error.prototype.toString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/toString
and to better understand what sort of actual properties its attaching to
the error, which seems likely to only happen with a reproduction.

Can you say more about the easy solution for using an existing serializer?

A straw-person proposal might be as simple as providing a custom
serializer to JSON.stringify's second argument:

const a = {};const b = {a};a.b = b;JSON.stringify(
{ prop: 1, crop: 2, top: b, mop: { stop: b }, err: new Error("womp") },
(key, value) => (key ? "" + value : value)
);
// '{"prop":"1","crop":"2","top":"[object Object]","mop":"[object Object]","err":"Error: womp"}'

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/apollographql/apollo-server/issues/1433?email_source=notifications&email_token=AAA2DF7NQP7A5IE3IDNVZ7DPVXNUVA5CNFSM4FMK7OAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVTHDJQ#issuecomment-493253030,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA2DFYN6SQTTD7NEH5E6NTPVXNUVANCNFSM4FMK7OAA
.

[mdlavin]

I put a demonstration of the problem up at https://github.com/mdlavin/graphql-circular-error-axios . It's a server with a single query which makes a bad request to itself that will fail using Axios. You can see from the output that the actual Axios error is lost to the client because it is hidden by the internal failure "Converting circular structure to JSON" from the Apollo Server.

While this project uses Axios, I've seen this problem with other failures as well (AWS X-Ray most commonly)

[mdlavin]

@abernix did you have a chance to checkout the case from my comment above? I understand that this failure might be an edge case and I'm happy to do the work to get it fixed if you want to describe what you are looking for.

[mdlavin]

For people hitting this bug due to Apollo + AWS X-Ray, I put a public Gist up at https://gist.github.com/mdlavin/4e7dffd5786341cb807f9add897b26fa with my workaround

[avalla]

For people hitting this bug due to Apollo + AWS X-Ray, I put a public Gist up at https://gist.github.com/mdlavin/4e7dffd5786341cb807f9add897b26fa with my workaround

I had a similar issue using with moleculer services from resolvers. Your workaround works fine, thanks!

https://gist.github.com/avalla/e0ea17cda24cc6b6f9f39a69609816d5

[mdlavin]

@abernix did you have a chance to checkout the case from my comment above? I understand that this failure might be an edge case and I'm happy to do the work to get it fixed if you want to describe what you are looking for.

[jbaxleyiii]

After a lot of thinking about this we don't think that Apollo Server having its own serialization and protection around this is the right approach. In fact, the spec isn't specific about serialization (https://graphql.github.io/graphql-spec/draft/#sec-Serialization-Format) meaning custom serialization may treat circular responses differently

[twelve17]

@jbaxleyiii I can see in principle that the implementation should not have an opinion on serialization, but the error from the OP is not that the serialization is not matching a certain opinion, it is that it is failing to serialise at all, and thus the actual error is not being returned. Apollo Server using JSON.serialize means it already has an opinion on how circular references should be serialised. Using something like safe serialize could avoid those issues and still allow implementors' use of formatResponse to treat circular responses in a different way.

[goldo]

agreed, I think this should not be closed

[abernix]

@twelve17 Object serialization can be complicated, but the burden of determining the correct serialization for an object is best defined by the implementation that generated the object.

Contrary to your claim above, Apollo Server's use of JSON.stringify on an error object does not mean that Apollo Server has an opinion on how to serialize an object with cycles in it, merely that we have an object that we would like to be converted into a JSON string.

In fact, it would be somewhat odd if we took an opinionated approach on that particular serialization concern since JSON.stringify itself already defers to an object's toJSON property (if it's defined as a function) to allow exactly this sort of customization by the object creator (rather than leaning on other implementations which need to serialize it).

And of course, there's more than one way to handle cycles!

So, what does a toJSON implementation look like, in practice?:

$ node
> const a = {}, b = {a};
undefined
> a.b = b;
{ a: { b: [Circular] } }
> const err = new Error('cycles');
undefined
> err.value = a
{ b: { a: [Circular] } }
> JSON.stringify(err)
TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
> err.toJSON = () => 'special!';
[Function]
> JSON.stringify(err)
'"special!"'
>

This allows an object to declare exactly how it should be serialized, which is even more relevant for a project like Axios who has consciously chosen to create cycles in their error objects.

A cursory search shows that this certainly isn't a new problem for Axios users, as is demonstrated by the existence of https://github.com/axios/axios/issues/836 and https://github.com/axios/axios/issues/1301. As recently as the end of last year they accepted https://github.com/axios/axios/pull/1625, which defines a toJSON function on their errors, in an attempt to resolve this precise problem!

Now, if Apollo Server's use of JSON.stringify to serialize errors into a JSON string is just straight up not obeying that object's toJSON implementation, then that seems like something worth investigating further, but that's different than taking an opinionated stance on how cycles should be serialized in an Error object.

Of course, luckily, there isn't much to debug in Apollo Server since we just call JSON.stringify:

https://github.com/apollographql/apollo-server/blob/d8ade4df4d343cd2ad441ebcb88c87a7f986dde6/packages/apollo-server-core/src/runHttpQuery.ts#L441-L443

As to the solution here? It looks like the fix in that PR (https://github.com/axios/axios/pull/1625) was released into [email protected] as of 2019-05-30T16:13:16.930Z. Sadly, that's was just a few days after the reproduction provided above by @mdlavin in https://github.com/apollographql/apollo-server/issues/1433#issuecomment-494368741 (which, tangentially, to be honest, is a very nice and clean reproduction! Thank you!) was made.

While slightly better timing here would have maybe avoided this frustation entirely, I'm happy to say that merely updating that reproduction to use [email protected] with npm install [email protected], solves the problem and changes its output from:

Before updating axios (With [email protected])

$ # With [email protected]
$ npm start

> [email protected] start /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios
> ts-node src/server.ts

Apollo Server on http://localhost:52196/graphql
A network error occurred while executing the query
The error was:
{
  "message": "Converting circular structure to JSON",
  "extensions": {
    "code": "INTERNAL_SERVER_ERROR",
    "exception": {
      "stacktrace": [
        "TypeError: Converting circular structure to JSON",
        "    at JSON.stringify (<anonymous>)",
        "    at prettyJSONStringify (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-server-core/src/runHttpQuery.ts:434:15)",
        "    at /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-server-core/src/runHttpQuery.ts:307:16",
        "    at Generator.next (<anonymous>)",
        "    at fulfilled (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-server-core/dist/runHttpQuery.js:4:58)",
        "    at process._tickCallback (internal/process/next_tick.js:68:7)"
      ]
    }
  }
}

After updating axios (to 0.19.0)

$ npm install [email protected]
... snipped ...
$ npm start

> [email protected] start /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios
> ts-node src/server.ts

Apollo Server on http://localhost:51836/graphql
{ Error: GraphQL error: write EPROTO 4370908608:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:../deps/openssl/openssl/ssl/record/ssl3_record.c:252:

    at new ApolloError (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:92:26)
    at /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:1581:34
    at /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:2001:15
    at Set.forEach (<anonymous>)
    at /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:1999:26
    at Map.forEach (<anonymous>)
    at QueryManager.broadcastQueries (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:1997:20)
    at /Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/apollo-client/bundle.umd.js:2124:19
    at Object.next (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/zen-observable/lib/Observable.js:308:23)
    at notifySubscription (/Users/jesse/Dev/apollo/apollo-platform-devkit/apps/graphql-circular-error-axios/node_modules/zen-observable/lib/Observable.js:130:18)
  graphQLErrors:
   [ { message:
        'write EPROTO 4370908608:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:../deps/openssl/openssl/ssl/record/ssl3_record.c:252:\n',
       locations: [Array],
       path: [Array],
       extensions: [Object] } ],
  networkError: null,
  message:
   'GraphQL error: write EPROTO 4370908608:error:1408F10B:SSL routines:ssl3_get_record:wrong version number:../deps/openssl/openssl/ssl/record/ssl3_record.c:252:\n',
  extraInfo: undefined }

[twelve17]

@abernix Thank you so much for the detailed explanation. I had forgotten about the delegation to each object’s “.toJSON” call. I appreciate your insight and I stand corrected. Also, thanks for the references to Axios’s fix!