Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RFC] Add error path to response #230

Merged
merged 12 commits into from
May 12, 2017
Merged

[RFC] Add error path to response #230

Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
af713aa
Add error path to response
Jun 24, 2016
775589b
Updates to error result format
Oct 28, 2016
72a9c2c
Improve organization
Oct 28, 2016
72d5ab9
Clarify why this feature is useful
Oct 28, 2016
fd3cdea
Fix typo
Oct 28, 2016
70ae404
Fix index in error path
Oct 31, 2016
de1cc89
Add example for non-null field
Oct 31, 2016
14f5e53
Formatting, and change array to list
Nov 1, 2016
a8d7df1
Change "should" to "must"
Nov 8, 2016
59aaa22
Clarify what's required when data is null.
leebyron May 12, 2017
f2eff64
Move "errors" to top field.
leebyron May 12, 2017
b74fbb8
Another instance of errors over data
leebyron May 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 75 additions & 8 deletions spec/Section 7 -- Response.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,16 @@ error is a map.
If no errors were encountered during the requested operation, the `errors`
entry should not be present in the result.

If the `data` entry in the response is `null` or not present, the `errors`
entry in the response must not be empty. It must contain at least one error.
The errors it contains should indicate why no data was able to be returned.

If the `data` entry in the response is not `null`, the `errors` entry in the
response may contain any errors that occurred during execution. If errors
occurred during execution, it should contain those errors.

**Error result format**

Every error must contain an entry with the key `message` with a string
description of the error intended for the developer as a guide to understand
and correct the error.
Expand All @@ -135,14 +145,71 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

response field which experienced the error. This allows clients to identify
whether a `null` result is intentional or caused by a runtime error.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

represent array indices should be 0-indexed integers. If the error happens
Copy link
Contributor

@theorygeek theorygeek Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


For example, if fetching one of the friends' names fails in the following
query:

```graphql
{
hero(episode: $episode) {
name
heroFriends: friends {
id
name
}
}
}
```

The response might look like:

```js
{
"data": {
"hero": {
"name": "R2-D2",
"heroFriends": [
{
"id": "1000",
"name": "Luke Skywalker"
},
{
"id": "1002",
"name": null
},
{
"id": "1003",
"name": "Leia Organa"
}
]
}
},
"errors": [
{
"message": "Name for character with ID 1002 could not be fetched.",
"locations": [ { "line": 6, "column": 7 }],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"path": [ "hero", "heroFriends", 3, "name" ]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
]
}
```

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include an example of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link

@rwe rwe Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


GraphQL servers may provide additional entries to error as they choose to
produce more helpful or machine-readable errors, however future versions of the
spec may describe additional entries to errors.

If the `data` entry in the response is `null` or not present, the `errors`
entry in the response must not be empty. It must contain at least one error.
The errors it contains should indicate why no data was able to be returned.

If the `data` entry in the response is not `null`, the `errors` entry in the
response may contain any errors that occurred during execution. If errors
occurred during execution, it should contain those errors.