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

Typed Errors and Graphene #1427

Open
marcelofern opened this issue Jun 17, 2022 · 10 comments
Open

Typed Errors and Graphene #1427

marcelofern opened this issue Jun 17, 2022 · 10 comments

Comments

@marcelofern
Copy link

Hi folks,

I'm raising this issue to gauge ideas from the Python community on error handling best practices.

The usual way to handle errors in GraphQL is by inspecting the top-level errors key:

{
  "errors": {
    # ... your error details ...
  },
  "data": null
}

However, this is usually problematic for API clients as they don't know what to expect from this errors key.
This means that error discoverability is impacted.

Another downside, is that the data key must be null when these high-level errors are raised.
In many situations API clients are interested in calling a mutation and, even if it fails, they'd like to receive data back. This data can be anything, but it's usually the object being mutated itself.

Another approach is extending upon this idea of typed errors that is strongly supported by Lee Byron as you can see here.

This seems to solve both problems above (along with many others). Here's my take on what this would look like in Graphene:

class ErrorInterface(graphene.Interface):
    message = graphene.NonNull(graphene.String)


class ThingAErrorType(graphene.ObjectType):
    class Meta:
        interfaces = [ErrorInterface]


class ThingBErrorType(graphene.ObjectType):
    class Meta:
        interfaces = [ErrorInterface]


class MySweetMutationErrorUnion(graphene.Union):
    class Meta:
        types = [ThingAErrorType, ThingBErrorType]


class MySweetMutation():
    error = graphene.Field(MySweetMutationErrorUnion)
    output = graphene.Field(MySweetOutputType)

    def mutate(self, info, input):
        try:
             thing_a = do_thing_a(input)
        except ThingAException:
             return MySweetMutation(error=ThingAErrorType())
        try:
             thing_b = do_thing_b(input)
        except ThingBException:
             return MySweetMutation(error=ThingBErrorType())
        # happy path!
        output = MySweetOutputType(thing_a=thing_a, thing_b=thing_b)
        return MySweetMutation(output=output)

If we have a look at what the schema looks like, we have this:

image

image

And finally, API clients can query this mutation like this:

    mutation mySweetMutation($input: MySweetMutationInput!) {
        mySweetMutation(input: $input) {
            output {
                # ....
            }
            error {
                ... on ThingAError {
                    __typename
                    message
                }
                ... on ThingBError {
                    __typename
                    message
                }
                 # We have an interface here so that we
                 # can extend the union with more errors without breaking
                 # backwards compatibility!!
                 __typename
                 message
            }
        }
    }

I'm interested in your thoughts in this approach.
Thanks!

@marcelofern marcelofern changed the title Should I use typed errors with Graphene? Typed Errors and Graphene Jun 17, 2022
@theodesp
Copy link

@marcelofern This looks like a nice improvement in the error representation of a mutation that plays well with the introspected schema. However is there anything that you would like from this library as you can implement this in userland already without any changes to the spec?

@marcelofern
Copy link
Author

marcelofern commented Jun 27, 2022

Hi @theodesp

That's a great question.

My initial thought was to post this here to get some engagement first.

If this is a scenario that the community is interested in, the next step is to enhance the documentation about errors to have a note about Expected Errors.

This would be in the same vein as what Strawberry, another Python GraphQL implementation, discusses in its documentation Link

@erikwrede
Copy link
Member

erikwrede commented Jun 27, 2022

I see your point, and I definitely see an improvement for API users using this approach.
What I don't like here is the non-uniform approach for querying vs mutating. Sometimes queries might also produce errors (for example due to an invalid attribute passed to a resolver). How should we transform this behavior to queries? I think handling mutation errors different from query errors might be confusing to users. On the other hand, adding error fields to all object types might also collide with fields named error currently present in the Schema.
Let me know what you think about my points, looking forward to further discuss this 🙂

Edit:
Wouldn't the error extensions be a good place to add additional error information? We could provide a framework for that and remove the need for additional object types. Apollo Server (gql-js) provides an interesting example:

https://www.apollographql.com/docs/apollo-server/data/errors/#including-custom-error-details

What I prefer about this approach is that all resolver errors are in the same place. When using typed errors as above, you need too check the response type (is it an error or is it a response), parse those accordingly and additionally check the errors array in the gql-response. Effectively, that creates a necessity for two different error handlers covering (not exclusively) very similar validation errors. (Errors Array: GQL failed to parse Date from Input: Invalid Format (graphene-Error), Error ObjectType: I expected a date in 2021 (custom object tyooe as described above)).

@erikwrede
Copy link
Member

Another example of a specification for error handling can be found here: https://hl7.org/fhir/graphql.html#errors

I'm wondering, given these possibilities, and the chance to implement all of @marcelofern's requested features in userland; is there any work needed here, or should we instead work on providing a comprehensive way to add extensions to your execution result?

@marcelofern
Copy link
Author

Hi @erikwrede and thanks for your answer. Apologies for the late reply.

I see your point, and I definitely see an improvement for API users using this approach.
What I don't like here is the non-uniform approach for querying vs mutating. Sometimes queries might also produce errors (for example due to an invalid attribute passed to a resolver). How should we transform this behavior to queries? I think handling mutation errors different from query errors might be confusing to users. On the other hand, adding error fields to all object types might also collide with fields named error currently present in the Schema.
Let me know what you think about my points, looking forward to further discuss this slightly_smiling_face

I think the same structure applies to queries, even though my example wasn't that thorough and only mentioned mutations.

The only difference for a query would be that instead of returning the basic scalars, it would return a Type, with output and errors as needed.

A simple example:

    query FindProperty($postcode: String!, $address: String!) {
        findProperty(postcode: $postcode, address: $address: String!) {
            output {
                ... on Property {
                    streetAddress
                    postCode
            }
            error {
                ... on PostCodeMustBeEuropean {
                    __typename
                    message
                }
                ... on AddressDoesNotSupportNonASCIICharacters {
                    __typename
                    message
                }
                 __typename
                 message
            }
        }
    }

To respond the other portion of your question, error can be renamed to something different if it overlaps with an already defined error field. An example would be domainError. Goes without saying that this will be on a case-by-case basis depending on an API client's necessity.

Wouldn't the error extensions be a good place to add additional error information? We could provide a framework for that and remove the need for additional object types. Apollo Server (gql-js) provides an interesting example:

Error extensions are very useful for high-level unrecoverable crashes, but unfortunately they aren't sufficient for handling domain-level errors.

Whenever a high-level error pops up in GraphQL, the data value must be null. This means that if you are performing various queries in a single network transaction, if one of those queries fail, you won't have data for the whole request. This is a big blocker if you still want to have the remaining queries giving data back to your application. The same applies to mutations.

I'll bring two answers from @leebyron himself where he share his opinions on this [1][2]. To be clear, I'm not suggesting to be authoritative and follow the advice of one of the creators of GraphQL for the sake of it, but I do think that his thoughts make a lot of sense here and shed a lot of light in the high-level vs domain-level error handling in GraphQL.


Ultimately everything we need is already supported by the framework. What I'm gauging for, is to understand whether a "common practice" should be established, and thus, whether the documentation should make mention of it (such as noted on strawberry's documentation).

@erikwrede
Copy link
Member

@marcelofern your points convince me. While I think the query part of this feature needs some extra discussion to get a great opinionated feature set, the mutations can be implemented in the suggested way.

I've discussed this with @jamietdavidson and a possible way to implement this in graphene could be by adding separate ErrorMutation types opinionating this optional feature.

@marcelofern
Copy link
Author

@erikwrede an easy win would be to add a "how to handle errors" section in the documentation. Strawberry has a pretty good write up about this already, but I can see that Graphene is currently lacking one. This new section is the most obvious place for this to live under I can think of.

This hypothetical section wouldn't only include this discussion about error types we're having here, but also a more general take on errors and how they are serviced (in the same vein as Straberry's page).

I can offer my help if you think that this approach sounds reasonable.

@erikwrede
Copy link
Member

@marcelofern sounds great! Documentation is definitely a topic that needs work.
The problem is that a general overhaul is necessary and we currently have no way to update the website. Trying to get that sorted but it might take some time. Once that's done and we know which way to go, documentation work can get started.

Is it okay for you to hold for just a little longer? Hoping to make progress there soon.

@marcelofern
Copy link
Author

@erikwrede

Absolutely, no worries at all. If you want my help clarifying anything just let me know. Cheers!

@erikwrede
Copy link
Member

erikwrede commented Aug 14, 2022

@marcelofern
Sorry for the long wait! I've got more clarity on docs now. Working on getting the docs hosted on github pages.
That process will not negatively affect any ongoing documentation work.
Feel free to PR your suggested documentation!

Keep in mind: I want to add doctest to the docs, to prevent PRs from invalidating any of the docs in the future. It would be awesome if your PR already contained doctests so we could have a clean start.

Additionally, we have decided to integrate the typed errors into graphene as a an optional feature in the Mutation base class. Are you interested in helping? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants