Let graphene handle serializer errors

[annshress]

Currently, we have separate errors field rather than using already existing error feature provided by graphene.

https://github.com/graphql-python/graphene-django/blob/master/graphene_django/rest_framework/mutation.py#L77
https://github.com/graphql-python/graphene-django/blob/master/graphene_django/rest_framework/mutation.py#L128

So instead we could simply raise the ValidationError raised by serializer, and let graphene handle it.

[patrick91]

mmh, I'm not sure this is something we want. I usually treat the graphene errors as something that's not fixable by the end user. But that's my personal preference, also because graphql errors are a bit limited.

I'm up for suggestions here, @syrusakbary @spockNinja

[spockNinja]

I agree with @patrick91. I believe the error fields on the SerializerMutation are quite intentionally separate from GraphQL errors. These are generally field-level validation errors and will not only have a different structure from normal errors, but are handled differently by the client.

For example, getting a GraphQL query syntax error is not your end-user's fault, and should be handled by a developer before you get into production. However, the "errors" on the SerializerMutation will happen all the time in production and should be passed on to your end user so that they can fix the data in their form and try again.

[annshress]

Makes sense. Thanks for feedback.

[sliverc]

I have just bumped into the same issue and first thought this is a bug before I have encountered this issue.

I think this decision should be reconsidered, let me outline why:

1. When executing a mutation with a validation error it returns 200 Success status code even though there is a failure and nothing has been mutated
2. Client always has to request the errors in its query even though in general it is simply interested in the success results
3. when a graphql error occurs graphene currently returns a 400 Bad Request error which indicates that request is invalid and can be fixed. If it were a programmer error it would need to return 500 but this is not the case
4. The specification doesn't state that those are not fixable by the users (a user for me is the api client). It actually now also allows extensions to give more information to the client.

Reference for status codes: https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

What do you think?

I am happy to work on a PR if we can agree that this should be adjusted.

[hcharley]

I just got tripped up on this as well @sliverc. I had to read through the SerializerMutation source code to figure out what was happening.

[patrick91]

@sliverc @Charlex I see your point, but I'm not sure how we should manage errors, would something like this work?

{
  "errors": [
    {
      "message": "Validation failed.",
      "locations": [ { "line": 6, "column": 7 } ],
      "path": [ "createHero" ],
      "extensions": {
        "fieldErrors": {
          "name": ["This field cannot be empty"]
        },
        "timestamp": "Fri Feb 9 14:33:09 UTC 2018"
      }
    }
  ]
}

[patrick91]

Related: graphql/graphql-spec#391

[sliverc]

GraphQL errors encode exceptional scenarios - like a service being down or some other internal failure. Errors which are part of the API domain should be captured within that domain.

I am a bit confused by this statement in the related issue as the specification doesn't explicitly state that this is the case (at least that's how I read it).

The specification rather states:

If the result of resolving a field is null (either because the function to resolve the field returned null or because an error occurred), and that field is of a Non-Null type, then a field error is thrown. The error must be added to the "errors" list in the response.

Trying to rephrase this for our case:
Each field returned by mutation which is null because of an error should have an error in errors.

Hence I suggest Graphene Django adds one error for each validation exception. This might need changes on graphql-core as well? The way how you have suggested would also be a good first step to move forward.

Also such a change depends on a fix for issue graphql-python/graphql-core#203

All in all changing this won't be trivial as changes in graphql-core are most likely necessary. But could we still reopen this issue? This way users with the same issue will find it better as the discussion continues. And as stated when we agree on a solution I am open to help with a PR if needed.

[patrick91]

@sliverc sorry I missed your reply! Thank you @annshress for reopening this!

I'm honestly a bit conflicted between the various option, I was actually writing a blog post about dealing with user errors in GraphQL. My main reason to have errors in the schema is that they are typed[1], so they super easy to use (especially if you want to show them inline with the fields).
Unfortunately, I've seen people misusing these errors and not actually requesting them when doing mutations, so I was thinking of other approaches for handling them, and from what I've seen there are 3 options:

A. Errors in GraphQL, I like it, but they can be easily ignored by devs (happens a lot, unfortunately) but also they are not typed (I'm not sure having extensions will help us here)
B. Errors as fields like we have now (but typed better) same here, it is really easy to not request the field errors or miss a single field. But they are typed
C. Return a union type for the mutation, so when using the mutation and its return type we would need to add a check for the type, this basically adds a hint to the developers that they have to fetch the errors, but even here it is easy to miss a field.

Example for option A:

mutation {
  createUser(name: "example") {
    user { 
      name
    }
  }
}

Here there's no need to request for the errors, they would be all passed into the error field of the response.

Example for option B:

mutation {
  createUser(name: "example") {
    user { 
      name
    }
    errors {
      name
      nonFieldErrors
    }
  }
}

Here we have to fetch the errors by field (including nonFieldErrors), again this is quite easy to miss

Example for option C:

mutation {
  createUser(name: "example") {
    ... on User { 
      name
    }
    ... on CreateUserErrors {
      name
      nonFieldErrors
    }
  }
}

Here we need to handle the union type, this kinda forces us to check the errors but you could still miss a field.

I'm working on a PR to improve Form and Serializer mutations, and I might include a new error handling, I'm honestly thinking of implementing all 3 solutions and let whoever is using the library which one they prefer, I'll keep you posted :)

[1] Graphene Django Mutation is not nicely typed but that's another issue

[sliverc]

@patrick91 Thanks for your feedback and working on this. Providing all three options might be a way forward.

Personally I think though this is rather a matter of what the specification states and not what we prefer to do in graphene. Maybe this needs to be raised again within context of graphql specification as it seems the specification is not clear at this point.

[vinayan3]

I've run into this today in terms of handling errors in my application. I have been creating Union types between Nodes and Errors so I can return DRF validation errors.

I'm hooking this up to an apollo-client which thinks because the request has 200 response everything is fine. However, there is a validation errors in the data key. For now I'm making the client check the data key for errors.

It'd be great if there was a way to reduce the tedium and have something that produces errors into a single field.

@patrick91 for option A how will we handle multiple validation errors? The graphql.error.GraphQLError only encapsulates a single instance of an error. DRF's validation errors are an array with multiple messages. It'd be great if we didn't lose that existing functionality.