gateway(feat): Allow complete SDL - follow-up to #4209 (#4228)

This commit adds additional normalization to composition inputs in order to prevent duplicate directive errors when constructing the schema. The oversight from #4209 that's being corrected by this PR is to also remove specified directive definitions (@deprecated, @Skip, @include, and @SpecifiedBy) from composition inputs. Failure to remove them during composition results in duplicate definitions and graphql validation errors if they are included in the composition inputs.
apollographql · Jun 11, 2020 · 672fb1b · 672fb1b
1 parent 470efde
commit 672fb1b
Show file tree

Hide file tree

Showing 6 changed files with 211 additions and 25 deletions.
diff --git a/packages/apollo-federation/CHANGELOG.md b/packages/apollo-federation/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 > 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.
 
-- _Nothing yet! Stay tuned._
+- Remove remaining common primitives from SDL during composition. This is a follow up to [#4209](https://github.com/apollographql/apollo-server/pull/4209), and additionally removes directives which are included in a schema by default (`@skip`, `@include`, `@deprecated`, and `@specifiedBy`) [#4228](https://github.com/apollographql/apollo-server/pull/4209)
 
 ## 0.16.6
 

diff --git a/packages/apollo-federation/src/composition/__tests__/compose.test.ts b/packages/apollo-federation/src/composition/__tests__/compose.test.ts
@@ -77,7 +77,6 @@ describe('composeServices', () => {
     const { schema } = composeServices([serviceA]);
 
     const directives = schema.getDirectives();
-    expect(directives).toHaveLength(3);
     expect(directives.every(isSpecifiedDirective));
   });
 

diff --git a/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts b/packages/apollo-federation/src/composition/__tests__/composeAndValidate.test.ts
@@ -1,6 +1,11 @@
 import { composeAndValidate } from '../composeAndValidate';
 import gql from 'graphql-tag';
-import { GraphQLObjectType, DocumentNode } from 'graphql';
+import {
+  GraphQLObjectType,
+  DocumentNode,
+  GraphQLScalarType,
+  specifiedDirectives,
+} from 'graphql';
 import {
   astSerializer,
   typeSerializer,
@@ -99,12 +104,12 @@ it('composes and validates all (24) permutations without error', () => {
     reviewsService,
     accountsService,
     productsService,
-  ]).map(config => {
+  ]).map((config) => {
     const { errors } = composeAndValidate(config);
 
     if (errors.length) {
       console.error(
-        `Errors found with composition [${config.map(item => item.name)}]`,
+        `Errors found with composition [${config.map((item) => item.name)}]`,
       );
     }
 
@@ -624,16 +629,29 @@ describe('composition of schemas with directives', () => {
     );
   });
 
-  it(`doesn't strip the special case @deprecated type-system directive`, () => {
+  it(`doesn't strip the special case @deprecated and @specifiedBy type-system directives`, () => {
+    const specUrl = "http://my-spec-url.com";
+    const deprecationReason = "Don't remove me please";
+
+    // Detecting >15.1.0 by the new addition of the `specifiedBy` directive
+    const isAtLeastGraphqlVersionFifteenPointOne =
+      specifiedDirectives.length >= 4;
+
     const serviceA = {
       typeDefs: gql`
+        # This directive needs to be conditionally added depending on the testing
+        # environment's version of graphql (>= 15.1.0 includes this new directive)
+        ${isAtLeastGraphqlVersionFifteenPointOne
+          ? `scalar MyScalar @specifiedBy(url: "${specUrl}")`
+          : ''}
+
         type EarthConcern {
           environmental: String!
         }
 
         extend type Query {
           importantDirectives: [EarthConcern!]!
-            @deprecated(reason: "Don't remove me please")
+            @deprecated(reason: "${deprecationReason}")
         }
       `,
       name: 'serviceA',
@@ -649,6 +667,130 @@ describe('composition of schemas with directives', () => {
     const field = queryType.getFields()['importantDirectives'];
 
     expect(field.isDeprecated).toBe(true);
-    expect(field.deprecationReason).toEqual("Don't remove me please");
+    expect(field.deprecationReason).toEqual(deprecationReason);
+
+    if (isAtLeastGraphqlVersionFifteenPointOne) {
+      const specifiedBy = schema.getDirective('specifiedBy');
+      expect(specifiedBy).toMatchInlineSnapshot(`"@specifiedBy"`);
+      const customScalar = schema.getType('MyScalar');
+      expect((customScalar as GraphQLScalarType).specifiedByUrl).toEqual(specUrl);
+    }
   });
 });
+
+it('composition of full-SDL schemas without any errors', () => {
+  const serviceA = {
+    typeDefs: gql`
+      # Default directives
+      directive @deprecated(
+        reason: String = "No longer supported"
+      ) on FIELD_DEFINITION | ENUM_VALUE
+      directive @specifiedBy(url: String!) on SCALAR
+      directive @include(
+        if: String = "Included when true."
+      ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+      directive @skip(
+        if: String = "Skipped when true."
+      ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+
+      # Federation directives
+      directive @key(fields: _FieldSet!) on OBJECT | INTERFACE
+      directive @external on FIELD_DEFINITION
+      directive @requires(fields: _FieldSet!) on FIELD_DEFINITION
+      directive @provides(fields: _FieldSet!) on FIELD_DEFINITION
+      directive @extends on OBJECT | INTERFACE
+
+      # Custom type system directive (disregarded by gateway, unconcerned with serviceB's implementation)
+      directive @myTypeSystemDirective on FIELD_DEFINITION
+      # Custom executable directive (must be implemented in all services, definition must be identical)
+      directive @myExecutableDirective on FIELD
+
+      scalar _Any
+      scalar _FieldSet
+
+      union _Entity
+
+      type _Service {
+        sdl: String
+      }
+
+      schema {
+        query: RootQuery
+        mutation: RootMutation
+      }
+
+      type RootQuery {
+        _service: _Service!
+        _entities(representations: [_Any!]!): [_Entity]!
+        product: Product
+      }
+
+      type Product @key(fields: "sku") {
+        sku: String!
+        price: Float
+      }
+
+      type RootMutation {
+        updateProduct: Product
+      }
+    `,
+    name: 'serviceA',
+  };
+
+  const serviceB = {
+    typeDefs: gql`
+      # Default directives
+      directive @deprecated(
+        reason: String = "No longer supported"
+      ) on FIELD_DEFINITION | ENUM_VALUE
+      directive @specifiedBy(url: String!) on SCALAR
+      directive @include(
+        if: String = "Included when true."
+      ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+      directive @skip(
+        if: String = "Skipped when true."
+      ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+
+      # Federation directives
+      directive @key(fields: _FieldSet!) on OBJECT | INTERFACE
+      directive @external on FIELD_DEFINITION
+      directive @requires(fields: _FieldSet!) on FIELD_DEFINITION
+      directive @provides(fields: _FieldSet!) on FIELD_DEFINITION
+      directive @extends on OBJECT | INTERFACE
+
+      # Custom type system directive (disregarded by gateway, unconcerned with serviceA's implementation)
+      directive @myDirective on FIELD_DEFINITION
+
+      # Custom executable directive (must be implemented in all services, definition must be identical)
+      directive @myExecutableDirective on FIELD
+
+      scalar _Any
+      scalar _FieldSet
+
+      union _Entity
+
+      type _Service {
+        sdl: String
+      }
+
+      type Query {
+        _service: _Service!
+        _entities(representations: [_Any!]!): [_Entity]!
+        review: Review
+      }
+
+      type Review @key(fields: "id") {
+        id: String!
+        content: String
+      }
+
+      type Mutation {
+        createReview: Review
+      }
+    `,
+    name: 'serviceB',
+  };
+
+  const { errors } = composeAndValidate([serviceA, serviceB]);
+  expect(errors).toHaveLength(0);
+});
diff --git a/packages/apollo-federation/src/composition/__tests__/normalize.test.ts b/packages/apollo-federation/src/composition/__tests__/normalize.test.ts
@@ -3,9 +3,10 @@ import {
   defaultRootOperationTypes,
   replaceExtendedDefinitionsWithExtensions,
   normalizeTypeDefs,
-  stripFederationPrimitives,
+  stripCommonPrimitives,
 } from '../normalize';
 import { astSerializer } from '../../snapshotSerializers';
+import { specifiedDirectives } from 'graphql';
 
 expect.addSnapshotSerializer(astSerializer);
 
@@ -143,9 +144,31 @@ describe('SDL normalization and its respective parts', () => {
     });
   });
 
-  describe('stripFederationPrimitives', () => {
-    it(`removes all federation directive definitions`, () => {
+  describe('stripCommonPrimitives', () => {
+    it(`removes all common directive definitions`, () => {
+      // Detecting >15.1.0 by the new addition of the `specifiedBy` directive
+      const isAtLeastGraphqlVersionFifteenPointOne =
+        specifiedDirectives.length >= 4;
+
       const typeDefs = gql`
+        # Default directives
+
+        # This directive needs to be conditionally added depending on the testing
+        # environment's version of graphql (>= 15.1.0 includes this new directive)
+        ${isAtLeastGraphqlVersionFifteenPointOne
+          ? 'directive @specifiedBy(url: String!) on SCALAR'
+          : ''}
+        directive @deprecated(
+          reason: String = "No longer supported"
+        ) on FIELD_DEFINITION | ENUM_VALUE
+        directive @include(
+          if: String = "Included when true."
+        ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+        directive @skip(
+          if: String = "Skipped when true."
+        ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+
+        # Federation directives
         directive @key(fields: _FieldSet!) on OBJECT | INTERFACE
         directive @external on FIELD_DEFINITION
         directive @requires(fields: _FieldSet!) on FIELD_DEFINITION
@@ -157,7 +180,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         type Query {
           thing: String
         }
@@ -173,7 +196,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         directive @custom on OBJECT
 
         type Query {
@@ -198,7 +221,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         type Query {
           thing: String
         }
@@ -220,7 +243,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         scalar CustomScalar
 
         type CustomType {
@@ -244,7 +267,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         type Query {
           thing: String
         }
@@ -263,7 +286,7 @@ describe('SDL normalization and its respective parts', () => {
         }
       `;
 
-      expect(stripFederationPrimitives(typeDefs)).toMatchInlineSnapshot(`
+      expect(stripCommonPrimitives(typeDefs)).toMatchInlineSnapshot(`
         type Custom {
           field: String
         }
@@ -273,7 +296,28 @@ describe('SDL normalization and its respective parts', () => {
 
   describe('normalizeTypeDefs', () => {
     it('integration', () => {
+      // Detecting >15.1.0 by the new addition of the `specifiedBy` directive
+      const isAtLeastGraphqlVersionFifteenPointOne =
+        specifiedDirectives.length >= 4;
+
       const typeDefsToNormalize = gql`
+        # Default directives
+
+        # This directive needs to be conditionally added depending on the testing
+        # environment's version of graphql (>= 15.1.0 includes this new directive)
+        ${isAtLeastGraphqlVersionFifteenPointOne
+          ? 'directive @specifiedBy(url: String!) on SCALAR'
+          : ''}
+        directive @deprecated(
+          reason: String = "No longer supported"
+        ) on FIELD_DEFINITION | ENUM_VALUE
+        directive @include(
+          if: String = "Included when true."
+        ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+        directive @skip(
+          if: String = "Skipped when true."
+        ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
+
         directive @key(fields: _FieldSet!) on OBJECT | INTERFACE
         directive @external on FIELD_DEFINITION
         directive @requires(fields: _FieldSet!) on FIELD_DEFINITION

diff --git a/packages/apollo-federation/src/composition/normalize.ts b/packages/apollo-federation/src/composition/normalize.ts
@@ -7,6 +7,7 @@ import {
   Kind,
   InterfaceTypeDefinitionNode,
   VisitFn,
+  specifiedDirectives,
 } from 'graphql';
 import {
   findDirectivesOnTypeOrField,
@@ -17,10 +18,10 @@ import {
 import federationDirectives from '../directives';
 
 export function normalizeTypeDefs(typeDefs: DocumentNode) {
-  // The order of this is important - `stripFederationPrimitives` must come after
+  // The order of this is important - `stripCommonPrimitives` must come after
   // `defaultRootOperationTypes` because it depends on the `Query` type being named
   // its default: `Query`.
-  return stripFederationPrimitives(
+  return stripCommonPrimitives(
     defaultRootOperationTypes(
       replaceExtendedDefinitionsWithExtensions(typeDefs),
     ),
@@ -261,12 +262,12 @@ export function replaceExtendedDefinitionsWithExtensions(
 // primitives, but in many cases it's more difficult to exclude them.
 //
 // This removes the following from a GraphQL Document:
-// directives: @external, @key, @requires, @provides, @extends
+// directives: @external, @key, @requires, @provides, @extends, @skip, @include, @deprecated, @specifiedBy
 // scalars: _Any, _FieldSet
 // union: _Entity
 // object type: _Service
 // Query fields: _service, _entities
-export function stripFederationPrimitives(document: DocumentNode) {
+export function stripCommonPrimitives(document: DocumentNode) {
   const typeDefinitionVisitor: VisitFn<
     any,
     ObjectTypeDefinitionNode | ObjectTypeExtensionNode
@@ -295,12 +296,12 @@ export function stripFederationPrimitives(document: DocumentNode) {
   };
 
   return visit(document, {
-    // Remove all federation directive definitions from the document
+    // Remove all common directive definitions from the document
     DirectiveDefinition(node) {
-      const isFederationDirective = federationDirectives.some(
+      const isCommonDirective = [...federationDirectives, ...specifiedDirectives].some(
         (directive) => directive.name === node.name.value,
       );
-      return isFederationDirective ? null : node;
+      return isCommonDirective ? null : node;
     },
     // Remove all federation scalar definitions from the document
     ScalarTypeDefinition(node) {

diff --git a/packages/apollo-federation/src/composition/utils.ts b/packages/apollo-federation/src/composition/utils.ts
@@ -91,7 +91,7 @@ export function stripTypeSystemDirectivesFromTypeDefs(typeDefs: DocumentNode) {
   const typeDefsWithoutTypeSystemDirectives = visit(typeDefs, {
     Directive(node) {
       // The `deprecated` directive is an exceptional case that we want to leave in
-      if (node.name.value === 'deprecated') return;
+      if (node.name.value === 'deprecated' || node.name.value === 'specifiedBy') return;
 
       const isFederationDirective = federationDirectives.some(
         ({ name }) => name === node.name.value,