Faster approach to code for reusing fragments in fetches (#2604)

* Faster approach to code for reusing fragments in fetches

This commit somehwat "inverse" the way fragment reuse is tried.
Before this commit, fragment reuse was tried depth-first, meaning
that we tried reusing fragment on the leaf of the result set and
then up on every selection. Unfortunately, doing so creates a
number of subtlety that force the checks done on every selection
to be somewhat costly, which adds up.

This commit change the process a bit to test for fragment reuse
"at the top level" first (and so always fully expanded selections),
and recurse down for the part that didn't matched anything. In practice,
doing so simplify things a bit due to always dealing with expanded
selections, and this allow to the checks simpler and more
efficient.

Overall, this commit usually improves the time spend for trying fragment
reuse during query planning (sometimes substantially), hence lowering
query planning time.

* Review feedback

Author: Sylvain Lebresne

.changeset/brown-bats-float.md

@@ -0,0 +1,11 @@
+---
+"@apollo/query-planner": patch
+"@apollo/composition": patch
+"@apollo/federation-internals": patch
+"@apollo/gateway": patch
+---
+
+Re-work the code use to try to reuse query named fragments to improve performance (thus sometimes improving query
+planning performance), to fix a possibly raised assertion error (with a message of form like `Cannot add selection of
+field X to selection set of parent type Y`), and to fix a rare issue where an interface or union field was not being
+queried for all the types it should be.

composition-js/src/validate.ts

@@ -189,7 +189,7 @@ function buildWitnessOperation(witness: RootPath<Transition>): Operation {
     schema,
     root.rootKind,
     buildWitnessNextStep([...witness].map(e => e[0]), 0)!,
-    new VariableDefinitions()
+    new VariableDefinitions(),
   );
 }
 

gateway-js/src/executeQueryPlan.ts

