From 2252426b4f3b1c3c18a4202588d24747af30dffb Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Mon, 8 Jul 2024 12:29:29 +0000 Subject: [PATCH 1/3] Support INTERFACE_FIELD_NO_IMPLEM --- CHANGELOG.md | 5 +- README.md | 2 +- .../errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts | 6 +- .../interface-field-no-implementation-rule.ts | 69 +++++++++++++++++++ .../validation/validate-supergraph.ts | 2 + 5 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 src/supergraph/validation/rules/interface-field-no-implementation-rule.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a1753bd..11490ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,10 @@ ### Patch Changes -- [#64](https://github.com/the-guild-org/federation/pull/64) [`9ec8078`](https://github.com/the-guild-org/federation/commit/9ec80789a8e4926c04dc77d5f5b85347d5934c76) Thanks [@kamilkisiela](https://github.com/kamilkisiela)! - fix: detect incorrect subtypes of interface fields across subgraphs +- [#64](https://github.com/the-guild-org/federation/pull/64) + [`9ec8078`](https://github.com/the-guild-org/federation/commit/9ec80789a8e4926c04dc77d5f5b85347d5934c76) + Thanks [@kamilkisiela](https://github.com/kamilkisiela)! - fix: detect incorrect subtypes of + interface fields across subgraphs ## 0.11.3 diff --git a/README.md b/README.md index 42b3a50..adb4208 100644 --- a/README.md +++ b/README.md @@ -182,10 +182,10 @@ Your feedback and bug reports are welcome and appreciated. - ✅ `INTERFACE_OBJECT_USAGE_ERROR` - ✅ `REQUIRED_INACCESSIBLE` - ✅ `SATISFIABILITY_ERROR` +- ✅ `INTERFACE_FIELD_NO_IMPLEM` ### TODOs -- [ ] `INTERFACE_FIELD_NO_IMPLEM` - [ ] `DISALLOWED_INACCESSIBLE` - [ ] `EXTERNAL_ARGUMENT_DEFAULT_MISMATCH` - [ ] `EXTERNAL_ARGUMENT_TYPE_MISMATCH` diff --git a/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts b/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts index 9ecc918..d09763b 100644 --- a/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts +++ b/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts @@ -2,7 +2,7 @@ import { expect, test } from 'vitest'; import { graphql, testVersions } from '../../shared/testkit.js'; testVersions((api, version) => { - test.skipIf(api.library === 'guild')('INTERFACE_FIELD_NO_IMPLEM', () => { + test('INTERFACE_FIELD_NO_IMPLEM', () => { expect( api.composeServices([ { @@ -49,7 +49,7 @@ testVersions((api, version) => { errors: expect.arrayContaining([ expect.objectContaining({ message: expect.stringContaining( - `Interface field "User.email" is declared in subgraph "users" but type "Author", which implements "User" only in subgraph "feed" does not have field "email".`, + `Interface field "User.email" is declared in subgraph "users" but type "Author", which implements "User" ${api.library === 'apollo' ? 'only ' : ''}in subgraph "feed" does not have field "email".`, ), extensions: expect.objectContaining({ code: 'INTERFACE_FIELD_NO_IMPLEM', @@ -58,7 +58,5 @@ testVersions((api, version) => { ]), }), ); - - // KNOW: check all interface fields are implemented when one subgraph introduces a new field to the interface }); }); diff --git a/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts new file mode 100644 index 0000000..2b1db36 --- /dev/null +++ b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts @@ -0,0 +1,69 @@ +import { GraphQLError } from 'graphql'; +import { SupergraphVisitorMap } from '../../composition/visitor.js'; +import { SupergraphState } from '../../state.js'; +import { SupergraphValidationContext } from '../validation-context.js'; + +export function InterfaceFieldNoImplementationRule( + context: SupergraphValidationContext, + supergraph: SupergraphState, +): SupergraphVisitorMap { + return { + ObjectType(objectTypeState) { + if (objectTypeState.interfaces.size === 0) { + return; + } + + for (const interfaceName of objectTypeState.interfaces) { + const interfaceTypeState = getTypeFromSupergraph(supergraph, interfaceName); + + if (!interfaceTypeState) { + throw new Error(`Expected an interface to exist in supergraph state`); + } + + if (interfaceTypeState.kind !== 'interface') { + throw new Error('Expected interface, got ' + interfaceTypeState.kind); + } + + for (const [fieldName, interfaceFieldState] of interfaceTypeState.fields) { + for (const [graph, objectTypeInGraph] of objectTypeState.byGraph) { + // check if object in the graph, implements an interface of the same name + if (!objectTypeInGraph.interfaces.has(interfaceName)) { + // if not, continue + continue; + } + + const objectFieldState = objectTypeState.fields.get(fieldName); + + // if not, make sure it implements the field + if (!objectFieldState || !objectFieldState.byGraph.has(graph)) { + const interfaceFieldDefinedInGraphs = Array.from( + interfaceFieldState.byGraph.keys(), + ).map(context.graphIdToName); + const declaredIn = + interfaceFieldDefinedInGraphs.length === 1 + ? `subgraph "${interfaceFieldDefinedInGraphs[0]}"` + : `subgraphs "${interfaceFieldDefinedInGraphs.map(g => `"${g}"`).join(', ')}"`; + + context.reportError( + new GraphQLError( + `Interface field "${interfaceName}.${fieldName}" is declared in ${declaredIn} but type "${objectTypeState.name}", which implements "${interfaceName}" in subgraph "${context.graphIdToName(graph)}" does not have field "${fieldName}".`, + { + extensions: { + code: 'INTERFACE_FIELD_NO_IMPLEM', + }, + }, + ), + ); + } + } + } + } + }, + }; +} + +function getTypeFromSupergraph(state: SupergraphState, name: string) { + return ( + state.objectTypes.get(name) ?? state.interfaceTypes.get(name) ?? state.unionTypes.get(name) + ); +} diff --git a/src/supergraph/validation/validate-supergraph.ts b/src/supergraph/validation/validate-supergraph.ts index a1fa83f..2059bdd 100644 --- a/src/supergraph/validation/validate-supergraph.ts +++ b/src/supergraph/validation/validate-supergraph.ts @@ -26,6 +26,7 @@ import { RequiredInputFieldMissingInSomeSubgraphRule } from './rules/required-in import { RequiredQueryRule } from './rules/required-query-rule.js'; import { SatisfiabilityRule } from './rules/satisfiablity-rule.js'; import { SubgraphNameRule } from './rules/subgraph-name-rule.js'; +import { InterfaceFieldNoImplementationRule } from './rules/interface-field-no-implementation-rule.js'; import { TypesOfTheSameKindRule } from './rules/types-of-the-same-kind-rule.js'; import { createSupergraphValidationContext } from './validation-context.js'; @@ -56,6 +57,7 @@ export function validateSupergraph( } const postSupergraphRules = [ + InterfaceFieldNoImplementationRule, ExtensionWithBaseRule, FieldsOfTheSameTypeRule, FieldArgumentsOfTheSameTypeRule, From 2934bade94d2f5d173c4e984fe31708b413265a5 Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Mon, 8 Jul 2024 15:58:45 +0200 Subject: [PATCH 2/3] Support INTERFACE_FIELD_NO_IMPLEM --- .changeset/light-ligers-jog.md | 5 ++ .../errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts | 56 ++++++++++++++++++- .../interface-field-no-implementation-rule.ts | 35 +++++++++++- 3 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 .changeset/light-ligers-jog.md diff --git a/.changeset/light-ligers-jog.md b/.changeset/light-ligers-jog.md new file mode 100644 index 0000000..04d6bf8 --- /dev/null +++ b/.changeset/light-ligers-jog.md @@ -0,0 +1,5 @@ +--- +"@theguild/federation-composition": minor +--- + +Support INTERFACE_FIELD_NO_IMPLEM diff --git a/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts b/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts index d09763b..3f4ceb9 100644 --- a/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts +++ b/__tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts @@ -2,7 +2,7 @@ import { expect, test } from 'vitest'; import { graphql, testVersions } from '../../shared/testkit.js'; testVersions((api, version) => { - test('INTERFACE_FIELD_NO_IMPLEM', () => { + test('INTERFACE_FIELD_NO_IMPLEM (entity)', () => { expect( api.composeServices([ { @@ -59,4 +59,58 @@ testVersions((api, version) => { }), ); }); + + test('INTERFACE_FIELD_NO_IMPLEM (data)', () => { + expect( + api.composeServices([ + { + name: 'foo', + typeDefs: graphql` + type Query { + foo: Foo + } + + type Foo implements Person { + name: String + age: Int + } + + interface Person { + name: String + age: Int + } + `, + }, + { + name: 'bar', + typeDefs: graphql` + type Query { + bar: Bar + } + + type Bar implements Person { + name: String + } + + interface Person { + name: String + } + `, + }, + ]), + ).toEqual( + expect.objectContaining({ + errors: expect.arrayContaining([ + expect.objectContaining({ + message: expect.stringContaining( + `Interface field "Person.age" is declared in subgraph "foo" but type "Bar", which implements "Person" ${api.library === 'apollo' ? 'only ' : ''}in subgraph "bar" does not have field "age".`, + ), + extensions: expect.objectContaining({ + code: 'INTERFACE_FIELD_NO_IMPLEM', + }), + }), + ]), + }), + ); + }); }); diff --git a/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts index 2b1db36..7b64a70 100644 --- a/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts +++ b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts @@ -24,7 +24,38 @@ export function InterfaceFieldNoImplementationRule( throw new Error('Expected interface, got ' + interfaceTypeState.kind); } + const nonRequiredFields: string[] = []; + + for (const [graph, interfaceStateInGraph] of interfaceTypeState.byGraph) { + if (!interfaceStateInGraph.isInterfaceObject) { + continue; + } + + for (const [fieldName, interfaceFieldState] of interfaceTypeState.fields) { + const interfaceFieldStateInGraph = interfaceFieldState.byGraph.get(graph); + if (!interfaceFieldStateInGraph) { + continue; + } + + if (interfaceFieldStateInGraph.external) { + continue; + } + + nonRequiredFields.push(fieldName); + } + } + for (const [fieldName, interfaceFieldState] of interfaceTypeState.fields) { + // skip fields that are defined in interface objects or in interface entities + if (nonRequiredFields.includes(fieldName)) { + continue; + } + + // TODO: detect if a field is missing in a non-entity object type definition + if (objectTypeState.fields.has(fieldName) && objectTypeState.isEntity) { + continue; + } + for (const [graph, objectTypeInGraph] of objectTypeState.byGraph) { // check if object in the graph, implements an interface of the same name if (!objectTypeInGraph.interfaces.has(interfaceName)) { @@ -35,14 +66,14 @@ export function InterfaceFieldNoImplementationRule( const objectFieldState = objectTypeState.fields.get(fieldName); // if not, make sure it implements the field - if (!objectFieldState || !objectFieldState.byGraph.has(graph)) { + if (!objectFieldState?.byGraph.has(graph)) { const interfaceFieldDefinedInGraphs = Array.from( interfaceFieldState.byGraph.keys(), ).map(context.graphIdToName); const declaredIn = interfaceFieldDefinedInGraphs.length === 1 ? `subgraph "${interfaceFieldDefinedInGraphs[0]}"` - : `subgraphs "${interfaceFieldDefinedInGraphs.map(g => `"${g}"`).join(', ')}"`; + : `subgraphs ${interfaceFieldDefinedInGraphs.map(g => `"${g}"`).join(', ')}`; context.reportError( new GraphQLError( From c066c0759ee3bdbcb409bb83b60ff44629ec78d1 Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Mon, 8 Jul 2024 16:12:18 +0200 Subject: [PATCH 3/3] More strict --- .../validation/rules/interface-field-no-implementation-rule.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts index 7b64a70..0492442 100644 --- a/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts +++ b/src/supergraph/validation/rules/interface-field-no-implementation-rule.ts @@ -66,7 +66,8 @@ export function InterfaceFieldNoImplementationRule( const objectFieldState = objectTypeState.fields.get(fieldName); // if not, make sure it implements the field - if (!objectFieldState?.byGraph.has(graph)) { + // if (!objectFieldState?.byGraph.has(graph)) { + if (!objectFieldState) { const interfaceFieldDefinedInGraphs = Array.from( interfaceFieldState.byGraph.keys(), ).map(context.graphIdToName);