fix handling of __typename optimizations

.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.

internals-js/src/operations.ts

@@ -69,7 +69,7 @@
 }
 
 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 @@
 
   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 @@
   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 @@
       this.appliedDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -189,7 +200,7 @@
       this.appliedDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -200,7 +211,7 @@
       this.appliedDirectives,
       newAlias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -211,7 +222,7 @@
       newDirectives,
       this.alias,
     );
-    this.copyAttachementsTo(newField);
+    this.copyAttachmentsTo(newField);
     return newField;
   }
 
@@ -505,13 +516,13 @@
     // 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 @@
     }
 
     const updated = new FragmentElement(this.sourceType, this.typeCondition, updatedDirectives);
-    this.copyAttachementsTo(updated);
+    this.copyAttachmentsTo(updated);
     return updated;
   }
 
@@ -655,7 +666,7 @@
       .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 @@
     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;

query-planner-js/src/buildPlan.ts

@@ -106,6 +106,7 @@
 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 @@
    * 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 @@
     // (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 @@
         }
         if (!firstFieldSelection && updated.kind === 'FieldSelection') {
           firstFieldSelection = updated;
+          firstFieldIndex = updatedSelections
+            ? updatedSelections.length
+            : i;
         }
       }
 
@@ -3473,7 +3478,7 @@
     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 @@
 
           // 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 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 [