Inspect non-error types to produce helpful error messages for failing resolvers #1600

kommander · 2018-12-09T22:31:08Z

This is a proposal to handle previously serialised error objects according to section 7.1.2 of the spec. The result of receiving an error as object, without being an instance of the Error type, is an error message of [object Object]. The serialisation problem can be observed when working with remote executable schemas in Apollo GraphQL Tools, when throwing an Error in a remote resolver, it bubbles up as deserialised GraphQL Error according to the spec, but is not an Error instance.

A local resolver can also accidentally throw an Object/literal, which is valid JS, ~~but~~ and an error none-the-less.

try { throw { message:'woohaa!' }; } catch(e) { console.log(e, 'Is Error:', e instanceof Error); }
// > { message: 'woohaa!' } 'Is Error:' false

My take is that GraphQL should handle valid error Objects, as the spec states:

Every error must contain an entry with the key message with a string description of the error intended for the developer as a guide to understand and correct the error.

This may be fixed in graphql-tools, but I guess this is haunting other implementations as well. I'd event go as far as to deserialise the original stack trace.

I'll try to find a fix for remote executable schemas in apollograqphql/graphql-tools in the meantime, which could be a correct deserialisation in delegateToSchema.

EDIT: Heres a reproduction with apollo, resulting in an [object Object] error message in both cases https://github.com/kommander/graphql-bug

facebook-github-bot · 2018-12-09T22:31:14Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-12-09T22:59:06Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Neitsch · 2019-01-06T03:45:48Z

Hey @kommander , thank you for the PR. I understand that it is valid JS to throw an arbitrary object. However in that case we should rather look into stringifying the object maybe? This will solve it more generally and not assume that message contains sufficient information and is present on the object.

kommander · 2019-01-16T07:01:03Z

@Neitsch Thanks for getting back to me on this. Do you mean stringifying as in just JSON.stringify it if it's not an Error instance? That could work. Not a nice message, but at least something to debug on.

kommander · 2019-01-16T08:24:11Z

@Neitsch I tried with JSON.stringify and doesn't look too bad. I think I found a good place to test cover it as well.

Neitsch · 2019-01-18T15:19:32Z

@IvanGoncharov - do you have thoughts here? I don't have any particular investment in this, but can't hurt if it helps Apollo?

IvanGoncharov · 2019-01-18T15:27:48Z

src/execution/execute.js

+    return error;
+  }
+  if (error instanceof Object) {
+    return new Error(JSON.stringify(error));


We have special inspect function for serializing objects to a string

Thanks for your input. What are you using to inspect ?

Function called inspect :)
It's already imported here:

graphql-js/src/execution/execute.js

Line 13 in 4abedd3

import inspect from '../jsutils/inspect';

hahaha, thanks for pointing that out 👍

IvanGoncharov · 2019-01-18T15:29:24Z

src/execution/execute.js

+  if (error instanceof Error) {
+    return error;
+  }
+  if (error instanceof Object) {


error instanceof Object doesn't handle some corner cases, e.g. Object.create(null)
better to user typeof error === 'object'

IvanGoncharov · 2019-01-18T15:55:42Z

In general you shouldn't throw non-Error.

The serialisation problem can be observed when working with remote executable schemas in Apollo GraphQL Tools, when throwing an Error in a remote resolver, it bubbles up as deserialised GraphQL Error according to the spec, but is not an Error instance.

Note you can return instanceof Error from your resolvers or you can embedded remote errors directly into rootValue. For example here is how I implemented it:
https://github.com/APIs-guru/graphql-faker/blob/master/src/proxy.ts#L81-L103

This may be fixed in graphql-tools, but I guess this is haunting other implementations as well. I'd event go as far as to deserialise the original stack trace.

It should be definetly fixed in graphql-tools since you will loose a lot of info. For example error extensions.

My take is that GraphQL should handle valid error Objects, as the spec states:

Every error must contain an entry with the key message with a string description of the error intended for the developer as a guide to understand and correct the error.

This is citation from Section 7.1 Response Format so it's describe how error should be reported back to a client so it's not related to how resolver errors catched.
For that please see this section:
https://facebook.github.io/graphql/June2018/#sec-Errors-and-Non-Nullability

Even though I think it's incorrect behaviour to throw non-Error objects we should definetly handle it I propose to do the following:

function asErrorInstance(error: mixed): Error {
  if (error instanceof Error) {
    return error;
  }
  return new Error('Unexpected error value: ' + inspect(error));
}

kommander · 2019-01-18T16:53:31Z

In general you shouldn't throw non-Error.

Completely agree.

It should be definetly fixed in graphql-tools since you will loose a lot of info. For example error extensions.

Most definitely agreed as well.

I think this might have uncovered something else... resolveType reject with a valid Error comes through as string.

EDIT: I pushed the broken test so you can see whats happening. I couldn't find a lead right away.

IvanGoncharov · 2019-01-18T17:00:35Z

src/execution/__tests__/executor-test.js

@@ -393,6 +397,10 @@ describe('Execute: Handles basic execution tasks', () => {
        // eslint-disable-next-line no-throw-literal
        throw 'Error getting syncRawError';
      },
+      syncObjectError() {


Since we don't distinguish between different types of non-Errors I don't think we need to add this test.

kommander · 2019-01-18T17:04:39Z

I think I found the mistake, the test itself was rejecting with a non-error... fixing.

IvanGoncharov · 2019-01-18T17:21:00Z

src/execution/__tests__/executor-test.js

          locations: [{ line: 10, column: 9 }],
          path: ['asyncRawReject'],
        },
        {
-          message: '',
+          message: 'Unexpected error value: undefined',


During troubleshooting, I think it would be really helpful to receive this error instead of the empty string 🎉

👍 Yes! Just had a situation today again where the error was just empty... debugging something like that is hell.

kommander · 2019-01-18T17:23:10Z

I think travis had a problem with coverage reports. Tests are running fine. Maybe someone can retrigger it.

IvanGoncharov · 2019-01-18T17:24:02Z

I think travis had a problem with coverage reports. Tests are running fine. Maybe someone can retrigger it.

It's my fault I broke the coveralls 😢

IvanGoncharov · 2019-01-18T17:35:52Z

@kommander I think it's ready to merge.
One last issue is with PR title:

Handle previously serialized error in field resolver

I want to squash and merge this PR into a single commit but can't come up with a good commit message. Do you have any suggestions?

IvanGoncharov · 2019-01-18T17:37:28Z

@kommander Something that describes PR content instead of original intention behind it.

kommander · 2019-01-18T17:39:21Z

"Inspect non-error types to produce helpful error messages for failing resolvers (even though you should not throw literals, ever)"? Maybe drop the last part.

kommander · 2019-01-18T17:58:43Z

@IvanGoncharov Thanks, may Dijkstra be with you!

IvanGoncharov · 2019-01-18T18:50:26Z

@kommander Thanks for taking the time to submit PR and sorry for the long review.

KrisSiegel · 2019-03-25T21:46:15Z

Hey @IvanGoncharov, do you know when this code change will be released? I tested it in the npm branch and this fixes an issue I am currently having so I would love to be able to pull it from npm versus github :)

IvanGoncharov · 2019-03-25T23:47:33Z

@KrisSiegel I'm working on it ⏳
As a temporary solution, you can try to use NPM package built from master:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge

KrisSiegel · 2019-03-25T23:50:42Z

Thanks. Yeah, I'm currently doing that but targeting a specific commit hash so my build doesn't change each time CI kicks off :D

kommander mentioned this pull request Dec 9, 2018

Remote schema incorrectly serialize other contents if error presents ardatan/graphql-tools#905

Closed

facebook-github-bot added the CLA Signed label Dec 9, 2018

kommander mentioned this pull request Dec 11, 2018

Extended stitching features spring-media/themis-graphql#33

Merged

kommander added 2 commits January 16, 2019 08:05

Handle previously serialized/stringified errors

6cf65c8

Stringify non-error objects, test cover

c5a139d

kommander force-pushed the fix-remote-result-error-handling branch from fa26adc to c5a139d Compare January 16, 2019 08:21

Satisfy linter

97118fe

IvanGoncharov reviewed Jan 18, 2019

View reviewed changes

Better type check

0c1e110

Use inspect util

21d3fb0

Inspect non-errors generally

64e8cff

IvanGoncharov reviewed Jan 18, 2019

View reviewed changes

Fix abstract promise test, reset error tests

6f3672f

IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Jan 18, 2019

IvanGoncharov reviewed Jan 18, 2019

View reviewed changes

IvanGoncharov approved these changes Jan 18, 2019

View reviewed changes

IvanGoncharov changed the title ~~Handle previously serialized error in field resolver~~ Inspect non-error types to produce helpful error messages for failing resolvers Jan 18, 2019

IvanGoncharov merged commit 5b17d41 into graphql:master Jan 18, 2019

IvanGoncharov mentioned this pull request Jan 22, 2019

Handling non Error instance errors is broken #1393

Closed

snyk-bot mentioned this pull request Aug 31, 2019

[Snyk] Upgrade graphql from 14.1.1 to 14.5.0 sandorTuranszky/production-ready-expressjs-server#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inspect non-error types to produce helpful error messages for failing resolvers #1600

Inspect non-error types to produce helpful error messages for failing resolvers #1600

kommander commented Dec 9, 2018 •

edited

facebook-github-bot commented Dec 9, 2018

facebook-github-bot commented Dec 9, 2018

Neitsch commented Jan 6, 2019

kommander commented Jan 16, 2019

kommander commented Jan 16, 2019

Neitsch commented Jan 18, 2019

IvanGoncharov Jan 18, 2019

kommander Jan 18, 2019

IvanGoncharov Jan 18, 2019

kommander Jan 18, 2019

IvanGoncharov Jan 18, 2019

IvanGoncharov commented Jan 18, 2019

kommander commented Jan 18, 2019 •

edited

IvanGoncharov Jan 18, 2019

kommander commented Jan 18, 2019

IvanGoncharov Jan 18, 2019

kommander Jan 18, 2019

kommander commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019 •

edited

IvanGoncharov commented Jan 18, 2019

kommander commented Jan 18, 2019

kommander commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019

KrisSiegel commented Mar 25, 2019 •

edited

IvanGoncharov commented Mar 25, 2019

KrisSiegel commented Mar 25, 2019

Inspect non-error types to produce helpful error messages for failing resolvers #1600

Inspect non-error types to produce helpful error messages for failing resolvers #1600

Conversation

kommander commented Dec 9, 2018 • edited

facebook-github-bot commented Dec 9, 2018

facebook-github-bot commented Dec 9, 2018

Neitsch commented Jan 6, 2019

kommander commented Jan 16, 2019

kommander commented Jan 16, 2019

Neitsch commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Jan 18, 2019

kommander commented Jan 18, 2019 • edited

Choose a reason for hiding this comment

kommander commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kommander commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019 • edited

IvanGoncharov commented Jan 18, 2019

kommander commented Jan 18, 2019

kommander commented Jan 18, 2019

IvanGoncharov commented Jan 18, 2019

KrisSiegel commented Mar 25, 2019 • edited

IvanGoncharov commented Mar 25, 2019

KrisSiegel commented Mar 25, 2019

kommander commented Dec 9, 2018 •

edited

kommander commented Jan 18, 2019 •

edited

IvanGoncharov commented Jan 18, 2019 •

edited

KrisSiegel commented Mar 25, 2019 •

edited