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)) 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..8a8ca3bdc 100644 --- a/query-graphs-js/src/graphPath.ts +++ b/query-graphs-js/src/graphPath.ts @@ -1490,8 +1490,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 +2390,7 @@ export function createInitialOptions( excludedEdges: ExcludedDestinations, excludedConditions: ExcludedConditions, overrideConditions: Map, + initialSubgraphConstraint: string | null, ): SimultaneousPathsWithLazyIndirectPaths[] { const lazyInitialPath = new SimultaneousPathsWithLazyIndirectPaths( [initialPath], @@ -2393,7 +2401,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-graphs-js/src/nonLocalSelectionsEstimation.ts b/query-graphs-js/src/nonLocalSelectionsEstimation.ts index a7f7e2d20..b045d9c2d 100644 --- a/query-graphs-js/src/nonLocalSelectionsEstimation.ts +++ b/query-graphs-js/src/nonLocalSelectionsEstimation.ts @@ -600,6 +600,10 @@ export class NonLocalSelectionsMetadata { * This calls {@link checkNonLocalSelectionsLimitExceeded} for each of the * selections in the open branches stack; see that function's doc comment for * more information. + * + * To support mutations, we allow indicating the initial subgraph is + * constrained, in which case indirect options will be ignored until the first + * field (similar to query planning). */ checkNonLocalSelectionsLimitExceededAtRoot( stack: [Selection, SimultaneousPathsWithLazyIndirectPaths[]][], @@ -607,6 +611,7 @@ export class NonLocalSelectionsMetadata { supergraphSchema: Schema, inconsistentAbstractTypesRuntimes: Set, overrideConditions: Map, + isInitialSubgraphConstrained: boolean, ): boolean { for (const [selection, simultaneousPaths] of stack) { const tailVertices = new Set(); @@ -616,7 +621,10 @@ export class NonLocalSelectionsMetadata { } } const tailVerticesInfo = - this.estimateVerticesWithIndirectOptions(tailVertices); + this.estimateVerticesWithIndirectOptions( + tailVertices, + isInitialSubgraphConstrained, + ); // Note that top-level selections aren't avoided via fully-local selection // set optimization, so we always add them here. @@ -626,12 +634,16 @@ export class NonLocalSelectionsMetadata { if (selection.selectionSet) { const selectionHasDefer = selection.hasDefer(); + const isInitialSubgraphConstrainedAfterElement = + isInitialSubgraphConstrained + && selection.kind === 'FragmentSelection'; const nextVertices = this.estimateNextVerticesForSelection( selection.element, tailVerticesInfo, state, supergraphSchema, overrideConditions, + isInitialSubgraphConstrainedAfterElement, ); if (this.checkNonLocalSelectionsLimitExceeded( selection.selectionSet, @@ -641,6 +653,7 @@ export class NonLocalSelectionsMetadata { supergraphSchema, inconsistentAbstractTypesRuntimes, overrideConditions, + isInitialSubgraphConstrainedAfterElement, )) { return true; } @@ -674,6 +687,10 @@ export class NonLocalSelectionsMetadata { * Note that this function takes in whether the parent selection of the * selection set has @defer, as that affects whether the optimization is * disabled for that selection set. + * + * To support mutations, we allow indicating the initial subgraph is + * constrained, in which case indirect options will be ignored until the first + * field (similar to query planning). */ private checkNonLocalSelectionsLimitExceeded( selectionSet: SelectionSet, @@ -683,6 +700,7 @@ export class NonLocalSelectionsMetadata { supergraphSchema: Schema, inconsistentAbstractTypesRuntimes: Set, overrideConditions: Map, + isInitialSubgraphConstrained: boolean, ): boolean { // Compute whether the selection set is non-local, and if so, add its // selections to the count. Any of the following causes the selection set to @@ -709,12 +727,16 @@ export class NonLocalSelectionsMetadata { const oldCount = state.count; if (selection.selectionSet) { + const isInitialSubgraphConstrainedAfterElement = + isInitialSubgraphConstrained + && selection.kind === 'FragmentSelection'; const nextVertices = this.estimateNextVerticesForSelection( element, parentVertices, state, supergraphSchema, overrideConditions, + isInitialSubgraphConstrainedAfterElement, ); if (this.checkNonLocalSelectionsLimitExceeded( selection.selectionSet, @@ -724,6 +746,7 @@ export class NonLocalSelectionsMetadata { supergraphSchema, inconsistentAbstractTypesRuntimes, overrideConditions, + isInitialSubgraphConstrainedAfterElement, )) { return true; } @@ -822,6 +845,12 @@ export class NonLocalSelectionsMetadata { * selection for a set of parent vertices (including indirect options), this * function can be used to estimate an upper bound on the next vertices after * taking the selection (also with indirect options). + * + * To support mutations, we allow indicating the initial subgraph will be + * constrained after taking the element, in which case indirect options will + * be ignored (and caching will be skipped). This is to ensure that top-level + * mutation fields are not executed on a different subgraph than the initial + * one during query planning. */ private estimateNextVerticesForSelection( element: OperationElement, @@ -829,6 +858,7 @@ export class NonLocalSelectionsMetadata { state: NonLocalSelectionsState, supergraphSchema: Schema, overrideConditions: Map, + isInitialSubgraphConstrainedAfterElement: boolean, ): NextVerticesInfo { const selectionKey = element.kind === 'Field' ? element.definition.name @@ -837,6 +867,28 @@ export class NonLocalSelectionsMetadata { // For empty type condition, the vertices don't change. return parentVertices; } + if (isInitialSubgraphConstrainedAfterElement) { + // When the initial subgraph is constrained, skip caching entirely. Note + // that caching is not skipped when the initial subgraph is constrained + // before this element but not after. Because of that, there may be cache + // entries for remaining vertices that were actually part of a complete + // digraph, but this is only a slight caching inefficiency and doesn't + // affect the computation's result. + assert( + parentVertices.nextVerticesWithIndirectOptions.types.size === 0, + () => 'Initial subgraph was constrained which indicates no indirect' + + ' options should be taken, but the parent vertices unexpectedly had' + + ' a complete digraph which indicates indirect options were taken' + + ' upstream in the path.', + ); + return this.estimateNextVerticesForSelectionWithoutCaching( + element, + parentVertices.nextVerticesWithIndirectOptions.remainingVertices, + supergraphSchema, + overrideConditions, + true, + ); + } let cache = state.nextVerticesCache.get(selectionKey); if (!cache) { cache = { @@ -866,6 +918,7 @@ export class NonLocalSelectionsMetadata { indirectOptions.sameTypeOptions, supergraphSchema, overrideConditions, + false, ); cache.typesToNextVertices.set(typeName, cacheEntry); } @@ -879,6 +932,7 @@ export class NonLocalSelectionsMetadata { [vertex], supergraphSchema, overrideConditions, + false, ); cache.remainingVerticesToNextVertices.set(vertex, cacheEntry); } @@ -922,16 +976,23 @@ export class NonLocalSelectionsMetadata { * (We do account for override conditions, which are relatively * straightforward.) * - * Since we're iterating through next vertices in the process, for efficiency - * sake we also compute whether there are any reachable cross-subgraph edges - * from the next vertices (without indirect options). This method assumes that - * inline fragments have type conditions. + * Since we're iterating through next vertices in the process, for + * efficiency's sake we also compute whether there are any reachable + * cross-subgraph edges from the next vertices (without indirect options). + * This method assumes that inline fragments have type conditions. + * + * To support mutations, we allow indicating the initial subgraph will be + * constrained after taking the element, in which case indirect options will + * be ignored. This is to ensure that top-level mutation fields are not + * executed on a different subgraph than the initial one during query + * planning. */ private estimateNextVerticesForSelectionWithoutCaching( element: OperationElement, parentVertices: Iterable, supergraphSchema: Schema, overrideConditions: Map, + isInitialSubgraphConstrainedAfterElement: boolean, ): NextVerticesInfo { const nextVertices = new Set(); switch (element.kind) { @@ -955,7 +1016,7 @@ export class NonLocalSelectionsMetadata { } }; for (const vertex of parentVertices) { - // As an upper bound for efficiency sake, we consider both + // As an upper bound for efficiency's sake, we consider both // non-type-exploded and type-exploded options. processHeadVertex(vertex); const downcasts = this.verticesToObjectTypeDowncasts.get(vertex); @@ -1041,16 +1102,33 @@ export class NonLocalSelectionsMetadata { assertUnreachable(element); } - return this.estimateVerticesWithIndirectOptions(nextVertices); + return this.estimateVerticesWithIndirectOptions( + nextVertices, + isInitialSubgraphConstrainedAfterElement, + ); } /** - * Estimate the indirect options for the given next vertices, and add them to - * the given vertices. As an upper bound for efficiency's sake, we assume we - * can take any indirect option (i.e. ignore any edge conditions). + * Estimate the indirect options for the given next vertices, and return the + * given next vertices along with `nextVerticesWithIndirectOptions` which + * contains these direct and indirect options. As an upper bound for + * efficiency's sake, we assume we can take any indirect option (i.e. ignore + * any edge conditions). + * + * Since we're iterating through next vertices in the process, for + * efficiency's sake we also compute whether there are any reachable + * cross-subgraph edges from the next vertices (without indirect options). + * + * To support mutations, we allow ignoring indirect options, as we don't want + * top-level mutation fields to be executed on a different subgraph than the + * initial one. In that case, `nextVerticesWithIndirectOptions` will not have + * any `types`, and the given vertices will be added to `remainingVertices` + * (despite them potentially being part of the complete digraph for their + * type). This is fine, as caching logic accounts for this accordingly. */ private estimateVerticesWithIndirectOptions( nextVertices: Set, + ignoreIndirectOptions: boolean, ): NextVerticesInfo { const nextVerticesInfo: NextVerticesInfo = { nextVertices, @@ -1063,7 +1141,16 @@ export class NonLocalSelectionsMetadata { for (const nextVertex of nextVertices) { nextVerticesInfo.nextVerticesHaveReachableCrossSubgraphEdges ||= nextVertex.hasReachableCrossSubgraphEdges; - + + // As noted above, we don't want top-level mutation fields to be executed + // on a different subgraph than the initial one, so we support ignoring + // indirect options here. + if (ignoreIndirectOptions) { + nextVerticesInfo.nextVerticesWithIndirectOptions.remainingVertices + .add(nextVertex); + continue; + } + const typeName = nextVertex.type.name const optionsMetadata = this.typesToIndirectOptions.get(typeName); if (optionsMetadata) { @@ -1085,7 +1172,7 @@ export class NonLocalSelectionsMetadata { continue; } } - // We need to add the remaining vertex, and if its our first time seeing + // We need to add the remaining vertex, and if it's our first time seeing // it, we also add any of its interface object options. if ( !nextVerticesInfo.nextVerticesWithIndirectOptions.remainingVertices diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index bd6ffa36e..84b0ab503 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -6247,6 +6247,171 @@ 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 + } + } + }, + }, + }, + } + `); + }); + + it('ignores mutation @key at top-level for mutations', () => { + const subgraph1 = { + name: 'Subgraph1', + typeDefs: gql` + type Query { + dummy: Int + } + + type Mutation @key(fields: "__typename") { + f: F @shareable + } + + type F @key(fields: "id") { + id: ID! + x: Int + } + `, + }; + + const subgraph2 = { + name: 'Subgraph2', + typeDefs: gql` + type Mutation @key(fields: "__typename") { + 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); + // Note that a plan that uses a mutation @key will typically be more costly + // than a plan that doesn't, so the query planner's plan won't show that we + // properly ignored the @key. We instead check both the number of evaluated + // plans and the plan itself. + expect( + queryPlanner.lastGeneratedPlanStatistics()?.evaluatedPlanCount, + ).toStrictEqual(1); + 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 60aeeb51f..4912cc592 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 ( @@ -431,6 +433,7 @@ class QueryPlanningTraversal { this.parameters.supergraphSchema, this.parameters.inconsistentAbstractTypesRuntimes, this.parameters.overrideConditions, + initialSubgraphConstraint !== null, ) ) { throw Error(`Number of non-local selections exceeds limit of ${ @@ -527,8 +530,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 +798,7 @@ class QueryPlanningTraversal { context, this.typeConditionedFetching, null, + null, excludedDestinations, addConditionExclusion(excludedConditions, edge.conditions), ).findBestPlan(); @@ -3593,7 +3598,7 @@ function computePlanInternal({ const { operation, processor } = parameters; if (operation.rootKind === 'mutation') { - const dependencyGraphs = computeRootSerialDependencyGraph( + const dependencyGraphs = computeRootSerialDependencyGraphForMutation( parameters, hasDefers, nonLocalSelectionsState, @@ -3770,6 +3775,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 +3783,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 +3838,7 @@ function onlyRootSubgraph(graph: FetchDependencyGraph): string { return subgraphs[0]; } -function computeRootSerialDependencyGraph( +function computeRootSerialDependencyGraphForMutation( parameters: PlanningParameters, hasDefers: boolean, nonLocalSelectionsState: NonLocalSelectionsState | null, @@ -3805,7 +3849,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 +3858,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(),