Bug: Query Planner Doesn't Properly Account for Conditional `@requires`

[ryan01010111]

Issue Description

Given:

1. Subgraphs "A" and "B," where both define an entity T, with a @key of id
2. "A" defines field T.a
3. "B" defines fields T.b and T.c, where T.c @requires T.a

When executing a query that conditionally selects T.c, and unconditionally selects other, sibling fields on the entity (T) provided by the same subgraph ("B"), as in:

query q($v: Boolean!) {
    t {
        id
        b
        c @skip(if: $v)
    }
}

and v is the value true

Then:
T.b is never fetched, and is subsequently resolved to null, which can lead to inconsistent data responses.

Cause

The query plan executor receives a FetchNode with requirements (selection set) that include the field required by the excluded (via @skip) field, and the representation collection process (via executeSelectionSet()) attempts to extract that "required" field from the fetched entity data, in which it is (correctly) excluded. This results in null being returned for the fetch's selection set, and a "silent failure" in the case of null-able fields.

Link to Reproduction

https://codesandbox.io/p/devbox/fldc84

Reproduction Steps

1. If not already running (should be in CodeSandbox), start all services with:

npm run dev

2. In a free terminal, send example request to reproduce the bug with:

npm run sendRequest

[duckki]

Thank you for reporting. I can see the issue here. Let me rephrase it:

Suppose subgraph A and B as below:

#-----------------------------------------------------------------------------------
#!subgraph A

  type T @key(fields: "id") {
    id: ID!
    a: Boolean!
  }

  type Query {
    start: T
  }

#-----------------------------------------------------------------------------------
#!subgraph B

  type T @key(fields: "id") {
    id: ID!
    a: Boolean! @external
    b: Boolean
    c: String! @requires(fields: "a")
  }

And the query:

query($myVar: Boolean!) {
  start {
    id
    b
    c @skip(if: $myVar)
}

Apollo federation generates a query plan like this:

QueryPlan {
  Sequence {
    Fetch(service: "A") {
      {
        start {
          __typename
          id
          ... on T @skip(if: $myVar) {
            a
          }
        }
      }
    },
    Flatten(path: "start") {
      Fetch(service: "B") {
        {
          ... on T {
            __typename
            id
            a
          }
        } =>
        {
          ... on T {
            b
            c @skip(if: $myVar)
          }
        }
      },
    },
  },
}

If $myVar is true, a won't be fetched in the initial fetch. Then, the second fetch will be skipped entirely, since the required field a is missing. This seems to be a bug.

As pointed out, the query can be modified:

query($myVar: Boolean!) {
  start {
    id
    b
    ... on T @skip(if: $myVar) {
        c
    }
}

Then, it generates a correct query plan:

QueryPlan {
  Sequence {
    Fetch(service: "A") {
      {
        start {
          __typename
          id
          ... on T @skip(if: $myVar) {
            __typename
            id
            a
          }
        }
      }
    },
    Flatten(path: "start") {
      Fetch(service: "B") {
        {
          ... on T {
            __typename
            id
          }
          ... on T {
            __typename
            id
            a
          }
        } =>
        {
          ... on T {
            b
          }
          ... on T @skip(if: $myVar) {
            c
          }
        }
      },
    },
  },
}

In the better query plan, the second fetch has two separate entity match cases: one for b and one for c. So, even if c is skipped, b gets fetched regardless.

(Note to myself: Both JS QP and Rust QP have this issue.)

[ryan01010111]

Thanks, @duckki! I was just about to ask if you have any ideas for a patch. I'm still lacking a good bit of context here, so anything to point me in the right direction will be greatly appreciated.