@@ -218,7 +218,7 @@ export async function executeQueryPlan(
             input: unfilteredData,
             introspectionHandling: (f) => executeIntrospection(
               operationContext.schema,
-              f.expandAllFragments().toSelectionNode(),
+              f.expandFragments().toSelectionNode(),
               operationContext.operation.variableDefinitions,
               variables,
             ),

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

@@ -1,11 +1,10 @@
 import {
-    CompositeType,
   defaultRootName,
   Schema,
   SchemaRootKind,
 } from '../../dist/definitions';
 import { buildSchema } from '../../dist/buildSchema';
-import { MutableSelectionSet, Operation, operationFromDocument, parseOperation, parseSelectionSet } from '../../dist/operations';
+import { MutableSelectionSet, Operation, operationFromDocument, parseOperation } from '../../dist/operations';
 import './matchers';
 import { DocumentNode, FieldNode, GraphQLError, Kind, OperationDefinitionNode, OperationTypeNode, SelectionNode, SelectionSetNode } from 'graphql';
 
@@ -57,18 +56,19 @@ describe('fragments optimization', () => {
     // this are just about testing the reuse of fragments and this make things shorter/easier to write.
     // There is tests in `buildPlan.test.ts` that double-check that we don't reuse fragments used only
     // once in actual query plans.
-    const optimized = withoutFragments.optimize(operation.selectionSet.fragments!, 1);
+    const optimized = withoutFragments.optimize(operation.fragments!, 1);
     expect(optimized.toString()).toMatchString(operation.toString());
   }
 
-  test('handles fragments using other fragments', () => {
+  test('optimize fragments using other fragments when possible', () => {
     const schema = parseSchema(`
       type Query {
-        t: T1
+        t: I
       }
 
       interface I {
         b: Int
+        u: U
       }
 
       type T1 implements I {
@@ -154,8 +154,68 @@ describe('fragments optimization', () => {
       }
     `);
 
-    const optimized = withoutFragments.optimize(operation.selectionSet.fragments!);
+    const optimized = withoutFragments.optimize(operation.fragments!);
     expect(optimized.toString()).toMatchString(`
+      fragment OnU on U {
+        ... on I {
+          b
+        }
+        ... on T1 {
+          a
+          b
+        }
+        ... on T2 {
+          x
+          y
+        }
+      }
+
+      {
+        t {
+          ...OnU
+          u {
+            ...OnU
+          }
+        }
+      }
+    `);
+  });
+
+  test('handles fragments using other fragments', () => {
+    const schema = parseSchema(`
+      type Query {
+        t: I
+      }
+
+      interface I {
+        b: Int
+        c: Int
+        u1: U
+        u2: U
+      }
+
+      type T1 implements I {
+        a: Int
+        b: Int
+        c: Int
+        me: T1
+        u1: U
+        u2: U
+      }
+
+      type T2 implements I {
+        x: String
+        y: String
+        b: Int
+        c: Int
+        u1: U
+        u2: U
+      }
+
+      union U = T1 | T2
+    `);
+
+    const operation = parseOperation(schema, `
       fragment OnT1 on T1 {
         a
         b
@@ -168,17 +228,127 @@ describe('fragments optimization', () => {
 
       fragment OnI on I {
         b
+        c
       }
 
-      {
+      fragment OnU on U {
+        ...OnI
+        ...OnT1
+        ...OnT2
+      }
+
+      query {
         t {
-          ...OnI
           ...OnT1
           ...OnT2
-          u {
-            ...OnI
+          u1 {
+            ...OnU
+          }
+          u2 {
+            ...OnU
+          }
+          ... on T1 {
+            me {
+              ...OnI
+            }
+          }
+        }
+      }
+    `);
+
+    const withoutFragments = parseOperation(schema, operation.toString(true, true));
+    expect(withoutFragments.toString()).toMatchString(`
+      {
+        t {
+          ... on T1 {
+            a
+            b
+            me {
+              ... on I {
+                b
+                c
+              }
+            }
+          }
+          ... on T2 {
+            x
+            y
+          }
+          u1 {
+            ... on U {
+              ... on I {
+                b
+                c
+              }
+              ... on T1 {
+                a
+                b
+              }
+              ... on T2 {
+                x
+                y
+              }
+            }
+          }
+          u2 {
+            ... on U {
+              ... on I {
+                b
+                c
+              }
+              ... on T1 {
+                a
+                b
+              }
+              ... on T2 {
+                x
+                y
+              }
+            }
+          }
+        }
+      }
+    `);
+
+    const optimized = withoutFragments.optimize(operation.fragments!);
+    // We should reuse and keep all fragments, because 1) onU is used twice and 2)
+    // all the other ones are used once in the query, and once in onU definition.
+    expect(optimized.toString()).toMatchString(`
+      fragment OnT1 on T1 {
+        a
+        b
+      }
+
+      fragment OnT2 on T2 {
+        x
+        y
+      }
+
+      fragment OnI on I {
+        b
+        c
+      }
+
+      fragment OnU on U {
+        ...OnI
+        ...OnT1
+        ...OnT2
+      }
+
+      {
+        t {
+          ... on T1 {
             ...OnT1
-            ...OnT2
+            me {
+              ...OnI
+            }
+          }
+          ...OnT2
+          u1 {
+            ...OnU
+          }
+          u2 {
+            ...OnU
           }
         }
       }
@@ -509,24 +679,24 @@ describe('fragments optimization', () => {
 
         {
           t {
-            ...Frag2
             ...Frag1
+            ...Frag2
           }
         }
       `,
       expanded: `
         {
           t {
-            a
             b {
-              __typename
               x
+              __typename
             }
+            c
             d {
               m
               n
             }
-            c
+            a
           }
         }
       `,
@@ -1265,62 +1435,3 @@ describe('unsatisfiable branches removal', () => {
     expect(withoutUnsatisfiableBranches(input)).toBe(output);
   });
 });
-
-test('contains ignores unecessary fragments even when subtyping is involved', () => {
-  const schema = parseSchema(`
-      type Query {
-        a: A!
-      }
-
-      interface IA1 {
-        b: IB1!
-      }
-
-      interface IA2 {
-        b: IB2!
-      }
-
-      type A implements IA1 & IA2 {
-        b: B!
-      }
-
-      interface IB1 {
-        v1: Int!
-      }
-
-      interface IB2 {
-        v2: Int!
-      }
-
-      type B implements IB1 & IB2 {
-        v1: Int!
-        v2: Int!
-      }
-  `);
-
-  const typeA = schema.type('A') as CompositeType;
-
-  const s1 = parseSelectionSet({
-    parentType: typeA,
-    source: '{ b { v1 v2 } }'
-  });
-
-  const s2 = parseSelectionSet({
-    parentType: typeA,
-    source: '{ ... on IA1 { b { v1 } } ... on IA2 { b { v2 } } }'
-  });
-
-  // Here, A is a concrete type, and IA1 and IA2 are just 2 of its interfaces, so
-  //   a { ... on IA1 { b { v1 } } ... on IA2 { b { v2 } } }
-  // is basically exactly the same as:
-  //   a { b { v1 } b { v2 } }
-  // which is the same as:
-  //   a { b { v1 v2 } }
-  // and that is why we want the `contains` below to work (note that a simple `contains`
-  // that doesn't handle this kind of subtlety could have its use too, it would just be
-  // a different contract, but we want "our" `contains` to handle this because of named
-  // fragments "reconstruction" where that kind of subtlety arises).
-  //
-  // Here, the added subtlety is that there is interface subtyping involved too.
-  expect(s2.contains(s1)).toBeTruthy();
-});