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.

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 #187

Closed
wants to merge 1 commit into from
Closed

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented Jun 24, 2016

Just an idea for discussion - adding the path to the response, like this implementation in graphql-js:

path: [ 'hero', 'friends', 0, 'secretBackstory' ]

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.

This is already implemented in errors thrown from resolvers in graphql-js, so it would just be a matter of having the right error formatting to include that information.

This would be extremely helpful to differentiate when a field is null in the result because the result was actually null, and when it is because of a runtime error in the resolver.

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.
@ghost ghost added the CLA Signed label Jun 24, 2016
@stubailo stubailo changed the title Add error path to response [RFC] Add error path to response Jun 24, 2016
@ghost ghost added the CLA Signed label Jul 12, 2016
@dylanahsmith
Copy link
Contributor

This would be extremely helpful to differentiate when a field is null in the result because the result was actually null, and when it is because of a runtime error in the resolver.

I think the simpler way of addressing that concern would be to omit the field in the result if there was an error rather. Also, errors in non-null type fields could just not be propagated such that missing data could be used as an indicator of where the errors occur.

However, even with that change your proposal would be useful to associate errors with specific elements in an array.

@stubailo
Copy link
Contributor Author

There are a few reasons this is better than just returning undefined in this case:

  1. Being able to identify specific array elements that had errors
  2. Being able to associate specific error messages with specific parts of the result

You can imagine if you are rendering a huge page with one GraphQL query, it would be nice to show any relevant errors in the correct part of the UI, rather than at the top level. Of course, it is possible to build a custom error tagging system for this, but it would be much nicer if there was this relatively small built-in thing, especially since we already have line and column position for the error.

@dylanahsmith
Copy link
Contributor

You can imagine if you are rendering a huge page with one GraphQL query, it would be nice to show any relevant errors in the correct part of the UI, rather than at the top level.

It sounds like you are using top-level errors as a way to display user input errors, which isn't the intended purpose for those errors.

The top-level errors entry was meant to have error messages that are intended for the developer. This is mentioned in the graphql spec in the Errors section:

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.

This has been further clarified in a graphql spec issue (#117 (comment))

@stubailo
Copy link
Contributor Author

stubailo commented Jul 19, 2016

It sounds like you are using top-level errors as a way to display user input errors, which isn't the intended purpose for those errors.

No, I'm talking about a situation where a fetch to a backend API failed, causing that part of the GraphQL query to return null. It doesn't have anything to do with user input, and could happen on basically any fetch from a database or other API.

If the GraphQL errors field is intended to be used for runtime errors (which seems to match @leebyron's statement that it's for "exceptional" situations), then I'd say this use case certainly falls into that category. The only other option I see is for basically every single type to be a union type with Error, which seems undesirable since it will clutter the schema, and there is already an "official" place to put errors in the query result.

@ghost ghost added the CLA Signed label Jul 19, 2016
@leebyron
Copy link
Collaborator

Refreshing this RFC since I think it's really important to get included. This is critical information to let clients deduce which errors correspond to which fields.

@leebyron
Copy link
Collaborator

I think the spec addition needs to be a little more descriptive. Definitely it needs to include some examples and deeper explanation of the expected behavior for some of the edge cases:

  • When a field used an alias, the alias should be used in the path (so this is a path of response keys, not field names).
  • When a field that errored is contained with an array, that the position in the array response is included.
  • When a error occurs in a field that's non-nullable and the null ends up appearing in a parent field, the path should follow to the original erroring field, not the field that was nulled.

stubailo pushed a commit to stubailo/graphql that referenced this pull request Oct 28, 2016
@stubailo
Copy link
Contributor Author

Going to close and reopen this to have a better branch name than patch-1.

@stubailo stubailo closed this Oct 28, 2016
leebyron pushed a commit that referenced this pull request May 12, 2017
* 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: #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
IvanGoncharov pushed a commit to IvanGoncharov/graphql that referenced this pull request Jun 17, 2017
* 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: graphql#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
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