fix: updated generateQueryFragments option to only generate fragments at least 2 selections

internals-js/src/operations.ts

@@ -1390,28 +1390,6 @@ export class NamedFragments {
     });
   }
 
-  // When we rebase named fragments on a subgraph schema, only a subset of what the fragment handles may belong
-  // to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or
-  // want to consider to ignore the fragment for that subgraph, and that is when:
-  // 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway.
-  // 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using
-  //   the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the
-  //   fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's
-  //   enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely
-  //   to be common is when the subset ends up being just `__typename`: this would basically mean the fragment
-  //   don't really apply to the subgraph, and that this will ensure this is the case.
-  private selectionSetIsWorthUsing(selectionSet: SelectionSet): boolean {
-    const selections = selectionSet.selections();
-    if (selections.length === 0) {
-      return false;
-    }
-    if (selections.length === 1) {
-      const s = selections[0];
-      return !(s.kind === 'FieldSelection' && s.element.isLeafField());
-    }
-    return true;
-  }
-
   rebaseOn(schema: Schema): NamedFragments | undefined {
     return this.mapInDependencyOrder((fragment, newFragments) => {
       const rebasedType = schema.type(fragment.selectionSet.parentType.name);
@@ -1423,7 +1401,7 @@ export class NamedFragments {
       // Rebasing can leave some inefficiencies in some case (particularly when a spread has to be "expanded", see `FragmentSpreadSelection.rebaseOn`),
       // so we do a top-level normalization to keep things clean.
       rebasedSelection = rebasedSelection.normalize({ parentType: rebasedType });
-      return this.selectionSetIsWorthUsing(rebasedSelection)
+      return rebasedSelection.isWorthUsing()
         ? new NamedFragmentDefinition(schema, fragment.name, rebasedType).setSelectionSet(rebasedSelection)
         : undefined;
     });
@@ -1556,7 +1534,8 @@ export class SelectionSet {
     seenSelections: Map<string, [SelectionSet, NamedFragmentDefinition][]> = new Map(),
   ): [SelectionSet, NamedFragments] {
     const minimizedSelectionSet = this.lazyMap((selection) => {
-      if (selection.kind === 'FragmentSelection' && selection.element.typeCondition && selection.element.appliedDirectives.length === 0 && selection.selectionSet) {
+      if (selection.kind === 'FragmentSelection' && selection.element.typeCondition && selection.element.appliedDirectives.length === 0
+          && selection.selectionSet && selection.selectionSet.isWorthUsing() ) {
         // No proper hash code, so we use a unique enough number that's cheap to
         // compute and handle collisions as necessary.
         const mockHashCode = `on${selection.element.typeCondition}` + selection.selectionSet.selections().length;
@@ -1596,7 +1575,7 @@ export class SelectionSet {
       }
       return selection;
     });
-    
+
     return [minimizedSelectionSet, namedFragments];
   }
 
@@ -2142,6 +2121,34 @@ export class SelectionSet {
         : selectionsToString;
     }
   }
+
+  // `isWorthUsing` method is used to determine whether we want to factor out
+  // given selection set into a named fragment so it can be reused across the query.
+  // Currently, it is used in these cases:
+  // 1) to reuse existing named fragments in subgraph queries (when reuseQueryFragments is on)
+  // 2) to factor selection sets into named fragments (when generateQueryFragments is on).
+  //
+  // When we rebase named fragments on a subgraph schema, only a subset of what the fragment handles may belong
+  // to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or
+  // want to consider to ignore the fragment for that subgraph, and that is when:
+  // 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway.
+  // 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using
+  //   the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the
+  //   fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's
+  //   enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely
+  //   to be common is when the subset ends up being just `__typename`: this would basically mean the fragment
+  //   don't really apply to the subgraph, and that this will ensure this is the case.
+  isWorthUsing(): boolean {
+    const selections = this.selections();
+    if (selections.length === 0) {
+      return false;
+    }
+    if (selections.length === 1) {
+      const s = selections[0];
+      return !(s.kind === 'FieldSelection' && s.element.isLeafField());
+    }
+    return true;
+  }
 }
 
 type PathBasedUpdate = { path: OperationPath, selections?: Selection | SelectionSet | readonly Selection[] };

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

@@ -5088,25 +5088,24 @@ describe('Fragment autogeneration', () => {
 
     const plan = queryPlanner.buildQueryPlan(operation);
 
+    // Note: `... on B {}` won't be replaced, since it has only one field.
     expect(plan).toMatchInlineSnapshot(`
       QueryPlan {
         Fetch(service: "Subgraph1") {
           {
             t {
               __typename
               ..._generated_onA2_0
-              ..._generated_onB1_0
+              ... on B {
+                z
+              }
             }
           }
 
           fragment _generated_onA2_0 on A {
             x
             y
           }
-
-          fragment _generated_onB1_0 on B {
-            z
-          }
         },
       }
     `);
@@ -5127,48 +5126,91 @@ describe('Fragment autogeneration', () => {
               t {
                 ... on A {
                   x
+                  y
                 }
                 ... on B {
                   z
                 }
               }
             }
-            ... on B {
-              z
-            }
           }
         }
       `,
     );
 
     const plan = queryPlanner.buildQueryPlan(operation);
 
+    // Note: `... on B {}` won't be replaced, since it has only one field.
     expect(plan).toMatchInlineSnapshot(`
       QueryPlan {
         Fetch(service: "Subgraph1") {
           {
             t {
               __typename
               ..._generated_onA3_0
-              ..._generated_onB1_0
             }
           }
 
-          fragment _generated_onA1_0 on A {
+          fragment _generated_onA2_0 on A {
             x
-          }
-
-          fragment _generated_onB1_0 on B {
-            z
+            y
           }
 
           fragment _generated_onA3_0 on A {
             x
             y
+            t {
+              __typename
+              ..._generated_onA2_0
+              ... on B {
+                z
+              }
+            }
+          }
+        },
+      }
+    `);
+  });
+
+  it('handles fragments with one non-leaf field', () => {
+    const [api, queryPlanner] = composeAndCreatePlannerWithOptions([subgraph], {
+      generateQueryFragments: true,
+    });
+    const operation = operationFromDocument(
+      api,
+      gql`
+        query {
+          t {
+            ... on A {
+              t {
+                ... on B {
+                  z
+                }
+              }
+            }
+          }
+        }
+      `,
+    );
+
+    const plan = queryPlanner.buildQueryPlan(operation);
+
+    expect(plan).toMatchInlineSnapshot(`
+      QueryPlan {
+        Fetch(service: "Subgraph1") {
+          {
             t {
               __typename
               ..._generated_onA1_0
-              ..._generated_onB1_0
+            }
+          }
+
+          fragment _generated_onA1_0 on A {
+            t {
+              __typename
+              ... on B {
+                z
+              }
             }
           }
         },