Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions composition-js/src/__tests__/compose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
29 changes: 22 additions & 7 deletions composition-js/src/merging/merge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only merge below IF we have non external sources. Without this guard we end up merging access control directives twice -> once during normal merge field logic and then again when propagating @interfaceObject field directives.

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:
Expand Down