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

GraphQL error paths are missing with a subgraph is down #4548

Open
smyrick opened this issue Jan 25, 2024 · 2 comments
Open

GraphQL error paths are missing with a subgraph is down #4548

smyrick opened this issue Jan 25, 2024 · 2 comments

Comments

@smyrick
Copy link
Member

smyrick commented Jan 25, 2024

Describe the bug
If my subgraph is in a nested entity fetch query plan and the subgraph has a service error or is offline, the GraphQL errors reported from Router do not have a path assigned.

To Reproduce
Router version 1.35.0

  • Compose a supergraph of two schemas but only start up the server of the first subgraph that has the root resolver
  • Run a query that does a Flatten across a list
  • Notice the query plan for this is fieldA.@.fieldB
    • In older versions of Router this query plan path made into the GraphQL error path which is invalid
  • However in the error block there is not reference to fieldB and it's path even though it is the causing the error from the Router perspective

Expected behavior
All GraphQL errors have an associated path if we can process the request and send requests to subgraphs

Output
Router 1.35.0
Screenshot 2024-01-24 at 1 42 36 PM

Desktop (please complete the following information):

  • OS: MacOS
  • Version 14.3

Additional context
In previous versions of Router, 1.19.0, we actually has an invalid path that included the at @ symbol from query plans (which brought up this spec clarification graphql/graphql-spec#1073)

Router 1.19.0
Screenshot 2024-01-24 at 1 40 58 PM

For Router 1.35.0

Running this graph: https://github.com/apollosolutions/retail-supergraph

Here is the query:

query {
  searchProducts {
    id
    recommendedProducts {
      id
      reviews {
        id
        body
      }
    }
  }
}

Query plan

QueryPlan {
  Sequence {
    Fetch(service: "products") {
      {
        searchProducts {
          __typename
          id
        }
      }
    },
    Flatten(path: "searchProducts.@") {
      Fetch(service: "discovery") {
        {
          ... on Product {
            __typename
            id
          }
        } =>
        {
          ... on Product {
            recommendedProducts {
              __typename
              id
            }
          }
        }
      },
    },
    Flatten(path: "searchProducts.@.recommendedProducts.@") {
      Fetch(service: "products") {
        {
          ... on Product {
            __typename
            id
          }
        } =>
        {
          ... on Product {
            upc
          }
        }
      },
    },
    Flatten(path: "searchProducts.@.recommendedProducts.@") {
      Fetch(service: "reviews") {
        {
          ... on Product {
            __typename
            upc
          }
        } =>
        {
          ... on Product {
            reviews {
              id
              body
            }
          }
        }
      },
    },
  },
}
@smyrick
Copy link
Member Author

smyrick commented Jan 25, 2024

If we follow the spec strictly then we should have an error for every single searchProducts.1-n.reccomendProducts.1-n.reviews field but that seems excesive. So maybe we default to just returning one error for searchProducts.0.reccomendProducts.0.reviews?

@Geal
Copy link
Contributor

Geal commented Feb 7, 2024

looking at the code, there are multiple issues.
in subgraph_service.rs, in multiple places we convert a FetchError to a graphql error: https://github.com/apollographql/router/blob/dev/apollo-router/src/services/subgraph_service.rs#L711

But in other places we return the FetchError. Which might explain why you get the error path for some errors but not others.
And as you said, we should post process the error path to replace the @ and apply to every index in array

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

2 participants