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

feat: support partial errors and multiple errors per field resolver #3055

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bradleyoesch
Copy link
Contributor

@bradleyoesch bradleyoesch commented Aug 25, 2023

EDIT: this has been updated to no longer use an extension, but to provide native support instead

Create a new PartialResultsExtension to allow adding exceptions to info.context.partial_errors to get added to the result after execution, allowing users to add errors to the errors array from a field resolver while still resolving that field.

Description

NOTE: It's entirely possible/likely that I misunderstand how partial/multi-error handling works in graphql-python and that there is a simpler, already supported way to do this.

One powerful feature of GraphQL is returning both data AND errors. The typical way to return an error in python graphql is to simply raise it, and let the graphql library catch it and add to the Execution Context errors that ultimately map to the errors array in the response.

So this means in a field resolver we can either return data OR raise an error. But we can't raise multiple errors (possibly incorrect) and we can't return data AND "raise" an error (also possibly incorrect). I have not been able to find documentation or examples supporting either case.

To work around this, this extension creates an interface so developers can manually populate errors from resolvers that get added to the result after execution.

Caveats

I'll admit a couple things here:

  1. As noted above, my understanding may be lacking and there is already support for this kind of behavior
  2. Even if my understanding is correct and this extension works, this may not be the way this library would want to support this kind of thing
  3. There's a little hackiness as noted in the copied block of the extension code that I don't love
  4. Unclear how best to get the nodes arg for creating your own GraphQLErrors to ensure location and path fields exist in the error object in the response. See Pythonic wrapper for graphql core FieldNode. #874 for information around not exposing Nodes directly, in favor of strawberry's own wrapper type Selection.

Usage example:

import strawberry
from strawberry.extensions import PartialResultsExtension

schema = strawberry.Schema(Query, extensions=[PartialResultsExtension])

# ...

@strawberry.field
def query(self, info) -> bool:
    info.context.partial_errors.append(Exception("Partial failure"))
    return True

Results:

{
  "data": {
    "query": true
  },
  "errors": [
    {
      "message": "Partial failure"
    }
  ]
}

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

  • None

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@botberry
Copy link
Member

botberry commented Aug 25, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Update execution to allow adding instances of Exception type to info.context.partial_errors to get added to the result after execution, allowing users to add errors to the errors array from a field resolver while still resolving that field.

Usage examples:

Basic usage:

import strawberry


@strawberry.field
def query(self, info) -> bool:
    info.context.partial_errors.append(Exception("Partial failure"))
    return True

Results:

{
  "data": {
    "query": true
  },
  "errors": [
    {
      "message": "Partial failure"
    }
  ]
}

Located usage:

import strawberry

from graphql import located_error


@strawberry.field
def query(self, info) -> bool:
    nodes = [next(n for n in info.field_nodes if n.name.value == "query")]
    info.context.partial_errors.append(
        located_error(
            Exception("Error with location and path information"),
            nodes=nodes,
            path=info.path.as_list(),
        ),
    )
    return True

Results:

{
  "data": {
    "query": true
  },
  "errors": [
    {
      "message": "Error with location and path information",
      "location": {
        "line": 1,
        "column": 9
      },
      "path": ["query"]
    }
  ]
}

Here's the tweet text:

🆕 Release (next) is out! Thanks to Bradley Oesch for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

@bradleyoesch
Copy link
Contributor Author

Some example use cases:

  • Query for data matching ids [0, 1, 2] and there's no id 1, so you return data: [data0, null, data2] with errors array with some message and the path pointing to the index of the missing data
  • Plural mutation where some fail and some don't, similar kind of response
  • Operation with multiple business-logic-like validation errors, e.g. user tries to do some action on "item id 123" but gets multiple errors, e.g. "User does not have enough coins for this action", whatever, something that is not like a graphql-schema-validation type of error

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 25, 2023

CodSpeed Performance Report

Merging #3055 will not alter performance

Comparing bradleyoesch:feature/partial-results-extension (57e03c2) with main (bea5cac)

Summary

✅ 13 untouched benchmarks

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Attention: Patch coverage is 96.65072% with 7 lines in your changes missing coverage. Please review.

