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
Throwing circular object in resolver breaks apollo-server (500 HTTP error) #1433
Comments
Workaround: use the
|
just came across this error. Shouldn't it be handle by apollo itself ? |
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 |
@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 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? |
@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:
|
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 ***@***.***> 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
<#1433?email_source=notifications&email_token=AAA2DF6PKPZPXWA3PMMPATLPVW3RTA5CNFSM4FMK7OAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVS4JZQ#issuecomment-493208806>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA2DF3QFYN3RUYPVEQFSETPVW3RTANCNFSM4FMK7OAA>
.
|
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 |
Axios adds a |
Sentry for example actually checks for such a method before logging the error; getsentry/sentry-javascript@7e081b5#diff-a88b07b3d334ad11919cdcaf8b58d2b1R211 |
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
A straw-person proposal might be as simple as providing a custom serializer to 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"}' |
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 ***@***.***> 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
<#1433?email_source=notifications&email_token=AAA2DF7NQP7A5IE3IDNVZ7DPVXNUVA5CNFSM4FMK7OAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVTHDJQ#issuecomment-493253030>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAA2DFYN6SQTTD7NEH5E6NTPVXNUVANCNFSM4FMK7OAA>
.
|
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) |
@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. |
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 |
@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. |
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 |
@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 |
agreed, I think this should not be closed |
@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 In fact, it would be somewhat odd if we took an opinionated approach on that particular serialization concern since And of course, there's more than one way to handle cycles! So, what does a $ 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 axios/axios#836 and axios/axios#1301. As recently as the end of last year they accepted axios/axios#1625, which defines a Now, if Apollo Server's use of Of course, luckily, there isn't much to debug in Apollo Server since we just call apollo-server/packages/apollo-server-core/src/runHttpQuery.ts Lines 441 to 443 in d8ade4d
As to the solution here? It looks like the fix in that PR (axios/axios#1625) was released into While slightly better timing here would have maybe avoided this frustation entirely, I'm happy to say that merely updating that reproduction to use Before updating
|
@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! |
If you still face this issue, change to |
OK, so the title isn't 100% accurate. Putting this code in a resolver:
Lead to this error when the request failed (printed in console & returned to client):
I wanted to simplify a bit so came up with this, but it worked fine (good error with
message === "[Object object]"
):Not sure why the first thing is bad but second is OK.
FYI I'm using
apollo-server-express@2.0.0-rc.5
The text was updated successfully, but these errors were encountered: