Skip to content

stitching should automatically update path of subschema errors #1650

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

Closed
wants to merge 1 commit into from

Conversation

yaacovCR
Copy link
Collaborator

@yaacovCR yaacovCR commented Jun 16, 2020

Currently, the path is just dropped, and so we rely on upstream graphql putting in the correct path when the error is reported, but this causes problems because field resolution may not occur in the desired order, and the wrong path may be reported.

This fix for #1047 includes multiple changes to error proxying, some now exposed to users.

= no longer relying on graphql-js to automatically set path of stitched error
= now relying on graphql-js not modifying path of resolver-returned error even if references a path different from resolver
= streamlines object error annotations to represent a map of errors by the field name
= introduces depth property for proxied results that must be updated during proxied result traversal for properly searching the error arrays
= requires updating of advanced wrapping/hoisting resolvers

@theguild-bot
Copy link
Collaborator

theguild-bot commented Jun 16, 2020

The latest changes of this PR are available as alpha in npm: 6.0.11-alpha-029b3b1.0

Quickly update your package.json by running:

npx match-version @graphql-tools 6.0.11-alpha-029b3b1.0

fix includes changes to error proxying
= no longer relying on graphql-js to automatically set path of stitched error
= now relying on graphql-js not modifying path of resolver-returned error even if references a path different from resolver
= streamlines object error annotations to represent a map of errors by the field name
= introduces depth property for proxied results that must be updated during proxied result traversal for properly searching the error arrays
= requires updating of advanced wrapping/hoisting resolvers
@yaacovCR yaacovCR marked this pull request as ready for review June 19, 2020 12:55
@yaacovCR
Copy link
Collaborator Author

To properly handle #1047, at least in my estimation, we need to change the internal API of the annotation of proxied results with errors to hard-code the error paths so that the correct initial path is reported, broken since its introduction (in some edge cases).

This requires breaking changes to the non-documented, but exported handleResult (#1568, #1569) and getErrors functions (#1481).

A decision is therefore required as to whether we are following semver with respect to non-documented, but exported functions. If we are, then this should be deferred to v7, otherwise we should do a minor release, I suppose.

@ardatan @marcammann @tlivings @Grmiade @mikebm, any thoughts?

My personal preference is to wait until v7.

At that time, I think we should also move all the errors stuff from /utils to /delegate, similar to Transforms, the errors stuff really belongs there.

@yaacovCR yaacovCR changed the title inital changes for re-attempt at #1047 stitching should automatically update path of subschema errors Jun 19, 2020
@yaacovCR
Copy link
Collaborator Author

yaacovCR commented Jun 19, 2020

Failing tests are not with respect to #1047, but rather with respect to the changes to the WrapField and HoistFields transforms.

These internal transforms have access to and utilize the internal error API and now require updating of the stitched error paths.

I did not fully flesh this out as not clear to me (yet) when this fix for #1047 will be merged, and I ideally would like to change those transforms soon to use the public API, as part of #1634 (comment)

@tlivings
Copy link
Contributor

Since the error contains a path, can't that path be used to replace a field on a result with an error object, which allows graphql-js to properly handle the errors on the response?

@yaacovCR
Copy link
Collaborator Author

The problem case is a schema with an object with two non-null fields mandatoryField1 and mandatoryField2 and one nullable field optionalField1 that can sometimes returns null without any associated error, sometimes can return null with an associated error, the error you get from the subschema may be that you are missing mandatoryField2, and stitching would have to be smart enough to recreate the parent object and place that error into mandatoryField1 AND mandatoryField2, but not into optionalField1. This requires complex parsing of the error tree -- AND MAY STILL require API changes in terms of how errors are annotated -- although maybe not, PRs are welcome.

The simpler solution in my opinion is that advanced by this PR which is not to recreate the parent object with an error tree filled with multiple errors, but just return the correct error for the parent object with the path for mandatoryField2.

I am definitely open to other solutions, but having banged my head against this for about a week now, I will not be implementing them myself (although PRs are welcome) and so I return to the above discussion about whether this should wait until v7 or be released as v6.1, @ardatan would love if you or others can weigh in, and would love love love if someone else would take this bug off my hands. :)

@yaacovCR yaacovCR mentioned this pull request Jul 13, 2020
@yaacovCR yaacovCR closed this Jul 13, 2020
@ardatan ardatan deleted the fix-errors-again branch July 14, 2020 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants