diff --git a/.changeset/giant-teachers-divide.md b/.changeset/giant-teachers-divide.md new file mode 100644 index 000000000..a1b5aef2f --- /dev/null +++ b/.changeset/giant-teachers-divide.md @@ -0,0 +1,12 @@ +--- +"@apollo/composition": patch +--- + +Propagate directives from @interfaceObject fields to @external implementations + +When an implementation re-declares a field as `@external` (e.g. to reference it in `@requires`), the field's only +resolvable definition lives on the abstracting `@interfaceObject`. Directives like `@tag` applied there were not +being propagated to the implementation's copy in the supergraph. + +During `add_interface_object_fields`, detect implementation fields where every `@join__field` is `external: true` and +the field is provided by an `@interfaceObject`, then copy applicable directives onto the implementation field. diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index f078ac218..246383531 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -4214,6 +4214,53 @@ describe('composition', () => { result = composeAsFed2Subgraphs([subgraph2, subgraph1]); assertCompositionSuccess(result); }) + + it('propagates @tag from @interfaceObject field to locally @external implementation field', () => { + const subgraphA = { + typeDefs: gql` + type Query { + product: Product + } + + type Product @key(fields: "id") @interfaceObject { + id: ID! + name: String @tag(name: "custom") + } + `, + name: 'subgraphA', + }; + + const subgraphB = { + typeDefs: gql` + type Query { + book: Book + } + + interface Product @key(fields: "id") { + id: ID! + } + + type Book implements Product @key(fields: "id") { + id: ID! + name: String @external + description: String @requires(fields: "name") + } + `, + name: 'subgraphB', + }; + + const result = composeAsFed2Subgraphs([subgraphA, subgraphB]); + assertCompositionSuccess(result); + + const supergraph = result.schema; + const bookType = supergraph.type('Book'); + assert(bookType && isObjectType(bookType), 'Book should be an object type'); + const nameField = bookType.field('name'); + expect(nameField).toBeDefined(); + const tags = nameField!.appliedDirectivesOf('tag'); + expect(tags).toHaveLength(1); + expect(tags[0].arguments()['name']).toBe('custom'); + }); }); describe('@authenticated', () => { diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 32acd241f..5eb6252e2 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -1377,10 +1377,23 @@ class Merger { // For each merged object types, we check if we're missing a field from one of the implemented interface. // If we do, then we look if one of the subgraph provides that field as a (non-external) interface object // type, and if that's the case, we add the field to the object. + const joinFieldDirective = this.joinSpec.fieldDirective(this.merged); for (const type of this.merged.objectTypes()) { for (const implementedItf of type.interfaces()) { for (const itfField of implementedItf.fields()) { - if (type.field(itfField.name)) { + const existingField = type.field(itfField.name); + if (existingField) { + // An implementation may locally declare a field as `@external` purely to reference it in + // a `@requires`, in which case it already exists on the object so it won't be propagated + // from the interface object. But that field's only "real" definition is the abstracting + // `@interfaceObject` field, so directives that only apply there (e.g. `@tag`) still need + // to be propagated onto it. + const joinFieldApplications = existingField.appliedDirectivesOf(joinFieldDirective); + const isFullyExternal = joinFieldApplications.length > 0 + && joinFieldApplications.every((d) => d.arguments().external === true); + if (isFullyExternal && this.isFieldProvidedByAnInterfaceObject(itfField.name, implementedItf.name)) { + this.copyNonJoinAppliedDirectives(itfField, existingField); + } continue; } @@ -1980,12 +1993,14 @@ class Merger { // Note that we don't truly merge externals: we don't want, for instance, a field that is non-nullable everywhere to appear nullable in the // supergraph just because someone fat-fingered the type in an external definition. But after merging the non-external definitions, we // validate the external ones are consistent. - this.mergeDescription(withoutExternal, dest); - this.recordFieldAppliedDirectivesToMerge(withoutExternal, dest); - this.addArgumentsShallow(withoutExternal, dest); - for (const destArg of dest.arguments()) { - const subgraphArgs = mapSources(withoutExternal, f => f?.argument(destArg.name)); - this.mergeArgument(subgraphArgs, destArg); + if (someSources(withoutExternal, isDefined)) { + this.mergeDescription(withoutExternal, dest); + this.recordFieldAppliedDirectivesToMerge(withoutExternal, dest); + this.addArgumentsShallow(withoutExternal, dest); + for (const destArg of dest.arguments()) { + const subgraphArgs = mapSources(withoutExternal, f => f?.argument(destArg.name)); + this.mergeArgument(subgraphArgs, destArg); + } } // Note that due to @interfaceObject, it's possible that `withoutExternal` is "empty" (has no // non-undefined at all) but to still get here. That is, we can have: