fix(proxiedResults): handle top level errors when using type merging #1648
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…with error on query toplevel
If the toplevel query of delegation has failed and reports an error on toplevel, with null as result,
`handleNull()` returns the errors has result, then `mergeProxiedResults()`, concats an error array consisting of
`[undefined]`, the errors props not being set on exception (in a previous fix on mergeProxiedResults, it was
assumed that sources where always execution results).
I suspect that the real problem is the `handleNull` with `handleResults` logics that mangles errors and values
results processing, but I'm not confortable enought to revize that.
The solution here is to twist the assomption that `sources` parameters of `mergeProxiedResults()` only
receives results object but also exceptions if any occured, and treat them accordingly:
- by putting the exception in the errors array
- by removing the related source from the results merge
without the fix, we would endup with the non-informative following message in query errors, triggered
when trying to process the `[undefined]` errors array mentionned above:
```
TypeError: Cannot read property 'path' of undefined
at Object.getErrors (graphql-tools/packages/utils/src/errors.ts:52:16)
at defaultMergedResolver (graphql-tools/packages/delegate/src/defaultMergedResolver.ts:25:18)
at resolveFieldValueOrError (graphql-tools/node_modules/graphql/execution/execute.js:485:18)
at resolveField (graphql-tools/node_modules/graphql/execution/execute.js:443:16)
at executeFields (graphql-tools/node_modules/graphql/execution/execute.js:280:18)
at collectAndExecuteSubfields (graphql-tools/node_modules/graphql/execution/execute.js:731:10)
at completeObjectValue (graphql-tools/node_modules/graphql/execution/execute.js:721:10)
at completeValue (graphql-tools/node_modules/graphql/execution/execute.js:609:12)
at completeValueCatchingError (graphql-tools/node_modules/graphql/execution/execute.js:513:19)
at resolveField (graphql-tools/node_modules/graphql/execution/execute.js:444:10) {
locations: [ [Object] ],
path: [ 'userById', 'id' ]
```
|
Closed in error! |
|
Looks good to me. throwing if any top level field gives you an error would mean that fewer fields get returned, doesn't seem sensible to drop the subschemas that are working |
|
The "mangling" happens because delegation is optimized for stitching so that users can simply return the result of delegation and allow GraphQL to do its thing. I don't see that changing anytime soon even though it is not so straightforward. |
|
Ho, the comment is not precise enough. By mangling, I just wanted to mean that the original error was replaced by another one, unrelated, and thrown due to invalid data (undefined entry) in the errors array. I've absolutely no problems with subschema errors aggregations :) |
yaacovCR
changed the title
|
The latest changes of this PR are available as alpha in npm: Quickly update your package.json by running: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: once again, I've doubts to this being the right fix. maybe errors on query toplevel fields should throw errors instead of merely go through merging, the thing is, yet again, I'm not familiar enough with underlying principles to figure out the right call. But somehow, this fix solves the issue at the right place — on processing subschema results.
If the toplevel query of delegation has failed and reports an error on toplevel, with null as result,
handleNull()returns the errors has result, thenmergeProxiedResults(), concats an error array consisting of[undefined], the errors props not being set on exception (in a previous fix on mergeProxiedResults, it was assumed that sources where always execution results).I suspect that the real problem is the
handleNullwithhandleResultslogics that mangles errors and values results processing, but I'm not confortable enought to revize that.The solution here is to twist the assomption that
sourcesparameters ofmergeProxiedResults()only receives results object but also exceptions if any occured, and treat them accordingly:without the fix, we would endup with the non-informative following message in query errors, triggered when trying to process the
[undefined]errors array mentionned above: