Preserve source error extensions when merging schemas

[hwillson]

Currently, when merging multiple schemas together, custom error extensions (like custom errors code's, custom error properties, etc.) are dropped when errors are returned from child resolvers. This is caused by the current schema stitching error handling code creating copies of errors before reporting them, but not including all properties of the original error in the copy.

This PR adjusts the schema stitching error handling code to preserve the originalError details when creating error copies. This helps mergeSchemas (and other parts of graphql-tools) include all source error extensions, when reporting errors through the merged schema.

This should help address:

• Schema stitching swallows original error stack traces from the children #743
• code always INTERNAL_SERVER_ERROR when using mergeSchemas #1037
• Proper error handling [minor changes required]: fixing Object object error messages #1046
• Apollo Error Code lost when using mergeSchemas apollographql/apollo-server#1582
• and probably many others ...

I'm going to publish an RC for this shortly, to give people a chance to test this and provide feedback, before this is merged.

[hwillson]

These changes are available in graphql-tools@next (5.0.0-rc.0). The changes here will have to be part of a major version bump, since people might be relying on the current broken schema stitching behaviour (e.g. they might be expecting stitched error codes to always be INTERNAL_SERVER_ERROR, instead of the proper source error code, and have logic based off of that).

[lucaslim]

Hey @hwillson, I think we need to return originalErrors here too
https://github.com/apollographql/graphql-tools/blob/7fa081e334d9bf7d73fe0f8abb7a627826e36beb/src/stitching/errors.ts#L51-L57

If and when a sub resolver throws an error and some how this line it does not return an array

https://github.com/apollographql/graphql-tools/blob/7fa081e334d9bf7d73fe0f8abb7a627826e36beb/src/stitching/errors.ts#L26

originalError will not be included and retained.

This works for me: KijijiCA@da27e57

[nechomer]

Hey, @hwillson, it is awesome you took the job of fixing the issue.
i have tried your solution in a personal project.
I am using Apollo 2 with schema stitching.
each of my backend services exposes its own schema.
if an error is being thrown from a child resolver of a backend service, it does not propagate fully to the gateway.

what @lucaslim referred to helped a bit.
only if adding https://github.com/apollographql/graphql-tools/issues/1046#issuecomment-457445794 too.

are you planning on addressing this issue too?

I have created a project with a micro-services setup of merged schemas.
this project uses apollo2 and throws errors from query resolvers and inner resolvers to experiment.

it can be found here https://github.com/nechomer/micro-services-graphql-apollo2

[hwillson]

@nechomer Just to confirm, did you try with graphql-tools@next (5.0.0-rc.0) across the board? What I mean by this, is that did you make sure you're only using graphql-tools@next in your services? apollo-server for example will pull in a different (non next) version of graphql-tools, since it isn't configured to use the RC version. You can verify this by running npm ls graphql-tools in your apps - to properly test graphql-tools@5.0.0-rc.0, it should be the only version you see show up. You might have to tweak your package-lock.json files to override the version apollo-server is using, then consider using npm ci (instead of npm i) just to make sure your package-lock's aren't touched.

Here's a quick repro I put together that has been updated to use graphql-tools@next (including an overridden package-lock for apollo-server), that shows this working: https://github.com/hwillson/graphql-tools-stitching-errors

Let me know if that helps - thanks!

[nechomer]

@hwillson , yes I did try setting package-lock with the appropriate version 5.0.0-rc.0 but it still does not work.

I have opened a branch with graphql-tools@next:
https://github.com/nechomer/micro-services-graphql-apollo2/tree/graphql-tools%40next

attached is a screenshot where it shows it is not working.
additionally, the outputs of npm ls graphql-tools in each of the services.

not working:

npm ls graphql-tools :

[owenallenaz]

We're dealing with this same issue, and one thing I've found is that there are 2 distinct issues, rather than just one. The first is the [object Object] problem which can be rectified by the prototype fixed called out in other tickets. The other is that if our resolver is a top level resolver, it comes through fine. It's when it's nested that it does not. So if I have query like { auth { accounts { count } } } and auth throws the error, it comes across correctly (with the prototype fix). If accounts throws the error, it does not, the stacktrace is eaten and replaced with one from the root apollo server instead of the child apollo server. The error code is lost as well. I believe it has to do with the marking of errors as OWN vs CHILDREN in the https://github.com/apollographql/graphql-tools/blob/master/src/stitching/defaultMergedResolver.ts .

[GarethSharpe]

@owenallenaz Could you point me in the direction of the prototype fix? Not having much fun with this and need a temporary solution. Cheers.

[owenallenaz]

https://gist.github.com/owenallenaz/d206464acae26c8612a63e0a69256e73 . This isn't a fully functional gist, since it's in a private repo, but I pulled out the meaningful sections of the code. I have only tested this in our env in which each root resolver is pulled from a remote schema. The root server's schema is just Query {} Mutation {} and then each microservice is pulled in via the links array.

[nechomer]

Hi @hwillson,
any update regarding this issue?

[pcorey]

Thanks for tackling this @hwillson. I was playing around with this fix locally, and it only seems to maintain originalError if an error is thrown on an array-type object. Your originalError extension only happens within the Array.isArray(object) guard.

I have a resolver that returns an object, not an array, and originalError isn't being kept around. Adding your originalError extension to this line, which is the fallthrough for non-array objects seems to fix my issue:

return {
  ...object,
  [ERROR_SYMBOL]: childrenErrors.map(error => ({
    ...error,
    ...(error.path ? { path: error.path.slice(1), originalError: error.originalError } : {})
  }))
};

originalError is kept around and I'm able to access it in my formatError callback further down the line.

Should this be included in the PR?

Thanks again!

[pcorey]

@benjamn I'm also interested in your opinion on my comment above, since you've got a pending review on this PR.

[RickyV33]

Are these changes going to make it in anytime soon?

[yaacovCR]

@pcorey, the below is your code snippet above, spread across a few extra lines, with my comments:

return {
  ...object,
  [ERROR_SYMBOL]: childrenErrors.map(error => ({
    ...error,                               // destructuring of all properties from "error"
    ...(error.path ? {                      // destructuring of a custom object which will supplement/replace properties from "error" 
      path: error.path.slice(1),
      originalError: error.originalError    // <== proposed addition
    } : {})
  }))
};

Looks to me like your more explicit destructuring should be unnecessary. However, I imagine that is not working out? Do you have a more complete code example you could share?

[pcorey]

@yaacovCR You're totally right. I'm not sure what I was thinking... I'll dig into it and see if I can reproduce what I was seeing. Disregard for now. Thanks!

[yaacovCR]

@pcorey, I think you were led down the wrong path because the code changes suggested by @hwillson uses that approach in the array section above your suggested line.

I think the underlying problem here was the treatment of GraphQLErrors as plain objects.

Check out #1147 for a different approach -- released to graphql-tools-fork.

[yaacovCR]

Closing in favor of #1307