From 3d8514fc88db5fcd8aad7480b8e7f5c82d9e8cce Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Tue, 29 Jul 2025 11:53:52 -0400 Subject: [PATCH 1/3] Fix bug where a subgraph jump inserted due to @requires would mistakenly try to collect its key fields from a subgraph that didn't have them. --- .../src/__tests__/buildPlan.test.ts | 120 ++++++++++++++++++ query-planner-js/src/buildPlan.ts | 25 +++- 2 files changed, 143 insertions(+), 2 deletions(-) diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 97a008564..bd6ffa36e 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -3162,6 +3162,126 @@ describe('@requires', () => { } `); }); + + it('avoids selecting inapplicable @key from parent fetch group', () => { + // Previously, there was an issue where a subgraph jump inserted due to a + // @requires field would try (as an optimization) to collect its locally + // satisfiable key from the parent, but the parent may not have that locally + // satisfiable key. We now explicitly check for this, falling back to the + // current node if needed (which should be guaranteed to have it). + const subgraph1 = { + name: 'A', + typeDefs: gql` + type Query { + t: T + } + type T @key(fields: "id1") { + id1: ID! + } + `, + }; + + const subgraph2 = { + name: 'B', + typeDefs: gql` + type T @key(fields: "id2") @key(fields: "id1") { + id1: ID! + id2: ID! + x: Int @external + req: Int @requires(fields: "x") + } + `, + }; + + const subgraph3 = { + name: 'C', + typeDefs: gql` + type T @key(fields: "id2") { + id2: ID! + x: Int + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlanner( + subgraph1, + subgraph2, + subgraph3, + ); + const operation = operationFromDocument( + api, + gql` + { + t { + req + } + } + `, + ); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "A") { + { + t { + __typename + id1 + } + } + }, + Flatten(path: "t") { + Fetch(service: "B") { + { + ... on T { + __typename + id1 + } + } => + { + ... on T { + __typename + id2 + } + } + }, + }, + Flatten(path: "t") { + Fetch(service: "C") { + { + ... on T { + __typename + id2 + } + } => + { + ... on T { + x + } + } + }, + }, + Flatten(path: "t") { + Fetch(service: "B") { + { + ... on T { + __typename + x + id2 + } + } => + { + ... on T { + req + } + } + }, + }, + }, + } + `); + }); }); describe('fetch operation names', () => { diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 0ba2b77d7..60aeeb51f 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -4814,13 +4814,34 @@ function createPostRequiresGroup( // we need the path here, so this will have to do for now, and if this ever breaks in practice, we'll at least have an example to // guide us toward improving/fixing. assert(parent.path, `Missing path-in-parent for @requires on ${edge} with group ${group} and parent ${parent}`); + let requirePath = path.forParentOfGroup(parent.path, parent.group.parentType.schema()); + let preRequireGroup = parent.group; + + // The `postRequireGroup` needs a key. This can come from `group` (and code + // in `canSatisfyConditions()` guarantees such a locally satisfiable key + // exists in `group`), but it can also potentially come from `parent.group`, + // and previous code had (wrongfully) always assumed it could. + // + // To keep this previous optimization, we now explicitly check whether the + // known locally satisfiable key can be rebased in `parent.group`, and we + // fall back to `group` if it doesn't. + const keyCondition = getLocallySatisfiableKey(dependencyGraph.federatedQueryGraph, edge.head); + assert(keyCondition, () => `Due to @requires, validation should have required a key to be present for ${edge}`); + if (!keyCondition.canRebaseOn(typeAtPath(preRequireGroup.selection.parentType, requirePath.inGroup()))) { + requirePath = path; + preRequireGroup = group; + // It's possible we didn't add `group` as a parent previously, so we do so + // here similarly to how `handleRequiresTree()` specifies it. + postRequireGroup.addParent({ group, path: [] }); + } + addPostRequireInputs( dependencyGraph, - path.forParentOfGroup(parent.path, parent.group.parentType.schema()), + requirePath, entityType, edge, context, - parent.group, + preRequireGroup, postRequireGroup, ); return { From 50d879695673d349c093fa7ca1c23cc1cf8c6c60 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Tue, 29 Jul 2025 10:51:56 -0700 Subject: [PATCH 2/3] Create changeset --- .changeset/afraid-singers-arrive.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/afraid-singers-arrive.md diff --git a/.changeset/afraid-singers-arrive.md b/.changeset/afraid-singers-arrive.md new file mode 100644 index 000000000..1bd38c705 --- /dev/null +++ b/.changeset/afraid-singers-arrive.md @@ -0,0 +1,5 @@ +--- +"@apollo/query-planner": patch +--- + +Fix bug in query planning when a subgraph jump for @requires can sometimes try to fetch @key fields from a subgraph that doesn't have them. From e30a72d76e698b222d77a6b0a277fc0b7cdd54f7 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Wed, 30 Jul 2025 13:13:00 -0400 Subject: [PATCH 3/3] Update changeset message to include example error message. --- .changeset/afraid-singers-arrive.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/afraid-singers-arrive.md b/.changeset/afraid-singers-arrive.md index 1bd38c705..495260833 100644 --- a/.changeset/afraid-singers-arrive.md +++ b/.changeset/afraid-singers-arrive.md @@ -2,4 +2,4 @@ "@apollo/query-planner": patch --- -Fix bug in query planning when a subgraph jump for @requires can sometimes try to fetch @key fields from a subgraph that doesn't have them. +Fix bug in query planning where a subgraph jump for `@requires` can sometimes try to fetch `@key` fields from a subgraph that doesn't have them. This bug would previously cause query planning to error with a message that looks like "Cannot add selection of field `T.id` to selection set of parent type `T`".