Handle Errors with circular references gracefully

[mdlavin]

Fixes #1433

TODO:

• Update CHANGELOG.md with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[mdlavin]

Looking at the error in the linting build, I don't believe that I introduced this problem.

@martijnwalraven / @abernix if you'd like me to take a different approach fixing this problem, I'm happy to update the code. Just let me know what I can do

[mdlavin]

I rebased on master with hope that it would fix the linting failure in the last commit

[mdlavin]

I figured out the issue with the linting failures. It was a problem in the package-lock.json file that I fixed.

[mdlavin]

@martijnwalraven @abernix is there anything I should change about this PR?

[martijnwalraven]

@mdlavin Sorry about the radio silence, but both @abernix and myself have been preoccupied with other (but related!) projects. Although I realize circular references in errors can be a problem, it should never actually get to the point that we try to serialize these. That seems like a mistake in the current implementation.

We've experimented with making some related changes on a separate branch, getting rid of FormatErrorExtension altogether and clarifying the behavior of formatError. In short, errors should always be formatted before a response is serialized, respecting the contract of formatError in graphql-js, which means circular fields in Error instances shouldn't become part of the GraphQLFormattedError we serialize.

[mdlavin]

I'm happy to hear that the problem is getting fixed and I'm not attached to my version of the fix =). Is there hope for your branch merging soon or would be make sense to merge this fix now so that people are unblocked quickly?

[mdlavin]

@martijnwalraven any updates on your new branch? I'd love to help get this problem fixed if I can. I run into it daily and it's making my server return 500s =(.

If there is anything I can do, please point me at the work. I'm happy to fix whatever is needed.

[martijnwalraven]

@mdlavin That work was part of #2574 and has recently been merged into #2482. Could you try and see if 2.5.0-alpha.6 solves your issue?

[mdlavin]

Definitely. I've updated to alpha 6 and I'll watch our server to see if things improve. Thanks for the update!

[mdlavin]

I tested with 2.5.0-alpha.6 and I still see "Converting circular structure to JSON" errors in the log that I did before. I don't notice any improvement in the alpha release

[mdlavin]

I rested this with 2.5.0-rc.0 and it still throws "Converting circular structure to JSON" errors. @martijnwalraven did you expect this to be fixed? I can fix the merge conflicts and update this PR if you'd like me to

[mdlavin]

I rested this with 2.5.0 and it still throws "Converting circular structure to JSON" errors. @martijnwalraven did you expect this to be fixed? I can fix the merge conflicts and update this PR if you'd like me to

[abernix]

I've commented with some thoughts on the original issue #1433 (comment).

[abernix]

Thank you for opening this originally, and thank you very much for the excellent reproduction you provided in #1433 (comment).

That said, I think this PR is no longer necessary with the notes I've left in #1433 (comment). I hope you'll understand this approach and find this satisfactory!