graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

fix: return correct originalError after normalize for deprecated GraphQLError

Open marko-zub opened this issue 3 years ago • 6 comments

In graphql-js v16 property becomes nested error.originalError.originalError in toNormalizedOptions, but should be just error.originalError.

Added condition to check if error instance of GraphQLError then just return args[4].originalError.

Issue happens in executeFieldsSerially.

marko-zub avatar Dec 22 '22 16:12 marko-zub

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: marko-zub / name: marko (befe1abd6a14493df8b3c5fff1928846a98282c8)

@yaacovCR please approve github workflow. Lint errors has been fixed and added test. Thanks

marko-zub avatar Dec 29 '22 21:12 marko-zub

@yaacovCR What next steps to merge current PR?

marko-zub avatar Jan 03 '23 10:01 marko-zub

Hi @marko-zub --

As far as I follow here, this PR is asserting that when the GraphQLError constructor is supplied an object of type GraphQLError as the originalError, the new GraphQLError object's originalError property should point to the originalError within the supplied GraphQLError property.

Whereas the existing behavior (as far as I can tell) in both v15, v16, v17-alpha all assume that the new GraphQLError object's originalError property should point to the intermediate GraphQLError object which will have its own originalError property.

I suppose the question is whether the interpretation of the originalError should be more like rootError (this PR) or the immediatelyCausativeError (existing behavior).

Considering that the latter option preserves more data, I guess I am in favor. It is also similar to the new cause property for Error objects. In fact, perhaps it could eventually be superseded by this property.

Links: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#differentiate_between_similar_errors https://github.com/tc39/proposal-error-cause

But from your text above, it seems like you believe there is something that changed with v16 or something v16-specific that requires fixing/changing?

yaacovCR avatar Jan 04 '23 11:01 yaacovCR

@yaacovCR Issue specific for v16 we rely on error.originalError property (in v15 it works correctly), but in v16 after execution executeFieldsSerially and pass GraphQLError in locatedError we receive nested error.originalError.originalError property it's seems that reason is in toNormalizedOptions

Please check test which demonstrate the possible reason of issue.

marko-zub avatar Jan 04 '23 12:01 marko-zub

I don’t yet see the code change resulting in a change of behavior. Are you able to set up a small reproduction that works in version 15 and brakes with version 16?

yaacovCR avatar Jan 04 '23 12:01 yaacovCR

Closing for now, please feel free to reopen with more information!

yaacovCR avatar Sep 26 '24 21:09 yaacovCR