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

fix(gateway): Ignore thrown errors from extendSchema #478

Merged
merged 5 commits into from
Feb 26, 2021
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
2 changes: 2 additions & 0 deletions federation-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.

- Ignore thrown errors from `extendSchema` during composition (these particular errors are already validated against and returned as composition errors) [PR #478](https://github.com/apollographql/federation/pull/478)

## v0.21.0

- __BREAKING__: Drop support for Node.js 8 and Node.js 10. This package now only targets Node.js 12+ LTS (Long-Term Support) versions, the same as `@apollo/gateway`, which first received this treatment in https://github.com/apollographql/apollo-server/pull/4031. Node.js 8 has already lapsed from the [Node.js Foundation's LTS schedule](https://github.com/nodejs/release) and Node.js 10 (in _Maintenance LTS_ right now) is targeted to be end-of-life'd (EOL) at the end of April 2021. [PR #311](https://github.com/apollographql/federation/pull/311)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
assertCompositionFailure,
assertCompositionSuccess,
compositionHasErrors,
CompositionResult,
} from '../utils';

expect.addSnapshotSerializer(astSerializer);
Expand Down Expand Up @@ -159,6 +160,46 @@ it('errors when a type extension has no base', () => {
`);
});

it("doesn't throw errors when a type is unknown, but captures them instead", () => {
const serviceA = {
typeDefs: gql`
type Query {
foo: Bar!
}

extend type Bar @key(fields: "id") {
id: ID! @external
thing: String
}
`,
name: 'serviceA',
};

let compositionResult: CompositionResult;
expect(
() => (compositionResult = composeAndValidate([serviceA])),
).not.toThrow();

assertCompositionFailure(compositionResult);
const { errors } = compositionResult;
expect(errors).toMatchInlineSnapshot(`
Array [
Object {
"code": "MISSING_ERROR",
"message": "Unknown type \\"Bar\\".",
},
Object {
"code": "EXTENSION_WITH_NO_BASE",
"message": "[serviceA] Bar -> \`Bar\` is an extension type, but \`Bar\` is not defined in any service",
},
Object {
"code": "MISSING_ERROR",
"message": "Type Query must define one or more fields.",
},
]
`);
});

it('treats types with @extends as type extensions', () => {
const serviceA = {
typeDefs: gql`
Expand Down Expand Up @@ -395,7 +436,8 @@ describe('composition of value types', () => {
}
`);
assertCompositionSuccess(compositionResult);
expect(compositionResult.schema.getType('CatalogItemEnum')).toMatchInlineSnapshot(`
expect(compositionResult.schema.getType('CatalogItemEnum'))
.toMatchInlineSnapshot(`
enum CatalogItemEnum {
COUCH
MATTRESS
Expand Down Expand Up @@ -648,10 +690,13 @@ describe('composition of value types', () => {

assertCompositionSuccess(compositionResult);
const { schema, composedSdl } = compositionResult;
expect((schema.getType('Product') as GraphQLObjectType).getInterfaces())
.toHaveLength(2);
expect(
(schema.getType('Product') as GraphQLObjectType).getInterfaces(),
).toHaveLength(2);

expect(printSchema(schema)).toContain('type Product implements Named & Node');
expect(printSchema(schema)).toContain(
'type Product implements Named & Node',
);
expect(composedSdl).toContain('type Product implements Named & Node');
});
});
Expand Down
14 changes: 12 additions & 2 deletions federation-js/src/composition/compose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,12 @@ export function buildSchemaFromDefinitionsAndExtensions({
};

errors = validateSDL(definitionsDocument, schema, compositionRules);
schema = extendSchema(schema, definitionsDocument, { assumeValidSDL: true });

try {
schema = extendSchema(schema, definitionsDocument, {
assumeValidSDL: true,
});
} catch (e) {}

// Extend the schema with the extension definitions (as an AST node)
const extensionsDocument: DocumentNode = {
Expand All @@ -415,7 +420,11 @@ export function buildSchemaFromDefinitionsAndExtensions({

errors.push(...validateSDL(extensionsDocument, schema, compositionRules));

schema = extendSchema(schema, extensionsDocument, { assumeValidSDL: true });
try {
schema = extendSchema(schema, extensionsDocument, {
assumeValidSDL: true,
});
} catch {}

// Remove federation directives from the final schema
schema = new GraphQLSchema({
Expand Down Expand Up @@ -514,6 +523,7 @@ export function addFederationMetadataToSchemaNodes({
// TODO: Why don't we need to check for non-object types here
if (isObjectType(namedType)) {
const field = namedType.getFields()[fieldName];
if (!field) break;
Copy link
Member Author

Choose a reason for hiding this comment

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

As seen here in the new test case, when a type goes unrecognized, fields which return that type are abandoned. This handles the case where a field which is expected to exist cannot actually be found in the schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

break was a mistake here, but corrected in #481


const fieldFederationMetadata: FederationField = {
...getFederationMetadata(field),
Expand Down