Project coverage is 94.40%. Comparing base (bea5cac) to head (57e03c2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3055      +/-   ##
==========================================
- Coverage   96.58%   94.40%   -2.19%     
==========================================
  Files         524      520       -4     
  Lines       33632    32680     -952     
  Branches     5577     3745    -1832     
==========================================
- Hits        32485    30850    -1635     
- Misses        912     1543     +631     
- Partials      235      287      +52     

@bradleyoesch bradleyoesch changed the title feat: add extension to support partial errors and multiple errors per field resolver feat: support partial errors and multiple errors per field resolver Sep 1, 2023

@strawberry.field
def query(self, info) -> bool:
info.context.partial_errors.append(Exception("Partial failure"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving this off of context and into info would be preferred, since context can be anything by implementing frameworks/clients.

I'll look into it

Copy link
Contributor Author

@bradleyoesch bradleyoesch Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone with an iffy route of trying to support attribute setting and indexing generically, though it's kind of gross.

Since info comes from the graphql core library, I'm not so sure moving it into info makes much sense.

Since I don't love either approach, it kind of implies that maybe this isn't a great idea anyway 😅

@@ -215,6 +222,82 @@ mutation RegisterUser($username: String!, $password: String!) {

This approach allows you to express the possible error states in the schema and so provide a robust interface for your client to account for all the potential outcomes from a mutation.

### Appending to `errors` array

As seen in the [Unhandled execution errors][#unhandled-execution-errors] section above, errors are automatically added to the errors array by raising the error in execution.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is how to refer to another heading on this page

@patrick91
Copy link
Member

I'll take a proper look at this probably next week, but some notes that pop into my mind:

  1. I wonder if we could somehow use exception groups, granted we find a way to backport them (or maybe we can allow returning exceptions in resolvers? I wonder if other frameworks do this
  2. can you add a test with the exact use case you have? that should make it easier to understand
  3. Do you think it is important to make sure we return the right path for the error?

@patrick91
Copy link
Member

also, thank you so much for working on this!

@bradleyoesch
Copy link
Contributor Author

@patrick91

  1. I wonder if we could somehow use exception groups, granted we find a way to backport them (or maybe we can allow returning exceptions in resolvers? I wonder if other frameworks do this

I think this could be cool. I'm not sure what would happen since the graphql-core library only catches exceptions, not groups. Note that if we got this to work (with no other changes), this would enable multiple errors per resolver, but not an error and data in a resolver.

I think having the option of the resolver returning either the data type OR something like an ExecutionResult (that models data and errors) would be cool, and then that context type could add any errors to the errors array, mapping any to GraphQL errors with path/location data, etc (which as I describe down in #3), I think should be more easily accessible per resolver. DGS (netflix java graphql library) defines a DataFetcherResult that you can return.

  1. can you add a test with the exact use case you have? that should make it easier to understand

I think this test_partial_errors_yes_data_yes_errors test is pretty straightforward, but if you're asking for a less contrived test, yeah I can add a test e.g. resolving a list and some are null and you want some errors to point to the null ones

  1. Do you think it is important to make sure we return the right path for the error?

I think it's important to give the users the ability to easily add the right path/location for the error (and if we can do it automatically, great). I'll note that this example with a located error test shows that the easiest way (that I could find) to get the nodes for a resolver is to use info.field_nodes, which was deprecated in #874 when abstracting out some of the raw types.

I think it could be worth a separate issue to add in some functionality for easily getting the current resolver's located node and path, and then accessing that in here.

@bradleyoesch bradleyoesch force-pushed the feature/partial-results-extension branch 2 times, most recently from b1a285f to 948dc65 Compare September 9, 2023 17:23
@bradleyoesch
Copy link
Contributor Author

@patrick91 Updated with an example use case, a mutation with a partial failure

948dc65

@cadlagtrader
Copy link

cadlagtrader commented Dec 6, 2023

Hello @patrick91 - do you plan to merge this PR soon ?
That would be very useful for my use case too (market data partially missing)

@patrick91
Copy link
Member

I have some time off friday, so I should be able to review and merge! thanks for the ping!

@bradleyoesch
Copy link
Contributor Author

@patrick91 bump on this for review whenever you have time

@bradleyoesch bradleyoesch force-pushed the feature/partial-results-extension branch from 948dc65 to abad1da Compare July 11, 2024 02:42
}


@pytest.mark.parametrize("context_type", ("class", "dict"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lil gross too 🤷

@bradleyoesch
Copy link
Contributor Author

@patrick91 I totally lost this one! Rebased main and tried to be a little more generically supportive of context types, but tbh I don't love the approach too much atm. Curious if you have any thoughts coming back to this now?

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

Successfully merging this pull request may close these issues.

4 participants