Issue with fragment reuse when a fragment is nested and not nested at the same time

[smikheiev]

Issue Description

It looks like query planner doesn't properly handle a case when a fragment is nested and not nested at the same time. It "eats" an inline fragment and replaces it with a type fragment.

Imagine there're two types:

type A {
  a: String
}

type B {
  b: String
  child: A
}

and a query can return both of these types as a union:

type Query {
  a: Anything
}

union Anything = A | B

A client fetches both types - A and B, using a fragment for type A:

query {
  a {
    ... on A {
      ...ASelect
    }
    ... on B {
      b
      child {
        ...ASelect
      }
    }
  }
}

fragment ASelect on A {
  __typename
  a
}

The query planner will skip ... on A and replace it with ...ASelect fragment:

QueryPlan {
  Fetch(service: "Subgraph1") {
    {
      a {
        __typename
        ...ASelect
        ... on B {
          b
          child {
            ...ASelect
          }
        }
      }
    }

    fragment ASelect on A {
      __typename
      a
    }
  },
}

Shouldn't in this case the query plan look like this:

QueryPlan {
  Fetch(service: "Subgraph1") {
    {
      a {
        __typename
        ... on A {
          ...ASelect
        }
        ... on B {
          b
          child {
            ...ASelect
          }
        }
      }
    }
    
    fragment ASelect on A {
      __typename
      a
    }
  },
}

? 🤔

Link to Reproduction

https://github.com/smikheiev/apollo-federation-fork/pull/1/files

Reproduction Steps

I added a test for the described situation here. Run it with yarn test query-planner-js/src/__tests__/buildPlan.test.ts

[pcmanus]

Is this creating an issue for you?

In general, there is no real guarantees that the fetch in the query plan maps exactly to the syntax of the original query (there is multiple cases where that's won't the case, this is just one of them). The only real guarantee ultimately is that executing the plan will fulfil the original query correctly (and well, hopefully efficiently).

Here, the inline ... on A inline fragment wrapping ...ASelect is rather redundant, since ASelect is defined on A, so:

... on A {
  ...ASelect
}

really expands to:

... on A {
  ... on A {
    __typename
   a
  }
}

which, I think, make the redundancy clear. And so the fetch sent by the plan is equivalent to the original query, just a bit more efficient.

I'll add to that the way the query planner works internally (for various technical reasons) is that it does expand fragments first, plan the query, and then see if it can reuse some of the fragments on the resulting fetches, so the reason why the fetch looks the way it does is a bit more complex in practice that it may sound like, and it would be hard to preserve the ... on A inline fragments without making things blatantly less efficient in other cases.

I personally feel likes it's a feature that the planner can somewhat "minimise" the fetches to subgraphs as long as it keeps things correct, but if that case creates problems for you, I'm certainly interested in understanding why better.