diff --git a/.changeset/slow-falcons-brake.md b/.changeset/slow-falcons-brake.md new file mode 100644 index 0000000..96c2f5f --- /dev/null +++ b/.changeset/slow-falcons-brake.md @@ -0,0 +1,5 @@ +--- +'@theguild/federation-composition': patch +--- + +SATISFIABILITY_ERROR - allow to resolve a field via entity type's child diff --git a/__tests__/supergraph-composition.spec.ts b/__tests__/supergraph-composition.spec.ts index 88d57f4..d61ac70 100644 --- a/__tests__/supergraph-composition.spec.ts +++ b/__tests__/supergraph-composition.spec.ts @@ -449,13 +449,7 @@ testImplementations(api => { } const subgraphs = await getSubgraphsOfHugeSchema(); - const result = api.composeServices( - subgraphs, - { - disableValidationRules: ['SatisfiabilityRule'], - }, - true, - ); + const result = api.composeServices(subgraphs); assertCompositionSuccess(result); }); diff --git a/__tests__/supergraph/errors/SATISFIABILITY_ERROR.spec.ts b/__tests__/supergraph/errors/SATISFIABILITY_ERROR.spec.ts index 2e62871..92e877f 100644 --- a/__tests__/supergraph/errors/SATISFIABILITY_ERROR.spec.ts +++ b/__tests__/supergraph/errors/SATISFIABILITY_ERROR.spec.ts @@ -1392,6 +1392,56 @@ testVersions((api, version) => { ); }); + test(`field resolvable via entity type's child`, () => { + assertCompositionSuccess( + api.composeServices([ + { + name: 'foo', + typeDefs: graphql` + type Query { + foo: Foo + } + + type Foo { + user: User + } + + type User @key(fields: "id profile { id }") @extends { + id: ID! + profile: Profile! @external + } + + type Profile @extends { + id: ID! + } + `, + }, + { + name: 'bar', + typeDefs: graphql` + type Query { + bar: Bar + } + + type Bar { + user: User + } + + type User @key(fields: "id profile { id }") { + id: ID! + profile: Profile! + } + + type Profile { + id: ID! + name: String + } + `, + }, + ]), + ); + }); + test('fed v1: same types but extra root field in a subgraph missing a field', () => { expect( api.composeServices([ diff --git a/benchmark.ts b/benchmark.ts index 298bcc1..754bd47 100644 --- a/benchmark.ts +++ b/benchmark.ts @@ -63,9 +63,7 @@ group('huge schema', () => { }); bench('guild', () => { - assertCompositionSuccess( - guildComposeServices(hugeSchema, { disableValidationRules: ['SatisfiabilityRule'] }), - ); + assertCompositionSuccess(guildComposeServices(hugeSchema)); }); }); diff --git a/src/supergraph/validation/rules/satisfiablity-rule.ts b/src/supergraph/validation/rules/satisfiablity-rule.ts index 5707104..4235722 100644 --- a/src/supergraph/validation/rules/satisfiablity-rule.ts +++ b/src/supergraph/validation/rules/satisfiablity-rule.ts @@ -22,27 +22,29 @@ import type { SupergraphVisitorMap } from '../../composition/visitor.js'; import type { SupergraphState } from '../../state.js'; import type { SupergraphValidationContext } from '../validation-context.js'; -function canGraphMoveToGraph( +function canGraphMoveToGraphByEntity( supergraphState: SupergraphState, - objectTypeState: ObjectTypeState, + entityName: string, sourceGraphId: string, targetGraphId: string, ): boolean { - const sourceGraphKeys = objectTypeState.byGraph.get(sourceGraphId)!.keys; - const targetGraphKeys = objectTypeState.byGraph.get(targetGraphId)!.keys; + const objectTypeState = supergraphState.objectTypes.get(entityName); + + if (!objectTypeState) { + throw new Error(`Type "${entityName}" not found in supergraph state`); + } + + const objectTypeStateInSourceGraph = objectTypeState.byGraph.get(sourceGraphId); + const objectTypeStateInTargetGraph = objectTypeState.byGraph.get(targetGraphId); + + const sourceGraphKeys = objectTypeStateInSourceGraph?.keys || []; + const targetGraphKeys = objectTypeStateInTargetGraph?.keys || []; // no keys in both graphs? can't move. if (sourceGraphKeys.length === 0 && targetGraphKeys.length === 0) { return false; } - const fieldsOfSourceGraph = Array.from(objectTypeState.fields.values()).filter(f => - f.byGraph.get(sourceGraphId), - ); - const nonExternalFieldsOfSourceGraph = fieldsOfSourceGraph.filter( - f => f.byGraph.get(sourceGraphId)!.external === false, - ); - if (sourceGraphKeys.length === 0) { // if the source type has no keys, // we need to check if the fields resolvable by the type @@ -354,46 +356,50 @@ export function SatisfiabilityRule( supergraphState: SupergraphState, ): SupergraphVisitorMap { const typeDependencies = buildOutputTypesDependencies(supergraphState); - /** - * Map of graph IDs to a set of graph IDs that the graph can move to. - */ - let currentMovabilityGraphType: string; - let movabilityGraph: DepGraph; + let movabilityGraph = new Map>(); - return { - // ObjectType runs before ObjectTypeField - ObjectType(objectState) { - // I want to check which graph can move to which graph. - // Once I know it, - // I can quickly check if a missing field - // can be resolved by talking to a graph - // that can move to the graph that has the field. - currentMovabilityGraphType = objectState.name; - movabilityGraph = new DepGraph({ - circular: true, - }); + function getMovabilityGraphForType(typeName: string) { + const existingMovabilityGraph = movabilityGraph.get(typeName); - const graphIds = Array.from(objectState.byGraph.keys()); + if (existingMovabilityGraph) { + return existingMovabilityGraph; + } - for (const sourceGraphId of objectState.byGraph.keys()) { - movabilityGraph.addNode(sourceGraphId); - } + const objectState = supergraphState.objectTypes.get(typeName); - for (const sourceGraphId of objectState.byGraph.keys()) { - const otherGraphIds = graphIds.filter(g => g !== sourceGraphId); + if (!objectState) { + throw new Error(`State of object type "${typeName}" not found in Supergraph state`); + } - for (const destGraphId of otherGraphIds) { - if (canGraphMoveToGraph(supergraphState, objectState, sourceGraphId, destGraphId)) { - movabilityGraph.addDependency(sourceGraphId, destGraphId); - } + const graph = new DepGraph({ + circular: true, + }); + + const graphIds = Array.from(objectState.byGraph.keys()); + + for (const sourceGraphId of objectState.byGraph.keys()) { + graph.addNode(sourceGraphId); + } + + for (const sourceGraphId of objectState.byGraph.keys()) { + const otherGraphIds = graphIds.filter(g => g !== sourceGraphId); + + for (const destGraphId of otherGraphIds) { + if ( + canGraphMoveToGraphByEntity(supergraphState, objectState.name, sourceGraphId, destGraphId) + ) { + graph.addDependency(sourceGraphId, destGraphId); } } - }, - ObjectTypeField(objectState, fieldState) { - if (currentMovabilityGraphType !== objectState.name) { - throw new Error('ObjectTypeField runs before ObjectType! This should not happen.'); - } + } + + movabilityGraph.set(typeName, graph); + return graph; + } + + return { + ObjectTypeField(objectState, fieldState) { if ( objectState.name === 'Query' || objectState.name === 'Mutation' || @@ -497,6 +503,14 @@ export function SatisfiabilityRule( }, }; + const currentObjectTypeMovabilityGraph = getMovabilityGraphForType(objectState.name); + + if (!currentObjectTypeMovabilityGraph) { + throw new Error( + `Movability graph for object type "${objectState.name}" not found in Supergraph state`, + ); + } + if (uniqueKeyFieldsSet.size > 0) { // // We're dealing with entities @@ -512,7 +526,13 @@ export function SatisfiabilityRule( // we need to run it for each root type if ( - canGraphResolveField(objectState, fieldState, graphId, supergraphState, movabilityGraph) + canGraphResolveField( + objectState, + fieldState, + graphId, + supergraphState, + currentObjectTypeMovabilityGraph, + ) ) { continue; } @@ -576,11 +596,16 @@ export function SatisfiabilityRule( .map(([g, _]) => g); // Dependencies of current graph that cannot resolve the field by jumping between graphs - const leafs = graphsWithField.map(g => findLeafs(movabilityGraph, graphId, g)).flat(1); + const leafs = graphsWithField + .map(g => findLeafs(currentObjectTypeMovabilityGraph, graphId, g)) + .flat(1); // If there are no leafs, it means that the field is resolvable by jumping between graphs. // It's only true if the graph can jump to at least one graph. - if (leafs.length === 0 && movabilityGraph.directDependenciesOf(graphId).length > 0) { + if ( + leafs.length === 0 && + currentObjectTypeMovabilityGraph.directDependenciesOf(graphId).length > 0 + ) { continue; } @@ -630,7 +655,10 @@ export function SatisfiabilityRule( }) .flat(1) : otherGraphIds - .filter(g => !movabilityGraph.directDependenciesOf(graphId).includes(g)) + .filter( + g => + !currentObjectTypeMovabilityGraph.directDependenciesOf(graphId).includes(g), + ) .map(gid => { const keys = objectState.byGraph.get(gid)!.keys.map(k => k.fields); @@ -785,6 +813,40 @@ export function SatisfiabilityRule( continue; } + // Check if we can move to a graph that has the field via entity type + + // find entity types referencing the object type + const entityTypesReferencingLookingObject = dependenciesOfObjectType + .map(typeName => supergraphState.objectTypes.get(typeName)) + .filter( + (t): t is ObjectTypeState => + // entity type is an object type with @key + !!t && Array.from(t.byGraph.values()).some(tg => tg.keys.length > 0), + ); + + const graphIdsUnableToResolveFieldViaEntityType: string[] = []; + for (const [graphId] of graphsWithoutField) { + // a list of entity types defined in the currently checked graph + const localEntityTypes = entityTypesReferencingLookingObject.filter(et => + et.byGraph.has(graphId), + ); + const isFieldResolvableThroughEntity = localEntityTypes.some(et => + // check if a graph without the field + // can move (via entity type) to at least one graph with the field + graphsWithField.some(targetGraphId => + canGraphMoveToGraphByEntity(supergraphState, et.name, graphId, targetGraphId), + ), + ); + + if (!isFieldResolvableThroughEntity) { + graphIdsUnableToResolveFieldViaEntityType.push(graphId); + } + } + + if (graphIdsUnableToResolveFieldViaEntityType.length === 0) { + continue; + } + const schemaDefinitionOfGraph = subgraphState.schema; const rootTypes = ( [