diff --git a/.changeset/pink-ducks-fail.md b/.changeset/pink-ducks-fail.md new file mode 100644 index 000000000..46f4939c9 --- /dev/null +++ b/.changeset/pink-ducks-fail.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +--- + +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 diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index d3cba514b..f0717e03d 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` @@ -7821,3 +7821,155 @@ test('avoid considering indirect paths from the root when a more direct one exis 4, ); }); + +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` + 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(` + 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), );