From 15bfa6befb4d4552504efb8e4b483c226d2ada44 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 10 Oct 2024 16:24:52 -0700 Subject: [PATCH 1/4] fix(query-planner): handle multiple __typename selections with/without directives - fixed sibling typename optimization to only fold plain __typename without alias/directives. - fixed selectionsInPrintOrder to not drop duplicate __typename selections. --- internals-js/src/operations.ts | 13 +++- .../src/__tests__/buildPlan.test.ts | 69 +++++++++++++++++++ query-planner-js/src/buildPlan.ts | 2 +- 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index ea2bf3eb4..9e2f4be5e 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -2114,10 +2114,10 @@ export class SelectionSet { // By default, we will print the selection the order in which things were added to it. // If __typename is selected however, we put it first. It's a detail but as __typename is a bit special it looks better, // and it happens to mimic prior behavior on the query plan side so it saves us from changing tests for no good reasons. - const isNonAliasedTypenameSelection = (s: Selection) => s.kind === 'FieldSelection' && !s.element.alias && s.element.name === typenameFieldName; - const typenameSelection = this._selections.find((s) => isNonAliasedTypenameSelection(s)); + const isPlainTypenameSelection = (s: Selection) => s.kind === 'FieldSelection' && s.isPlainTypenameField(); + const typenameSelection = this._selections.find((s) => isPlainTypenameSelection(s)); if (typenameSelection) { - return [typenameSelection].concat(this.selections().filter(s => !isNonAliasedTypenameSelection(s))); + return [typenameSelection].concat(this.selections().filter(s => !isPlainTypenameSelection(s))); } else { return this._selections; } @@ -3053,6 +3053,13 @@ export class FieldSelection extends AbstractSelection, undefined, Fie return this.element.definition.name === typenameFieldName; } + // Is this a plain simple __typename without any directive or alias? + isPlainTypenameField(): boolean { + return this.element.definition.name === typenameFieldName + && this.element.appliedDirectives.length == 0 + && !this.element.alias; + } + withAttachment(key: string, value: string): FieldSelection { const updatedField = this.element.copy(); updatedField.addAttachment(key, value); diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index d0e5d3864..d9da62695 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -5749,6 +5749,75 @@ describe('__typename handling', () => { `); }); + it('preserves __typename with a directive', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + t: T + } + + type T @key(fields: "id") { + id: ID! + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1); + // Especially, when there are multiple __typename selections. + let operation = operationFromDocument( + api, + gql` + query($v: Boolean!) { + t { + __typename + __typename @skip(if: $v) + } + } + `, + ); + + let plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + __typename + __typename @skip(if: $v) + } + } + }, + } + `); + + operation = operationFromDocument( + api, + gql` + query($v: Boolean!) { + t { + __typename @skip(if: $v) + __typename + } + } + `, + ); + + plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { + t { + __typename + __typename @skip(if: $v) + } + } + }, + } + `); + }); + it('does not needlessly consider options for __typename', () => { const subgraph1 = { name: 'Subgraph1', diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index dce90edad..7dfabd9f2 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -3425,7 +3425,7 @@ export class QueryPlanner { if ( !typenameSelection && selection.kind === 'FieldSelection' - && selection.element.name === typenameFieldName + && selection.isPlainTypenameField() && !parentMaybeInterfaceObject ) { // The reason we check for `!typenameSelection` is that due to aliasing, there can be more than one __typename selection From cc0c7329f1289927eb85fde6f571b92503de52b7 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 11 Oct 2024 07:57:17 -0700 Subject: [PATCH 2/4] prettier --- query-planner-js/src/__tests__/buildPlan.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index d9da62695..155c48530 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -5768,7 +5768,7 @@ describe('__typename handling', () => { let operation = operationFromDocument( api, gql` - query($v: Boolean!) { + query ($v: Boolean!) { t { __typename __typename @skip(if: $v) @@ -5794,7 +5794,7 @@ describe('__typename handling', () => { operation = operationFromDocument( api, gql` - query($v: Boolean!) { + query ($v: Boolean!) { t { __typename @skip(if: $v) __typename From bb5d85dfdcb424e12c5bef040d248fa76a278d67 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 11 Oct 2024 17:45:09 -0700 Subject: [PATCH 3/4] added changeset --- .changeset/wise-yaks-switch.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/wise-yaks-switch.md diff --git a/.changeset/wise-yaks-switch.md b/.changeset/wise-yaks-switch.md new file mode 100644 index 000000000..fabbb83a0 --- /dev/null +++ b/.changeset/wise-yaks-switch.md @@ -0,0 +1,8 @@ +--- +"@apollo/federation-internals": patch +"@apollo/query-planner": patch +--- + +Fixed a bug that `__typename` with applied directives gets lost in fetch operations. + +The query planner uses a technique called sibling typename optimization to simplify operations by folding __typename selections into a sibling selection. However, that optimization does not account for applied directives or aliasing. The bug was applying it even if the `__typename` has directives applied. `__typename` with directives (or alias) are now excluded from the optimization. From 894a1d6431266ab574cd0bce8907b710a3ba32eb Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Sat, 12 Oct 2024 14:43:50 -0700 Subject: [PATCH 4/4] revised changeset description --- .changeset/wise-yaks-switch.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/wise-yaks-switch.md b/.changeset/wise-yaks-switch.md index fabbb83a0..9d93d0efb 100644 --- a/.changeset/wise-yaks-switch.md +++ b/.changeset/wise-yaks-switch.md @@ -5,4 +5,4 @@ Fixed a bug that `__typename` with applied directives gets lost in fetch operations. -The query planner uses a technique called sibling typename optimization to simplify operations by folding __typename selections into a sibling selection. However, that optimization does not account for applied directives or aliasing. The bug was applying it even if the `__typename` has directives applied. `__typename` with directives (or alias) are now excluded from the optimization. +The sibling typename optimization used by query planner simplifies operations by folding `__typename` selections into their sibling selections. However, that optimization does not account for directives or aliases. The bug was applying the optimization even if the `__typename` has directives on it, which caused the selection to lose its directives. Now, `__typename` with directives (or aliases) are excluded from the optimization.