feat(composition): Relax @tag definition validation (#1022)

Add flexibility for @tag directive definition validation in subgraphs.
@tag definitions are now permitted to be a subset of the spec's
definition. This means that within the definition, `repeatable` is
optional as are each of the directive locations.

federation-js/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!_
+- Add flexibility for @tag directive definition validation in subgraphs. @tag definitions are now permitted to be a subset of the spec's definition. This means that within the definition, `repeatable` is optional as are each of the directive locations. [PR #1022](https://github.com/apollographql/federation/pull/1022)
 
 ## v0.32.0
 

federation-js/src/composition/utils.ts

@@ -32,6 +32,8 @@ import {
   isDirective,
   isNamedType,
   stripIgnoredCharacters,
+  NonNullTypeNode,
+  NamedTypeNode,
 } from 'graphql';
 import {
   ExternalFieldDefinition,
@@ -56,6 +58,14 @@ export function isDirectiveDefinitionNode(node: any): node is DirectiveDefinitio
   return node.kind === Kind.DIRECTIVE_DEFINITION;
 }
 
+export function isNonNullTypeNode(node: any): node is NonNullTypeNode {
+  return node.kind === Kind.NON_NULL_TYPE;
+}
+
+export function isNamedTypeNode(node: any): node is NamedTypeNode {
+  return node.kind === Kind.NAMED_TYPE;
+}
+
 // Create a map of { fieldName: serviceName } for each field.
 export function mapFieldNamesToServiceName<Node extends { name: NameNode }>(
   fields: ReadonlyArray<Node>,
@@ -139,14 +149,6 @@ export function stripExternalFieldsFromTypeDefs(
   return { typeDefsWithoutExternalFields, strippedFields };
 }
 
-export function stripDescriptions(astNode: ASTNode) {
-  return visit(astNode, {
-    enter(node) {
-      return 'description' in node ? { ...node, description: undefined } : node;
-    },
-  });
-}
-
 export function stripTypeSystemDirectivesFromTypeDefs(typeDefs: DocumentNode) {
   const typeDefsWithoutTypeSystemDirectives = visit(typeDefs, {
     Directive(node) {

federation-js/src/composition/validate/preNormalization/__tests__/tagDirective.test.ts

@@ -65,6 +65,21 @@ describe('tagDirective', () => {
       const errors = tagDirective(serviceA);
       expect(errors).toHaveLength(0);
     });
+
+    it('permits alternative, compatible @tag definitions', () => {
+      const serviceA = {
+        typeDefs: gql`
+          directive @tag(name: String!) on FIELD_DEFINITION | INTERFACE
+          type Query {
+            hello: String @tag(name: "hello")
+          }
+        `,
+        name: 'serviceA',
+      };
+
+      const errors = tagDirective(serviceA);
+      expect(errors).toHaveLength(0);
+    });
   });
 
   describe('reports errors', () => {
@@ -99,35 +114,130 @@ describe('tagDirective', () => {
       `);
     });
 
-    it('when @tag usage and definition exist, but definition is incorrect', () => {
-      const serviceA = {
-        typeDefs: gql`
-          directive @tag(name: String!) on FIELD_DEFINITION
+    describe('incompatible definition', () => {
+      it('locations incompatible', () => {
+        const serviceA = {
+          typeDefs: gql`
+            directive @tag(name: String!) on FIELD_DEFINITION | SCHEMA
 
-          type Query {
-            hello: String @tag(name: "hello")
-          }
-        `,
-        name: 'serviceA',
-      };
+            type Query {
+              hello: String @tag(name: "hello")
+            }
+          `,
+          name: 'serviceA',
+        };
 
-      const errors = tagDirective(serviceA);
+        const errors = tagDirective(serviceA);
 
-      expect(errors).toMatchInlineSnapshot(`
-        Array [
-          Object {
-            "code": "TAG_DIRECTIVE_DEFINITION_INVALID",
-            "locations": Array [
-              Object {
-                "column": 1,
-                "line": 2,
-              },
-            ],
-            "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions matches the following:
-        	directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION",
-          },
-        ]
-      `);
+        expect(errors).toMatchInlineSnapshot(`
+          Array [
+            Object {
+              "code": "TAG_DIRECTIVE_DEFINITION_INVALID",
+              "locations": Array [
+                Object {
+                  "column": 1,
+                  "line": 2,
+                },
+              ],
+              "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:
+          	directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION",
+            },
+          ]
+        `);
+      });
+
+      it('name arg missing', () => {
+        const serviceA = {
+          typeDefs: gql`
+            directive @tag on FIELD_DEFINITION
+
+            type Query {
+              hello: String @tag
+            }
+          `,
+          name: 'serviceA',
+        };
+
+        const errors = tagDirective(serviceA);
+
+        expect(errors).toMatchInlineSnapshot(`
+          Array [
+            Object {
+              "code": "TAG_DIRECTIVE_DEFINITION_INVALID",
+              "locations": Array [
+                Object {
+                  "column": 1,
+                  "line": 2,
+                },
+              ],
+              "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:
+          	directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION",
+            },
+          ]
+        `);
+      });
+
+      it('name arg incompatible', () => {
+        const serviceA = {
+          typeDefs: gql`
+            directive @tag(name: String) on FIELD_DEFINITION
+
+            type Query {
+              hello: String @tag(name: "hello")
+            }
+          `,
+          name: 'serviceA',
+        };
+
+        const errors = tagDirective(serviceA);
+
+        expect(errors).toMatchInlineSnapshot(`
+          Array [
+            Object {
+              "code": "TAG_DIRECTIVE_DEFINITION_INVALID",
+              "locations": Array [
+                Object {
+                  "column": 1,
+                  "line": 2,
+                },
+              ],
+              "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:
+          	directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION",
+            },
+          ]
+        `);
+      });
+
+      it('additional args', () => {
+        const serviceA = {
+          typeDefs: gql`
+            directive @tag(name: String!, additional: String) on FIELD_DEFINITION
+
+            type Query {
+              hello: String @tag(name: "hello")
+            }
+          `,
+          name: 'serviceA',
+        };
+
+        const errors = tagDirective(serviceA);
+
+        expect(errors).toMatchInlineSnapshot(`
+          Array [
+            Object {
+              "code": "TAG_DIRECTIVE_DEFINITION_INVALID",
+              "locations": Array [
+                Object {
+                  "column": 1,
+                  "line": 2,
+                },
+              ],
+              "message": "[@tag] -> Found @tag definition in service serviceA, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:
+          	directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION",
+            },
+          ]
+        `);
+      });
     });
 
     it('when @tag usage is missing args', () => {

federation-js/src/composition/validate/preNormalization/tagDirective.ts

@@ -1,17 +1,19 @@
-import { federationDirectives } from '../../../directives';
+import {
+  directiveDefinitionsAreCompatible,
+  federationDirectives,
+} from '../../../directives';
 import {
   DirectiveDefinitionNode,
   KnownDirectivesRule,
   visit,
   BREAK,
-  print,
-  NameNode,
+  parse,
 } from 'graphql';
 import { KnownArgumentNamesOnDirectivesRule } from 'graphql/validation/rules/KnownArgumentNamesRule';
 import { ProvidedRequiredArgumentsOnDirectivesRule } from 'graphql/validation/rules/ProvidedRequiredArgumentsRule';
 import { validateSDL } from 'graphql/validation/validate';
 import { ServiceDefinition } from '../../types';
-import { errorWithCode, logDirective, stripDescriptions } from '../../utils';
+import { errorWithCode, logDirective } from '../../utils';
 
 // Likely brittle but also will be very obvious if this breaks. Based on the
 // content of the error message itself to remove expected errors related to
@@ -47,36 +49,30 @@ export const tagDirective = ({
     },
   });
 
-  // Ensure the tag directive definition is correct
-  if (tagDirectiveDefinition) {
-    const printedTagDefinition =
-      'directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION';
+  const printedTagDefinition =
+    'directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION';
+  const parsedTagDefinition = parse(printedTagDefinition)
+    .definitions[0] as DirectiveDefinitionNode;
 
-    if (
-      print(normalizeDirectiveDefinitionNode(tagDirectiveDefinition)) !==
-      printedTagDefinition
-    ) {
-      errors.push(
-        errorWithCode(
-          'TAG_DIRECTIVE_DEFINITION_INVALID',
-          logDirective('tag') +
-            `Found @tag definition in service ${serviceName}, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions matches the following:\n\t${printedTagDefinition}`,
-          tagDirectiveDefinition,
-        ),
-      );
-    }
+  if (
+    tagDirectiveDefinition &&
+    !directiveDefinitionsAreCompatible(
+      parsedTagDefinition,
+      tagDirectiveDefinition,
+    )
+  ) {
+    errors.push(
+      errorWithCode(
+        'TAG_DIRECTIVE_DEFINITION_INVALID',
+        logDirective('tag') +
+          `Found @tag definition in service ${serviceName}, but the @tag directive definition was invalid. Please ensure the directive definition in your schema's type definitions is compatible with the following:\n\t${printedTagDefinition}`,
+        tagDirectiveDefinition,
+      ),
+    );
   }
 
   return errors.filter(
     ({ message }) =>
       !errorsMessagesToFilter.some((keyWord) => message === keyWord),
   );
 };
-
-function normalizeDirectiveDefinitionNode(node: DirectiveDefinitionNode) {
-  // Remove descriptions from the AST
-  node = stripDescriptions(node);
-  // Sort locations alphabetically
-  (node.locations as NameNode[]).sort((a, b) => a.value.localeCompare(b.value));
-  return node;
-}

federation-js/src/directives.ts

@@ -14,6 +14,10 @@ import {
   TypeSystemExtensionNode,
   TypeDefinitionNode,
   ExecutableDefinitionNode,
+  DirectiveDefinitionNode,
+  print,
+  ASTNode,
+  visit,
 } from 'graphql';
 
 export const KeyDirective = new GraphQLDirective({
@@ -135,5 +139,48 @@ export function typeIncludesDirective(
 ): boolean {
   if (isInputObjectType(type)) return false;
   const directives = gatherDirectives(type as GraphQLNamedTypeWithDirectives);
-  return directives.some(directive => directive.name.value === directiveName);
+  return directives.some((directive) => directive.name.value === directiveName);
+}
+
+export function directiveDefinitionsAreCompatible(
+  baseDefinition: DirectiveDefinitionNode,
+  toCompare: DirectiveDefinitionNode,
+) {
+  if (baseDefinition.name.value !== toCompare.name.value) return false;
+  // arguments must be equal in length
+  if (baseDefinition.arguments?.length !== toCompare.arguments?.length) {
+    return false;
+  }
+  // arguments must be equal in type
+  for (const arg of baseDefinition.arguments ?? []) {
+    const toCompareArg = toCompare.arguments?.find(
+      (a) => a.name.value === arg.name.value,
+    );
+    if (!toCompareArg) return false;
+    if (
+      print(stripDescriptions(arg)) !== print(stripDescriptions(toCompareArg))
+    ) {
+      return false;
+    }
+  }
+  // toCompare's locations must exist in baseDefinition's locations
+  if (
+    toCompare.locations.some(
+      (location) =>
+        !baseDefinition.locations.find(
+          (baseLocation) => baseLocation.value === location.value,
+        ),
+    )
+  ) {
+    return false;
+  }
+  return true;
+}
+
+function stripDescriptions(astNode: ASTNode) {
+  return visit(astNode, {
+    enter(node) {
+      return 'description' in node ? { ...node, description: undefined } : node;
+    },
+  });
 }

harmonizer/src/lib.rs

@@ -31,8 +31,8 @@ composition implementation while we work toward something else.
 #![warn(missing_docs, future_incompatible, unreachable_pub, rust_2018_idioms)]
 use deno_core::{op_sync, JsRuntime};
 use serde::{Deserialize, Serialize};
+use std::fmt::Display;
 use std::sync::mpsc::channel;
-use std::{fmt::Display, io::Write};
 use thiserror::Error;
 
 /// The `ServiceDefinition` represents everything we need to know about a
@@ -163,15 +163,10 @@ pub fn harmonize(service_list: ServiceList) -> Result<String, Vec<CompositionErr
         // The op_fn callback takes a state object OpState,
         // a structured arg of type `T` and an optional ZeroCopyBuf,
         // a mutable reference to a JavaScript ArrayBuffer
-        op_sync(|_state, _msg: Option<String>, zero_copy| {
-            let mut out = std::io::stdout();
-
-            // Write the contents of every buffer to stdout
-            if let Some(buf) = zero_copy {
-                out.write_all(&buf)
-                    .expect("failure writing buffered output");
+        op_sync(|_state, maybe_msg: Option<String>, _zero_copy| {
+            if let Some(msg) = maybe_msg {
+                println!("{}", msg);
             }
-
             Ok(()) // No meaningful result
         }),
     );