diff --git a/composition-js/src/__tests__/validation_errors.test.ts b/composition-js/src/__tests__/validation_errors.test.ts index 1a69b413e..1e063a8bd 100644 --- a/composition-js/src/__tests__/validation_errors.test.ts +++ b/composition-js/src/__tests__/validation_errors.test.ts @@ -413,6 +413,151 @@ describe('when shared field has non-intersecting runtime types in different subg }); }); +describe('@shareable on top-level mutation fields', () => { + it('errors when queries may require multiple calls to mutation field', () => { + const subgraphA = { + name: 'A', + typeDefs: gql` + type Query { + dummy: Int + } + + type Mutation { + f: F @shareable + } + + type F { + x: Int + } + ` + }; + const subgraphB = { + name: 'B', + typeDefs: gql` + type Mutation { + f: F @shareable + } + + type F { + y: Int + } + ` + }; + + const result = composeAsFed2Subgraphs([subgraphA, subgraphB]); + expect(result.errors).toBeDefined(); + expect(errorMessages(result)).toMatchStringArray([ + ` + Supergraph API queries using the mutation field \"Mutation.f\" at top-level must be satisfiable without needing to call that field from multiple subgraphs, but every subgraph with that field encounters satisfiability errors. Please fix these satisfiability errors for (at least) one of the following subgraphs with the mutation field: + - When calling \"Mutation.f\" at top-level from subgraph \"A\": + The following supergraph API query: + mutation { + f { + y + } + } + cannot be satisfied by the subgraphs because: + - from subgraph \"A\": + - cannot find field \"F.y\". + - cannot move to subgraph \"B\", which has field \"F.y\", because type \"F\" has no @key defined in subgraph \"B\". + - When calling \"Mutation.f\" at top-level from subgraph \"B\": + The following supergraph API query: + mutation { + f { + x + } + } + cannot be satisfied by the subgraphs because: + - from subgraph \"B\": + - cannot find field \"F.x\". + - cannot move to subgraph \"A\", which has field \"F.x\", because type \"F\" has no @key defined in subgraph \"A\". + ` + ]); + }); + + it('errors normally for mutation fields that are not actually shared', () => { + const subgraphA = { + name: 'A', + typeDefs: gql` + type Query { + dummy: Int + } + + type Mutation { + f: F @shareable + } + + type F @key(fields: "id") { + id: ID! + x: Int + } + ` + }; + const subgraphB = { + name: 'B', + typeDefs: gql` + type F @key(fields: "id", resolvable: false) { + id: ID! + y: Int + } + ` + }; + + const result = composeAsFed2Subgraphs([subgraphA, subgraphB]); + expect(result.errors).toBeDefined(); + expect(errorMessages(result)).toMatchStringArray([ + ` + The following supergraph API query: + mutation { + f { + y + } + } + cannot be satisfied by the subgraphs because: + - from subgraph "A": + - cannot find field "F.y". + - cannot move to subgraph "B", which has field "F.y", because none of the @key defined on type "F" in subgraph "B" are resolvable (they are all declared with their "resolvable" argument set to false). + ` + ]); + }); + + it('succeeds when queries do not require multiple calls to mutation field', () => { + const subgraphA = { + name: 'A', + typeDefs: gql` + type Query { + dummy: Int + } + + type Mutation { + f: F @shareable + } + + type F @key(fields: "id") { + id: ID! + x: Int + } + ` + }; + const subgraphB = { + name: 'B', + typeDefs: gql` + type Mutation { + f: F @shareable + } + + type F @key(fields: "id", resolvable: false) { + id: ID! + y: Int + } + ` + }; + + const result = composeAsFed2Subgraphs([subgraphA, subgraphB]); + expect(result.errors).toBeUndefined(); + }); +}); + describe('other validation errors', () => { it('errors when maxValidationSubgraphPaths is exceeded', () => { diff --git a/composition-js/src/validate.ts b/composition-js/src/validate.ts index b1c552558..e9ac81148 100644 --- a/composition-js/src/validate.ts +++ b/composition-js/src/validate.ts @@ -39,6 +39,7 @@ import { ContextSpecDefinition, CONTEXT_VERSIONS, NamedSchemaElement, + NamedType, } from "@apollo/federation-internals"; import { Edge, @@ -59,6 +60,7 @@ import { ConditionResolver, UnadvanceableClosures, isUnadvanceableClosures, + Vertex, } from "@apollo/query-graphs"; import { CompositionHint, HINTS } from "./hints"; import { ASTNode, GraphQLError, print } from "graphql"; @@ -113,7 +115,7 @@ function shareableFieldNonIntersectingRuntimeTypesError( + `\nShared field "${field.coordinate}" return type "${field.type}" has a non-intersecting set of possible runtime types across subgraphs. Runtime types in subgraphs are:` + `\n${typeStrings.join(';\n')}.` + `\nThis is not allowed as shared fields must resolve the same way in all subgraphs, and that imply at least some common runtime types between the subgraphs.`; - const error = new ValidationError(message, invalidState.supergraphPath, invalidState.subgraphPathInfos.map((p) => p.path.path), witness); + const error = new ValidationError(message, invalidState.supergraphPath, invalidState.allSubgraphPathInfos().map((p) => p.path.path), witness); return ERRORS.SHAREABLE_HAS_MISMATCHED_RUNTIME_TYPES.err(error.message, { nodes: subgraphNodes(invalidState, (s) => (s.type(field.parent.name) as CompositeType | undefined)?.field(field.name)?.sourceAST), }); @@ -431,8 +433,16 @@ export class ValidationState { constructor( // Path in the supergraph corresponding to the current state. public readonly supergraphPath: RootPath, - // All the possible paths we could be in the subgraph. - public readonly subgraphPathInfos: SubgraphPathInfo[], + // All the possible paths we could be in the subgraph. When the supergraph + // path's top-level selection is a mutation field, the possible paths are + // instead partitioned by the name of the subgraph containing the mutation + // field. + public readonly subgraphPathInfos: + | SubgraphPathInfo[] + | { + mutationField: FieldDefinition; + paths: Map; + }, // When we encounter an `@override`n field with a label condition, we record // its value (T/F) as we traverse the graph. This allows us to ignore paths // that can never be taken by the query planner (i.e. a path where the @@ -469,6 +479,73 @@ export class ValidationState { ); } + // Returns whether the entire entire visit to this state can be skipped. If + // the state is partitioned, note that each individual partition must be + // skippable for the state to be skippable. + canSkipVisit( + subgraphNameToGraphEnumValue: Map, + previousVisits: QueryGraphState, + ): boolean { + const vertex = this.supergraphPath.tail; + if (this.subgraphPathInfos instanceof Array) { + return this.canSkipVisitForSubgraphPaths( + vertex, + this.subgraphPathInfos, + subgraphNameToGraphEnumValue, + previousVisits, + ); + } else { + let canSkip = true; + for (const subgraphPathInfos of this.subgraphPathInfos.paths.values()) { + // Note that this method mutates the set of previous visits, so we + // purposely do not short-circuit return here. + if (!this.canSkipVisitForSubgraphPaths( + vertex, + subgraphPathInfos, + subgraphNameToGraphEnumValue, + previousVisits, + )) { + canSkip = false; + } + } + return canSkip; + } + } + + canSkipVisitForSubgraphPaths( + supergraphPathTail: Vertex, + subgraphPathInfos: SubgraphPathInfo[], + subgraphNameToGraphEnumValue: Map, + previousVisits: QueryGraphState, + ): boolean { + const currentVertexVisit: VertexVisit = { + subgraphContextKeys: this.currentSubgraphContextKeys( + subgraphNameToGraphEnumValue, + subgraphPathInfos, + ), + overrideConditions: this.selectedOverrideConditions + }; + const previousVisitsForVertex = previousVisits.getVertexState(supergraphPathTail); + if (previousVisitsForVertex) { + for (const previousVisit of previousVisitsForVertex) { + if (isSupersetOrEqual(currentVertexVisit, previousVisit)) { + // This means that we've already seen the type we're currently on in the supergraph, and when saw it we could be in + // one of `previousSources`, and we validated that we could reach anything from there. We're now on the same + // type, and have strictly more options regarding subgraphs. So whatever comes next, we can handle in the exact + // same way we did previously, and there is thus no way to bother. + debug.groupEnd(`Has already validated this vertex.`); + return true; + } + } + // We're gonna have to validate, but we can save the new set of sources here to hopefully save work later. + previousVisitsForVertex.push(currentVertexVisit); + } else { + // We save the current sources but do validate. + previousVisits.setVertexState(supergraphPathTail, [currentVertexVisit]); + } + return false; + } + /** * Validates that the current state can always be advanced for the provided supergraph edge, and returns the updated state if * so. @@ -480,17 +557,23 @@ export class ValidationState { * to a type condition for which there cannot be any runtime types), in which case not further validation is necessary "from that branch". * Additionally, when the state can be successfully advanced, an `hint` can be optionally returned. */ - validateTransition(context: ValidationContext, supergraphEdge: Edge, matchingContexts: string[]): { + validateTransition( + context: ValidationContext, + supergraphEdge: Edge, + matchingContexts: string[], + validationErrors: GraphQLError[], + satisfiabilityErrorsByMutationFieldAndSubgraph: Map< + string, Map + >, + ): { state?: ValidationState, - error?: GraphQLError, hint?: CompositionHint, } { assert(!supergraphEdge.conditions, () => `Supergraph edges should not have conditions (${supergraphEdge})`); const transition = supergraphEdge.transition; const targetType = supergraphEdge.tail.type; - const newSubgraphPathInfos: SubgraphPathInfo[] = []; - const deadEnds: UnadvanceableClosures[] = []; + // If the edge has an override condition, we should capture it in the state so // that we can ignore later edges that don't satisfy the condition. const newOverrideConditions = new Map([...this.selectedOverrideConditions]); @@ -500,52 +583,146 @@ export class ValidationState { supergraphEdge.overrideCondition.condition ); } + const newPath = this.supergraphPath.add( + transition, + supergraphEdge, + noConditionsResolution, + ); - for (const { path, contexts } of this.subgraphPathInfos) { - const options = advancePathWithTransition( - path, + let updatedState: ValidationState; + if (this.subgraphPathInfos instanceof Array) { + const { + newSubgraphPathInfos, + error, + } = this.validateTransitionforSubgraphPaths( + this.subgraphPathInfos, + newOverrideConditions, transition, targetType, - newOverrideConditions, + matchingContexts, + newPath, ); - if (isUnadvanceableClosures(options)) { - deadEnds.push(options); - continue; + if (error) { + validationErrors.push(error); + return {}; } - if (options.length === 0) { - // This means that the edge is a type condition and that if we follow the path to this subgraph, we're guaranteed that handling that - // type condition give us no matching results, and so we can handle whatever comes next really. - return { state: undefined }; + // As noted in `validateTransitionforSubgraphPaths()`, this being empty + // means that the edge is a type condition and that if we follow the path + // to this subgraph, we're guaranteed that handling that type condition + // gives us no matching results, and so we can handle whatever comes next + // really. + if (newSubgraphPathInfos.length === 0) { + return {}; } - let newContexts = contexts; - if (matchingContexts.length) { - const subgraphName = path.path.tail.source; - const typeName = path.path.tail.type.name; - newContexts = new Map([...contexts]); - for (const matchingContext in matchingContexts) { - newContexts.set( - matchingContext, - { - subgraphName, - typeName, + const mutationField = ValidationState.fieldIfTopLevelMutation( + this.supergraphPath, + supergraphEdge, + ); + if (mutationField) { + // If we just added a top-level mutation field, we partition the created + // state by the subgraph of the field. + const partitionedSubgraphPathInfos = + new Map(); + for (const subgraphPathInfo of newSubgraphPathInfos) { + let subgraph = ValidationState.subgraphOfTopLevelMutation( + subgraphPathInfo + ); + let subgraphPathInfos = partitionedSubgraphPathInfos.get(subgraph); + if (!subgraphPathInfos) { + subgraphPathInfos = []; + partitionedSubgraphPathInfos.set(subgraph, subgraphPathInfos); + } + subgraphPathInfos.push(subgraphPathInfo); + } + if (partitionedSubgraphPathInfos.size <= 1) { + // If there's not more than one subgraph, then the mutation field was + // never really shared, and we can continue with non-partitioned + // state. + updatedState = new ValidationState( + newPath, + [...partitionedSubgraphPathInfos.values()].flat(), + newOverrideConditions, + ); + } else { + // Otherwise, we need the partitioning, and we set up the error stacks + // for each (field, subgraph) pair. + let errorsBySubgraph = satisfiabilityErrorsByMutationFieldAndSubgraph + .get(mutationField.coordinate); + if (!errorsBySubgraph) { + errorsBySubgraph = new Map(); + satisfiabilityErrorsByMutationFieldAndSubgraph.set( + mutationField.coordinate, + errorsBySubgraph, + ); + } + for (const subgraph of partitionedSubgraphPathInfos.keys()) { + if (!errorsBySubgraph.has(subgraph)) { + errorsBySubgraph.set(subgraph, []); } - ) + } + updatedState = new ValidationState( + newPath, + { + mutationField, + paths: partitionedSubgraphPathInfos, + }, + newOverrideConditions, + ); } + } else { + updatedState = new ValidationState( + newPath, + newSubgraphPathInfos, + newOverrideConditions, + ); } - - newSubgraphPathInfos.push(...options.map((p) => ({ path: p, contexts: newContexts }))); - } - const newPath = this.supergraphPath.add(transition, supergraphEdge, noConditionsResolution); - if (newSubgraphPathInfos.length === 0) { - return { error: satisfiabilityError(newPath, this.subgraphPathInfos.map((p) => p.path.path), deadEnds.map((d) => d.toUnadvanceables())) }; + } else { + const partitionedSubgraphPathInfos = + new Map(); + for (const [subgraph, subgraphPathInfos] of this.subgraphPathInfos.paths) { + // The setup we do above when we enter a mutation field ensures these + // map entries exist. + const errors = satisfiabilityErrorsByMutationFieldAndSubgraph + .get(this.subgraphPathInfos.mutationField.coordinate)! + .get(subgraph)!; + const { + newSubgraphPathInfos, + error, + } = this.validateTransitionforSubgraphPaths( + subgraphPathInfos, + newOverrideConditions, + transition, + targetType, + matchingContexts, + newPath, + ); + if (error) { + errors.push(error); + continue; + } + // As noted in `validateTransitionforSubgraphPaths()`, this being empty + // means that the edge is a type condition and that if we follow the + // path to this subgraph, we're guaranteed that handling that type + // condition gives us no matching results, and so we can handle whatever + // comes next really. + if (newSubgraphPathInfos.length === 0) { + return {}; + } + partitionedSubgraphPathInfos.set(subgraph, newSubgraphPathInfos); + } + if (partitionedSubgraphPathInfos.size === 0) { + return {}; + } + updatedState = new ValidationState( + newPath, + { + mutationField: this.subgraphPathInfos.mutationField, + paths: partitionedSubgraphPathInfos, + }, + newOverrideConditions, + ); } - const updatedState = new ValidationState( - newPath, - newSubgraphPathInfos, - newOverrideConditions, - ); - // When handling a @shareable field, we also compare the set of runtime types for each subgraphs involved. // If there is no common intersection between those sets, then we record an error: a @shareable field should resolve // the same way in all the subgraphs in which it is resolved, and there is no way this can be true if each subgraph @@ -562,14 +739,15 @@ export class ValidationState { // Note that we ignore any path when the type is not an abstract type, because in practice this means an @interfaceObject // and this should not be considered as an implementation type. Besides @interfaceObject always "stand-in" for every // implementations so they never are a problem for this check and can be ignored. + let allSubgraphPathInfos = updatedState.allSubgraphPathInfos(); let hint: CompositionHint | undefined = undefined; if ( - newSubgraphPathInfos.length > 1 + allSubgraphPathInfos.length > 1 && transition.kind === 'FieldCollection' && isAbstractType(newPath.tail.type) && context.isShareable(transition.definition) ) { - const filteredPaths = newSubgraphPathInfos.map((p) => p.path.path).filter((p) => isAbstractType(p.tail.type)); + const filteredPaths = allSubgraphPathInfos.map((p) => p.path.path).filter((p) => isAbstractType(p.tail.type)); if (filteredPaths.length > 1) { // We start our intersection by using all the supergraph types, both because it's a convenient "max" set to start our intersection, // but also because that means we will ignore @inaccessible types in our checks (which is probably not very important because @@ -580,7 +758,7 @@ export class ValidationState { const runtimeTypesToSubgraphs = new MultiMap(); const runtimeTypesPerSubgraphs = new MultiMap(); let hasAllEmpty = true; - for (const { path } of newSubgraphPathInfos) { + for (const { path } of allSubgraphPathInfos) { const subgraph = path.path.tail.source; const typeNames = possibleRuntimeTypeNamesSorted(path.path); @@ -610,7 +788,8 @@ export class ValidationState { // which each subgraph can resolve is `null` and so that essentially guaranttes that all subgraph do resolve the same way. if (!hasAllEmpty) { if (intersection.length === 0) { - return { error: shareableFieldNonIntersectingRuntimeTypesError(updatedState, transition.definition, runtimeTypesToSubgraphs) }; + validationErrors.push(shareableFieldNonIntersectingRuntimeTypesError(updatedState, transition.definition, runtimeTypesToSubgraphs)); + return {}; } // As said, we accept it if there is an intersection, but if the runtime types are not all the same, we still emit a warning to make it clear that @@ -625,9 +804,105 @@ export class ValidationState { return { state: updatedState, hint }; } + validateTransitionforSubgraphPaths( + subgraphPathInfos: SubgraphPathInfo[], + newOverrideConditions: Map, + transition: Transition, + targetType: NamedType, + matchingContexts: string[], + newPath: RootPath, + ): { + newSubgraphPathInfos: SubgraphPathInfo[], + error?: never, + } | { + newSubgraphPathInfos?: never, + error: GraphQLError, + } { + const newSubgraphPathInfos: SubgraphPathInfo[] = []; + const deadEnds: UnadvanceableClosures[] = []; + for (const { path, contexts } of subgraphPathInfos) { + const options = advancePathWithTransition( + path, + transition, + targetType, + newOverrideConditions, + ); + if (isUnadvanceableClosures(options)) { + deadEnds.push(options); + continue; + } + if (options.length === 0) { + // This means that the edge is a type condition and that if we follow + // the path to this subgraph, we're guaranteed that handling that type + // condition give us no matching results, and so we can handle whatever + // comes next really. + return { newSubgraphPathInfos: [] }; + } + let newContexts = contexts; + if (matchingContexts.length) { + const subgraphName = path.path.tail.source; + const typeName = path.path.tail.type.name; + newContexts = new Map([...contexts]); + for (const matchingContext in matchingContexts) { + newContexts.set( + matchingContext, + { + subgraphName, + typeName, + } + ) + } + } + + newSubgraphPathInfos.push( + ...options.map((p) => ({ path: p, contexts: newContexts })) + ); + } + + return newSubgraphPathInfos.length === 0 + ? { + error: satisfiabilityError( + newPath, + subgraphPathInfos.map((p) => p.path.path), + deadEnds.map((d) => d.toUnadvanceables()) + ) + } + : { newSubgraphPathInfos }; + } + + private static fieldIfTopLevelMutation( + supergraphPath: RootPath, + edge: Edge, + ): FieldDefinition | null { + if (supergraphPath.size !== 0) { + return null; + } + if (edge.transition.kind !== 'FieldCollection') { + return null; + } + if (supergraphPath.root !== supergraphPath.graph.root('mutation')) { + return null; + } + return edge.transition.definition; + } + + private static subgraphOfTopLevelMutation( + subgraphPathInfo: SubgraphPathInfo + ): string { + const lastEdge = subgraphPathInfo.path.path.lastEdge(); + assert(lastEdge, "Path unexpectedly missing edge"); + return lastEdge.head.source; + } + + allSubgraphPathInfos(): SubgraphPathInfo[] { + return this.subgraphPathInfos instanceof Array + ? this.subgraphPathInfos + : Array.from(this.subgraphPathInfos.paths.values()).flat(); + } + currentSubgraphNames(): string[] { const subgraphs: string[] = []; - for (const pathInfo of this.subgraphPathInfos) { + for (const pathInfo of this.allSubgraphPathInfos()) { const source = pathInfo.path.path.tail.source; if (!subgraphs.includes(source)) { subgraphs.push(source); @@ -636,9 +911,12 @@ export class ValidationState { return subgraphs; } - currentSubgraphContextKeys(subgraphNameToGraphEnumValue: Map): Set { + currentSubgraphContextKeys( + subgraphNameToGraphEnumValue: Map, + subgraphPathInfos: SubgraphPathInfo[], + ): Set { const subgraphContextKeys: Set = new Set(); - for (const pathInfo of this.subgraphPathInfos) { + for (const pathInfo of subgraphPathInfos) { const tailSubgraphName = pathInfo.path.path.tail.source; const tailSubgraphEnumValue = subgraphNameToGraphEnumValue.get(tailSubgraphName); const tailTypeName = pathInfo.path.path.tail.type.name; @@ -657,15 +935,26 @@ export class ValidationState { } currentSubgraphs(): { name: string, schema: Schema }[] { - if (this.subgraphPathInfos.length === 0) { + const allSubgraphPathInfos = this.allSubgraphPathInfos(); + if (allSubgraphPathInfos.length === 0) { return []; } - const sources = this.subgraphPathInfos[0].path.path.graph.sources; + const sources = allSubgraphPathInfos[0].path.path.graph.sources; return this.currentSubgraphNames().map((name) => ({ name, schema: sources.get(name)!})); } toString(): string { - return `${this.supergraphPath} <=> [${this.subgraphPathInfos.map(s => s.path.toString()).join(', ')}]`; + if (this.subgraphPathInfos instanceof Array) { + return `${this.supergraphPath} <=> [${this.subgraphPathInfos.map( + p => p.path.toString() + ).join(', ')}]`; + } else { + return `${this.supergraphPath} <=> {${ + Array.from(this.subgraphPathInfos.paths.entries()).map( + ([s, p]) => `${s}: [${p.map(p => p.path.toString()).join(', ')}]` + ).join(', ') + }}`; + } } } @@ -698,12 +987,22 @@ class ValidationTraversal { private readonly validationErrors: GraphQLError[] = []; private readonly validationHints: CompositionHint[] = []; + // When we discover a shared top-level mutation field, we track satisfiability + // errors for each subgraph containing the field separately. This is because + // the query planner needs to avoid calling these fields more than once, which + // means there must be no satisfiability errors for (at least) one subgraph. + // The first Map key is the field coordinate, and the second Map key is the + // subgraph name. + private readonly satisfiabilityErrorsByMutationFieldAndSubgraph: Map< + string, Map + > = new Map(); + private readonly context: ValidationContext; private totalValidationSubgraphPaths = 0; private maxValidationSubgraphPaths: number; - + private static DEFAULT_MAX_VALIDATION_SUBGRAPH_PATHS = 1000000; - + constructor( supergraphSchema: Schema, subgraphNameToGraphEnumValue: Map, @@ -711,8 +1010,9 @@ class ValidationTraversal { federatedQueryGraph: QueryGraph, compositionOptions: CompositionOptions, ) { - this.maxValidationSubgraphPaths = compositionOptions.maxValidationSubgraphPaths ?? ValidationTraversal.DEFAULT_MAX_VALIDATION_SUBGRAPH_PATHS; - + this.maxValidationSubgraphPaths = compositionOptions.maxValidationSubgraphPaths + ?? ValidationTraversal.DEFAULT_MAX_VALIDATION_SUBGRAPH_PATHS; + this.conditionResolver = simpleValidationConditionResolver({ supergraph: supergraphSchema, queryGraph: federatedQueryGraph, @@ -731,20 +1031,34 @@ class ValidationTraversal { subgraphNameToGraphEnumValue, ); } - + pushStack(state: ValidationState): { error?: GraphQLError } { - this.totalValidationSubgraphPaths += state.subgraphPathInfos.length; + if (state.subgraphPathInfos instanceof Array) { + this.totalValidationSubgraphPaths += state.subgraphPathInfos.length; + } else { + for (const subgraphPathInfos of state.subgraphPathInfos.paths.values()) { + this.totalValidationSubgraphPaths += subgraphPathInfos.length; + } + } this.stack.push(state); if (this.totalValidationSubgraphPaths > this.maxValidationSubgraphPaths) { - return { error: ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err(`Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}`) }; + return { + error: ERRORS.MAX_VALIDATION_SUBGRAPH_PATHS_EXCEEDED.err( + `Maximum number of validation subgraph paths exceeded: ${this.totalValidationSubgraphPaths}` + ) + }; } return {}; } - + popStack() { const state = this.stack.pop(); - if (state) { + if (state?.subgraphPathInfos instanceof Array) { this.totalValidationSubgraphPaths -= state.subgraphPathInfos.length; + } else if (state) { + for (const subgraphPathInfos of state.subgraphPathInfos.paths.values()) { + this.totalValidationSubgraphPaths -= subgraphPathInfos.length; + } } return state; } @@ -759,34 +1073,62 @@ class ValidationTraversal { return { errors: [error], hints: this.validationHints }; } } + for (const [ + fieldCoordinate, + errorsBySubgraph, + ] of this.satisfiabilityErrorsByMutationFieldAndSubgraph) { + // Check if some subgraph has no satisfiability errors. If so, then that + // subgraph can be used to satisfy all queries to the top-level mutation + // field, and we can ignore the errors in other subgraphs. + let someSubgraphHasNoErrors = false; + for (const errors of errorsBySubgraph.values()) { + if (errors.length === 0) { + someSubgraphHasNoErrors = true; + break; + } + } + if (someSubgraphHasNoErrors) { + continue; + } + // Otherwise, queries on the top-level mutation field can't be satisfied + // through only one call to that field. + let messageParts = [ + `Supergraph API queries using the mutation field "${fieldCoordinate}"` + + ` at top-level must be satisfiable without needing to call that` + + ` field from multiple subgraphs, but every subgraph with that field` + + ` encounters satisfiability errors. Please fix these satisfiability` + + ` errors for (at least) one of the following subgraphs with the` + + ` mutation field:` + ]; + for (const [subgraph, errors] of errorsBySubgraph) { + messageParts.push( + `- When calling "${fieldCoordinate}" at top-level from subgraph` + +` "${subgraph}":` + ); + for (const error of errors) { + for (const line of error.message.split("\n")) { + if (line.length === 0) { + messageParts.push(line); + } else { + messageParts.push(" " + line); + } + } + } + } + this.validationErrors.push( + ERRORS.SATISFIABILITY_ERROR.err(messageParts.join("\n")) + ); + } return { errors: this.validationErrors, hints: this.validationHints }; } private handleState(state: ValidationState): { error?: GraphQLError } { debug.group(() => `Validation: ${this.stack.length + 1} open states. Validating ${state}`); - const vertex = state.supergraphPath.tail; - - const currentVertexVisit: VertexVisit = { - subgraphContextKeys: state.currentSubgraphContextKeys(this.context.subgraphNameToGraphEnumValue), - overrideConditions: state.selectedOverrideConditions - }; - const previousVisitsForVertex = this.previousVisits.getVertexState(vertex); - if (previousVisitsForVertex) { - for (const previousVisit of previousVisitsForVertex) { - if (isSupersetOrEqual(currentVertexVisit, previousVisit)) { - // This means that we've already seen the type we're currently on in the supergraph, and when saw it we could be in - // one of `previousSources`, and we validated that we could reach anything from there. We're now on the same - // type, and have strictly more options regarding subgraphs. So whatever comes next, we can handle in the exact - // same way we did previously, and there is thus no way to bother. - debug.groupEnd(`Has already validated this vertex.`); - return {}; - } - } - // We're gonna have to validate, but we can save the new set of sources here to hopefully save work later. - previousVisitsForVertex.push(currentVertexVisit); - } else { - // We save the current sources but do validate. - this.previousVisits.setVertexState(vertex, [currentVertexVisit]); + if (state.canSkipVisit( + this.context.subgraphNameToGraphEnumValue, + this.previousVisits + )) { + return {}; } // Note that if supergraphPath is terminal, this method is a no-op, which is expected/desired as @@ -816,10 +1158,15 @@ class ValidationTraversal { : []; debug.group(() => `Validating supergraph edge ${edge}`); - const { state: newState, error, hint } = state.validateTransition(this.context, edge, matchingContexts); - if (error) { + const { state: newState, hint } = state.validateTransition( + this.context, + edge, + matchingContexts, + this.validationErrors, + this.satisfiabilityErrorsByMutationFieldAndSubgraph, + ); + if (!newState) { debug.groupEnd(`Validation error!`); - this.validationErrors.push(error); continue; } if (hint) { 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(),