diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts index 0e23cbbce8..41158d4026 100644 --- a/composition-js/src/__tests__/override.compose.test.ts +++ b/composition-js/src/__tests__/override.compose.test.ts @@ -92,7 +92,7 @@ describe("composition involving @override directive", () => { @join__type(graph: SUBGRAPH2, key: \\"k\\") { k: ID - a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, external: true) + a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") b: Int @join__field(graph: SUBGRAPH2) }" `); @@ -314,7 +314,7 @@ describe("composition involving @override directive", () => { @join__type(graph: SUBGRAPH2, key: \\"k\\") { k: ID - a: Int @join__field(graph: SUBGRAPH1) + a: Int @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") }" `); diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 7dc14abb73..cf30984668 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -710,10 +710,10 @@ class Merger { this.hintOnInconsistentValueTypeField(sources, dest, destField); } const subgraphFields = sources.map(t => t?.field(destField.name)); - this.validateOverride(subgraphFields, destField); + const filteredFields = this.validateOverride(subgraphFields, destField); - this.mergeField(subgraphFields, destField); - this.validateFieldSharing(subgraphFields, destField); + this.mergeField(filteredFields, destField); + this.validateFieldSharing(filteredFields, destField); } } } @@ -918,6 +918,7 @@ class Merger { return acc; }, { subgraphsWithOverride: [], subgraphMap: {} }); + const overriddenSubgraphs: number[] = []; // for each subgraph that has an @override directive, check to see if any errors or hints should be surfaced subgraphsWithOverride.forEach((subgraphName) => { const { overrideDirective } = subgraphMap[subgraphName]; @@ -965,9 +966,13 @@ class Merger { })); } else { // if we get here, then the @override directive is valid - // if the field being overridden is a key, then we need to add an @external directive + // if the field being overridden is used, then we need to add an @external directive assert(fromField, 'fromField should not be undefined'); - fromField.applyDirective(this.metadata(fromIdx).externalDirective()); + if (this.metadata(fromIdx).isFieldUsed(fromField) || this.metadata(fromIdx).isKeyField(fromField)) { + fromField.applyDirective(this.metadata(fromIdx).externalDirective()); + } else { + overriddenSubgraphs.push(fromIdx); + } this.hints.push(new CompositionHint( hintOverriddenFieldCanBeRemoved, `Field "${coordinate}" on subgraph "${sourceSubgraphName}" is overridden. Consider removing it.`, @@ -976,6 +981,8 @@ class Merger { } } }); + + return sources.map((source, idx) => overriddenSubgraphs.includes(idx) ? undefined : source); } private mergeField(sources: FieldOrUndefinedArray, dest: FieldDefinition) { diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index 5ca07b4ae8..dcce5e0211 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts @@ -147,7 +147,7 @@ describe('lifecycle hooks', () => { // the supergraph (even just formatting differences), this ID will change // and this test will have to updated. expect(secondCall[0]!.compositionId).toEqual( - '4718616f8164d2ffeb33c08278051dc144718392a9c9e5470b1922a0cb4bb22a', + '7544614133e6f2b5f995badc562c803785b1ccd5e0a52be47a59d8cc95df0285', ); // second call should have previous info in the second arg expect(secondCall[1]!.compositionId).toEqual(expectedFirstId); diff --git a/internals-js/src/extractSubgraphsFromSupergraph.ts b/internals-js/src/extractSubgraphsFromSupergraph.ts index eec04f2fc5..1011906ba0 100644 --- a/internals-js/src/extractSubgraphsFromSupergraph.ts +++ b/internals-js/src/extractSubgraphsFromSupergraph.ts @@ -180,13 +180,6 @@ export function extractSubgraphsFromSupergraph(supergraph: Schema): Subgraphs { if (args.external) { subgraphField.applyDirective(subgraph.metadata().externalDirective()); } - if (args.override) { - subgraphField.applyDirective(subgraph.metadata().overrideDirective(), {'from': args.override}); - const overriddenSubgraph = subgraphs.get(args.override); - if (overriddenSubgraph) { - overriddenSubgraph.metadata().addSyntheticExternal(subgraphField.coordinate); - } - } } } } diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index dee898e8a8..409abc139a 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -258,21 +258,21 @@ function validateAllFieldSet>( } } -export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadata): Set { - const usedExternalCoordinates = new Set(); +export function collectUsedFields(metadata: FederationMetadata): Set> { + const usedFields = new Set>(); // Collects all external fields used by a key, requires or provides collectUsedExternaFieldsForDirective( metadata, metadata.keyDirective(), type => type, - usedExternalCoordinates, + usedFields, ); collectUsedExternaFieldsForDirective>( metadata, metadata.requiresDirective(), field => field.parent!, - usedExternalCoordinates, + usedFields, ); collectUsedExternaFieldsForDirective>( metadata, @@ -281,7 +281,7 @@ export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadat const type = baseType(field.type!); return isCompositeType(type) ? type : undefined; }, - usedExternalCoordinates, + usedFields, ); // Collects all external fields used to satisfy an interface constraint @@ -291,20 +291,20 @@ export function collectUsedExternalFieldsCoordinates(metadata: FederationMetadat for (const runtimeType of runtimeTypes) { const implemField = runtimeType.field(field.name); if (implemField && metadata.isFieldExternal(implemField)) { - usedExternalCoordinates.add(implemField.coordinate); + usedFields.add(implemField); } } } } - return usedExternalCoordinates; + return usedFields; } function collectUsedExternaFieldsForDirective>( metadata: FederationMetadata, definition: DirectiveDefinition<{fields: any}>, targetTypeExtractor: (element: TParent) => CompositeType | undefined, - usedExternalCoordinates: Set + usedExternalCoordinates: Set> ) { for (const application of definition.applications()) { const type = targetTypeExtractor(application.parent! as TParent); @@ -323,7 +323,7 @@ function collectUsedExternaFieldsForDirective metadata.isFieldExternal(field)) - .forEach((field) => usedExternalCoordinates.add(field.coordinate)); + .forEach((field) => usedExternalCoordinates.add(field)); } } @@ -332,13 +332,12 @@ function collectUsedExternaFieldsForDirective) => boolean; private _keysPredicate?: (field: FieldDefinition) => boolean; - private _syntheticExternals: Set = new Set(); + private _fieldUsedPredicate?: (field: FieldDefinition) => boolean; private _isFed2Schema?: boolean; constructor(readonly schema: Schema) { @@ -443,6 +442,7 @@ export class FederationMetadata { this._sharingPredicate = undefined; this._keysPredicate = undefined; this._isFed2Schema = undefined; + this._fieldUsedPredicate = undefined; } isFed2Schema(): boolean { @@ -473,6 +473,18 @@ export class FederationMetadata { return this._keysPredicate; } + private fieldUsedPredicate(): (field: FieldDefinition) => boolean { + if (!this._fieldUsedPredicate) { + const usedFields = Array.from(collectUsedFields(this)); + this._fieldUsedPredicate = (field: FieldDefinition) => !!usedFields.find((f) => f.coordinate === field.coordinate); + } + return this._fieldUsedPredicate; + } + + isFieldUsed(field: FieldDefinition): boolean { + return this.fieldUsedPredicate()(field); + } + isFieldExternal(field: FieldDefinition | InputFieldDefinition) { return this.externalTester().isExternal(field); } @@ -501,14 +513,6 @@ export class FederationMetadata { return this.keysPredicate()(field); } - addSyntheticExternal(coordinate: string) { - this._syntheticExternals.add(coordinate); - } - - isSyntheticExternal(field: FieldDefinition): boolean { - return this._syntheticExternals.has(field.coordinate); - } - federationDirectiveNameInSchema(name: string): string { if (this.isFed2Schema()) { const coreFeatures = this.schema.coreFeatures; diff --git a/internals-js/src/schemaUpgrader.ts b/internals-js/src/schemaUpgrader.ts index d09687281c..f5c32d830d 100644 --- a/internals-js/src/schemaUpgrader.ts +++ b/internals-js/src/schemaUpgrader.ts @@ -24,7 +24,6 @@ import { import { addSubgraphToError, collectTargetFields, - collectUsedExternalFieldsCoordinates, federationMetadata, FederationMetadata, printSubgraphNames, @@ -43,7 +42,7 @@ type UpgradeChanges = MultiMap; export type UpgradeSuccess = { subgraphs: Subgraphs, changes: Map, - errors?: never, + errors?: never, } export type UpgradeFailure = { @@ -235,7 +234,7 @@ export function upgradeSubgraphsIfNecessary(inputs: Subgraphs): UpgradeResult { * 2. do not have a definition for the same type in the same subgraph (this is a GraphQL extension otherwise). * * Not that type extensions in federation 1 generally have a @key but in really the code consider something a type extension even without - * it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here. + * it (which I'd argue is a unintended bug of fed1 since this leads to various problems) so we don't check for the presence of @key here. */ function isFederationTypeExtension(type: NamedType): boolean { const metadata = federationMetadata(type.schema()); @@ -522,7 +521,7 @@ class SchemaUpgrader { continue; } assert(isCompositeType(typeInOther), () => `Type ${type} is of kind ${type.kind} in ${this.subgraph.name} but ${typeInOther.kind} in ${other.name}`); - const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective()); + const keysInOther = typeInOther.appliedDirectivesOf(other.metadata().keyDirective()); if (keysInOther.length === 0) { continue; } @@ -564,14 +563,12 @@ class SchemaUpgrader { } private removeUnusedExternals() { - const usedExternalFieldsCoordinates = collectUsedExternalFieldsCoordinates(this.metadata); - for (const type of this.schema.types()) { if (!isObjectType(type) && !isInterfaceType(type)) { continue; } for (const field of type.fields()) { - if (this.metadata.isFieldExternal(field) && !usedExternalFieldsCoordinates.has(field.coordinate)) { + if (this.metadata.isFieldExternal(field) && !this.metadata.isFieldUsed(field)) { this.addChange(new UnusedExternalRemoval(field.coordinate)); field.remove(); }