Detach authorization-related errors from form-field errors

[rmtobin]

Description

When receiving a response back from a graphql query or mutation implemented in django-graphql-auth that includes an error, the response is nested within the "data" map instead of as a top-level key per the graphql spec: https://github.com/graphql/graphql-spec/blob/master/spec/Section%207%20--%20Response.md

e.g. actual response for an invalid email address to the "Register" mutation: {"data":{"register":{"success":false,"errors":{"email":[{"message":"Enter a valid email address.","code":"invalid"}]},__typename":"Register"}}}

Expected response: {errors:[{"message":"Enter a valid email address.","code":"invalid", "path":["register","user","email"]}],"data":{}}

Steps to Reproduce

• Send a GraphQL mutation payload to Django that uses any of the mutations defined in this library that would return an error.

This appears to be the problem: https://github.com/PedroBern/django-graphql-auth/blob/b270f427a2d4d80fffa50450044d1fd3699415d1/graphql_auth/bases.py#L6 I haven't tested it out, but this may "just work" without this error handling, instead letting graphene handle raised exceptions.

Not following the GraphQL spec and custom formatting errors like this make it difficult to use this library in conjunction with a frontend graphql client that has built in error handling (e.g. Apollo or Relay). Instead of using their error workflows, you have to write your own to parse out the "data" payload looking for errors since they expect all errors to appear as a top-level key "errors".

For what its worth, django-graphql-jwt does implement error handling to spec.

[PedroBern]

Hi @rmtobin the top-level key errors is meant to show unexpected errors. The errors inside the data are expected exceptions that the client knows it can happen. Perhaps I should have named it as exceptions instead of errors.

You can see the example on the link you provided:

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

It is a network error.

But will leave it open, if there are many people that agree with you, we can definitely change it! We just need people to comment here and work on a PR.

[rmtobin]

I definitely see your reasoning here, and it seems like there isn't currently a single agreed upon way to handle errors in graphql yet, so there are a lot of different opinions.

I think the way you currently have errors set up nested within the "data" key makes sense for say field validation or form errors, but IMO not for any of the authorization-related errors (PermissionDenied, token expired, etc), since the client will need to catch those cases and implement a token refresh or login redirect to deal with those cases. Working with both the Apollo client and Relay client, it's way easier to implement token refreshes if the error is at the top level.

If I end up having time I can throw together a PR. Thanks for responding and for maintaining this package - it has saved a bunch of time!

[PedroBern]

I think the way you currently have errors set up nested within the "data" key makes sense for say field validation or form errors, but IMO not for any of the authorization-related errors

You are right! We definitely could change this for better, I will leave it open with the enhancement.

[amorino]

I understand the reasoning but for example Unauthenticated. should be an error in the form of:

{errors:[{"message":"Enter a valid email address.","code":"invalid", "path":["register","user","email"]}],"data":{}}

It's a nightmare detecting Unauthenticated on nested objects.