Fix a query planning bug where invalid subgraph queries are generated with reuseQueryFragments set true (#2963)

Fixed #2952.

### Summary of changes

- updated computeExpandedSelectionSetAtType function not to trim
fragment's validators if the fragment's type condition is not an object
type.
- This change is necessary because `FieldsInSetCanMerge` rule is more
restrictive in that case.
- The untrimmed validator should avoid generating invalid queries, but
it may be less efficient.

### Test plan

- The operations.test.ts confirms the changes of named fragments'
validators.
- Added a new test (based on the github issue's reproducer) confirming
that subgraph queries are no longer invalid.

Author: duckki

.changeset/invalid-reused-fragment.md

@@ -0,0 +1,7 @@
+---
+"@apollo/query-planner": patch
+"@apollo/composition": patch
+"@apollo/federation-internals": patch
+---
+
+Fix a query planning bug where invalid subgraph queries are generated with `reuseQueryFragments` set true. ([#2952](https://github.com/apollographql/federation/issues/2952))

internals-js/src/__tests__/operations.test.ts

@@ -3100,6 +3100,38 @@ describe('named fragment selection set restrictions at type', () => {
     return frag.expandedSelectionSetAtType(type);
   }
 
+  test('for fragment on object types', () => {
+    const schema = parseSchema(`
+      type Query {
+        t1: T1
+      }
+
+      type T1 {
+        x: Int
+        y: Int
+      }
+    `);
+
+    const operation = parseOperation(schema, `
+      {
+        t1 {
+          ...FonT1
+        }
+      }
+
+      fragment FonT1 on T1 {
+        x
+        y
+      }
+    `);
+
+    const frag = operation.fragments?.get('FonT1')!;
+
+    const { selectionSet, validator } = expandAtType(frag, schema, 'T1');
+    expect(selectionSet.toString()).toBe('{ x y }');
+    expect(validator?.toString()).toBeUndefined();
+  });
+
   test('for fragment on interfaces', () => {
     const schema = parseSchema(`
       type Query {
@@ -3159,17 +3191,32 @@ describe('named fragment selection set restrictions at type', () => {
 
     let { selectionSet, validator } = expandAtType(frag, schema, 'I1');
     expect(selectionSet.toString()).toBe('{ x ... on T1 { x } ... on T2 { x } ... on I2 { x } ... on I3 { x } }');
-    expect(validator?.toString()).toBeUndefined();
+    // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for
+    // fragments on non-object types.
+    expect(validator?.toString()).toMatchString(`
+      {
+        x: [
+          I1.x
+          T1.x
+          T2.x
+          I2.x
+          I3.x
+        ]
+      }
+      `);
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'T1'));
     expect(selectionSet.toString()).toBe('{ x }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     // Note: one could remark that having `T1.x` in the `validator` below is a tad weird: this is
     // because in this case `normalized` removed that fragment and so when we do `normalized.minus(original)`,
     // then it shows up. It a tad difficult to avoid this however and it's ok for what we do (`validator`
     // is used to check for field conflict and save for efficiency, we could use the `original` instead).
     expect(validator?.toString()).toMatchString(`
       {
         x: [
+          I1.x
           T1.x
           T2.x
           I2.x
@@ -3239,8 +3286,16 @@ describe('named fragment selection set restrictions at type', () => {
     // this happens due to the "lifting" of selection mentioned above, is a bit hard to avoid,
     // and is essentially harmess (it may result in a bit more cpu cycles in some cases but
     // that is likely negligible).
+    // Note: Due to `FieldsInSetCanMerge` rule, we can't use trimmed validators for
+    // fragments on non-object types.
     expect(validator?.toString()).toMatchString(`
       {
+        x: [
+          T1.x
+        ]
+        z: [
+          T2.z
+        ]
         y: [
           T1.y
         ]
@@ -3252,8 +3307,13 @@ describe('named fragment selection set restrictions at type', () => {
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'U2'));
     expect(selectionSet.toString()).toBe('{ ... on T1 { x y } }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     expect(validator?.toString()).toMatchString(`
       {
+        x: [
+          T1.x
+        ]
         z: [
           T2.z
         ]
@@ -3268,11 +3328,16 @@ describe('named fragment selection set restrictions at type', () => {
 
     ({ selectionSet, validator } = expandAtType(frag, schema, 'U3'));
     expect(selectionSet.toString()).toBe('{ ... on T2 { z w } }');
+    // Note: Fragments on non-object types always have the full validator and thus
+    // results in the same validator regardless of the type they are expanded at.
     expect(validator?.toString()).toMatchString(`
       {
         x: [
           T1.x
         ]
+        z: [
+          T2.z
+        ]
         y: [
           T1.y
         ]

internals-js/src/operations.ts

@@ -1224,6 +1224,14 @@ export class NamedFragmentDefinition extends DirectiveTargetElement<NamedFragmen
     const expandedSelectionSet = this.expandedSelectionSet();
     const selectionSet = expandedSelectionSet.normalize({ parentType: type });
 
+    if (!isObjectType(this.typeCondition)) {
+      // When the type condition of the fragment is not an object type, the `FieldsInSetCanMerge` rule is more
+      // restrictive and any fields can create conflicts. Thus, we have to use the full validator in this case.
+      // (see https://github.com/graphql/graphql-spec/issues/1085 for details.)
+      const validator = FieldsConflictValidator.build(expandedSelectionSet);
+      return { selectionSet, validator };
+    }
+
     // Note that `trimmed` is the difference of 2 selections that may not have been normalized on the same parent type,
     // so in practice, it is possible that `trimmed` contains some of the selections that `selectionSet` contains, but
     // that they have been simplified in `selectionSet` in such a way that the `minus` call does not see it. However,
@@ -2880,7 +2888,7 @@ class FieldsConflictValidator {
         continue;
       }
 
-      // We're basically checking [FieldInSetCanMerge](https://spec.graphql.org/draft/#FieldsInSetCanMerge()),
+      // We're basically checking [FieldsInSetCanMerge](https://spec.graphql.org/draft/#FieldsInSetCanMerge()),
       // but from 2 set of fields (`thisFields` and `thatFields`) of the same response that we know individually
       // merge already.
       for (const [thisField, thisValidator] of thisFields.entries()) {

query-planner-js/src/__tests__/buildPlan.test.ts

@@ -4,15 +4,24 @@ import {
   operationFromDocument,
   ServiceDefinition,
   Supergraph,
+  buildSubgraph,
 } from '@apollo/federation-internals';
 import gql from 'graphql-tag';
 import {
   FetchNode,
   FlattenNode,
+  PlanNode,
   SequenceNode,
+  SubscriptionNode,
   serializeQueryPlan,
 } from '../QueryPlan';
-import { FieldNode, OperationDefinitionNode, parse } from 'graphql';
+import {
+  FieldNode,
+  OperationDefinitionNode,
+  parse,
+  validate,
+  GraphQLError,
+} from 'graphql';
 import {
   composeAndCreatePlanner,
   composeAndCreatePlannerWithOptions,
@@ -5041,8 +5050,157 @@ describe('Named fragments preservation', () => {
       }
     `);
   });
+
+  it('validates fragments on non-object types across spreads', () => {
+    const subgraph1 = {
+      name: 'subgraph1',
+      typeDefs: gql`
+        type Query {
+          theEntity: AnyEntity
+        }
+
+        union AnyEntity = EntityTypeA | EntityTypeB
+
+        type EntityTypeA @key(fields: "unifiedEntityId") {
+          unifiedEntityId: ID!
+          unifiedEntity: UnifiedEntity
+        }
+
+        type EntityTypeB @key(fields: "unifiedEntityId") {
+          unifiedEntityId: ID!
+          unifiedEntity: UnifiedEntity
+        }
+
+        interface UnifiedEntity {
+          id: ID!
+        }
+
+        type Generic implements UnifiedEntity @key(fields: "id") {
+          id: ID!
+        }
+
+        type Movie implements UnifiedEntity @key(fields: "id") {
+          id: ID!
+        }
+
+        type Show implements UnifiedEntity @key(fields: "id") {
+          id: ID!
+        }
+      `,
+    };
+
+    const subgraph2 = {
+      name: 'subgraph2',
+      typeDefs: gql`
+        interface Video {
+          videoId: Int!
+          taglineMessage(uiContext: String): String
+        }
+
+        interface UnifiedEntity {
+          id: ID!
+        }
+
+        type Generic implements UnifiedEntity @key(fields: "id") {
+          id: ID!
+          taglineMessage(uiContext: String): String
+        }
+
+        type Movie implements UnifiedEntity & Video @key(fields: "id") {
+          videoId: Int!
+          id: ID!
+          taglineMessage(uiContext: String): String
+        }
+
+        type Show implements UnifiedEntity & Video @key(fields: "id") {
+          videoId: Int!
+          id: ID!
+          taglineMessage(uiContext: String): String
+        }
+      `,
+    };
+
+    const [api, queryPlanner] = composeAndCreatePlannerWithOptions(
+      [subgraph1, subgraph2],
+      { reuseQueryFragments: true },
+    );
+
+    const query = gql`
+      query Search {
+        theEntity {
+          ... on EntityTypeA {
+            unifiedEntity {
+              ... on Generic {
+                taglineMessage(uiContext: "Generic")
+              }
+            }
+          }
+          ... on EntityTypeB {
+            unifiedEntity {
+              ...VideoSummary
+            }
+          }
+        }
+      }
+
+      fragment VideoSummary on Video {
+        videoId # Note: This extra field selection is necessary, so this fragment is not ignored.
+        taglineMessage(uiContext: "Video")
+      }
+    `;
+    const operation = operationFromDocument(api, query, { validate: true });
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+    const validationErrors = validateSubFetches(plan.node, subgraph2);
+    expect(validationErrors).toHaveLength(0);
+  });
 });
 
+/**
+ * For each fetch in a PlanNode validate the generated operation is actually spec valid against its subgraph schema
+ * @param plan
+ * @param subgraphs
+ */
+function validateSubFetches(
+  plan: PlanNode | SubscriptionNode | undefined,
+  subgraphDef: ServiceDefinition,
+): {
+  errors: readonly GraphQLError[];
+  serviceName: string;
+  fetchNode: FetchNode;
+}[] {
+  if (!plan) {
+    return [];
+  }
+  const fetches = findFetchNodes(subgraphDef.name, plan);
+  const results: {
+    errors: readonly GraphQLError[];
+    serviceName: string;
+    fetchNode: FetchNode;
+  }[] = [];
+  for (const fetch of fetches) {
+    const subgraphName: string = fetch.serviceName;
+    const operation: string = fetch.operation;
+    const subgraph = buildSubgraph(
+      subgraphName,
+      'http://subgraph',
+      subgraphDef.typeDefs,
+    );
+    const gql_errors = validate(
+      subgraph.schema.toGraphQLJSSchema(),
+      parse(operation),
+    );
+    if (gql_errors.length > 0) {
+      results.push({
+        errors: gql_errors,
+        serviceName: subgraphName,
+        fetchNode: fetch,
+      });
+    }
+  }
+  return results;
+}
+
 describe('Fragment autogeneration', () => {
   const subgraph = {
     name: 'Subgraph1',