From 2f0f205bf03273efdf6b05c52a2713d6a10e62e1 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 5 Mar 2024 16:06:03 -0800 Subject: [PATCH 1/5] add example test case --- composition-js/src/__tests__/hints.test.ts | 44 ++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index c4824db7d..2f0cbbddb 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/composition-js/src/__tests__/hints.test.ts @@ -1354,3 +1354,47 @@ describe('when a directive causes an implicit federation version upgrade', () => expect(result).toNotRaiseHints(); }); }) + +describe('fully external, reference-only types/fields', () => { + it('value types', () => { + const meSubgraph = gql` + type Query { + me: Account + } + + type Account @key(fields: "id") { + id: ID! + name: String + permissions: Permissions + } + + type Permissions { + canView: Boolean + canEdit: Boolean + } + `; + + const accountSubgraph = gql` + type Query { + account: Account + } + + type Account @key(fields: "id") { + id: ID! + permissions: Permissions @external + isViewer: Boolean @requires(fields: "permissions { canView }") + } + + type Permissions @external { + canView: Boolean + } + `; + + const result = mergeDocuments(meSubgraph, accountSubgraph); + expect(result).toRaiseHint( + HINTS.INCONSISTENT_OBJECT_VALUE_TYPE_FIELD, + 'Field "Permissions.canEdit" of non-entity object type "Permissions" is defined in some but not all subgraphs that define "Permissions": "Permissions.canEdit" is defined in subgraph "Subgraph1" but not in subgraph "Subgraph2".', + 'Permissions' + ); + }) +}) From edf66887d6aefbd32ad4e567aba9622de44c731a Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 5 Mar 2024 16:31:47 -0800 Subject: [PATCH 2/5] proposed fix + test update --- composition-js/src/__tests__/hints.test.ts | 6 +----- composition-js/src/merging/merge.ts | 7 ++++++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index 2f0cbbddb..0dda7e1c1 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/composition-js/src/__tests__/hints.test.ts @@ -1391,10 +1391,6 @@ describe('fully external, reference-only types/fields', () => { `; const result = mergeDocuments(meSubgraph, accountSubgraph); - expect(result).toRaiseHint( - HINTS.INCONSISTENT_OBJECT_VALUE_TYPE_FIELD, - 'Field "Permissions.canEdit" of non-entity object type "Permissions" is defined in some but not all subgraphs that define "Permissions": "Permissions.canEdit" is defined in subgraph "Subgraph1" but not in subgraph "Subgraph2".', - 'Permissions' - ); + expect(result).toNotRaiseHints(); }) }) diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 2397e1187..68ccf8bf9 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -967,7 +967,7 @@ class Merger { } for (const source of sources) { // As soon as we find a subgraph that has the type but not the field, we hint. - if (source && !source.field(field.name)) { + if (source && !source.field(field.name) && !this.typeIsFullyExternal(source)) { this.mismatchReporter.reportMismatchHint({ code: hintId, message: `Field "${field.coordinate}" of ${typeDescription} type "${dest}" is defined in some but not all subgraphs that define "${dest}": `, @@ -1081,6 +1081,11 @@ class Merger { return this.metadata(sourceIdx).isFieldFullyExternal(field); } + private typeIsFullyExternal(object: ObjectType | InterfaceType): boolean { + return object.hasAppliedDirective('external') + || object.fields().every(f => f.hasAppliedDirective('external')); + } + private validateAndFilterExternal(sources: (FieldDefinition | undefined)[]): (FieldDefinition | undefined)[] { const filtered: (FieldDefinition | undefined)[] = []; for (const [i, source] of sources.entries()) { From 0a0770e39d70b2095ac3065f15fe873c19577882 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 5 Mar 2024 16:34:48 -0800 Subject: [PATCH 3/5] changeset --- .changeset/weak-bottles-crash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/weak-bottles-crash.md diff --git a/.changeset/weak-bottles-crash.md b/.changeset/weak-bottles-crash.md new file mode 100644 index 000000000..fd39c9a4b --- /dev/null +++ b/.changeset/weak-bottles-crash.md @@ -0,0 +1,5 @@ +--- +"@apollo/composition": patch +--- + +Stop emitting "inconsistent value type" hints against definitions that are strictly external From 47dea4a026ec432d810a608ab462213ce38e029a Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 18 Mar 2024 09:27:26 -0700 Subject: [PATCH 4/5] address feedback --- .changeset/weak-bottles-crash.md | 2 +- composition-js/src/merging/merge.ts | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.changeset/weak-bottles-crash.md b/.changeset/weak-bottles-crash.md index fd39c9a4b..a5a428475 100644 --- a/.changeset/weak-bottles-crash.md +++ b/.changeset/weak-bottles-crash.md @@ -2,4 +2,4 @@ "@apollo/composition": patch --- -Stop emitting "inconsistent value type" hints against definitions that are strictly external +Stop emitting "inconsistent value type" hints against definitions where the type is marked `@external` or all fields are marked `@external`. diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 68ccf8bf9..36724a0f3 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -965,9 +965,9 @@ class Merger { typeDescription = 'interface' break; } - for (const source of sources) { + for (const [index, source] of sources.entries()) { // As soon as we find a subgraph that has the type but not the field, we hint. - if (source && !source.field(field.name) && !this.typeIsFullyExternal(source)) { + if (source && !source.field(field.name) && !this.areAllFieldsExternal(index, source)) { this.mismatchReporter.reportMismatchHint({ code: hintId, message: `Field "${field.coordinate}" of ${typeDescription} type "${dest}" is defined in some but not all subgraphs that define "${dest}": `, @@ -1081,9 +1081,8 @@ class Merger { return this.metadata(sourceIdx).isFieldFullyExternal(field); } - private typeIsFullyExternal(object: ObjectType | InterfaceType): boolean { - return object.hasAppliedDirective('external') - || object.fields().every(f => f.hasAppliedDirective('external')); + private areAllFieldsExternal(sourceIdx: number, type: ObjectType | InterfaceType): boolean { + return type.fields().every(f => this.isExternal(sourceIdx, f)); } private validateAndFilterExternal(sources: (FieldDefinition | undefined)[]): (FieldDefinition | undefined)[] { From 173c4779dfad6b3e350c5ed7d586daaca7cfaf42 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Mon, 18 Mar 2024 09:35:51 -0700 Subject: [PATCH 5/5] add test case --- composition-js/src/__tests__/hints.test.ts | 110 ++++++++++++++------- 1 file changed, 76 insertions(+), 34 deletions(-) diff --git a/composition-js/src/__tests__/hints.test.ts b/composition-js/src/__tests__/hints.test.ts index ecec041ae..eb64fae62 100644 --- a/composition-js/src/__tests__/hints.test.ts +++ b/composition-js/src/__tests__/hints.test.ts @@ -1353,44 +1353,86 @@ describe('when a directive causes an implicit federation version upgrade', () => assertCompositionSuccess(result); expect(result).toNotRaiseHints(); }); -}) +}); -describe('fully external, reference-only types/fields', () => { - it('value types', () => { - const meSubgraph = gql` - type Query { - me: Account - } +describe('when a partially-defined type is marked @external or all fields are marked @external', () => { + describe('value types', () => { + it('with type marked @external', () => { + const meSubgraph = gql` + type Query { + me: Account + } - type Account @key(fields: "id") { - id: ID! - name: String - permissions: Permissions - } + type Account @key(fields: "id") { + id: ID! + name: String + permissions: Permissions + } - type Permissions { - canView: Boolean - canEdit: Boolean - } - `; + type Permissions { + canView: Boolean + canEdit: Boolean + } + `; - const accountSubgraph = gql` - type Query { - account: Account - } + const accountSubgraph = gql` + type Query { + account: Account + } - type Account @key(fields: "id") { - id: ID! - permissions: Permissions @external - isViewer: Boolean @requires(fields: "permissions { canView }") - } + type Account @key(fields: "id") { + id: ID! + permissions: Permissions @external + isViewer: Boolean @requires(fields: "permissions { canView }") + } - type Permissions @external { - canView: Boolean - } - `; + type Permissions @external { + canView: Boolean + } + `; + + const result = mergeDocuments(meSubgraph, accountSubgraph); + expect(result).toNotRaiseHints(); + }); + + it('with all fields marked @external', () => { + const meSubgraph = gql` + type Query { + me: Account + } - const result = mergeDocuments(meSubgraph, accountSubgraph); - expect(result).toNotRaiseHints(); - }) -}) + type Account @key(fields: "id") { + id: ID! + name: String + permissions: Permissions + } + + type Permissions { + canView: Boolean + canEdit: Boolean + canDelete: Boolean + } + `; + + const accountSubgraph = gql` + type Query { + account: Account + } + + type Account @key(fields: "id") { + id: ID! + permissions: Permissions @external + isViewer: Boolean @requires(fields: "permissions { canView canEdit }") + } + + type Permissions { + canView: Boolean @external + canEdit: Boolean @external + } + `; + + const result = mergeDocuments(meSubgraph, accountSubgraph); + expect(result).toNotRaiseHints(); + }); + }); +});