From db715351fae8645276704aa6b4350cd86f4785e6 Mon Sep 17 00:00:00 2001 From: Josemar Luedke <230476+josemarluedke@users.noreply.github.com> Date: Fri, 31 Jan 2020 07:49:57 -0800 Subject: [PATCH] Strip all Type System Directives during composition (#3736) * tests: Show composition failures w/ undeclared type-system directives. > Note: This is not the solution, just a test that demonstrates the failure! Recently, the beginnings of [Executable directive] support were brought into federation through https://github.com/apollographql/apollo-server/pull/3464 and its follow-up, https://github.com/apollographql/apollo-server/pull/3536. The implementation of executable directives currently in place within federation requires that federated directives be declared identically across all implementing services, even if their implementation is different (or a no-op). This is working as intended! However, [Type system directives] need some additional pruning from the composed schema. Specifically, while type system directive declarations don't need to be declared identically across implementing services, their _definitions_ are currently (intentionally!) removed from the composed schema. That would be fine, but the _usages_ of those directives are currently being left in-tact, which results in validation errors within composed schemas since the directives, while still defined in the implementing services, have been removed. It's certainly important that the implementing services maintain those type system directives in order to act upon them within the services themselves, I don't believe those directives need to be preserved in the composed schema that runs at the gateway and is used to generate the query plan. This commit is mostly documenting a reproduction of an issue that was brought to my attention and introduces what I believe is a test case to build a solution against. [Type system directive]: https://graphql.github.io/graphql-spec/draft/#TypeSystemDirectiveLocation [Executable directive]: https://graphql.github.io/graphql-spec/draft/#ExecutableDirectiveLocation * Remove directives from field definition in composition * Expand functionality to all custom type system directives * Update stripping mechanism to include all TypeSystemDirectives * Jest auto-prettification of snapshots * Rename var in composition to match implementation * Add changelog entry for PR #3736 * nit: Line-length. It's bad in this file altogether, but broad re-formatting of this file won't do anything better than stomp on history. Since this line is already changing, it's worth a fix in the same commit. * Change `definition` to be a `const`-ant rather than `let`. The overall height of this block seems to be a ripe opportunity to try to lock down the re-assignment of this objects' reference, even if it wasn't introduced by this PRs intended change. * Add a comment about what `null` return does on visits. * Add a comment about why we're stripping type system directives. Co-authored-by: Jesse Rosenberger Co-authored-by: Trevor Scheer --- packages/apollo-federation/CHANGELOG.md | 2 + .../__tests__/composeAndValidate.test.ts | 111 +++++++++++++++--- .../src/composition/compose.ts | 10 +- .../src/composition/utils.ts | 14 +++ 4 files changed, 117 insertions(+), 20 deletions(-) diff --git a/packages/apollo-federation/CHANGELOG.md b/packages/apollo-federation/CHANGELOG.md index 87c43bc9b62..77affcda5dc 100644 --- a/packages/apollo-federation/CHANGELOG.md +++ b/packages/apollo-federation/CHANGELOG.md @@ -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 the appropriate changes within that release will be moved into the new section. +* Strip all Type System Directives during composition [#3736](https://github.com/apollographql/apollo-server/pull/3736) + # v0.11.1 > [See complete versioning details.](https://github.com/apollographql/apollo-server/commit/2a4c654986a158aaccf947ee56a4bfc48a3173c7) diff --git a/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts b/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts index 9980434dade..f3e20cec8bf 100644 --- a/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts +++ b/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts @@ -329,11 +329,11 @@ describe('composition of value types', () => { `union CatalogItem = Couch | Mattress`, ); expect(schema.getType('Couch')).toMatchInlineSnapshot(` - type Couch { - sku: ID! - material: String! - } - `); + type Couch { + sku: ID! + material: String! + } + `); }); it('input types', () => { @@ -345,11 +345,11 @@ describe('composition of value types', () => { `); expect(errors).toHaveLength(0); expect(schema.getType('NewProductInput')).toMatchInlineSnapshot(` - input NewProductInput { - sku: ID! - type: String - } - `); + input NewProductInput { + sku: ID! + type: String + } + `); }); it('interfaces', () => { @@ -360,10 +360,10 @@ describe('composition of value types', () => { `); expect(errors).toHaveLength(0); expect(schema.getType('Product')).toMatchInlineSnapshot(` - interface Product { - sku: ID! - } - `); + interface Product { + sku: ID! + } + `); }); it('enums', () => { @@ -375,11 +375,11 @@ describe('composition of value types', () => { `); expect(errors).toHaveLength(0); expect(schema.getType('CatalogItemEnum')).toMatchInlineSnapshot(` - enum CatalogItemEnum { - COUCH - MATTRESS - } - `); + enum CatalogItemEnum { + COUCH + MATTRESS + } + `); }); }); @@ -551,3 +551,76 @@ describe('composition of value types', () => { }); }); }); + +describe('composition of schemas with directives', () => { + /** + * To see which usage sites indicate whether a directive is "executable" or + * merely for use by the type-system ("type-system"), see the GraphQL spec: + * https://graphql.github.io/graphql-spec/June2018/#sec-Type-System.Directives + */ + it('preserves executable and purges type-system directives', () => { + const serviceA = { + typeDefs: gql` + "directives at FIELDs are executable" + directive @audit(risk: Int!) on FIELD + + "directives at FIELD_DEFINITIONs are for the type-system" + directive @transparency(concealment: Int!) on FIELD_DEFINITION + + type EarthConcern { + environmental: String! @transparency(concealment: 5) + } + + extend type Query { + importantDirectives: [EarthConcern!]! + } + `, + name: 'serviceA', + }; + + const serviceB = { + typeDefs: gql` + "directives at FIELDs are executable" + directive @audit(risk: Int!) on FIELD + + "directives at FIELD_DEFINITIONs are for the type-system" + directive @transparency(concealment: Int!) on FIELD_DEFINITION + + "directives at OBJECTs are for the type-system" + directive @experimental on OBJECT + + extend type EarthConcern @experimental { + societal: String! @transparency(concealment: 6) + } + `, + name: 'serviceB', + }; + + const { schema, errors } = composeAndValidate([serviceA, serviceB]); + expect(errors).toHaveLength(0); + + const audit = schema.getDirective('audit'); + expect(audit).toMatchInlineSnapshot(`"@audit"`); + + const transparency = schema.getDirective('transparency'); + expect(transparency).toBeUndefined(); + + const type = schema.getType('EarthConcern') as GraphQLObjectType; + + expect(type.astNode).toMatchInlineSnapshot(` + type EarthConcern { + environmental: String! + } + `); + + const fields = type.getFields(); + + expect(fields['environmental'].astNode).toMatchInlineSnapshot( + `environmental: String!`, + ); + + expect(fields['societal'].astNode).toMatchInlineSnapshot( + `societal: String!`, + ); + }); +}); diff --git a/packages/apollo-federation/src/composition/compose.ts b/packages/apollo-federation/src/composition/compose.ts index 50b8d19e86d..d88d087be0e 100644 --- a/packages/apollo-federation/src/composition/compose.ts +++ b/packages/apollo-federation/src/composition/compose.ts @@ -29,6 +29,7 @@ import { mapValues, isFederationDirective, executableDirectiveLocations, + stripTypeSystemDirectivesFromTypeDefs, } from './utils'; import { ServiceDefinition, @@ -135,7 +136,14 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { externalFields.push(...strippedFields); - for (let definition of typeDefsWithoutExternalFields.definitions) { + // Type system directives from downstream services are not a concern of the + // gateway, but rather the services on which the fields live which serve + // those types. In other words, its up to an implementing service to + // act on such directives, not the gateway. + const typeDefsWithoutTypeSystemDirectives = + stripTypeSystemDirectivesFromTypeDefs(typeDefsWithoutExternalFields); + + for (const definition of typeDefsWithoutTypeSystemDirectives.definitions) { if ( definition.kind === Kind.OBJECT_TYPE_DEFINITION || definition.kind === Kind.OBJECT_TYPE_EXTENSION diff --git a/packages/apollo-federation/src/composition/utils.ts b/packages/apollo-federation/src/composition/utils.ts index a70bd812de9..34ca9b06aa8 100644 --- a/packages/apollo-federation/src/composition/utils.ts +++ b/packages/apollo-federation/src/composition/utils.ts @@ -83,6 +83,20 @@ export function stripExternalFieldsFromTypeDefs( return { typeDefsWithoutExternalFields, strippedFields }; } +export function stripTypeSystemDirectivesFromTypeDefs(typeDefs: DocumentNode) { + const typeDefsWithoutTypeSystemDirectives = visit(typeDefs, { + Directive(node) { + const isFederationDirective = federationDirectives.some( + ({ name }) => name === node.name.value, + ); + // Returning `null` to a visit will cause it to be removed from the tree. + return isFederationDirective ? undefined : null; + }, + }) as DocumentNode; + + return typeDefsWithoutTypeSystemDirectives; +} + /** * Returns a closure that strips fields marked with `@external` and adds them * to an array.