diff --git a/.changeset/shaggy-phones-know.md b/.changeset/shaggy-phones-know.md new file mode 100644 index 000000000..fc51db809 --- /dev/null +++ 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 index 5db67d995..6bc6a65ce 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -69,7 +69,7 @@ function haveSameDirectives(op1: TElement, op } abstract class AbstractOperationElement> extends DirectiveTargetElement { - private attachements?: Map; + private attachments?: Map; constructor( schema: Schema, @@ -97,21 +97,21 @@ abstract class AbstractOperationElement> 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) { - if (this.attachements) { - for (const [k, v] of this.attachements.entries()) { - elt.addAttachement(k, v); + protected copyAttachmentsTo(elt: AbstractOperationElement) { + if (this.attachments) { + for (const [k, v] of this.attachments.entries()) { + elt.addAttachment(k, v); } } } @@ -170,6 +170,17 @@ export class Field ex baseType(): NamedType { return baseType(this.definition.type!); } + + copy(): Field { + const newField = new Field( + this.definition, + this.args, + this.appliedDirectives, + this.alias, + ); + this.copyAttachmentsTo(newField); + return newField; + } withUpdatedArguments(newArgs: TArgs): Field { const newField = new Field( @@ -178,7 +189,7 @@ export class Field ex this.appliedDirectives, this.alias, ); - this.copyAttachementsTo(newField); + this.copyAttachmentsTo(newField); return newField; } @@ -189,7 +200,7 @@ export class Field ex this.appliedDirectives, this.alias, ); - this.copyAttachementsTo(newField); + this.copyAttachmentsTo(newField); return newField; } @@ -200,7 +211,7 @@ export class Field ex this.appliedDirectives, newAlias, ); - this.copyAttachementsTo(newField); + this.copyAttachmentsTo(newField); return newField; } @@ -211,7 +222,7 @@ export class Field ex newDirectives, this.alias, ); - this.copyAttachementsTo(newField); + this.copyAttachmentsTo(newField); return newField; } @@ -505,13 +516,13 @@ export class FragmentElement extends AbstractOperationElement { // 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[]): 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 { } 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 { .concat(new Directive(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, 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, 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 index 9825a9bcb..bc550aba3 100644 --- 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 [