From b688db5b6419f84bcaa203c0e946bb87360b2eb9 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 15 Aug 2024 09:26:29 -0500 Subject: [PATCH 1/5] Fragment variable definitions erased in subgraph queries Fixes #3112 Before creating an Operation, we call `collectUsedVariables`, which will only pull values from the selection set and not from fragments. This isn't great, because a variable used in a fragment won't be collected, and it doesn't make sense to collect variables from fragments because it's before they are optimized and many will be unused. The inelegant solution I came up with is to pass in available variables in calls to `optimize` or `generateQueryFragments` for an operation where we can add back in the unused variables. This should be ok, because we are guaranteed that exactly one of them will get called by `toPlanNode`. Pretty sure there won't be too much overhead added because we'll only call this once per subgraph fetch. --- .changeset/poor-seahorses-whisper.md | 6 ++++ internals-js/src/operations.ts | 41 +++++++++++++++++++++++----- query-planner-js/src/buildPlan.ts | 22 ++++++++++++--- 3 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 .changeset/poor-seahorses-whisper.md diff --git a/.changeset/poor-seahorses-whisper.md b/.changeset/poor-seahorses-whisper.md new file mode 100644 index 000000000..5a96d8d4a --- /dev/null +++ b/.changeset/poor-seahorses-whisper.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +"@apollo/federation-internals": patch +--- + +Fix issue where variable was not passed into subgraph wwhen embedded in a fragment diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index fb2adb6a9..b52565f27 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -56,6 +56,8 @@ import { assert, mapKeys, mapValues, MapWithCachedArrays, MultiMap, SetMultiMap import { argumentsEquals, argumentsFromAST, isValidValue, valueToAST, valueToString } from "./values"; import { v1 as uuidv1 } from 'uuid'; +export const DEFAULT_MIN_USAGES_TO_OPTIMIZE = 2; + function validate(condition: any, message: () => string, sourceAST?: ASTNode): asserts condition { if (!condition) { throw ERRORS.INVALID_GRAPHQL.err(message(), { nodes: sourceAST }); @@ -934,25 +936,41 @@ export class Operation extends DirectiveTargetElement { this.appliedDirectives, ); } + + private collectVariablesFromFragments(allAvailableVariables: VariableDefinitions, fragments: NamedFragments): VariableDefinitions { + const varDefs = new VariableDefinitions(); + varDefs.addAll(this.variableDefinitions); + + const collector = new VariableCollector(); + collectVariablesInNamedFragments(fragments, collector); + + for (const v of collector.variables()) { + const def = allAvailableVariables.definition(v.name); + if (def) { + varDefs.add(def); + } + } + return varDefs; + } // Returns a copy of this operation with the provided updated selection set and fragments. - private withUpdatedSelectionSetAndFragments(newSelectionSet: SelectionSet, newFragments: NamedFragments | undefined): Operation { + private withUpdatedSelectionSetAndFragments(newSelectionSet: SelectionSet, newFragments: NamedFragments | undefined, allAvailableVariables?: VariableDefinitions): Operation { if (this.selectionSet === newSelectionSet && newFragments === this.fragments) { return this; } - + return new Operation( this.schema(), this.rootKind, newSelectionSet, - this.variableDefinitions, + (allAvailableVariables && newFragments) ? this.collectVariablesFromFragments(allAvailableVariables, newFragments) : this.variableDefinitions, newFragments, this.name, this.appliedDirectives, ); } - optimize(fragments?: NamedFragments, minUsagesToOptimize: number = 2): Operation { + optimize(fragments?: NamedFragments, minUsagesToOptimize: number = DEFAULT_MIN_USAGES_TO_OPTIMIZE, allAvailableVariables?: VariableDefinitions): Operation { assert(minUsagesToOptimize >= 1, `Expected 'minUsagesToOptimize' to be at least 1, but got ${minUsagesToOptimize}`) if (!fragments || fragments.isEmpty()) { return this; @@ -1001,16 +1019,17 @@ export class Operation extends DirectiveTargetElement { } } - return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined); + return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined, allAvailableVariables); } - generateQueryFragments(): Operation { + generateQueryFragments(allAvailableVariables: VariableDefinitions = new VariableDefinitions()): Operation { const [minimizedSelectionSet, fragments] = this.selectionSet.minimizeSelectionSet(); + return new Operation( this.schema(), this.rootKind, minimizedSelectionSet, - this.variableDefinitions, + allAvailableVariables ? this.collectVariablesFromFragments(allAvailableVariables, fragments) : this.variableDefinitions, fragments, this.name, this.appliedDirectives, @@ -4021,3 +4040,11 @@ export function hasSelectionWithPredicate(selectionSet: SelectionSet, predicate: } return false; } + +export function collectVariablesInNamedFragments(fragments: NamedFragments, collector: VariableCollector) { + for (const namedFragment of fragments.definitions()) { + namedFragment.selectionSet.usedVariables().forEach(v => { + collector.add(v); + }); + } +} diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 6a27faaa1..8287d1ae5 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -25,7 +25,6 @@ import { Variable, VariableDefinition, VariableDefinitions, - VariableCollector, newDebugLogger, selectionOfElement, selectionSetOfElement, @@ -64,6 +63,8 @@ import { isInputType, possibleRuntimeTypes, NamedType, + VariableCollector, + DEFAULT_MIN_USAGES_TO_OPTIMIZE, } from "@apollo/federation-internals"; import { advanceSimultaneousPathsWithOperation, @@ -1589,18 +1590,31 @@ class FetchGroup { ); if (this.generateQueryFragments) { - operation = operation.generateQueryFragments(); + operation = operation.generateQueryFragments(variableDefinitions); } else { - operation = operation.optimize(fragments?.forSubgraph(this.subgraphName, subgraphSchema)); + operation = operation.optimize( + fragments?.forSubgraph(this.subgraphName, subgraphSchema), + DEFAULT_MIN_USAGES_TO_OPTIMIZE, + variableDefinitions, + ); } + // collect all used variables in the selection and in used Fragments + const usedVariables = new Set(selection.usedVariables().map(v => v.name)); + if (operation.fragments) { + for (const namedFragment of operation.fragments.definitions()) { + namedFragment.selectionSet.usedVariables().forEach(v => { + usedVariables.add(v.name); + }); + } + } const operationDocument = operationToDocument(operation); const fetchNode: FetchNode = { kind: 'Fetch', id: this.id, serviceName: this.subgraphName, requires: inputNodes ? trimSelectionNodes(inputNodes.selections) : undefined, - variableUsages: selection.usedVariables().map(v => v.name), + variableUsages: (Array.from(usedVariables)), operation: stripIgnoredCharacters(print(operationDocument)), operationKind: schemaRootKindToOperationKind(operation.rootKind), operationName: operation.name, From 6d0a5d0fbda3ca71a166dd6f86f960c663efbc2c Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 15 Aug 2024 09:27:47 -0500 Subject: [PATCH 2/5] typo --- .changeset/poor-seahorses-whisper.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/poor-seahorses-whisper.md b/.changeset/poor-seahorses-whisper.md index 5a96d8d4a..0dd498b33 100644 --- a/.changeset/poor-seahorses-whisper.md +++ b/.changeset/poor-seahorses-whisper.md @@ -3,4 +3,4 @@ "@apollo/federation-internals": patch --- -Fix issue where variable was not passed into subgraph wwhen embedded in a fragment +Fix issue where variable was not passed into subgraph when embedded in a fragment From 74c1db3ded8b0f9770287556b79705c84d52ef00 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 21 Aug 2024 13:25:07 -0500 Subject: [PATCH 3/5] Updates from code review comments --- internals-js/src/operations.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index b52565f27..00ef35605 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -959,11 +959,18 @@ export class Operation extends DirectiveTargetElement { return this; } + let newVariableDefinitions: VariableDefinitions; + if (allAvailableVariables && newFragments) { + newVariableDefinitions = this.collectVariablesFromFragments(allAvailableVariables, newFragments); + newVariableDefinitions.addAll(this.variableDefinitions); + } else { + newVariableDefinitions = this.variableDefinitions; + } return new Operation( this.schema(), this.rootKind, newSelectionSet, - (allAvailableVariables && newFragments) ? this.collectVariablesFromFragments(allAvailableVariables, newFragments) : this.variableDefinitions, + newVariableDefinitions, newFragments, this.name, this.appliedDirectives, From f75cf8120ae9fcaba8d99dc8e0c5474e0239d17d Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 21 Aug 2024 13:28:24 -0500 Subject: [PATCH 4/5] generate query fragments does not need to collect variables from fragments --- internals-js/src/operations.ts | 4 ++-- query-planner-js/src/buildPlan.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 00ef35605..cc9b453ee 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -1029,14 +1029,14 @@ export class Operation extends DirectiveTargetElement { return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined, allAvailableVariables); } - generateQueryFragments(allAvailableVariables: VariableDefinitions = new VariableDefinitions()): Operation { + generateQueryFragments(): Operation { const [minimizedSelectionSet, fragments] = this.selectionSet.minimizeSelectionSet(); return new Operation( this.schema(), this.rootKind, minimizedSelectionSet, - allAvailableVariables ? this.collectVariablesFromFragments(allAvailableVariables, fragments) : this.variableDefinitions, + this.variableDefinitions, fragments, this.name, this.appliedDirectives, diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 8287d1ae5..bd3233f24 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -1590,7 +1590,7 @@ class FetchGroup { ); if (this.generateQueryFragments) { - operation = operation.generateQueryFragments(variableDefinitions); + operation = operation.generateQueryFragments(); } else { operation = operation.optimize( fragments?.forSubgraph(this.subgraphName, subgraphSchema), From c99cfcc904c6b89aae1ac1787222f6f786b4e610 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Wed, 21 Aug 2024 18:45:38 -0700 Subject: [PATCH 5/5] Avoid unnecessary copying and ordering changes --- internals-js/src/operations.ts | 59 ++++++++++++++++--------------- query-planner-js/src/buildPlan.ts | 2 +- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index cc9b453ee..5db67d995 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -937,35 +937,38 @@ export class Operation extends DirectiveTargetElement { ); } - private collectVariablesFromFragments(allAvailableVariables: VariableDefinitions, fragments: NamedFragments): VariableDefinitions { - const varDefs = new VariableDefinitions(); - varDefs.addAll(this.variableDefinitions); - + private collectUndefinedVariablesFromFragments(fragments: NamedFragments): Variable[] { const collector = new VariableCollector(); - collectVariablesInNamedFragments(fragments, collector); - - for (const v of collector.variables()) { - const def = allAvailableVariables.definition(v.name); - if (def) { - varDefs.add(def); - } + for (const namedFragment of fragments.definitions()) { + namedFragment.selectionSet.usedVariables().forEach(v => { + if (!this.variableDefinitions.definition(v)) { + collector.add(v); + } + }); } - return varDefs; + return collector.variables(); } // Returns a copy of this operation with the provided updated selection set and fragments. - private withUpdatedSelectionSetAndFragments(newSelectionSet: SelectionSet, newFragments: NamedFragments | undefined, allAvailableVariables?: VariableDefinitions): Operation { + private withUpdatedSelectionSetAndFragments( + newSelectionSet: SelectionSet, + newFragments: NamedFragments | undefined, + allAvailableVariables?: VariableDefinitions, + ): Operation { if (this.selectionSet === newSelectionSet && newFragments === this.fragments) { return this; } - let newVariableDefinitions: VariableDefinitions; + let newVariableDefinitions = this.variableDefinitions; if (allAvailableVariables && newFragments) { - newVariableDefinitions = this.collectVariablesFromFragments(allAvailableVariables, newFragments); - newVariableDefinitions.addAll(this.variableDefinitions); - } else { - newVariableDefinitions = this.variableDefinitions; + const undefinedVariables = this.collectUndefinedVariablesFromFragments(newFragments); + if (undefinedVariables.length > 0) { + newVariableDefinitions = new VariableDefinitions(); + newVariableDefinitions.addAll(this.variableDefinitions); + newVariableDefinitions.addAll(allAvailableVariables.filter(undefinedVariables)); + } } + return new Operation( this.schema(), this.rootKind, @@ -977,7 +980,11 @@ export class Operation extends DirectiveTargetElement { ); } - optimize(fragments?: NamedFragments, minUsagesToOptimize: number = DEFAULT_MIN_USAGES_TO_OPTIMIZE, allAvailableVariables?: VariableDefinitions): Operation { + optimize( + fragments?: NamedFragments, + minUsagesToOptimize: number = DEFAULT_MIN_USAGES_TO_OPTIMIZE, + allAvailableVariables?: VariableDefinitions, + ): Operation { assert(minUsagesToOptimize >= 1, `Expected 'minUsagesToOptimize' to be at least 1, but got ${minUsagesToOptimize}`) if (!fragments || fragments.isEmpty()) { return this; @@ -1026,7 +1033,11 @@ export class Operation extends DirectiveTargetElement { } } - return this.withUpdatedSelectionSetAndFragments(optimizedSelection, finalFragments ?? undefined, allAvailableVariables); + return this.withUpdatedSelectionSetAndFragments( + optimizedSelection, + finalFragments ?? undefined, + allAvailableVariables, + ); } generateQueryFragments(): Operation { @@ -4047,11 +4058,3 @@ export function hasSelectionWithPredicate(selectionSet: SelectionSet, predicate: } return false; } - -export function collectVariablesInNamedFragments(fragments: NamedFragments, collector: VariableCollector) { - for (const namedFragment of fragments.definitions()) { - namedFragment.selectionSet.usedVariables().forEach(v => { - collector.add(v); - }); - } -} diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index bd3233f24..d12701a91 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -1614,7 +1614,7 @@ class FetchGroup { id: this.id, serviceName: this.subgraphName, requires: inputNodes ? trimSelectionNodes(inputNodes.selections) : undefined, - variableUsages: (Array.from(usedVariables)), + variableUsages: Array.from(usedVariables), operation: stripIgnoredCharacters(print(operationDocument)), operationKind: schemaRootKindToOperationKind(operation.rootKind), operationName: operation.name,