fix handling of __typename optimizations (#3156)

Fixes handling of a `__typename` selection during query planning process. When expanding fragments we were keeping references to the same `Field`s regardless where those fragments appeared in our original selection set. This was generally fine as in most cases we would have same inline fragment selection sets across whole operation but was causing problems when we were applying another optimization by collapsing those expanded inline fragments creating a new selection set. As a result, if any single field selection (within that fragment) would perform optimization around the usage of `__typename`, ALL occurrences of that field selection would get that optimization as well. See example below ```graphql foo { f1 { ... on Bar { __typename ...onBar # will be collapsed and sub selections will optimize __typename } } f2 { ...onBar # sub selections will get __typename optimization from above } } fragment onBar on Bar { b c } ``` --------- Co-authored-by: Sachin D. Shinde <[email protected]>
apollographql · Sep 26, 2024 · 2192f35 · 2192f35
1 parent cda078c
commit 2192f35
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 26 deletions.
diff --git a/.changeset/shaggy-phones-know.md b/.changeset/shaggy-phones-know.md
@@ -0,0 +1,8 @@
+---
+"@apollo/query-planner": patch
+"@apollo/federation-internals": patch
+---
+
+Fixes handling of a `__typename` selection during query planning process.
+
+When expanding fragments we were keeping references to the same `Field`s regardless where those fragments appeared in our original selection set. This was generally fine as in most cases we would have same inline fragment selection sets across whole operation but was causing problems when we were applying another optimization by collapsing those expanded inline fragments creating a new selection set. As a result, if any single field selection (within that fragment) would perform optimization around the usage of `__typename`, ALL occurrences of that field selection would get that optimization as well.
diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts
@@ -69,7 +69,7 @@ function haveSameDirectives<TElement extends OperationElement>(op1: TElement, op
 }
 
 abstract class AbstractOperationElement<T extends AbstractOperationElement<T>> extends DirectiveTargetElement<T> {
-  private attachements?: Map<string, string>;
+  private attachments?: Map<string, string>;
 
   constructor(
     schema: Schema,
@@ -97,21 +97,21 @@ abstract class AbstractOperationElement<T extends AbstractOperationElement<T>> e
 
   protected abstract collectVariablesInElement(collector: VariableCollector): void;
 
-  addAttachement(key: string, value: string) {
-    if (!this.attachements) {
-      this.attachements = new Map();
+  addAttachment(key: string, value: string) {
+    if (!this.attachments) {
+      this.attachments = new Map();
     }
-    this.attachements.set(key, value);
+    this.attachments.set(key, value);
   }
 
-  getAttachement(key: string): string | undefined {
-    return this.attachements?.get(key);
+  getAttachment(key: string): string | undefined {
+    return this.attachments?.get(key);
   }
 
-  protected copyAttachementsTo(elt: AbstractOperationElement<any>) {
-    if (this.attachements) {
-      for (const [k, v] of this.attachements.entries()) {
-        elt.addAttachement(k, v);
+  protected copyAttachmentsTo(elt: AbstractOperationElement<any>) {
+    if (this.attachments) {
+      for (const [k, v] of this.attachments.entries()) {
+        elt.addAttachment(k, v);
       }
     }
   }
@@ -170,6 +170,17 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
   baseType(): NamedType {
     return baseType(this.definition.type!);
   }
+
+  copy(): Field<TArgs> {
+    const newField = new Field<TArgs>(
+      this.definition,
+      this.args,
+      this.appliedDirectives,
+      this.alias,
+    );
+    this.copyAttachmentsTo(newField);
+    return newField;
+  }
 
   withUpdatedArguments(newArgs: TArgs): Field<TArgs> {
     const newField = new Field<TArgs>(
@@ -178,7 +189,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
       this.appliedDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -189,7 +200,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
       this.appliedDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -200,7 +211,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
       this.appliedDirectives,
       newAlias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -211,7 +222,7 @@ export class Field<TArgs extends {[key: string]: any} = {[key: string]: any}> ex
       newDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -505,13 +516,13 @@ export class FragmentElement extends AbstractOperationElement<FragmentElement> {
     // schema (typically, the supergraph) than `this.sourceType` (typically, a subgraph), then the new condition uses the
     // definition of the proper schema (the supergraph in such cases, instead of the subgraph).
     const newFragment = new FragmentElement(newSourceType, newCondition?.name, this.appliedDirectives);
-    this.copyAttachementsTo(newFragment);
+    this.copyAttachmentsTo(newFragment);
     return newFragment;
   }
 
   withUpdatedDirectives(newDirectives: Directive<OperationElement>[]): FragmentElement {
     const newFragment = new FragmentElement(this.sourceType, this.typeCondition, newDirectives);
-    this.copyAttachementsTo(newFragment);
+    this.copyAttachmentsTo(newFragment);
     return newFragment;
   }
 
@@ -590,7 +601,7 @@ export class FragmentElement extends AbstractOperationElement<FragmentElement> {
     }
 
     const updated = new FragmentElement(this.sourceType, this.typeCondition, updatedDirectives);
-    this.copyAttachementsTo(updated);
+    this.copyAttachmentsTo(updated);
     return updated;
   }
 
@@ -655,7 +666,7 @@ export class FragmentElement extends AbstractOperationElement<FragmentElement> {
       .concat(new Directive<FragmentElement>(deferDirective.name, newDeferArgs));
 
     const updated = new FragmentElement(this.sourceType, this.typeCondition, updatedDirectives);
-    this.copyAttachementsTo(updated);
+    this.copyAttachmentsTo(updated);
     return updated;
   }
 
@@ -3042,6 +3053,12 @@ export class FieldSelection extends AbstractSelection<Field<any>, undefined, Fie
     return this.element.definition.name === typenameFieldName;
   }
 
+  withAttachment(key: string, value: string): FieldSelection {
+    const updatedField = this.element.copy();
+    updatedField.addAttachment(key, value);
+    return this.withUpdatedElement(updatedField);
+  }
+
   withUpdatedComponents(field: Field<any>, selectionSet: SelectionSet | undefined): FieldSelection {
     if (this.element === field && this.selectionSet === selectionSet) {
       return this;

diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts
@@ -106,6 +106,7 @@ import { enforceQueryPlannerConfigDefaults, QueryPlannerConfig, validateQueryPla
 import { generateAllPlansAndFindBest } from "./generateAllPlans";
 import { QueryPlan, ResponsePath, SequenceNode, PlanNode, ParallelNode, FetchNode, SubscriptionNode, trimSelectionNodes } from "./QueryPlan";
 
+
 const debug = newDebugLogger('plan');
 
 // Somewhat random string used to optimise handling __typename in some cases. See usage for details. The concrete value
@@ -3391,10 +3392,10 @@ export class QueryPlanner {
    * implements an alternative: to avoid the query planning spending time of exploring options for
    * __typename, we "remove" the __typename selections from the operation. But of course, we still
    * need to ensure that __typename is effectively queried, so as we do that removal, we also "tag"
-   * one of the "sibling" selection (using `addAttachement`) to remember that __typename needs to
+   * one of the "sibling" selection (using `addAttachment`) to remember that __typename needs to
    * be added back eventually. The core query planning algorithm will ignore that tag, and because
    * __typename has been otherwise removed, we'll save any related work. But as we build the final
-   * query plan, we'll check back for those "tags" (see `getAttachement` in `computeGroupsForTree`),
+   * query plan, we'll check back for those "tags" (see `getAttachment` in `computeGroupsForTree`),
    * and when we fine one, we'll add back the request to __typename. As this only happen after the
    * query planning algorithm has computed all choices, we achieve our goal of not considering useless
    * choices due to __typename. Do note that if __typename is the "only" selection of some selection
@@ -3413,6 +3414,7 @@ export class QueryPlanner {
     // (for instance, the fragment condition could be "less precise" than the parent type, in which case query planning
     // will ignore it) and tagging those could lose the tagging.
     let firstFieldSelection: FieldSelection | undefined = undefined;
+    let firstFieldIndex = -1;
     for (let i = 0; i < selections.length; i++) {
       const selection = selections[i];
       let updated: Selection | undefined;
@@ -3445,6 +3447,9 @@ export class QueryPlanner {
         }
         if (!firstFieldSelection && updated.kind === 'FieldSelection') {
           firstFieldSelection = updated;
+          firstFieldIndex = updatedSelections
+            ? updatedSelections.length
+            : i;
         }
       }
 
@@ -3473,7 +3478,7 @@ export class QueryPlanner {
     if (typenameSelection) {
       if (firstFieldSelection) {
         // Note that as we tag the element, we also record the alias used if any since that needs to be preserved.
-        firstFieldSelection.element.addAttachement(SIBLING_TYPENAME_KEY, typenameSelection.element.alias ? typenameSelection.element.alias : '');
+        updatedSelections[firstFieldIndex] = firstFieldSelection.withAttachment(SIBLING_TYPENAME_KEY, typenameSelection.element.alias ? typenameSelection.element.alias : '');
       } else {
         // If we have no other field selection, then we can't optimize __typename and we need to add
         // it back to the updated subselections (we add it first because that's usually where we
@@ -4328,10 +4333,10 @@ function computeGroupsForTree(
 
           // We have a operation element, field or inline fragment. We first check if it's been "tagged" to remember that __typename
           // must be queried. See the comment on the `optimizeSiblingTypenames()` method to see why this exists.
-          const typenameAttachment = operation.getAttachement(SIBLING_TYPENAME_KEY);
+          const typenameAttachment = operation.getAttachment(SIBLING_TYPENAME_KEY);
           if (typenameAttachment !== undefined) {
             // We need to add the query __typename for the current type in the current group.
-            // Note that the value of the "attachement" is the alias or '' if there is no alias
+            // Note that the value of the "attachment" is the alias or '' if there is no alias
             const alias = typenameAttachment === '' ? undefined : typenameAttachment;
             const typenameField = new Field(operation.parentType.typenameField()!, undefined, undefined, alias);
             group.addAtPath(path.inGroup().concat(typenameField));
@@ -4631,12 +4636,12 @@ function addTypenameFieldForAbstractTypes(selectionSet: SelectionSet, parentType
 function addBackTypenameInAttachments(selectionSet: SelectionSet): SelectionSet {
   return selectionSet.lazyMap((s) => {
     const updated = s.mapToSelectionSet((ss) => addBackTypenameInAttachments(ss));
-    const typenameAttachment = s.element.getAttachement(SIBLING_TYPENAME_KEY);
+    const typenameAttachment = s.element.getAttachment(SIBLING_TYPENAME_KEY);
     if (typenameAttachment === undefined) {
       return updated;
     } else {
       // We need to add the query __typename for the current type in the current group.
-      // Note that the value of the "attachement" is the alias or '' if there is no alias
+      // Note that the value of the "attachment" is the alias or '' if there is no alias
       const alias = typenameAttachment === '' ? undefined : typenameAttachment;
       const typenameField = new Field(s.element.parentType.typenameField()!, undefined, undefined, alias);
       return [