Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support INTERFACE_FIELD_NO_IMPLEM #66

Merged
merged 3 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/light-ligers-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@theguild/federation-composition": minor
---

Support INTERFACE_FIELD_NO_IMPLEM
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
58 changes: 55 additions & 3 deletions __tests__/supergraph/errors/INTERFACE_FIELD_NO_IMPLEM.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 (entity)', () => {
expect(
api.composeServices([
{
Expand Down Expand Up @@ -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',
Expand All @@ -58,7 +58,59 @@ 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
}

// KNOW: check all interface fields are implemented when one subgraph introduces a new field to the interface
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',
}),
}),
]),
}),
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
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);
}

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)) {
// if not, continue
continue;
}

const objectFieldState = objectTypeState.fields.get(fieldName);

// if not, make sure it implements the field
// if (!objectFieldState?.byGraph.has(graph)) {
if (!objectFieldState) {
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)
);
}
2 changes: 2 additions & 0 deletions src/supergraph/validation/validate-supergraph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -56,6 +57,7 @@ export function validateSupergraph(
}

const postSupergraphRules = [
InterfaceFieldNoImplementationRule,
ExtensionWithBaseRule,
FieldsOfTheSameTypeRule,
FieldArgumentsOfTheSameTypeRule,
Expand Down
Loading