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..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 @@ -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..55693724b 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", @@ -125,7 +127,10 @@ describe("composition involving @override directive", () => { type A @key(fields: "id") { id: ID! b: B @override(from: "Subgraph2") + z: String! @shareable + z2: String! @shareable } + type B @key(fields: "id") { id: ID! v: String @shareable @@ -143,11 +148,15 @@ 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") + 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! @@ -168,6 +177,8 @@ 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) }" `); @@ -179,7 +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: \\"b { v }\\") + 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\\") }" `); }); @@ -974,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 } @@ -983,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 b967065b7..bdd77643e 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 { @@ -379,15 +381,21 @@ class Merger { 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>; 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(); + + const { overriddenFieldMap, fieldsWithOverride } = this.getFieldsWithOverrideDirective(); + this.fieldsWithOverride = fieldsWithOverride; + this.completelyOverriddenFieldMap = overriddenFieldMap; + this.composeDirectiveManager = new ComposeDirectiveManager( this.subgraphs, (error: GraphQLError) => { this.errors.push(error) }, @@ -2045,10 +2053,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 = 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: this.getFieldSet(source, sourceMeta.providesDirective()), + 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, @@ -2058,6 +2072,69 @@ class Merger { }); } } + + private removeOverriddenFieldFromFieldSet | InputFieldDefinition>( + source: T, + overriddenCoordinates: Set | undefined, + 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 (!overriddenCoordinates) { + return fields; + } + + 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.has(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 +3608,34 @@ 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(): { 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)) { + for (const application of directive.applications()) { + const field = application.parent; + assert(isFieldDefinition(field), () => `Expected ${application.parent} to be a field`); + const coordinate = field.coordinate; + 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 { + fieldsWithOverride, + overriddenFieldMap, + }; } private getFieldsWithAppliedDirective( 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/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/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',