Fix repeatable custom directives

composition-js/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 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)
+
 ## 2.1.0
 
 - Don't apply @shareable when upgrading fed1 supergraphs if it's already @shareable [PR #2043](https://github.com/apollographql/federation/pull/2043)

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

@@ -269,6 +269,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();
@@ -284,6 +289,7 @@ class Merger {
     );
     this.subgraphsSchema = subgraphs.values().map(subgraph => subgraph.schema);
     this.subgraphNamesToJoinSpecName = this.prepareSupergraph();
+    this.appliedDirectivesToMerge = [];
   }
 
   private prepareSupergraph(): Map<string, string> {
@@ -409,6 +415,8 @@ class Merger {
       this.mergeDirectiveDefinition(this.subgraphsSchema.map(s => s.directive(definition.name)), definition);
     }
 
+    this.mergeAllAppliedDirectives();
+
     // We merge enum dead last because enums can be used as both input and output types and the merging behavior
     // depends on their usage and it's easier to check said usage if everything else has been merge (at least
     // anything that may use an enum type, so all fields and arguments).
@@ -1906,10 +1914,31 @@ class Merger {
     return source.locations.filter(loc => isExecutableDirectiveLocation(loc));
   }
 
+  // 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 mergeAppliedDirectives(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);
+      }
     }
   }
 

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