From 5dd2b5c307b4f91fac2e28da9c1a3390a5e060ab Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Mon, 29 Sep 2025 14:25:26 -0700 Subject: [PATCH 1/2] Fix query planning for @shareable mutation fields --- .../src/__tests__/graphPath.test.ts | 1 + query-graphs-js/src/graphPath.ts | 14 +++- .../src/__tests__/buildPlan.test.ts | 79 +++++++++++++++++++ query-planner-js/src/buildPlan.ts | 55 +++++++++++-- 4 files changed, 141 insertions(+), 8 deletions(-) diff --git a/query-graphs-js/src/__tests__/graphPath.test.ts b/query-graphs-js/src/__tests__/graphPath.test.ts index a1326938e..8d943dc2f 100644 --- a/query-graphs-js/src/__tests__/graphPath.test.ts +++ b/query-graphs-js/src/__tests__/graphPath.test.ts @@ -72,6 +72,7 @@ function createOptions(supergraph: Schema, queryGraph: QueryGraph): Simultaneous [], [], new Map(), + null, ); } diff --git a/query-graphs-js/src/graphPath.ts b/query-graphs-js/src/graphPath.ts index e32dbb656..5a80ee726 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -1573,7 +1573,11 @@ function advancePathWithNonCollectingAndTypePreservingTransitions `${toAdvance} should be a root path if it starts with subgraph entering edge ${subgraphEnteringEdge.edge}`); - prevSubgraphEnteringVertex = rootVertexForSubgraph(toAdvance.graph, edge.tail.source, toAdvance.root.rootKind); + // Since mutation options need to originate from the same subgraph, we pretend we cannot find a root vertex + // in another subgraph (effectively skipping the optimization). + prevSubgraphEnteringVertex = toAdvance.root.rootKind !== 'mutation' + ? rootVertexForSubgraph(toAdvance.graph, edge.tail.source, toAdvance.root.rootKind) + : undefined; // If the entering edge is the root entering of subgraphs, then the "prev subgraph" is really `edge.tail.source` and // so `edge` always get us back to that (but `subgraphEnteringEdge.edge.head.source` would be `FEDERATED_GRAPH_ROOT_SOURCE`, // so the test we do in the `else` branch would not work here). @@ -2383,6 +2387,7 @@ export function createInitialOptions( excludedEdges: ExcludedDestinations, excludedConditions: ExcludedConditions, overrideConditions: Map, + initialSubgraphConstraint: string | null, ): SimultaneousPathsWithLazyIndirectPaths[] { const lazyInitialPath = new SimultaneousPathsWithLazyIndirectPaths( [initialPath], @@ -2393,7 +2398,12 @@ export function createInitialOptions( overrideConditions, ); if (isFederatedGraphRootType(initialPath.tail.type)) { - const initialOptions = lazyInitialPath.indirectOptions(initialContext, 0); + let initialOptions = lazyInitialPath.indirectOptions(initialContext, 0); + if (initialSubgraphConstraint !== null) { + initialOptions.paths = initialOptions + .paths + .filter((path) => path.tail.source === initialSubgraphConstraint); + } return createLazyOptions(initialOptions.paths.map(p => [p]), lazyInitialPath, initialContext, overrideConditions); } else { return [lazyInitialPath]; diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 97a008564..3e126b92f 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -6127,6 +6127,85 @@ describe('mutations', () => { } `); }); + + it('executes a single mutation operation on a @shareable field', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + dummy: Int + } + + type Mutation { + f: F @shareable + } + + type F @key(fields: "id") { + id: ID! + x: Int + } + `, + }; + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + type Mutation { + f: F @shareable + } + + type F @key(fields: "id", resolvable: false) { + id: ID! + y: Int + } + `, + }; + + const [api, queryPlanner] = composeAndCreatePlanner(subgraph1, subgraph2); + const operation = operationFromDocument( + api, + gql` + mutation { + f { + x + y + } + } + `, + ); + + const plan = queryPlanner.buildQueryPlan(operation); + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "Subgraph2") { + { + f { + __typename + id + y + } + } + }, + Flatten(path: "f") { + Fetch(service: "Subgraph1") { + { + ... on F { + __typename + id + } + } => + { + ... on F { + x + } + } + }, + }, + }, + } + `); + }); }); describe('interface type-explosion', () => { diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 0ba2b77d7..bcd8e1a68 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -398,6 +398,7 @@ class QueryPlanningTraversal { initialContext: PathContext, typeConditionedFetching: boolean, nonLocalSelectionsState: NonLocalSelectionsState | null, + initialSubgraphConstraint: string | null, excludedDestinations: ExcludedDestinations = [], excludedConditions: ExcludedConditions = [], ) { @@ -418,6 +419,7 @@ class QueryPlanningTraversal { excludedDestinations, excludedConditions, parameters.overrideConditions, + initialSubgraphConstraint, ); this.stack = mapOptionsToSelections(selectionSet, initialOptions); if ( @@ -527,8 +529,9 @@ class QueryPlanningTraversal { // If we have no options, it means there is no way to build a plan for that branch, and // that means the whole query planning has no plan. // This should never happen for a top-level query planning (unless the supergraph has *not* been - // validated), but can happen when computing sub-plans for a key condition. - if (this.isTopLevel) { + // validated), but can happen when computing sub-plans for a key condition and when computing + // a top-level plan for a mutation field on a specific subgraph. + if (this.isTopLevel && this.rootKind !== 'mutation') { debug.groupEnd(() => `No valid options to advance ${selection} from ${advanceOptionsToString(options)}`); throw new Error(`Was not able to find any options for ${selection}: This shouldn't have happened.`); } else { @@ -794,6 +797,7 @@ class QueryPlanningTraversal { context, this.typeConditionedFetching, null, + null, excludedDestinations, addConditionExclusion(excludedConditions, edge.conditions), ).findBestPlan(); @@ -3593,7 +3597,7 @@ function computePlanInternal({ const { operation, processor } = parameters; if (operation.rootKind === 'mutation') { - const dependencyGraphs = computeRootSerialDependencyGraph( + const dependencyGraphs = computeRootSerialDependencyGraphForMutation( parameters, hasDefers, nonLocalSelectionsState, @@ -3770,6 +3774,7 @@ function computeRootParallelBestPlan( emptyContext, parameters.config.typeConditionedFetching, nonLocalSelectionsState, + null, ); const plan = planningTraversal.findBestPlan(); // Getting no plan means the query is essentially unsatisfiable (it's a valid query, but we can prove it will never return a result), @@ -3777,6 +3782,44 @@ function computeRootParallelBestPlan( return plan ?? createEmptyPlan(parameters); } +function computeRootParallelBestPlanForMutation( + parameters: PlanningParameters, + selection: SelectionSet, + startFetchIdGen: number, + hasDefers: boolean, + nonLocalSelectionsState: NonLocalSelectionsState | null, +): [FetchDependencyGraph, OpPathTree, number] { + let bestPlan: + | [FetchDependencyGraph, OpPathTree, number] + | undefined; + const mutationSubgraphs = parameters.federatedQueryGraph + .outEdges(parameters.root).map((edge) => edge.tail.source); + for (const mutationSubgraph of mutationSubgraphs) { + const planningTraversal = new QueryPlanningTraversal( + parameters, + selection, + startFetchIdGen, + hasDefers, + parameters.root.rootKind, + defaultCostFunction, + emptyContext, + parameters.config.typeConditionedFetching, + nonLocalSelectionsState, + mutationSubgraph, + ); + const plan = planningTraversal.findBestPlan(); + if (!bestPlan || (plan && plan[2] < bestPlan[2])) { + bestPlan = plan; + } + } + if (!bestPlan) { + throw new Error( + `Was not able to plan ${parameters.operation.toString(false, false)} starting from a single subgraph: This shouldn't have happened.`, + ); + } + return bestPlan; +} + function createEmptyPlan( parameters: PlanningParameters, ): [FetchDependencyGraph, OpPathTree, number] { @@ -3794,7 +3837,7 @@ function onlyRootSubgraph(graph: FetchDependencyGraph): string { return subgraphs[0]; } -function computeRootSerialDependencyGraph( +function computeRootSerialDependencyGraphForMutation( parameters: PlanningParameters, hasDefers: boolean, nonLocalSelectionsState: NonLocalSelectionsState | null, @@ -3805,7 +3848,7 @@ function computeRootSerialDependencyGraph( const splittedRoots = splitTopLevelFields(operation.selectionSet); const graphs: FetchDependencyGraph[] = []; let startingFetchId = 0; - let [prevDepGraph, prevPaths] = computeRootParallelBestPlan( + let [prevDepGraph, prevPaths] = computeRootParallelBestPlanForMutation( parameters, splittedRoots[0], startingFetchId, @@ -3814,7 +3857,7 @@ function computeRootSerialDependencyGraph( ); let prevSubgraph = onlyRootSubgraph(prevDepGraph); for (let i = 1; i < splittedRoots.length; i++) { - const [newDepGraph, newPaths] = computeRootParallelBestPlan( + const [newDepGraph, newPaths] = computeRootParallelBestPlanForMutation( parameters, splittedRoots[i], prevDepGraph.nextFetchId(), From 57c8040c28ff3f147eb8f638f7bcad66eecfa6ad Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Mon, 29 Sep 2025 15:53:15 -0700 Subject: [PATCH 2/2] Add changeset --- .changeset/many-rings-glow.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/many-rings-glow.md diff --git a/.changeset/many-rings-glow.md b/.changeset/many-rings-glow.md new file mode 100644 index 000000000..769e2a3d5 --- /dev/null +++ b/.changeset/many-rings-glow.md @@ -0,0 +1,6 @@ +--- +"@apollo/query-planner": patch +"@apollo/query-graphs": patch +--- + +Fixes a bug where query planning may unexpectedly error due to attempting to generate a plan where a `@shareable` mutation field is called more than once across multiple subgraphs. ([#3304](https://github.com/apollographql/federation/pull/3304))