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 #3464
and its follow-up, #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 <git@jro.cc>
Co-authored-by: Trevor Scheer <trevor.scheer@gmail.com>

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)

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!`,
+    );
+  });
+});

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

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.