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

Returning errors when resolving lists with nullable elements #71

Closed
tonyghita opened this issue Mar 22, 2017 · 7 comments
Closed

Returning errors when resolving lists with nullable elements #71

tonyghita opened this issue Mar 22, 2017 · 7 comments

Comments

@tonyghita
Copy link
Member

tonyghita commented Mar 22, 2017

Can anyone give guidance on how to handle the situation when a list resolver which resolves nullable elements encounters an error while resolving one or more elements?

Example:

query {
  list: [Element]
}

type Element {
  # ... has fields
}
type ListResolver struct {
  elementIDs []string
}

type ElementResolver struct {}

func (r  *ListResolver) Elements() ([]*ElementResolver, error) {
  resolvers := []*ElementResolver{}
  errors := ??? // something like errors.CompositeError? or []errors.QueryError{}?

  for _, id := range r.elementIDs {
    resolver, err := resolveElement(id)
    if err != nil {
      // collect the error somehow...
      errors = append(errors, err)
    }

    resolvers = append(resolvers, resolver)
  }

  return resolvers, errors
}

func resolveElement(id string) (*ElementResolver, error) {
   return nil, errors.New("unable to resolve element")
}
@neelance
Copy link
Collaborator

I'm not sure if this is what you're looking for, but here are two ideas:

query {
  list: ElementList!
}

type ElementList {
  elements: [Element!]!
  errors: [String!]!
}

type Element {
  # ... has fields
}

or

query {
  list: [MaybeElement!]!
}

type MaybeElement {
  element: Element
  error: String
}

type Element {
  # ... has fields
}

@nicksrandall
Copy link
Member

I believe that GraphQL spec says that if anything errors, the entire value should be null and the service should immediately return the error. So in this case, I think that as soon as you encounter an error, I would immediately return it and not worry about resolving the rest of the items.

@tonyghita
Copy link
Member Author

I think since the elements of the list are nullable, ideally I should be able to return the ones that successfully resolved and be able to determine why the null elements failed.

The specification seems to get a little vague around this case. There are a few open issues and PRs where possible solutions are being discussed:

graphql/graphql-spec#259
graphql/graphql-spec#230

I may be thinking about this incorrectly, but my ideal response would have all the elements that resolved successfully, and errors for each element that resolved unsuccessfully. Each error would also include the path to the element, so the client could understand if the value was intentionally null or null due to an error.

@neelance I see why you suggested making the elements non-nullable as a workaround, but I think there is a valid use-case for a list with nullable elements.

@neelance
Copy link
Collaborator

Hmm, there are those two types of errors, expected and unexpected ones. A bit similar to errors vs. panics in Go. My current understanding is that GraphQL errors are meant for the unexpected cases == your code has to be fixed, e.g. a syntax or validation error in the query. Now I'm wondering if the error return values that we currently have in the resolvers make sense at all.

@tonyghita
Copy link
Member Author

tonyghita commented Mar 22, 2017

Hmm. I think the errors do make sense as-is, but we may be missing one or more error behaviors/types.

The "Best Practices" section on nullability speaks a little bit to this use case: http://graphql.org/learn/best-practices/#nullability

I'm also trying to parse through the actual specification to better understand.

edit: Best I could find in the current spec http://facebook.github.io/graphql/#sec-Errors-and-Non-Nullability

They reference "field errors", which I think are different than "query errors" as you mentioned. Maybe the solution is as simple as adding a FieldError type in the errors package.

@tonyghita
Copy link
Member Author

tonyghita commented Apr 13, 2017

So, the solution I've come up with is to package all list errors into a single error on the list resolver.

Then, unwrap the errors as part of the error sanitization step after the query has been executed but before the response is returned to the user.

All the pieces to make this work already exist in the library, thanks to @neelance and @nicksrandall efforts.

If you're looking to approach the problem the same way, this library may be useful to you: https://github.com/hashicorp/go-multierror.

The last piece that remains unresolved is to add the path where the error occurred to make the error usable to the client.

I imagine this would look something like:

{
  "data": {
    "myItems": [
      "item1",
      "item2",
       null,
      "item4",
       null
    ] 
  },
  "errors": [
    {
      "message": "item3 was not found",
      "path": ["myItems", 2]
    }, {
      "message": "item5 was not found",
      "path": ["myItems", 4]
    }
  ]
}

Closing this issue for now... Let me know if I can clarify any points.

@edwardmp
Copy link

@tonyghita did you ever find a clean way to set the path in the way you described?

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

No branches or pull requests

4 participants