From 2cf5baa86349ed208c97a5272f522c6a5c91a21d Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 7 Nov 2023 09:49:39 -0600 Subject: [PATCH 1/4] Add a test for the case where a subgraph requires a value from another subgraph that has key confusion --- .../src/__tests__/buildPlan.test.ts | 109 +++++++++++++++--- 1 file changed, 95 insertions(+), 14 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index d3cba514b..fd9782c9d 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -3907,7 +3907,7 @@ describe('Named fragments preservation', () => { } } } - + fragment FooChildSelect on Foo { __typename foo @@ -3922,7 +3922,7 @@ describe('Named fragments preservation', () => { } } } - + fragment FooSelect on Foo { __typename foo @@ -4096,7 +4096,7 @@ describe('Named fragments preservation', () => { } } } - + fragment OnV on V { a b @@ -4170,7 +4170,7 @@ describe('Named fragments preservation', () => { } } } - + fragment Selection on A { x y @@ -4264,7 +4264,7 @@ describe('Named fragments preservation', () => { } } } - + fragment OnV on V { v1 v2 @@ -4372,7 +4372,7 @@ describe('Named fragments preservation', () => { ...OnT @include(if: $test2) } } - + fragment OnT on T { a b @@ -4572,7 +4572,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { inner { v { @@ -4711,7 +4711,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { w inner { @@ -4852,7 +4852,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { inner { v @@ -4991,7 +4991,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { w inner { @@ -6815,7 +6815,7 @@ describe('named fragments', () => { } } } - + fragment Fragment4 on I { __typename id1 @@ -6888,7 +6888,7 @@ describe('named fragments', () => { } } } - + fragment Fragment4 on I { id1 id2 @@ -7030,7 +7030,7 @@ describe('named fragments', () => { id } } - + fragment allTFields on T { v0 v1 @@ -7178,7 +7178,7 @@ describe('named fragments', () => { } } } - + fragment allUFields on U { v0 v1 @@ -7821,3 +7821,84 @@ test('avoid considering indirect paths from the root when a more direct one exis 4, ); }); + +describe('jump from requires uses a different key', () => { + it.skip('jump from requires subgraph uses a different key then jump into it.', () => { + const subgraph1 = { + name: 'A', + typeDefs: gql` + type Query { + u: U! + } + + type U @key(fields: "k1 { id }") { + k1: K + } + + type K @key(fields: "id") { + id: ID! + } + `, + }; + const subgraph2 = { + name: 'B', + typeDefs: gql` + type U @key(fields: "k1 { id }") @key(fields: "k2") { + k1: K! + k2: ID! + v: V! @external + f: ID! @requires(fields: "v { v }") + f2: Int! + } + + type K @key(fields: "id") { + id: ID! + } + + type V @key(fields: "id") { + id: ID! + v: String! @external + } + `, + }; + const subgraph3 = { + name: 'C', + typeDefs: gql` + type U @key(fields: "k1 { id }") @key(fields: "k2") { + k1: K! + k2: ID! + v: V! + } + + type K @key(fields: "id") { + id: ID! + } + + type V @key(fields: "id") { + id: ID! + v: String! + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlanner( + subgraph1, + subgraph2, + subgraph3, + ); + const operation = operationFromDocument( + api, + gql` + { + u { + f + } + } + `, + ); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + `); + }); +}); From ae65140b8f8a6875c67d75f0d02ef3ddf7f8ff94 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 14 Nov 2023 16:35:39 -0600 Subject: [PATCH 2/4] Fix whitespace issue in test and provide fix for indirect requires issue Fixes #2805 --- .../src/__tests__/buildPlan.test.ts | 106 +++++++++++++++--- query-planner-js/src/buildPlan.ts | 12 +- 2 files changed, 95 insertions(+), 23 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index fd9782c9d..c82af7c66 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -3810,7 +3810,7 @@ describe('Fed1 supergraph handling', () => { }); describe('Named fragments preservation', () => { - it('works with nested fragments', () => { + it('works with nested fragments 1', () => { const subgraph1 = { name: 'Subgraph1', typeDefs: gql` @@ -3907,7 +3907,7 @@ describe('Named fragments preservation', () => { } } } - + fragment FooChildSelect on Foo { __typename foo @@ -3922,7 +3922,7 @@ describe('Named fragments preservation', () => { } } } - + fragment FooSelect on Foo { __typename foo @@ -4096,7 +4096,7 @@ describe('Named fragments preservation', () => { } } } - + fragment OnV on V { a b @@ -4170,7 +4170,7 @@ describe('Named fragments preservation', () => { } } } - + fragment Selection on A { x y @@ -4264,7 +4264,7 @@ describe('Named fragments preservation', () => { } } } - + fragment OnV on V { v1 v2 @@ -4372,7 +4372,7 @@ describe('Named fragments preservation', () => { ...OnT @include(if: $test2) } } - + fragment OnT on T { a b @@ -4572,7 +4572,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { inner { v { @@ -4711,7 +4711,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { w inner { @@ -4852,7 +4852,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { inner { v @@ -4991,7 +4991,7 @@ describe('Named fragments preservation', () => { id } } - + fragment OuterFrag on Outer { w inner { @@ -6815,7 +6815,7 @@ describe('named fragments', () => { } } } - + fragment Fragment4 on I { __typename id1 @@ -6888,7 +6888,7 @@ describe('named fragments', () => { } } } - + fragment Fragment4 on I { id1 id2 @@ -7030,7 +7030,7 @@ describe('named fragments', () => { id } } - + fragment allTFields on T { v0 v1 @@ -7178,7 +7178,7 @@ describe('named fragments', () => { } } } - + fragment allUFields on U { v0 v1 @@ -7822,8 +7822,11 @@ test('avoid considering indirect paths from the root when a more direct one exis ); }); -describe('jump from requires uses a different key', () => { - it.skip('jump from requires subgraph uses a different key then jump into it.', () => { +describe('@requires references external field indirectly', () => { + it('key where @external is not at top level of selection of requires', () => { + // Field issue where we were seeing a FetchGroup created where the fields used by the key to jump subgraphs + // were not properly fetched. In the below test, this test will ensure that 'k2' is properly collected + // before it is used const subgraph1 = { name: 'A', typeDefs: gql` @@ -7899,6 +7902,75 @@ describe('jump from requires uses a different key', () => { const plan = queryPlanner.buildQueryPlan(operation); expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "A") { + { + u { + __typename + k1 { + id + } + } + } + }, + Flatten(path: "u") { + Fetch(service: "B") { + { + ... on U { + __typename + k1 { + id + } + } + } => + { + ... on U { + k2 + } + } + }, + }, + Flatten(path: "u") { + Fetch(service: "C") { + { + ... on U { + __typename + k2 + } + } => + { + ... on U { + v { + v + } + } + } + }, + }, + Flatten(path: "u") { + Fetch(service: "B") { + { + ... on U { + __typename + v { + v + } + k1 { + id + } + } + } => + { + ... on U { + f + } + } + }, + }, + }, + } `); }); }); + diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index d677c03a6..e31a81754 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -944,7 +944,7 @@ class FetchGroup { // `inputs.contains(selection)`, so if a group is shown useful, it means that there // is some selections not in the inputs, but as long as we add to selections (and we // never remove from selections; `MutableSelectionSet` don't have removing methods), - // then this won't change. Only changing inputs may require some recomputation. + // then this won't change. Only changing inputs may require some recomputation. this.isKnownUseful = false; } } @@ -1188,7 +1188,7 @@ class FetchGroup { if (inputs) { this.cachedCost = undefined; const updated = inputs.selectionSets().reduce((prev, value) => prev.minus(value), this.selection); - this._selection = MutableSelectionSet.ofWithMemoized(this.selection.minus(updated), conditionsMemoizer); + this._selection = MutableSelectionSet.ofWithMemoized(updated, conditionsMemoizer); } } @@ -1706,7 +1706,7 @@ function withFieldAliased(selectionSet: SelectionSet, aliases: FieldToAlias[]): const field = selection.element; const alias = pathElement && atCurrentLevel.get(pathElement); return !alias && selection.selectionSet === updatedSelectionSet - ? selection + ? selection : selection.withUpdatedComponents(alias ? field.withUpdatedAlias(alias.alias) : field, updatedSelectionSet); } else { return selection.selectionSet === updatedSelectionSet @@ -3701,8 +3701,8 @@ function addTypenameFieldForAbstractTypesInNamedFragments(fragments: NamedFragme } /** - * Given a selection select (`selectionSet`) and given a set of directive applications that can be eliminated (`unneededDirectives`; in - * practice those are conditionals (@skip and @include) already accounted for), returns an equivalent selection set but with unecessary + * Given a selection select (`selectionSet`) and given a set of directive applications that can be eliminated (`unneededDirectives`; in + * practice those are conditionals (@skip and @include) already accounted for), returns an equivalent selection set but with unecessary * "starting" fragments having the unneeded condition/directives removed. */ function removeUnneededTopLevelFragmentDirectives( @@ -3969,7 +3969,7 @@ function computeGroupsForTree( const inputSelections = newCompositeTypeSelectionSet(inputType); inputSelections.updates().add(edge.conditions!); newGroup.addInputs( - wrapInputsSelections(inputType, inputSelections.get(), newContext), + wrapInputsSelections(inputType, inputSelections.get(), newContext), computeInputRewritesOnKeyFetch(inputType.name, destType), ); From 8b61c10e97a9a28bfcb89f014c167f445dccfadb Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 14 Nov 2023 17:01:25 -0600 Subject: [PATCH 3/4] prettier fix and add changesets --- .changeset/pink-ducks-fail.md | 6 ++++++ query-planner-js/src/__tests__/buildPlan.test.ts | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 .changeset/pink-ducks-fail.md diff --git a/.changeset/pink-ducks-fail.md b/.changeset/pink-ducks-fail.md new file mode 100644 index 000000000..d51cb5ba3 --- /dev/null +++ b/.changeset/pink-ducks-fail.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +--- + +removeInputsFromSelection is tasked with removing inputs to a FetchGroup from the fetch group so that we don't retrieve the same values twice (and FetchGroups that don't functionally do anything can be removed). There was a bug in the code that meant that in certain cases, things that were required were actually being removed. This could manifest in subgraph jumps not being possible because the key it was using to make the jump was not available. + \ No newline at end of file diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index c82af7c66..f0717e03d 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -7973,4 +7973,3 @@ describe('@requires references external field indirectly', () => { `); }); }); - From 27e969fb381d555dd4d5be4369c822e80ea707ca Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Tue, 14 Nov 2023 21:11:30 -0600 Subject: [PATCH 4/4] update changeset --- .changeset/pink-ducks-fail.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/pink-ducks-fail.md b/.changeset/pink-ducks-fail.md index d51cb5ba3..46f4939c9 100644 --- a/.changeset/pink-ducks-fail.md +++ b/.changeset/pink-ducks-fail.md @@ -2,5 +2,5 @@ "@apollo/query-planner": patch --- -removeInputsFromSelection is tasked with removing inputs to a FetchGroup from the fetch group so that we don't retrieve the same values twice (and FetchGroups that don't functionally do anything can be removed). There was a bug in the code that meant that in certain cases, things that were required were actually being removed. This could manifest in subgraph jumps not being possible because the key it was using to make the jump was not available. +Fix query planning bug where keys or required fields can sometimes reach subgraphs with null values. ([#2805](https://github.com/apollographql/federation/issues/2805)) \ No newline at end of file