Errors thrown from resolvers have the execution path #396

Slava · 2016-06-01T20:49:44Z

I tried my best to make changes as least disruptive as possible.

The execution path is different from the location on errors, it is the path on the actual data, rather than the query, so you can tell precisely which element's resolver caused the error.

This information is also propagated in the info argument to resolvers for an opportunity of better instrumentation for tools like Apollo.

This path is also passed in the `info` object to resolvers. This information is useful for ease of debugging and more detailed logging.

coveralls · 2016-06-01T21:12:31Z

Coverage increased (+0.01%) to 98.297% when pulling 710f92c on meteor:master into 359ec76 on graphql:master.

coveralls · 2016-06-01T21:12:32Z

Coverage increased (+0.01%) to 98.299% when pulling a5ef8c7 on meteor:master into 359ec76 on graphql:master.

helfer · 2016-06-01T21:28:34Z

@leebyron If you have some time, could you take a quick look at this PR and give us feedback? This is something that would be really useful for Apollo and other projects for a couple of reasons:

It provides better error messages to developers
It lets GraphQL clients do smarter things with errors, such as discarding part of the result and not updating the store.
It lets UI frameworks display errors in the context where they make the most sense.
It's a step towards sending normalized partial responses. Also helpful for streaming.
It's also a step towards doing logging and profiling that knows about causal relationships (which resolver had to wait for which other resolver).

stealthybox · 2016-06-03T20:45:54Z

I just wanted to say a huge thank you to @helfer and the meteor team for pushing this functionality in response to my questions on Slack.
The work on this is incredible, and Errors are much clearer now.

I would like to highlight that this PR is not just restricted to error handling.
@Slava mentioned that the executionPath is included in info for all resolvers.
The ability to leverage the executionPath in resolve functions allows for communication of that path to the client.
This is a powerful construct that allows is the center of memoization, query-minification, and subscription in projects like Falcor.

I have been playing with builds of this fork for the past few days now -- the functionality appears to work as expected with nested Type and Field level resolve functions when querying with redundant fields and relations.

This PR totally gets my thumbs up.
I'm super excited about it.

JeffRMoore · 2016-06-03T21:31:41Z

I really like the idea of giving path information to the resolve function. It feels like the right abstraction to me for describing location (vs AST).

However, it seems like a lot of work and memory allocation, creating a path array for every value in the result.

Worth it?

Slava · 2016-06-03T22:41:26Z

@JeffRMoore I think it is a fair question to ask, I was considering two possible implementations:

copy the whole path whenever there is a need to extend the list (implemented in this PR)
have an implementation of a persistent single-linked list (that looks like a tree, essentially)

I decided to go with a simpler implementation that behaves just like a regular JS array to begin with. Since we are talking about this information being useful in a context of instrumentation and profiling, it seems like it makes sense to sacrifice a couple of object allocations per request for better tooling to reveal a much bigger sink of time?

leebyron · 2016-06-10T15:12:07Z

Sorry for the delay - this looks really good. We need this for some of the future proposals as well.

Major feedback is that we shouldn't need to introduce another kind of Error. The path should just be added to GraphQLError when necessary.

I'll add a few inline comments as well.

leebyron · 2016-06-10T15:13:29Z

src/type/definition.js

@@ -483,6 +483,7 @@ export type GraphQLResolveInfo = {
  rootValue: mixed,
  operation: OperationDefinition,
  variableValues: { [variableName: string]: mixed },
+  executionPath: Array<string | number>


This would be more accurately called responsePath since it describes the path the value occurs in the response, rather than the charted path in the executor. Though I think simply path is descriptive enough.

coveralls · 2016-06-10T18:52:27Z

Coverage increased (+0.01%) to 98.297% when pulling eba89d4 on meteor:master into 359ec76 on graphql:master.

coveralls · 2016-06-10T18:56:05Z

Coverage increased (+0.01%) to 98.297% when pulling eba89d4 on meteor:master into 359ec76 on graphql:master.

…eFieldsSerially

coveralls · 2016-06-10T19:14:12Z

Coverage increased (+0.01%) to 98.297% when pulling 6301f5a on meteor:master into 359ec76 on graphql:master.

coveralls · 2016-06-10T20:23:43Z

Coverage increased (+0.01%) to 98.296% when pulling 7a5be6a on meteor:master into 359ec76 on graphql:master.

coveralls · 2016-06-10T20:41:26Z

Coverage increased (+0.01%) to 98.3% when pulling b33d1a1 on meteor:master into 359ec76 on graphql:master.

Slava · 2016-06-10T20:42:20Z

@leebyron I have updated the PR responding to all your comments. Let me know if there is anything else I can do to improve this PR. If everything is clear, we can squash everything into 1 change.

leebyron · 2016-06-10T21:16:28Z

src/execution/execute.js

  };

  // Get the resolve function, regardless of if its result is normal
  // or abrupt (error).
  const result = resolveOrError(resolveFn, source, args, context, info);

-  return completeValueCatchingError(
+  const completed = completeValueCatchingError(


You can continue to return directly.

leebyron · 2016-06-10T21:20:17Z

This is looking really nice, thanks for your quick follow-ups!

Slava · 2016-06-10T22:00:00Z

updated

coveralls · 2016-06-10T22:04:48Z

Coverage increased (+0.01%) to 98.3% when pulling 3641873 on meteor:master into 359ec76 on graphql:master.

helfer · 2016-06-10T22:08:09Z

@leebyron Could we turn off the coveralls comments? They're really breaking up the flow. I believe there's a repo setting in coveralls for that. We've done it for all the Apollo repos.

leebyron · 2016-06-10T22:10:46Z

Good idea, @helfer, just did that for this repo, will do it for the others soon.

leebyron · 2016-06-10T22:11:11Z

This is great work, @Slava!

Merge commit '826cba3b76e87dbd25a01db5150f89624adaab32' into sogko/master * commit '826cba3b76e87dbd25a01db5150f89624adaab32': 0.6.1 Fix tests for node v0.10, widen test matrix Fix test assertions for validation test when using custom TypeInfo (graphql#395) Removes depencency on babel-runtime. Upgrade to Flow v0.28. More specific return types from methods in Schema Only type Scalar config rather than Scalar type, improve schema builder types Introduce formal definition of "Thunk" to aid in fixing more issues uncovered by Flow v0.28 Additional flow issues corrected in anticipation of Flow v0.28 Fix some flow issues in anticipation of Flow v0.28 export type InputObjectConfigFieldMapThunk (graphql#411) Variable naming follow-up to path generation Errors thrown from resolvers have the execution path (graphql#396) Update all dependencies, include flow-specific lint handling Revert "Update babel-cli and flow-bin package references" (graphql#403) move babel config to the babelrc (graphql#399) Update babel-cli and flow-bin package references (graphql#388) Fix typo (graphql#387)

ghost added the CLA Signed label Jun 1, 2016

Errors thrown from resolvers have the execution path

a5ef8c7

This path is also passed in the `info` object to resolvers. This information is useful for ease of debugging and more detailed logging.

Slava mentioned this pull request Jun 9, 2016

Logging support #402

Closed

leebyron reviewed Jun 10, 2016
View reviewed changes

Slava added 4 commits June 10, 2016 11:32

Remove PathedError

8c1cb24

rename property executionPath to path

955c0a8

remove an unnecessary block

c31ba0d

info.executionPath -> info.path

eba89d4

a minor tweak to make the body of executeFields look closer to execut…

6301f5a

…eFieldsSerially

remove the unnecessary clone of info

7a5be6a

Add a test for a path with non-nullable fields

b33d1a1

leebyron reviewed Jun 10, 2016
View reviewed changes

Slava added 2 commits June 10, 2016 14:57

stylistic changes

89abc97

remove stray property

3641873

leebyron merged commit 6223245 into graphql:master Jun 10, 2016

OlegIlyenko mentioned this pull request Jun 11, 2016

Include execution path in error objects sangria-graphql/sangria#143

Closed

stubailo mentioned this pull request Sep 9, 2016

Handle partial responses when the server returns an error apollographql/apollo-client#51

Closed

josh mentioned this pull request Sep 19, 2016

Add JSON-path to execution errors rmosolgo/graphql-ruby#259

Merged

stubailo mentioned this pull request Nov 8, 2016

What is the best approach to manage custom user errors? #560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Errors thrown from resolvers have the execution path #396

Errors thrown from resolvers have the execution path #396

Slava commented Jun 1, 2016

coveralls commented Jun 1, 2016

coveralls commented Jun 1, 2016

helfer commented Jun 1, 2016 •

edited

Loading

stealthybox commented Jun 3, 2016

JeffRMoore commented Jun 3, 2016

Slava commented Jun 3, 2016

leebyron commented Jun 10, 2016

leebyron Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

Slava commented Jun 10, 2016

leebyron Jun 10, 2016

leebyron commented Jun 10, 2016

Slava commented Jun 10, 2016

coveralls commented Jun 10, 2016

helfer commented Jun 10, 2016 •

edited

Loading

leebyron commented Jun 10, 2016

leebyron commented Jun 10, 2016

Errors thrown from resolvers have the execution path #396

Errors thrown from resolvers have the execution path #396

Conversation

Slava commented Jun 1, 2016

coveralls commented Jun 1, 2016

coveralls commented Jun 1, 2016

helfer commented Jun 1, 2016 • edited Loading

stealthybox commented Jun 3, 2016

JeffRMoore commented Jun 3, 2016

Slava commented Jun 3, 2016

leebyron commented Jun 10, 2016

leebyron Jun 10, 2016

Choose a reason for hiding this comment

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

coveralls commented Jun 10, 2016

Slava commented Jun 10, 2016

leebyron Jun 10, 2016

Choose a reason for hiding this comment

leebyron commented Jun 10, 2016

Slava commented Jun 10, 2016

coveralls commented Jun 10, 2016

helfer commented Jun 10, 2016 • edited Loading

leebyron commented Jun 10, 2016

leebyron commented Jun 10, 2016

helfer commented Jun 1, 2016 •

edited

Loading

helfer commented Jun 10, 2016 •

edited

Loading