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

[RFC] Add error path to response #230

Merged
merged 12 commits into from May 12, 2017

Conversation

Projects
None yet
9 participants
@stubailo
Contributor

stubailo commented Oct 28, 2016

Reopening from #187 with a real branch.

This is helpful for clients to identify why there is a null in the response, because that could be due to an error or an actual null result.

stubailo added some commits Jun 24, 2016

Add error path to response
Just like it is helpful to know the location in the query associated
with an error, it's helpful to know the location in the response.
If the field which experienced an error was declared as `Non-Null`, the `null`
result will bubble up to the next nullable field. In that case, the `path`
for the error should include the full path to the result field where the error
occurred, even if that field is not present in the response.

This comment has been minimized.

@leebyron

leebyron Oct 29, 2016

Collaborator

Can we include an example of this?

This comment has been minimized.

@stubailo

stubailo Oct 31, 2016

Contributor

Done!

@leebyron

This comment has been minimized.

Collaborator

leebyron commented Oct 29, 2016

This is looking good, thanks for writing this

{
"message": "Name for character with ID 1002 could not be fetched.",
"locations": [ { "line": 6, "column": 7 }],
"path": [ "hero", "heroFriends", 3, "name" ]

This comment has been minimized.

@aweiker

aweiker Oct 29, 2016

Where is the 3 coming from? My interpretation was that it would indicate what position in the heroFriends array had the issue using a zero based index. So for this example I thought it would be [ "hero", "heroFriends", 1, "name"].

This comment has been minimized.

@stubailo

stubailo Oct 29, 2016

Contributor

Wow, I just got that index 100% wrong. Yes, it should be 1.

@leebyron

Some thoughts inline - I think the question of should/must is most important to get right.

The RFC RFC for some inspiration on requirement wordage: https://www.ietf.org/rfc/rfc2119.txt

@@ -135,14 +145,103 @@ locations, where each location is a map with the keys `line` and `column`, both
positive numbers starting from `1` which describe the beginning of an
associated syntax element.
If an error can be associated to a particular field in the GraphQL result, it
should contain an entry with the key `path` that details the path of the

This comment has been minimized.

@leebyron

leebyron Oct 31, 2016

Collaborator

Should we should tighten up this language from should to must? Ideally the path is always provided in relevant error cases by all implementations. Should gives an opt-out, but I'm not sure if there's a reasonable opt-out rationale here.

If you agree, perhaps this should be:

If an error is associated with executing a particular field in a GraphQL 
result (referred to elsewhere as a field error), it must contain an entry 
with the key `path` that details the path of the

This comment has been minimized.

@aweiker

aweiker Oct 31, 2016

If this is changed to a must, this means that all implementors must update the engines to conform to this change which may be difficult to do in regards to complexity as now it's just not a single piece of data that needs to be known, but the absolute location inside of a graph.

This comment has been minimized.

@helfer

helfer Oct 31, 2016

Contributor

I think the benefits of making error paths a required part of the GraphQL spec outweighs the initial cost of making the change. Without error paths, developers will adopt a variety of ways to get meaningful errors to clients, and the tools that handle those errors won't be interoperable. If we require error paths to be in the spec now, we can easily drop that requirement later, but it will become progressively harder and less useful to change the spec from 'should' to 'must' at some later point.

This comment has been minimized.

@stubailo

stubailo Nov 1, 2016

Contributor

I don't think "must" will actually make the language stronger, because the server can just pretend that the error can't be associated with a particular part of the result. No matter what we do, some errors won't have a path, so clients need to do something sane in that case.

This comment has been minimized.

@leebyron

leebyron Nov 2, 2016

Collaborator

I'm with @helfer on this one, especially the argument that we could always loosen in the future but not as easily tighten. "Should" is essentially a strong recommendation to do something but provides an opt out should there be a good reason, though I'm having trouble thinking of what a reason would be.

@aweiker yes though I'm not sure this should be a challenge given that the response path is data that already needs to be known in order to appropriate build a serialized JSON value. Right now most implementations probably implicitly know this by the nature of recursing to execute all fields, but some information about the ancestor fields can be kept pretty inexpensively, as has already been showed in the reference implementation.

Every field error (during execution) has a path, but query errors (e.g. all validation errors) do not have a path.

This field should be a list of path segments starting at the root of the
response and ending with the field associated with the error. Path segments
that represent object fields should be strings, and path segments that

This comment has been minimized.

@leebyron

leebyron Oct 31, 2016

Collaborator

just fields, not object fields? Since scalar fields would also be included in the path.

"errors": [
{
"message": "Name for character with ID 1002 could not be fetched.",
"locations": [ { "line": 6, "column": 7 }],

This comment has been minimized.

@leebyron

leebyron Oct 31, 2016

Collaborator

nit-picky, but space between }] to match the style used in the rest of the example

This field should be a list of path segments starting at the root of the
response and ending with the field associated with the error. Path segments
that represent object fields should be strings, and path segments that
represent array indices should be 0-indexed integers. If the error happens

This comment has been minimized.

@theorygeek

theorygeek Nov 1, 2016

Contributor

The spec usually uses the the word "list" instead of "array"; should this be changed? Maybe:

and path segments that represent list elements should be 0-indexed integers.

that represent object fields should be strings, and path segments that
represent array indices should be 0-indexed integers. If the error happens
in an aliased field, the path to the error should use the aliased name, since
it represents a path in the response, not in the query.

This comment has been minimized.

@theorygeek

theorygeek Nov 1, 2016

Contributor

Wouldn't a path in the response and a path in the query be essentially identical? I think you want to contrast a path in the response against a path in the schema, instead.

This comment has been minimized.

@aweiker

aweiker Nov 1, 2016

An example of paths being different is when fragments are used.

This comment has been minimized.

@stubailo

stubailo Nov 1, 2016

Contributor

Yes. Also, more differences between query and response:

  1. Arrays - those don't show up in the query
  2. Arguments/aliases - these are reduced to exactly one field in the response

I don't think the error path has too much to do with the schema.

This comment has been minimized.

@theorygeek

theorygeek Nov 2, 2016

Contributor

Gotcha. I guess I was thinking of an alias as part of the query.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 8, 2016

@leebyron should I also change "should" to "must" in the passage above?

If an error can be associated to a particular point in the requested GraphQL
document, it should contain an entry with the key locations with a list of
locations, where each location is a map with the keys line and column, both
positive numbers starting from 1 which describe the beginning of an
associated syntax element.

@stubailo stubailo referenced this pull request Nov 18, 2016

Merged

Design docs #923

@stubailo

This comment has been minimized.

Contributor

stubailo commented Nov 18, 2016

@leebyron ping about the question above.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Feb 28, 2017

@leebyron anything blocking this PR? Let me know if I can make any more improvements.

If the field which experienced an error was declared as `Non-Null`, the `null`
result will bubble up to the next nullable field. In that case, the `path`
for the error should include the full path to the result field where the error
occurred, even if that field is not present in the response.

This comment has been minimized.

@erydo

erydo Mar 14, 2017

How would this apply to lists of Non-Null objects with Non-Null fields? It's almost as though the error paths would be exposing some strange shadow tree, where the depth is related to the first resolution error, but distance that shadow tree goes past the first null value is related to the highest nullable field.

That shadow tree would have numeric indices for lists that are never returned. Is that the intention?

For example:

type Foo {
  listOfInts: [Int!]!
}
type Bar {
  listOfFoos: [Foo!]!
}
const Bar = new GraphQLObjectType({
  name: 'Bar',
  fields: (() => ({
    listOfFoos: {
      type: new GraphQLNonNull(new GraphQLList(new GraphQLNonNull(Foo))),
      resolve: () => ([
        {"listOfInts": [0, 1, 2, 3]},
        {"listOfInts": [0, 1, 2, null]},
        {"listOfInts": [0, 1, 2, 3]},
      ]);
    },
  }),
});

The result would be null due to the propgating non-null error, but the error path would be something bizarrely referencing non-returned data, like:

["bar", "listOfFoos", 1, 3]

Unless I'm misunderstanding?

This comment has been minimized.

@stubailo

stubailo Mar 14, 2017

Contributor

Yes that is the intention - the error path includes path segments that go deeper than the actual JSON the client returned. If the client doesn't need those path segments it can ignore them, and the server doesn't really need to do any extra work to make it happen in standard execution.

@IvanGoncharov

This comment has been minimized.

Collaborator

IvanGoncharov commented Mar 29, 2017

+ 1 for this.

We already depend on path in our proxy tool.

It is critical for us since we modify queries (inject __typename, remove mocked fields, ...) before sending them to a GraphQL server. Because of that we have to recalculate line and column properties of locations array based on path value.

Clarify what's required when data is null.
`{"data": null}` is a legal response when loading a top level field of a nullable type that intentionally returned null. Updating this language to ensure it remains legal.

@leebyron leebyron merged commit d554d12 into facebook:master May 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leebyron

This comment has been minimized.

Collaborator

leebyron commented May 12, 2017

Super excited about adding this into the next spec release. We've been using it in graphql-js experimentally for a while and it's quite helpful

IvanGoncharov added a commit to APIs-guru/graphql that referenced this pull request Jun 17, 2017

[RFC] Add error path to response (facebook#230)
* Add error path to response

Just like it is helpful to know the location in the query associated
with an error, it's helpful to know the location in the response.

* Updates to error result format

Following up on @leebyron's comments: facebook#187 (comment)

* Improve organization

* Clarify why this feature is useful

* Fix typo

* Fix index in error path

* Add example for non-null field

* Formatting, and change array to list

* Change "should" to "must"

* Clarify what's required when data is null.

`{"data": null}` is a legal response when loading a top level field of a nullable type that intentionally returned null. Updating this language to ensure it remains legal.

* Move "errors" to top field.

New convention for highlighting errors

* Another instance of errors over data
@hlship

This comment has been minimized.

hlship commented Jun 12, 2018

This does not explain how to represent the path for a selection that uses a fragment.

@IvanGoncharov

This comment has been minimized.

Collaborator

IvanGoncharov commented Jun 12, 2018

@hlship path represent:

path of the response field

So path is not affected by fragments at all since fragments doesn't change response.

@hlship

This comment has been minimized.

hlship commented Jun 12, 2018

@IvanGoncharov

This comment has been minimized.

Collaborator

IvanGoncharov commented Jun 12, 2018

@hlship I think spec will benefit from clarification.
This example can be rewritten as:
http://facebook.github.io/graphql/June2018/#example-bc485

{
  hero(episode: $episode) {
    name
    heroFriends: friends {
      ...characterInfo
    }
  }
}

fragment characterInfo on Character {
  id
  name
}

Will it make the spec more clear?

IvanGoncharov added a commit to APIs-guru/graphql that referenced this pull request Jul 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment