-
Notifications
You must be signed in to change notification settings - Fork 828
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
Comments
@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? |
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 This would be in the same vein as what Strawberry, another Python GraphQL implementation, discusses in its documentation Link |
I see your point, and I definitely see an improvement for API users using this approach. Edit: 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)). |
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? |
Hi @erikwrede and thanks for your answer. Apologies for the late reply.
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 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 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). |
@marcelofern your points convince me. While I think the 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. |
@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. |
@marcelofern sounds great! Documentation is definitely a topic that needs work. Is it okay for you to hold for just a little longer? Hoping to make progress there soon. |
Absolutely, no worries at all. If you want my help clarifying anything just let me know. Cheers! |
@marcelofern 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? 🙂 |
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: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:
If we have a look at what the schema looks like, we have this:
And finally, API clients can query this mutation like this:
I'm interested in your thoughts in this approach.
Thanks!
The text was updated successfully, but these errors were encountered: