Fix merging of graphQL built-in directives

The patch from #1567 mistakenly broke then merging of graphQL built-in
directives (typically `@deprecated`). And unfortunately, we were missing
tests for it.

This commit fixes that issue and adds proper testing.

While adding tests, I also noticed that when merging repeatable
directives that have arguments, if the argument values didn't match
between subgraphs, we were failing composition (on account that
we don't know which of the arguments should "win"), but the error
returned was not useful to users. Or more precisely, merging wasn't
error out but instead was applying the non-repeatable directive multiple
times to the supergraph, and so upon validating the supergraph at
the end of merging, the user would get a "@x cannot appears multiple
times at this location", but that's not as actionnable as it should be.
The patch adds a better error for this (and a test for it).

composition-js/src/__tests__/compose.test.ts

@@ -1553,6 +1553,131 @@ describe('composition', () => {
     })
   });
 
+  describe('merging of directives', () => {
+    it('propagates graphQL built-in directives', () => {
+      const subgraphA = {
+        name: 'subgraphA',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable @deprecated(reason: "bad")
+          }
+        `,
+      };
+
+      const subgraphB = {
+        name: 'subgraphB',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+      assertCompositionSuccess(result);
+
+      const [_, api] = schemas(result);
+      expect(printSchema(api)).toMatchString(`
+        type Query {
+          a: String @deprecated(reason: "bad")
+        }
+      `);
+    });
+
+    it('merges graphQL built-in directives', () => {
+      const subgraphA = {
+        name: 'subgraphA',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable @deprecated(reason: "bad")
+          }
+        `,
+      };
+
+      const subgraphB = {
+        name: 'subgraphB',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable @deprecated(reason: "bad")
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+      assertCompositionSuccess(result);
+
+      const [_, api] = schemas(result);
+      expect(printSchema(api)).toMatchString(`
+        type Query {
+          a: String @deprecated(reason: "bad")
+        }
+      `);
+    });
+
+    it('errors on incompatible non-repeatable (built-in) directives', () => {
+      const subgraphA = {
+        name: 'subgraphA',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable @deprecated(reason: "bad")
+          }
+        `,
+      };
+
+      const subgraphB = {
+        name: 'subgraphB',
+        typeDefs: gql`
+          type Query {
+            a: String @shareable @deprecated
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+
+      expect(result.errors).toBeDefined();
+      expect(errors(result)).toStrictEqual([
+        ['NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH',
+          'Non-repeatable directive @deprecated is applied to "Query.a" in multiple subgraphs but with incompatible arguments: it uses arguments {reason: "bad"} in subgraph "subgraphA" but no arguments in subgraph "subgraphB"'],
+      ]);
+    });
+
+    it('propagates graphQL built-in directives even if redefined in the subgarph', () => {
+      const subgraphA = {
+        name: 'subgraphA',
+        typeDefs: gql`
+          type Query {
+            a: String @deprecated
+          }
+
+          # Do note that the code validates that this definition below
+          # is "compatible" with the "real one", which it is.
+          directive @deprecated on FIELD_DEFINITION
+        `,
+      };
+
+      const subgraphB = {
+        name: 'subgraphB',
+        typeDefs: gql`
+          type Query {
+            b: String
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+      assertCompositionSuccess(result);
+
+      const [_, api] = schemas(result);
+      expect(printSchema(api)).toMatchString(`
+        type Query {
+          a: String @deprecated
+          b: String
+        }
+      `);
+    });
+  });
+
   it('is not broken by similar field argument signatures (#1100)', () => {
     // This test is about validating the case from https://github.com/apollographql/federation/issues/1100 is fixed.
 

composition-js/src/merging/merge.ts

@@ -159,6 +159,16 @@ function isMergedField(field: InputFieldDefinition | FieldDefinition<CompositeTy
   return field.kind !== 'FieldDefinition' || !isFederationField(field);
 }
 
+function isGraphQLBuiltInDirective(def: DirectiveDefinition): boolean {
+  // `def.isBuiltIn` is not entirely reliable here because if it will be `false`
+  // if the user has manually redefined the built-in directive (if they do,
+  // we validate the definition is "compabitle" with the built-in version, but
+  // otherwise return the use one). But when merging, we want to essentially
+  // ignore redefinitions, so we instead just check if the "name" is that of
+  // built-in directive.
+  return !!def.schema().builtInDirective(def.name);
+}
+
 function isMergedDirective(definition: DirectiveDefinition | Directive): boolean {
   // Currently, we only merge "executable" directives, and a small subset of well-known type-system directives.
   // Note that some user directive definitions may have both executable and non-executable locations.
@@ -167,9 +177,14 @@ function isMergedDirective(definition: DirectiveDefinition | Directive): boolean
   if (MERGED_TYPE_SYSTEM_DIRECTIVES.includes(definition.name)) {
     return true;
   }
-  // If it's a directive application, then we skip it (even if the definition itself allows executable locations,
-  // this particular application is an type-system element and we don't want to merge it).
+  // If it's a directive application, then we skip it unless it's a graphQL built-in
+  // (even if the definition itself allows executable locations, this particular
+  // application is an type-system element and we don't want to merge it).
   if (definition instanceof Directive) {
+    return isGraphQLBuiltInDirective(definition.definition!);
+  } else if (isGraphQLBuiltInDirective(definition)) {
+    // We never "merge" graphQL built-in definitions, since they are built-in and
+    // don't need to be defined.
     return false;
   }
   return definition.locations.some(loc => executableDirectiveLocations.includes(loc));
@@ -1591,9 +1606,28 @@ class Merger {
 
     while (perSource.length > 0) {
       const directive = this.pickNextDirective(perSource);
-      // TODO: should we bother copying the args?
-      dest.applyDirective(directive.name, directive.arguments(false));
-      perSource = this.removeDirective(directive, perSource);
+      if (!directive.definition?.repeatable && dest.hasAppliedDirective(directive.name)) {
+        this.reportMismatchError(
+          ERRORS.NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH,
+          `Non-repeatable directive @${directive.name} is applied to "${(dest as any)['coordinate'] ?? dest}" in multiple subgraphs but with incompatible arguments: it uses `,
+          dest,
+          sources,
+          (elt) => {
+            const args = elt.appliedDirectivesOf(directive.name).pop()?.arguments();
+            return args === undefined
+              ? undefined
+              : Object.values(args).length === 0 ? 'no arguments' : (`arguments ${valueToString(args)}`);
+          }
+        );
+        // We only want to report the error once, so we remove any remaining instance of
+        // the directive
+        perSource = perSource
+          .map((ds) => ds.filter((d) => d.name !== directive.name))
+          .filter((ds) => ds.length > 0) ;
+      } else {
+        dest.applyDirective(directive.name, directive.arguments(false));
+        perSource = this.removeDirective(directive, perSource);
+      }
     }
   }
 

docs/source/errors.md

@@ -43,6 +43,7 @@ The following errors may be raised by composition:
 | `KEY_INVALID_FIELDS` | The `fields` argument of a `@key` directive is invalid (it has invalid syntax, includes unknown fields, ...). | 2.0.0 |  |
 | `KEY_UNSUPPORTED_ON_INTERFACE` | A `@key` directive is used on an interface, which is not (yet) supported. | 2.0.0 |  |
 | `NO_QUERIES` | None of the composed subgraphs expose any query. | 2.0.0 |  |
+| `NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH` | A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different. | 2.0.0 |  |
 | `PROVIDES_FIELDS_HAS_ARGS` | The `fields` argument of a `@provides` directive includes a field defined with arguments (which is not currently supported). | 2.0.0 |  |
 | `PROVIDES_FIELDS_MISSING_EXTERNAL` | The `fields` argument of a `@provides` directive includes a field that is not marked as `@external`. | 0.x |  |
 | `PROVIDES_INVALID_FIELDS_TYPE` | The value passed to the `fields` argument of a `@provides` directive is not a string. | 2.0.0 |  |

gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts

@@ -147,7 +147,7 @@ describe('lifecycle hooks', () => {
     // the supergraph (even just formatting differences), this ID will change
     // and this test will have to updated. 
     expect(secondCall[0]!.compositionId).toEqual(
-      'ce2a5aa4525435780c32e19001e9fb337041a66f3c8cb7dc3e8617e97bc8610e',
+      '3ca7f295b11b070d1e1b56a698cbfabb70cb2b5912a4ff0ecae2fb91e8709838',
     );
     // second call should have previous info in the second arg
     expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);

internals-js/src/definitions.ts

@@ -1241,7 +1241,11 @@ export class Schema {
 
   directive(name: string): DirectiveDefinition | undefined {
     const directive = this._directives.get(name);
-    return directive ? directive : this._builtInDirectives.get(name);
+    return directive ? directive : this.builtInDirective(name);
+  }
+
+  builtInDirective(name: string): DirectiveDefinition | undefined {
+    return this._builtInDirectives.get(name);
   }
 
   *allNamedSchemaElement(): Generator<NamedSchemaElement<any, any, any>, void, undefined> {

internals-js/src/error.ts

@@ -274,6 +274,11 @@ const ARGUMENT_DEFAULT_MISMATCH = makeCodeDefinition(
   'An argument (of a field/directive) has a default value that is incompatible with that of other declarations of that same argument in other subgraphs.',
 );
 
+const NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH = makeCodeDefinition(
+  'NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH',
+  'A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different.',
+);
+
 const EXTENSION_WITH_NO_BASE = makeCodeDefinition(
   'EXTENSION_WITH_NO_BASE',
   'A subgraph is attempting to `extend` a type that is not originally defined in any known subgraph.',
@@ -359,6 +364,7 @@ export const ERRORS = {
   ARGUMENT_TYPE_MISMATCH,
   INPUT_FIELD_DEFAULT_MISMATCH,
   ARGUMENT_DEFAULT_MISMATCH,
+  NON_REPEATABLE_DIRECTIVE_ARGUMENTS_MISMATCH,
   EXTENSION_WITH_NO_BASE,
   EXTERNAL_MISSING_ON_BASE,
   INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH,