From 7095708e94fa4ba5c5432b6c1d56e11b30ec9264 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Wed, 11 Dec 2024 16:45:07 -0600 Subject: [PATCH 1/5] When we see that `@provides` specifies an overridden field, remove it from the field selection. --- .changeset/twenty-plants-rescue.md | 6 + composition-js/src/__tests__/compose.test.ts | 34 +++++ .../src/__tests__/override.compose.test.ts | 7 +- composition-js/src/merging/merge.ts | 119 ++++++++++++++++-- .../src/__tests__/subgraphValidation.test.ts | 38 ------ internals-js/src/federation.ts | 23 ---- 6 files changed, 153 insertions(+), 74 deletions(-) create mode 100644 .changeset/twenty-plants-rescue.md diff --git a/.changeset/twenty-plants-rescue.md b/.changeset/twenty-plants-rescue.md new file mode 100644 index 000000000..9f3ed103b --- /dev/null +++ b/.changeset/twenty-plants-rescue.md @@ -0,0 +1,6 @@ +--- +"@apollo/composition": patch +"@apollo/federation-internals": patch +--- + +When `@provides` specifies an overridden field, remove it from the supergraph's selection set so that data is retrieved from the correct subgraph diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index e0a538fab..436f0e9c5 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -5290,3 +5290,37 @@ describe('@source* directives', () => { assertCompositionSuccess(result); }); }); + +it('errors on unused @external', () => { + const subgraphA = { + name: 'S', + typeDefs: gql` + type Query { + T: T! + } + + type T { + f: Int @external + } + `, + }; + + const subgraphB = { + name: 'T', + typeDefs: gql` + type Query { + a: Int! + } + + type T { + f: Int + } + `, + }; + + const result = composeAsFed2Subgraphs([subgraphA, subgraphB]); + expect(result.errors).toBeDefined(); + expect(errors(result)).toStrictEqual([ + ['EXTERNAL_UNUSED', '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).'] + ]); +}); diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts index 6f53f20a2..36973b0d2 100644 --- a/composition-js/src/__tests__/override.compose.test.ts +++ b/composition-js/src/__tests__/override.compose.test.ts @@ -125,6 +125,7 @@ describe("composition involving @override directive", () => { type A @key(fields: "id") { id: ID! b: B @override(from: "Subgraph2") + z: String! @shareable } type B @key(fields: "id") { id: ID! @@ -143,11 +144,12 @@ describe("composition involving @override directive", () => { typeDefs: gql` type T @key(fields: "k") { k: ID - a: A @shareable @provides(fields: "b { v }") + a: A @shareable @provides(fields: "b { v } z") } type A @key(fields: "id") { id: ID! b: B + z: String! @external } type B @key(fields: "id") { id: ID! @@ -168,6 +170,7 @@ describe("composition involving @override directive", () => { { id: ID! b: B @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) + z: String! @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, external: true) }" `); @@ -179,7 +182,7 @@ describe("composition involving @override directive", () => { @join__type(graph: SUBGRAPH2, key: \\"k\\") { k: ID - a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"b { v }\\") + a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"z\\") }" `); }); diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index b967065b7..ea41cbfbd 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -83,6 +83,8 @@ import { FeatureDefinition, CoreImport, inaccessibleIdentity, + parseSelectionSet, + isUnionType, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -378,16 +380,16 @@ class Merger { private joinDirectiveIdentityURLs = new Set(); private schemaToImportNameToFeatureUrl = new Map>(); private fieldsWithFromContext: Set; - private fieldsWithOverride: Set; + private fieldsWithOverride: Map; // map of field coordinate to index of overridden subgraph constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) { + this.names = subgraphs.names(); this.latestFedVersionUsed = this.getLatestFederationVersionUsed(); this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.fieldsWithFromContext = this.getFieldsWithFromContextDirective(); this.fieldsWithOverride = this.getFieldsWithOverrideDirective(); - this.names = subgraphs.names(); this.composeDirectiveManager = new ComposeDirectiveManager( this.subgraphs, (error: GraphQLError) => { this.errors.push(error) }, @@ -399,6 +401,8 @@ class Merger { (hint: CompositionHint) => { this.hints.push(hint); }, ); + this.validateAllExternalFieldsUsed(); + this.subgraphsSchema = subgraphs.values().map(({ schema }) => { if (!this.schemaToImportNameToFeatureUrl.has(schema)) { this.schemaToImportNameToFeatureUrl.set( @@ -2045,10 +2049,16 @@ class Merger { const external = this.isExternal(idx, source); const sourceMeta = this.subgraphs.values()[idx].metadata(); const name = this.joinSpecName(idx); + + // fields in this subgraph that are no longer viable because they've been overridden + const overriddenFields = Array.from(this.fieldsWithOverride.entries()) + .filter(([_, index]) => idx === index) + .map(([name, _]) => name); + const providesFieldSet = this.removeOverriddenFieldFromFieldSet(source, overriddenFields, sourceMeta); dest.applyDirective(joinFieldDirective, { graph: name, requires: this.getFieldSet(source, sourceMeta.requiresDirective()), - provides: this.getFieldSet(source, sourceMeta.providesDirective()), + provides: providesFieldSet, override: source.appliedDirectivesOf(sourceMeta.overrideDirective()).pop()?.arguments()?.from, type: allTypesEqual ? undefined : source.type?.toString(), external: external ? true : undefined, @@ -2058,6 +2068,64 @@ class Merger { }); } } + + private removeOverriddenFieldFromFieldSet | InputFieldDefinition>( + source: T, + overriddenCoordinates: string[], + sourceMeta: FederationMetadata, + ) { + const providesDirective = sourceMeta.providesDirective(); + const applications = source.appliedDirectivesOf(providesDirective); + assert(applications.length <= 1, () => `Found more than one application of ${providesDirective} on ${source}`); + if (applications.length === 0) { + return undefined; + } + const fields: string = applications[0].arguments().fields; + const parent = applications[0].parent; + const parentType = baseType(parent.type!); + if (!isCompositeType(parentType)) { + return undefined; + } + + // when we parse the selection set, we will have a list of selections. Anything that eventually goes to a overridden field + // cannot be considered valid in the supgraph + let validSelectionsIndex: boolean[] = []; + + const selectionSet = parseSelectionSet({ + parentType, + source: fields, + fieldAccessor: (t, f) => { + const field = t.field(f); + + // Every time we get back to to seeing the parentType, we know that it is a new selection. + // Assume it's valid until we see a contradiction + if (t.name === parentType.name) { + validSelectionsIndex.push(true); + } else if (isInterfaceType(parentType)) { + if (parentType.allImplementations().find(member => member.name === t.name)) { + validSelectionsIndex.push(true); + } + } else if (isUnionType(parentType)) { + if (parentType.members().find(member => t.name === member.type.name)) { + validSelectionsIndex.push(true); + } + } + assert(field, 'field should exist on type'); + if (overriddenCoordinates.includes(field.coordinate)) { + validSelectionsIndex[validSelectionsIndex.length-1] = false; + } + return field; + } + }); + + // now we should have a SelectionSet with an array of _selections the same length as `validSelectionsIndex` + // We should be able to filter out the ones that are invalid and we'll be left with the valid selection string + const selections = selectionSet.selections(); + assert(selections.length === validSelectionsIndex.length, 'selection parsing failed'); + const filteredSelections = selections.filter((_, index) => validSelectionsIndex[index]); + return filteredSelections.length > 0 ? filteredSelections.map(selection => selection.toString()).join(' ') : undefined; + } + private getFieldSet(element: SchemaElement, directive: DirectiveDefinition<{fields: string}>): string | undefined { const applications = element.appliedDirectivesOf(directive); assert(applications.length <= 1, () => `Found more than one application of ${directive} on ${element}`); @@ -3531,15 +3599,23 @@ class Merger { ); } - private getFieldsWithOverrideDirective(): Set { - return this.getFieldsWithAppliedDirective( - (subgraph: Subgraph) => subgraph.metadata().overrideDirective(), - (application: Directive>) => { - const field = application.parent; - assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`); - return field; + private getFieldsWithOverrideDirective(): Map { + const fields = new Map(); + for (const subgraph of this.subgraphs) { + const directive = subgraph.metadata().overrideDirective(); + if (isFederationDirectiveDefinedInSchema(directive)) { + for (const application of directive.applications()) { + const field = application.parent; + assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`); + const coordinate = field.coordinate; + if (!fields.has(coordinate)) { + const fromSubgraphName = application.arguments()['from']; + fields.set(coordinate, this.names.indexOf(fromSubgraphName)); + } + } } - ); + } + return fields; } private getFieldsWithAppliedDirective( @@ -3561,4 +3637,25 @@ class Merger { } return fields; } + + private validateAllExternalFieldsUsed(): void { + for (const subgraph of this.subgraphs) { + const metadata = subgraph.metadata(); + for (const type of metadata.schema.types()) { + if (!isObjectType(type) && !isInterfaceType(type)) { + continue; + } + for (const field of type.fields()) { + if (!metadata.isFieldExternal(field) || metadata.isFieldUsed(field)) { + continue; + } + this.mismatchReporter.pushError(ERRORS.EXTERNAL_UNUSED.err( + `[${subgraph.name}] Field "${field.coordinate}" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface;` + + ' the field declaration has no use and should be removed (or the field should not be @external).', + { nodes: field.sourceAST }, + )); + } + } + } + } } diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 99c5bfdee..8d2430540 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -148,24 +148,6 @@ describe('fieldset-based directives', () => { ]); }); - it('rejects unused @external', () => { - const subgraph = gql` - type Query { - t: T - } - - type T { - f: Int @external - } - `; - expect(buildForErrors(subgraph)).toStrictEqual([ - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], - ]); - }); - it('rejects @provides on non-object fields', () => { const subgraph = gql` type Query { @@ -220,10 +202,6 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS_TYPE', '[S] On field "Query.t", for @provides(fields: ["f"]): Invalid value for argument "fields": must be a string.', ], - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], ]); }); @@ -246,10 +224,6 @@ describe('fieldset-based directives', () => { 'REQUIRES_INVALID_FIELDS_TYPE', '[S] On field "T.g", for @requires(fields: ["f"]): Invalid value for argument "fields": must be a string.', ], - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], ]); }); @@ -293,10 +267,6 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS_TYPE', '[S] On field "Query.t", for @provides(fields: f): Invalid value for argument "fields": must be a string.', ], - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], ]); }); @@ -321,10 +291,6 @@ describe('fieldset-based directives', () => { 'REQUIRES_INVALID_FIELDS_TYPE', '[S] On field "T.g", for @requires(fields: f): Invalid value for argument "fields": must be a string.', ], - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], ]); }); @@ -361,10 +327,6 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS', '[S] On field "Query.t", for @provides(fields: "{{f}}"): Syntax Error: Expected Name, found "{".', ], - [ - 'EXTERNAL_UNUSED', - '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', - ], ]); }); diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index 8faa98082..25cbd2704 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -919,28 +919,6 @@ function collectUsedFieldsForDirective>( } } -/** - * Checks that all fields marked @external is used in a federation directive (@key, @provides or @requires) _or_ to satisfy an - * interface implementation. Otherwise, the field declaration is somewhat useless. - */ -function validateAllExternalFieldsUsed(metadata: FederationMetadata, errorCollector: GraphQLError[]): void { - for (const type of metadata.schema.types()) { - if (!isObjectType(type) && !isInterfaceType(type)) { - continue; - } - for (const field of type.fields()) { - if (!metadata.isFieldExternal(field) || metadata.isFieldUsed(field)) { - continue; - } - errorCollector.push(ERRORS.EXTERNAL_UNUSED.err( - `Field "${field.coordinate}" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface;` - + ' the field declaration has no use and should be removed (or the field should not be @external).', - { nodes: field.sourceAST }, - )); - } - } -} - function validateNoExternalOnInterfaceFields(metadata: FederationMetadata, errorCollector: GraphQLError[]) { for (const itf of metadata.schema.interfaceTypes()) { for (const field of itf.fields()) { @@ -1816,7 +1794,6 @@ export class FederationBlueprint extends SchemaBlueprint { } validateNoExternalOnInterfaceFields(metadata, errorCollector); - validateAllExternalFieldsUsed(metadata, errorCollector); validateKeyOnInterfacesAreAlsoOnAllImplementations(metadata, errorCollector); validateInterfaceObjectsAreOnEntities(metadata, errorCollector); From 214304c98e71b8ae0275e5a3818abac86a90e10d Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 12 Dec 2024 09:29:51 -0600 Subject: [PATCH 2/5] make change so that completely overridden fields are precomputed --- composition-js/src/merging/merge.ts | 48 ++++++++++++++++++++--------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index ea41cbfbd..70963dcd9 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -380,7 +380,10 @@ class Merger { private joinDirectiveIdentityURLs = new Set(); private schemaToImportNameToFeatureUrl = new Map>(); private fieldsWithFromContext: Set; - private fieldsWithOverride: Map; // map of field coordinate to index of overridden subgraph + + private fieldsWithOverride: Set; + // a map from the subgraph index to a list of coordinates that are completely overridden by another subgraph + private completelyOverriddenFieldMap: Map>; constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) { this.names = subgraphs.names(); @@ -388,8 +391,11 @@ class Merger { this.joinSpec = JOIN_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.linkSpec = LINK_VERSIONS.getMinimumRequiredVersion(this.latestFedVersionUsed); this.fieldsWithFromContext = this.getFieldsWithFromContextDirective(); - this.fieldsWithOverride = this.getFieldsWithOverrideDirective(); - + + const { overriddenFieldMap, fieldsWithOverride } = this.getFieldsWithOverrideDirective(); + this.fieldsWithOverride = fieldsWithOverride; + this.completelyOverriddenFieldMap = overriddenFieldMap; + this.composeDirectiveManager = new ComposeDirectiveManager( this.subgraphs, (error: GraphQLError) => { this.errors.push(error) }, @@ -2051,9 +2057,7 @@ class Merger { const name = this.joinSpecName(idx); // fields in this subgraph that are no longer viable because they've been overridden - const overriddenFields = Array.from(this.fieldsWithOverride.entries()) - .filter(([_, index]) => idx === index) - .map(([name, _]) => name); + const overriddenFields = this.completelyOverriddenFieldMap.get(idx); const providesFieldSet = this.removeOverriddenFieldFromFieldSet(source, overriddenFields, sourceMeta); dest.applyDirective(joinFieldDirective, { graph: name, @@ -2071,7 +2075,7 @@ class Merger { private removeOverriddenFieldFromFieldSet | InputFieldDefinition>( source: T, - overriddenCoordinates: string[], + overriddenCoordinates: Set | undefined, sourceMeta: FederationMetadata, ) { const providesDirective = sourceMeta.providesDirective(); @@ -2083,6 +2087,11 @@ class Merger { const fields: string = applications[0].arguments().fields; const parent = applications[0].parent; const parentType = baseType(parent.type!); + + if (!overriddenCoordinates) { + return fields; + } + if (!isCompositeType(parentType)) { return undefined; } @@ -2111,7 +2120,7 @@ class Merger { } } assert(field, 'field should exist on type'); - if (overriddenCoordinates.includes(field.coordinate)) { + if (overriddenCoordinates.has(field.coordinate)) { validSelectionsIndex[validSelectionsIndex.length-1] = false; } return field; @@ -3599,8 +3608,9 @@ class Merger { ); } - private getFieldsWithOverrideDirective(): Map { - const fields = new Map(); + private getFieldsWithOverrideDirective(): { fieldsWithOverride: Set, overriddenFieldMap: Map> } { + const overriddenFieldMap = new Map>(); + const fieldsWithOverride = new Set(); for (const subgraph of this.subgraphs) { const directive = subgraph.metadata().overrideDirective(); if (isFederationDirectiveDefinedInSchema(directive)) { @@ -3608,14 +3618,24 @@ class Merger { const field = application.parent; assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`); const coordinate = field.coordinate; - if (!fields.has(coordinate)) { - const fromSubgraphName = application.arguments()['from']; - fields.set(coordinate, this.names.indexOf(fromSubgraphName)); + const { from: fromSubgraphName, label } = application.arguments(); + fieldsWithOverride.add(coordinate); + + // we only want fields that are completely overridden (i.e. progressive overrides will have a label and we should ignore them) + if (!label) { + const fromSubgraphIndex = this.names.indexOf(fromSubgraphName); + if (!overriddenFieldMap.has(fromSubgraphIndex)) { + overriddenFieldMap.set(fromSubgraphIndex, new Set()); + } + overriddenFieldMap.get(fromSubgraphIndex)?.add(coordinate); } } } } - return fields; + return { + fieldsWithOverride, + overriddenFieldMap, + }; } private getFieldsWithAppliedDirective( From 22871eeeb05550c52cc5c0d49799a310d7a179f6 Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Thu, 12 Dec 2024 09:46:15 -0600 Subject: [PATCH 3/5] adding to the @provides test --- composition-js/src/__tests__/override.compose.test.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts index 36973b0d2..e6aed5d18 100644 --- a/composition-js/src/__tests__/override.compose.test.ts +++ b/composition-js/src/__tests__/override.compose.test.ts @@ -110,6 +110,8 @@ describe("composition involving @override directive", () => { `); }); + // @provides may not provide a value when the field is completely overridden in the local subgraph + // when that happens, the selection should be removed from the field set it("override field in a @provides", () => { const subgraph1 = { name: "Subgraph1", @@ -126,7 +128,9 @@ describe("composition involving @override directive", () => { id: ID! b: B @override(from: "Subgraph2") z: String! @shareable + z2: String! @shareable } + type B @key(fields: "id") { id: ID! v: String @shareable @@ -145,11 +149,14 @@ describe("composition involving @override directive", () => { type T @key(fields: "k") { k: ID a: A @shareable @provides(fields: "b { v } z") + a2: A @shareable @provides(fields: "b { v }") + a3: A @shareable @provides(fields: "z b { v } z2") } type A @key(fields: "id") { id: ID! b: B z: String! @external + z2: String! @external } type B @key(fields: "id") { id: ID! @@ -171,6 +178,7 @@ describe("composition involving @override directive", () => { id: ID! b: B @join__field(graph: SUBGRAPH1, override: \\"Subgraph2\\") @join__field(graph: SUBGRAPH2, usedOverridden: true) z: String! @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, external: true) + z2: String! @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, external: true) }" `); @@ -183,6 +191,8 @@ describe("composition involving @override directive", () => { { k: ID a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"z\\") + a2: A @join__field(graph: SUBGRAPH2) + a3: A @join__field(graph: SUBGRAPH2, provides: \\"z z2\\") }" `); }); From 9cc50f72789c9d3114c0fdbadd20036a952d7b6a Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Fri, 24 Jan 2025 10:53:39 -0600 Subject: [PATCH 4/5] Revert validate @external back to subgraph validation and add new parameter to join spec --- composition-js/src/__tests__/compose.test.ts | 8 ++-- .../src/__tests__/override.compose.test.ts | 10 ++--- composition-js/src/merging/merge.ts | 27 ++----------- .../__tests__/gateway/lifecycle-hooks.test.ts | 2 +- .../extractSubgraphsFromSupergraph.test.ts | 4 +- .../src/__tests__/subgraphValidation.test.ts | 38 +++++++++++++++++++ .../src/extractSubgraphsFromSupergraph.ts | 2 +- internals-js/src/federation.ts | 23 +++++++++++ internals-js/src/specs/joinSpec.ts | 8 +++- internals-js/src/supergraphs.ts | 2 + 10 files changed, 86 insertions(+), 38 deletions(-) diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index 436f0e9c5..7ed9386a0 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -70,7 +70,7 @@ describe('composition', () => { expect(result.supergraphSdl).toMatchString(` schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) + @link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION) { query: Query } @@ -79,7 +79,7 @@ describe('composition', () => { directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE @@ -231,7 +231,7 @@ describe('composition', () => { expect(result.supergraphSdl).toMatchString(` schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) + @link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION) { query: Query } @@ -240,7 +240,7 @@ describe('composition', () => { directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts index e6aed5d18..55693724b 100644 --- a/composition-js/src/__tests__/override.compose.test.ts +++ b/composition-js/src/__tests__/override.compose.test.ts @@ -190,9 +190,9 @@ describe("composition involving @override directive", () => { @join__type(graph: SUBGRAPH2, key: \\"k\\") { k: ID - a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"z\\") - a2: A @join__field(graph: SUBGRAPH2) - a3: A @join__field(graph: SUBGRAPH2, provides: \\"z z2\\") + a: A @join__field(graph: SUBGRAPH1) @join__field(graph: SUBGRAPH2, provides: \\"z\\", originalProvides: \\"b { v } z\\") + a2: A @join__field(graph: SUBGRAPH2, originalProvides: \\"b { v }\\") + a3: A @join__field(graph: SUBGRAPH2, provides: \\"z z2\\", originalProvides: \\"z b { v } z2\\") }" `); }); @@ -987,7 +987,7 @@ describe("composition involving @override directive", () => { expect(result.supergraphSdl).toMatchInlineSnapshot(` "schema @link(url: \\"https://specs.apollo.dev/link/v1.0\\") - @link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION) + @link(url: \\"https://specs.apollo.dev/join/v0.6\\", for: EXECUTION) { query: Query } @@ -996,7 +996,7 @@ describe("composition involving @override directive", () => { directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE - directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 70963dcd9..65ea3e272 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -406,8 +406,6 @@ class Merger { (error: GraphQLError) => { this.errors.push(error); }, (hint: CompositionHint) => { this.hints.push(hint); }, ); - - this.validateAllExternalFieldsUsed(); this.subgraphsSchema = subgraphs.values().map(({ schema }) => { if (!this.schemaToImportNameToFeatureUrl.has(schema)) { @@ -2058,11 +2056,13 @@ class Merger { // fields in this subgraph that are no longer viable because they've been overridden const overriddenFields = this.completelyOverriddenFieldMap.get(idx); + const originalProvides = this.getFieldSet(source, sourceMeta.providesDirective()); const providesFieldSet = this.removeOverriddenFieldFromFieldSet(source, overriddenFields, sourceMeta); dest.applyDirective(joinFieldDirective, { graph: name, requires: this.getFieldSet(source, sourceMeta.requiresDirective()), provides: providesFieldSet, + originalProvides: providesFieldSet === originalProvides ? undefined : originalProvides, override: source.appliedDirectivesOf(sourceMeta.overrideDirective()).pop()?.arguments()?.from, type: allTypesEqual ? undefined : source.type?.toString(), external: external ? true : undefined, @@ -3657,25 +3657,4 @@ class Merger { } return fields; } - - private validateAllExternalFieldsUsed(): void { - for (const subgraph of this.subgraphs) { - const metadata = subgraph.metadata(); - for (const type of metadata.schema.types()) { - if (!isObjectType(type) && !isInterfaceType(type)) { - continue; - } - for (const field of type.fields()) { - if (!metadata.isFieldExternal(field) || metadata.isFieldUsed(field)) { - continue; - } - this.mismatchReporter.pushError(ERRORS.EXTERNAL_UNUSED.err( - `[${subgraph.name}] Field "${field.coordinate}" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface;` - + ' the field declaration has no use and should be removed (or the field should not be @external).', - { nodes: field.sourceAST }, - )); - } - } - } - } -} + \ No newline at end of file diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index 43b65e309..e5194a528 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts @@ -149,7 +149,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).toMatchInlineSnapshot( - `"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`, + `"2d9e498fd22c9fab2bda597f91b67d289b7edb01ec5c245f0527d3699d15bddb"`, ); // second call should have previous info in the second arg expect(secondCall[1]!.compositionId).toEqual(expectedFirstId); diff --git a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts index 955f1c823..bee5e5e18 100644 --- a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts +++ b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts @@ -821,7 +821,7 @@ test('contextual arguments can be extracted', () => { const supergraph = ` schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) + @link(url: "https://specs.apollo.dev/join/v0.6", for: EXECUTION) @link(url: "https://specs.apollo.dev/context/v0.1") { query: Query @@ -833,7 +833,7 @@ directive @join__graph(name: String!, url: String!) on ENUM_VALUE directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR -directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!], originalProvides: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index 8d2430540..99c5bfdee 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -148,6 +148,24 @@ describe('fieldset-based directives', () => { ]); }); + it('rejects unused @external', () => { + const subgraph = gql` + type Query { + t: T + } + + type T { + f: Int @external + } + `; + expect(buildForErrors(subgraph)).toStrictEqual([ + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], + ]); + }); + it('rejects @provides on non-object fields', () => { const subgraph = gql` type Query { @@ -202,6 +220,10 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS_TYPE', '[S] On field "Query.t", for @provides(fields: ["f"]): Invalid value for argument "fields": must be a string.', ], + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], ]); }); @@ -224,6 +246,10 @@ describe('fieldset-based directives', () => { 'REQUIRES_INVALID_FIELDS_TYPE', '[S] On field "T.g", for @requires(fields: ["f"]): Invalid value for argument "fields": must be a string.', ], + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], ]); }); @@ -267,6 +293,10 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS_TYPE', '[S] On field "Query.t", for @provides(fields: f): Invalid value for argument "fields": must be a string.', ], + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], ]); }); @@ -291,6 +321,10 @@ describe('fieldset-based directives', () => { 'REQUIRES_INVALID_FIELDS_TYPE', '[S] On field "T.g", for @requires(fields: f): Invalid value for argument "fields": must be a string.', ], + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], ]); }); @@ -327,6 +361,10 @@ describe('fieldset-based directives', () => { 'PROVIDES_INVALID_FIELDS', '[S] On field "Query.t", for @provides(fields: "{{f}}"): Syntax Error: Expected Name, found "{".', ], + [ + 'EXTERNAL_UNUSED', + '[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external).', + ], ]); }); diff --git a/internals-js/src/extractSubgraphsFromSupergraph.ts b/internals-js/src/extractSubgraphsFromSupergraph.ts index 421c66bee..fd06f25bd 100644 --- a/internals-js/src/extractSubgraphsFromSupergraph.ts +++ b/internals-js/src/extractSubgraphsFromSupergraph.ts @@ -706,7 +706,7 @@ function addSubgraphField({ subgraphField.applyDirective(subgraph.metadata().requiresDirective(), {'fields': joinFieldArgs.requires}); } if (joinFieldArgs?.provides) { - subgraphField.applyDirective(subgraph.metadata().providesDirective(), {'fields': joinFieldArgs.provides}); + subgraphField.applyDirective(subgraph.metadata().providesDirective(), {'fields': joinFieldArgs.originalProvides ?? joinFieldArgs.provides}); } if (joinFieldArgs?.contextArguments) { const fromContextDirective = subgraph.metadata().fromContextDirective(); diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index 25cbd2704..8faa98082 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -919,6 +919,28 @@ function collectUsedFieldsForDirective>( } } +/** + * Checks that all fields marked @external is used in a federation directive (@key, @provides or @requires) _or_ to satisfy an + * interface implementation. Otherwise, the field declaration is somewhat useless. + */ +function validateAllExternalFieldsUsed(metadata: FederationMetadata, errorCollector: GraphQLError[]): void { + for (const type of metadata.schema.types()) { + if (!isObjectType(type) && !isInterfaceType(type)) { + continue; + } + for (const field of type.fields()) { + if (!metadata.isFieldExternal(field) || metadata.isFieldUsed(field)) { + continue; + } + errorCollector.push(ERRORS.EXTERNAL_UNUSED.err( + `Field "${field.coordinate}" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface;` + + ' the field declaration has no use and should be removed (or the field should not be @external).', + { nodes: field.sourceAST }, + )); + } + } +} + function validateNoExternalOnInterfaceFields(metadata: FederationMetadata, errorCollector: GraphQLError[]) { for (const itf of metadata.schema.interfaceTypes()) { for (const field of itf.fields()) { @@ -1794,6 +1816,7 @@ export class FederationBlueprint extends SchemaBlueprint { } validateNoExternalOnInterfaceFields(metadata, errorCollector); + validateAllExternalFieldsUsed(metadata, errorCollector); validateKeyOnInterfacesAreAlsoOnAllImplementations(metadata, errorCollector); validateInterfaceObjectsAreOnEntities(metadata, errorCollector); diff --git a/internals-js/src/specs/joinSpec.ts b/internals-js/src/specs/joinSpec.ts index bd45fa212..26ae6ce28 100644 --- a/internals-js/src/specs/joinSpec.ts +++ b/internals-js/src/specs/joinSpec.ts @@ -44,6 +44,7 @@ export type JoinFieldDirectiveArguments = { graph?: string, requires?: string, provides?: string, + originalProvides?: string, override?: string, type?: string, external?: boolean, @@ -176,6 +177,10 @@ export class JoinSpecDefinition extends FeatureDefinition { joinField.addArgument('contextArguments', new ListType(new NonNullType(contextArgumentsType))); } + + if (this.version.gte(new FeatureVersion(0, 6))) { + joinField.addArgument('originalProvides', schema.stringType()); + } if (this.isV01()) { const joinOwner = this.addDirective(schema, 'owner').addLocations(DirectiveLocation.OBJECT); @@ -289,6 +294,7 @@ export const JOIN_VERSIONS = new FeatureDefinitions(joinIden .add(new JoinSpecDefinition(new FeatureVersion(0, 2))) .add(new JoinSpecDefinition(new FeatureVersion(0, 3), new FeatureVersion(2, 0))) .add(new JoinSpecDefinition(new FeatureVersion(0, 4), new FeatureVersion(2, 7))) - .add(new JoinSpecDefinition(new FeatureVersion(0, 5), new FeatureVersion(2, 8))); + .add(new JoinSpecDefinition(new FeatureVersion(0, 5), new FeatureVersion(2, 8))) + .add(new JoinSpecDefinition(new FeatureVersion(0, 6), new FeatureVersion(2, 9))); registerKnownFeature(JOIN_VERSIONS); diff --git a/internals-js/src/supergraphs.ts b/internals-js/src/supergraphs.ts index da4d52751..e738365d0 100644 --- a/internals-js/src/supergraphs.ts +++ b/internals-js/src/supergraphs.ts @@ -17,6 +17,7 @@ export const DEFAULT_SUPPORTED_SUPERGRAPH_FEATURES = new Set([ 'https://specs.apollo.dev/join/v0.3', 'https://specs.apollo.dev/join/v0.4', 'https://specs.apollo.dev/join/v0.5', + 'https://specs.apollo.dev/join/v0.6', 'https://specs.apollo.dev/tag/v0.1', 'https://specs.apollo.dev/tag/v0.2', 'https://specs.apollo.dev/tag/v0.3', @@ -32,6 +33,7 @@ export const ROUTER_SUPPORTED_SUPERGRAPH_FEATURES = new Set([ 'https://specs.apollo.dev/join/v0.3', 'https://specs.apollo.dev/join/v0.4', 'https://specs.apollo.dev/join/v0.5', + 'https://specs.apollo.dev/join/v0.6', 'https://specs.apollo.dev/tag/v0.1', 'https://specs.apollo.dev/tag/v0.2', 'https://specs.apollo.dev/tag/v0.3', From 547a3fa2af76743f82317eddfc58d3572d8da22d Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Fri, 24 Jan 2025 10:58:01 -0600 Subject: [PATCH 5/5] whitespace --- composition-js/src/merging/merge.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 65ea3e272..bdd77643e 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -380,8 +380,8 @@ class Merger { private joinDirectiveIdentityURLs = new Set(); private schemaToImportNameToFeatureUrl = new Map>(); private fieldsWithFromContext: Set; - private fieldsWithOverride: Set; + // a map from the subgraph index to a list of coordinates that are completely overridden by another subgraph private completelyOverriddenFieldMap: Map>; @@ -406,7 +406,7 @@ class Merger { (error: GraphQLError) => { this.errors.push(error); }, (hint: CompositionHint) => { this.hints.push(hint); }, ); - + this.subgraphsSchema = subgraphs.values().map(({ schema }) => { if (!this.schemaToImportNameToFeatureUrl.has(schema)) { this.schemaToImportNameToFeatureUrl.set( @@ -3657,4 +3657,4 @@ class Merger { } return fields; } - \ No newline at end of file +}