Query planner generates invalid fetch node selection sets when reuseQueryFragments=true

[tinnou]

Issue Description

Issue Description

When reuseQueryFragments=true (the default config), in certain cases the query planner will generate a fetch that will fail GraphQL validation (Field Selection Merging - overlapping fields can be merged rule) at the subgraph GraphQL server. Specifically, this happens when a fragment from the query is reused and the query planner also queries the same field separately but the two aren't identical. The example below specifically showcases when field arguments mismatch.

Steps to Reproduce

I built a minimum viable example unit test that reproduces building an incorrect plan.

Subgraph discoveryDgs schema:

     type Query {
        pinotPreQuerySearchPage: PinotUIEntity
      }
    
      union PinotUIEntity = PinotBoxShotEntityTreatment | PinotEpisodicBillboardEntityTreatment | PinotBannerWithTrailerEntityTreatment
      
      type PinotBannerWithTrailerEntityTreatment @key(fields: "unifiedEntityId") {
        unifiedEntityId: ID!
        unifiedEntity: UnifiedEntity
      }
      
      type PinotBoxShotEntityTreatment @key(fields: "unifiedEntityId") {
        unifiedEntityId: ID!
        unifiedEntity: UnifiedEntity
      }
      
      type PinotEpisodicBillboardEntityTreatment @key(fields: "unifiedEntityId") {
        unifiedEntityId: ID!
        unifiedEntity: UnifiedEntity
      }
      
      interface UnifiedEntity {
          id: ID!
      }

      type GenericContainer implements UnifiedEntity @key(fields: "id"){
        id: ID!
      }
      
      type Supplemental implements UnifiedEntity @key(fields: "id") {
        id: ID!
      }
      
      type Show implements UnifiedEntity @key(fields: "id") {
        id: ID!
      }

and subgraph gusto schema:

      type Query {
        me: String
      }
      
      interface Video {
        videoId: Int!
        taglineMessage(uiContext: String): String
        bar: String
      }
      
      interface UnifiedEntity {
          id: ID!
      }
      
      type GenericContainer implements UnifiedEntity @key(fields: "id") {
        id: ID!
        taglineMessage(uiContext: String): String
      }
      
      type Supplemental implements UnifiedEntity & Video @key(fields: "id") {
        videoId: Int!
        id: ID!
        taglineMessage(uiContext: String): String
        bar: String
      }
      
      type Show implements UnifiedEntity & Video @key(fields: "id") {
        videoId: Int!
        id: ID!
        taglineMessage(uiContext: String): String
        bar: String
      }

Executing the following query:

    query Search {
          pinotPreQuerySearchPage {
            __typename
            ... on PinotBannerWithTrailerEntityTreatment {
              __typename
              unifiedEntity {
                __typename
                ... on GenericContainer {
                  id
                  taglineMessage(uiContext: "ODP")
                }
              }
            }
            ... on PinotEpisodicBillboardEntityTreatment {
              __typename
              unifiedEntity {
                __typename
                ...BillboardDataVideoSummary
              }
            }
          }
        }
        
        fragment BillboardDataVideoSummary on Video {
          __typename
          taglineMessage(uiContext: "BILLBOARD")
        }

The plan that is currently generated is as follow:

QueryPlan {
  Sequence {
    Fetch(service: "discoveryDgs") {
      {
        pinotPreQuerySearchPage {
          __typename
          ... on PinotBannerWithTrailerEntityTreatment {
            __typename
            unifiedEntity {
              __typename
              ... on GenericContainer {
                __typename
                id
              }
            }
          }
          ... on PinotEpisodicBillboardEntityTreatment {
            __typename
            unifiedEntity {
              __typename
              ... on Show {
                __typename
                id
              }
              ... on Supplemental {
                __typename
                id
              }
            }
          }
        }
      }
    },
    Flatten(path: "pinotPreQuerySearchPage.unifiedEntity") {
      Fetch(service: "gusto") {
        {
          ... on GenericContainer {
            __typename
            id
          }
          ... on Show {
            __typename
            id
          }
          ... on Supplemental {
            __typename
            id
          }
        } =>
        {
          ... on GenericContainer {
            taglineMessage(uiContext: "ODP")
          }
          ... on Show {
            ...BillboardDataVideoSummary
          }
          ... on Supplemental {
            ...BillboardDataVideoSummary
          }
        }
        
        fragment BillboardDataVideoSummary on Video {
          __typename
          taglineMessage(uiContext: "BILLBOARD")
        }
      },
    },
  },
}

The problem here is the selection set of the second fetch:

       {
          ... on GenericContainer {
            taglineMessage(uiContext: "ODP")
          }
          ... on Show {
            ...BillboardDataVideoSummary
          }
          ... on Supplemental {
            ...BillboardDataVideoSummary
          }
        }
        
        fragment BillboardDataVideoSummary on Video {
          __typename
          taglineMessage(uiContext: "BILLBOARD")
        }

It's not obvious, but the taglineMessage is selected across two spreads ... on GenericContainer and fragment BillboardDataVideoSummary on Video and although there is no type on the schema that can be both a GenericContainer and a Video, the GraphQL spec Field Selection Merging validates against fields having differing sets of arguments.

Hence, the selection set produces will error at subgraph GraphQL server validation.
The unit test proves this by calling graphql-js validation on every operation of every fetchNode on the query plan.
The test fails with the following error:

(...)Fields "taglineMessage" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.](...)

Impact

Impacts (some) queries using fragments.

Link to Reproduction

https://github.com/apollographql/federation/compare/main...tinnou:federation:query-planner-fragment-invalid-bfg-822?expand=1

Reproduction Steps

No response

Link to Reproduction

https://github.com/apollographql/federation/compare/main...tinnou:federation:query-planner-fragment-invalid-bfg-822?expand=1

Reproduction Steps

No response

[samuelAndalon]

graphql/graphql-js#4025

a better approach is to programmatically create fragments, instead of attempting to reuse fragments from the original incoming operation. I believe Apollo will copy/paste the fragmentifyDocument function in a release this week.

[duckki]

The query planner took necessary precautions to generate logically sound subgraph queries. But, unfortunately, the GraphQL type system is not strong enough to admit them in some cases. A related GraphQL spec discussion is here (graphql/graphql-spec#1085).

I created PR (#2963) to avoid the issue for now.