diff --git a/composition-js/CHANGELOG.md b/composition-js/CHANGELOG.md index 30541f6b6..975f304b2 100644 --- a/composition-js/CHANGELOG.md +++ b/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 diff --git a/composition-js/src/__tests__/__snapshots__/compose.composeDirective.test.ts.snap b/composition-js/src/__tests__/__snapshots__/compose.composeDirective.test.ts.snap index 37c75771c..8ec5c03b0 100644 --- a/composition-js/src/__tests__/__snapshots__/compose.composeDirective.test.ts.snap +++ b/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) }" `; diff --git a/composition-js/src/__tests__/compose.composeDirective.test.ts b/composition-js/src/__tests__/compose.composeDirective.test.ts index 6d14ad7b4..059004662 100644 --- a/composition-js/src/__tests__/compose.composeDirective.test.ts +++ b/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'}]]); + }); }); diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index 0911666b7..fee75ea5f 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -1,9 +1,7 @@ import { asFed2SubgraphDocument, assert, - buildSchema, buildSubgraph, - extractSubgraphsFromSupergraph, FEDERATION2_LINK_WTH_FULL_IMPORTS, inaccessibleIdentity, InputObjectType, @@ -11,45 +9,18 @@ import { 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', () => { diff --git a/composition-js/src/__tests__/override.compose.test.ts b/composition-js/src/__tests__/override.compose.test.ts index f53515602..7a4eb26d2 100644 --- a/composition-js/src/__tests__/override.compose.test.ts +++ b/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", () => { diff --git a/composition-js/src/__tests__/testHelper.ts b/composition-js/src/__tests__/testHelper.ts new file mode 100644 index 000000000..2d5a7ab18 --- /dev/null +++ b/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) + }; +} diff --git a/composition-js/src/__tests__/validation_errors.test.ts b/composition-js/src/__tests__/validation_errors.test.ts index b4977aa13..7f909f1d6 100644 --- a/composition-js/src/__tests__/validation_errors.test.ts +++ b/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) ?? []; diff --git a/composition-js/src/hints.ts b/composition-js/src/hints.ts index c171bcc77..ef6ed6297 100644 --- a/composition-js/src/hints.ts +++ b/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.', }); diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index d26743613..057a9efb3 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -274,6 +274,11 @@ class Merger { readonly enumUsages = new Map(); private composeDirectiveManager: ComposeDirectiveManager; private mismatchReporter: MismatchReporter; + private appliedDirectivesToMerge: { + names: Set, + sources: (SchemaElement | undefined)[], + dest: SchemaElement, + }[]; 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 { @@ -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 | undefined)[], dest: ArgumentDefinition) { 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 | undefined)[], dest: SchemaElement) { + // 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 | undefined)[], dest: SchemaElement) { + 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 | undefined)[]): Set { @@ -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 diff --git a/docs/source/hints.md b/docs/source/hints.md index be6685575..92752f651 100644 --- a/docs/source/hints.md +++ b/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` | diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index acf14f46b..9a5e61017 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/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);