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

subgraph errors should bubble up to the next nullable parent field #215

Closed
Geal opened this issue Nov 29, 2021 · 7 comments
Closed

subgraph errors should bubble up to the next nullable parent field #215

Geal opened this issue Nov 29, 2021 · 7 comments
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Nov 29, 2021

as described in https://www.apollographql.com/blog/graphql/basics/using-nullability-in-graphql/

With this schema:

type Restaurant {
  name: String!
  rating: Int!
  location: Location!
}

type Location {
  address: String!
}

type LatLng {
  lat: Int!
  lng: Int!
}

type Query {
  restaurants(query: String): [Restaurant]
}

if a subgraph returns a response with this content:

{
        name: "All Star Cafe",
        rating: 5,
        location: null
}

location must not be null, so we register an error, and mark the restaurant itself as null:

{
  "data": {
    "restaurants": [ null ]
  },
  "errors": [{
    "message": "Cannot return null for non-nullable field Restaurant.location."
  }]
}

spec: https://spec.graphql.org/June2018/#sec-Errors

If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field. In that case, the path for the error should include the full path to the result field where the error occurred, even if that field is not present in the response.

@Geal Geal added the triage label Nov 29, 2021
@Geal
Copy link
Contributor Author

Geal commented Nov 29, 2021

this should probably be done as part of the format_response phase (to avoid walking the tree multiple times)

@Geal
Copy link
Contributor Author

Geal commented Nov 30, 2021

@cecton ^

@cecton
Copy link
Contributor

cecton commented Dec 1, 2021

Looks interesting!

@cecton cecton self-assigned this Dec 1, 2021
@cecton
Copy link
Contributor

cecton commented Dec 1, 2021

I'll take this one

@cecton
Copy link
Contributor

cecton commented Dec 1, 2021

Cannot be done before #62 (can be done with #62 though...). Unassigning myself for now....

@cecton cecton removed their assignment Dec 1, 2021
@abernix abernix removed the triage label Feb 18, 2022
@Geal Geal self-assigned this Feb 22, 2022
@Geal
Copy link
Contributor Author

Geal commented Feb 22, 2022

more context on this: currently subgraph errors are not integrated in the response. If we have something obvious like a connection error to the subgraph, it will be present, but if there was an error for a specific entity, we would get from the subgraph a response with an errors array containing some errors and related path, and those are not gathered right now.

We need to gather the subgraph errors, append their path to the current query plan node's path, and erase the error message (we don't want to expose subgraph info there).
We need to wait until the entire response was received to check the errors and find the next nullable parent field because:

  • a field provided by a subgraph might be marked as non null, so it could cross the subgraph entity boundaries
  • if another field of the same object is provided by a different subgraph, we could end up with a removed entity due to the first error, but see it added again by the second subgraph query (with missing fields)

@Geal
Copy link
Contributor Author

Geal commented Feb 22, 2022

aggregating the errors is easy enough. Recognizing nullable fields will be a bit more challenging: when traversing the response with errors, for each field we will need to look up in the schema if it is nullable

@abernix abernix closed this as completed Mar 8, 2022
@abernix abernix added this to the v0.1.0-alpha.9 milestone Mar 16, 2022
tinnou pushed a commit to Netflix-Skunkworks/router that referenced this issue Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants