Fix repeatable custom directives (apollographql#2136)

Composing repeatable custom directives throws an error during composition saying they are not repeatable. This happens because we addDirectivesShallow, but the directive isn't fully created when this check is run. Solved by merging applied directives after all other elements are merged.

composition-js/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The Federation v0.x equivalent for this package can be found [here](https://github.com/apollographql/federation/blob/version-0.x/federation-js/CHANGELOG.md) on the `version-0.x` branch of this repo.
 
+## vNext
+
+- Fix composition of repeatable custom directives [PR #2136](https://github.com/apollographql/federation/pull/2136)
 - Allow fields with arguments in `@requires` [PR #2120](https://github.com/apollographql/federation/pull/2120).
 
 ## 2.1.0

composition-js/src/__tests__/__snapshots__/compose.composeDirective.test.ts.snap

@@ -58,8 +58,8 @@ type User
   @join__type(graph: SUBGRAPHB, key: \\"id\\")
 {
   id: Int
-  a: String @tag(name: \\"a\\", prop: \\"b\\") @join__field(graph: SUBGRAPHA)
-  b: String @mytag(name: \\"c\\") @join__field(graph: SUBGRAPHA)
+  a: String @join__field(graph: SUBGRAPHA) @tag(name: \\"a\\", prop: \\"b\\")
+  b: String @join__field(graph: SUBGRAPHA) @mytag(name: \\"c\\")
   subgraphB: String @join__field(graph: SUBGRAPHB)
 }"
 `;

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

@@ -2,7 +2,7 @@ import { assert, FEDERATION2_LINK_WTH_FULL_IMPORTS, printSchema, Schema } from '
 import { DirectiveLocation } from 'graphql';
 import gql from 'graphql-tag';
 import { composeServices, CompositionResult } from '../compose';
-import { errors } from './compose.test';
+import { errors } from './testHelper';
 
 const generateSubgraph = ({
   name,
@@ -925,5 +925,39 @@ describe('composing custom core directives', () => {
     expect(feature?.imports).toEqual([]);
     expect(feature?.nameInSchema).toEqual('mytag');
     expect(printSchema(schema)).toMatchSnapshot();
-    });
+  });
+
+  it('repeatable custom directives', () => {
+    const subgraphA = {
+      typeDefs: gql`
+        extend schema @composeDirective(name: "@auth")
+          @link(url: "https://specs.apollo.dev/federation/v2.1", import: ["@key", "@composeDirective", "@shareable"])
+          @link(url: "https://custom.dev/auth/v1.0", import: ["@auth"])
+          directive @auth(scope: String!) repeatable on FIELD_DEFINITION
+
+        type Query {
+          shared: String @shareable @auth(scope: "VIEWER")
+        }
+      `,
+      name: 'subgraphA',
+    };
+
+    const subgraphB = {
+      typeDefs: gql`
+        extend schema @composeDirective(name: "@auth")
+          @link(url: "https://specs.apollo.dev/federation/v2.1", import: ["@key", "@composeDirective", "@shareable"])
+          @link(url: "https://custom.dev/auth/v1.0", import: ["@auth"])
+          directive @auth(scope: String!) repeatable on FIELD_DEFINITION
+
+          type Query {
+          shared: String @shareable @auth(scope: "ADMIN")
+        }`,
+      name: 'subgraphB',
+    };
+
+    const result = composeServices([subgraphA, subgraphB]);
+    const schema = expectNoErrors(result);
+    const appliedDirectives = schema.elementByCoordinate('Query.shared')?.appliedDirectives;
+    expect(appliedDirectives?.map(d => [d.name, d.arguments()])).toMatchObject([['auth', { scope: 'VIEWER'}], ['auth', { scope: 'ADMIN'}]]);
+  });
 });

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

@@ -1,55 +1,26 @@
 import {
   asFed2SubgraphDocument,
   assert,
-  buildSchema,
   buildSubgraph,
-  extractSubgraphsFromSupergraph,
   FEDERATION2_LINK_WTH_FULL_IMPORTS,
   inaccessibleIdentity,
   InputObjectType,
   isObjectType,
   ObjectType,
   printSchema,
   printType,
-  Schema,
-  ServiceDefinition,
-  Subgraphs
 } from '@apollo/federation-internals';
-import { CompositionResult, composeServices, CompositionSuccess } from '../compose';
+import { CompositionResult, composeServices } from '../compose';
 import gql from 'graphql-tag';
 import './matchers';
 import { print } from 'graphql';
-
-export function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess {
-  if (r.errors) {
-    throw new Error(`Expected composition to succeed but got errors:\n${r.errors.join('\n\n')}`);
-  }
-}
-
-export function errors(r: CompositionResult): [string, string][] {
-  return r.errors?.map(e => [e.extensions.code as string, e.message]) ?? [];
-}
-
-// Returns [the supergraph schema, its api schema, the extracted subgraphs]
-export function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
-  // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing.
-  const schema = buildSchema(result.supergraphSdl);
-  expect(schema.isCoreSchema()).toBeTruthy();
-  return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)];
-}
-
-// Note that tests for composition involving fed1 subgraph are in `composeFed1Subgraphs.test.ts` so all the test of this
-// file are on fed2 subgraphs, but to avoid needing to add the proper `@link(...)` everytime, we inject it here automatically.
-export function composeAsFed2Subgraphs(services: ServiceDefinition[]): CompositionResult {
-  return composeServices(services.map((s) => asFed2Service(s)));
-}
-
-export function asFed2Service(service: ServiceDefinition): ServiceDefinition {
-  return {
-    ...service,
-    typeDefs: asFed2SubgraphDocument(service.typeDefs)
-  };
-}
+import {
+  assertCompositionSuccess,
+  schemas,
+  errors,
+  composeAsFed2Subgraphs,
+  asFed2Service,
+} from "./testHelper";
 
 describe('composition', () => {
   it('generates a valid supergraph', () => {

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

@@ -6,7 +6,7 @@ import {
   schemas,
   errors,
   composeAsFed2Subgraphs,
-} from "./compose.test";
+} from "./testHelper";
 
 describe("composition involving @override directive", () => {
   it.skip("@override whole type", () => {

composition-js/src/__tests__/testHelper.ts

@@ -0,0 +1,40 @@
+import {
+  asFed2SubgraphDocument,
+  buildSchema,
+  extractSubgraphsFromSupergraph,
+  Schema,
+  ServiceDefinition,
+  Subgraphs
+} from '@apollo/federation-internals';
+import { CompositionResult, composeServices, CompositionSuccess } from '../compose';
+
+export function assertCompositionSuccess(r: CompositionResult): asserts r is CompositionSuccess {
+  if (r.errors) {
+    throw new Error(`Expected composition to succeed but got errors:\n${r.errors.join('\n\n')}`);
+  }
+}
+
+export function errors(r: CompositionResult): [string, string][] {
+  return r.errors?.map(e => [e.extensions.code as string, e.message]) ?? [];
+}
+
+// Returns [the supergraph schema, its api schema, the extracted subgraphs]
+export function schemas(result: CompositionSuccess): [Schema, Schema, Subgraphs] {
+  // Note that we could user `result.schema`, but reparsing to ensure we don't lose anything with printing/parsing.
+  const schema = buildSchema(result.supergraphSdl);
+  expect(schema.isCoreSchema()).toBeTruthy();
+  return [schema, schema.toAPISchema(), extractSubgraphsFromSupergraph(schema)];
+}
+
+// Note that tests for composition involving fed1 subgraph are in `composeFed1Subgraphs.test.ts` so all the test of this
+// file are on fed2 subgraphs, but to avoid needing to add the proper `@link(...)` everytime, we inject it here automatically.
+export function composeAsFed2Subgraphs(services: ServiceDefinition[]): CompositionResult {
+  return composeServices(services.map((s) => asFed2Service(s)));
+}
+
+export function asFed2Service(service: ServiceDefinition): ServiceDefinition {
+  return {
+    ...service,
+    typeDefs: asFed2SubgraphDocument(service.typeDefs)
+  };
+}

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

@@ -1,7 +1,7 @@
 import { CompositionResult } from '../compose';
 import gql from 'graphql-tag';
 import './matchers';
-import { composeAsFed2Subgraphs } from './compose.test';
+import { composeAsFed2Subgraphs } from './testHelper';
 
 function errorMessages(r: CompositionResult): string[] {
   return r.errors?.map(e => e.message) ?? [];

composition-js/src/hints.ts

@@ -170,7 +170,7 @@ const UNUSED_ENUM_TYPE = makeCodeDefinition({
 });
 
 const INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS = makeCodeDefinition({
-  code: 'INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS',
+  code: 'INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS',
   level: HintLevel.WARN,
   description: 'A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different.',
 });

composition-js/src/merging/merge.ts

@@ -274,6 +274,11 @@ class Merger {
   readonly enumUsages = new Map<string, EnumTypeUsage>();
   private composeDirectiveManager: ComposeDirectiveManager;
   private mismatchReporter: MismatchReporter;
+  private appliedDirectivesToMerge: {
+    names: Set<string>,
+    sources: (SchemaElement<any, any> | undefined)[],
+    dest: SchemaElement<any, any>,
+  }[];
 
   constructor(readonly subgraphs: Subgraphs, readonly options: CompositionOptions) {
     this.names = subgraphs.names();
@@ -289,6 +294,7 @@ class Merger {
     );
     this.subgraphsSchema = subgraphs.values().map(subgraph => subgraph.schema);
     this.subgraphNamesToJoinSpecName = this.prepareSupergraph();
+    this.appliedDirectivesToMerge = [];
   }
 
   private prepareSupergraph(): Map<string, string> {
@@ -425,6 +431,8 @@ class Merger {
       this.errors.push(ERRORS.NO_QUERIES.err("No queries found in any subgraph: a supergraph must have a query root type."));
     }
 
+    this.mergeAllAppliedDirectives();
+
     // If we already encountered errors, `this.merged` is probably incomplete. Let's not risk adding errors that
     // are only an artifact of that incompleteness as it's confusing.
     if (this.errors.length === 0) {
@@ -626,7 +634,7 @@ class Merger {
     this.checkForExtensionWithNoBase(sources, dest);
     this.mergeDescription(sources, dest);
     this.addJoinType(sources, dest);
-    this.mergeAppliedDirectives(sources, dest);
+    this.recordAppliedDirectivesToMerge(sources, dest);
     switch (dest.kind) {
       case 'ScalarType':
         // Since we don't handle applied directives yet, we have nothing specific to do for scalars.
@@ -1034,7 +1042,7 @@ class Merger {
     // supergraph just because someone fat-fingered the type in an external definition. But after merging the non-external definitions, we
     // validate the external ones are consistent.
     this.mergeDescription(withoutExternal, dest);
-    this.mergeAppliedDirectives(withoutExternal, dest);
+    this.recordAppliedDirectivesToMerge(withoutExternal, dest);
     this.addArgumentsShallow(withoutExternal, dest);
     for (const destArg of dest.arguments()) {
       const subgraphArgs = withoutExternal.map(f => f?.argument(destArg.name));
@@ -1399,7 +1407,7 @@ class Merger {
 
   private mergeArgument(sources: (ArgumentDefinition<any> | undefined)[], dest: ArgumentDefinition<any>) {
     this.mergeDescription(sources, dest);
-    this.mergeAppliedDirectives(sources, dest);
+    this.recordAppliedDirectivesToMerge(sources, dest);
     this.mergeTypeReference(sources, dest, true);
     this.mergeDefaultValue(sources, dest, 'Argument');
   }
@@ -1567,7 +1575,7 @@ class Merger {
     // 2. it easier to see if the value is marked @inaccessible.
     const valueSources = sources.map(s => s?.value(value.name));
     this.mergeDescription(valueSources, value);
-    this.mergeAppliedDirectives(valueSources, value);
+    this.recordAppliedDirectivesToMerge(valueSources, value);
 
     const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(inaccessibleSpec.inaccessibleDirectiveSpec.name);
     const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph);
@@ -1694,7 +1702,7 @@ class Merger {
 
   private mergeInputField(sources: (InputFieldDefinition | undefined)[], dest: InputFieldDefinition) {
     this.mergeDescription(sources, dest);
-    this.mergeAppliedDirectives(sources, dest);
+    this.recordAppliedDirectivesToMerge(sources, dest);
     const allTypesEqual = this.mergeTypeReference(sources, dest, true);
     const mergeContext = new FieldMergeContext(sources);
     this.addJoinField({ sources, dest, allTypesEqual, mergeContext });
@@ -1911,11 +1919,33 @@ class Merger {
     return source.locations.filter(loc => isExecutableDirectiveLocation(loc));
   }
 
-  private mergeAppliedDirectives(sources: (SchemaElement<any, any> | undefined)[], dest: SchemaElement<any, any>) {
+  // In general, we want to merge applied directives after merging elements, the one exception
+  // is @inaccessible, which is necessary to exist in the supergraph for EnumValues to properly
+  // determine whether the fact that a value is both input / output will matter
+  private recordAppliedDirectivesToMerge(sources: (SchemaElement<any, any> | undefined)[], dest: SchemaElement<any, any>) {
+    const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(inaccessibleSpec.inaccessibleDirectiveSpec.name);
+    const inaccessibleName = inaccessibleInSupergraph?.name;
     const names = this.gatherAppliedDirectiveNames(sources);
-    for (const name of names) {
-      this.mergeAppliedDirective(name, sources, dest);
+
+    if (inaccessibleName && names.has(inaccessibleName)) {
+      this.mergeAppliedDirective(inaccessibleName, sources, dest);
+      names.delete(inaccessibleName);
+    }
+    this.appliedDirectivesToMerge.push({
+      names,
+      sources,
+      dest,
+    });
+  }
+
+  // to be called after elements are merged
+  private mergeAllAppliedDirectives() {
+    for (const { names, sources, dest } of this.appliedDirectivesToMerge) {
+      for (const name of names) {
+        this.mergeAppliedDirective(name, sources, dest);
+      }
     }
+    this.appliedDirectivesToMerge = [];
   }
 
   private gatherAppliedDirectiveNames(sources: (SchemaElement<any, any> | undefined)[]): Set<string> {
@@ -2024,7 +2054,7 @@ class Merger {
 
   private mergeSchemaDefinition(sources: SchemaDefinition[], dest: SchemaDefinition) {
     this.mergeDescription(sources, dest);
-    this.mergeAppliedDirectives(sources, dest);
+    this.recordAppliedDirectivesToMerge(sources, dest);
     // Before merging, we actually rename all the root types to their default name
     // in subgraphs (see federation.ts, `prepareSubgraphsForFederation`), so this
     // method should never report an error in practice as there should never be

docs/source/hints.md

@@ -33,7 +33,7 @@ The following hints might be generated during composition:
 | `INCONSISTENT_DESCRIPTION` | Indicates that an element has a description in more than one subgraph, and the descriptions are not equal. | `WARN` |
 | `INCONSISTENT_ARGUMENT_PRESENCE` | Indicates that an optional argument (of a field or directive definition) is not present in all subgraphs and will not be part of the supergraph. | `WARN` |
 | `FROM_SUBGRAPH_DOES_NOT_EXIST` | Source subgraph specified by @override directive does not exist | `WARN` |
-| `INCONSISTEN_NON_REPEATABLE_DIRECTIVE_ARGUMENTS` | A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different. | `WARN` |
+| `INCONSISTENT_NON_REPEATABLE_DIRECTIVE_ARGUMENTS` | A non-repeatable directive is applied to a schema element in different subgraphs but with arguments that are different. | `WARN` |
 | `DIRECTIVE_COMPOSITION_WARN` | Indicates that an issue was detected when composing custom directives. | `WARN` |
 
 </div>

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(
-      'cc95112b64179c4e549de788b051f44010a02877f568649a42caeeee6a135601',
+      '55f11e687010f75c791b917d9a46745b967c5b19d20443463ed075e500bff6c8',
     );
     // second call should have previous info in the second arg
     expect(secondCall[1]!.compositionId).toEqual(expectedFirstId);