diff --git a/.changeset/fix-composed-executable-directives.md b/.changeset/fix-composed-executable-directives.md new file mode 100644 index 00000000..980603c6 --- /dev/null +++ b/.changeset/fix-composed-executable-directives.md @@ -0,0 +1,7 @@ +--- +"@theguild/federation-composition": patch +--- + +fix: preserve composed directives with executable locations in supergraph + +`@composeDirective` was stripping directive definitions that only had executable locations (`QUERY`, `MUTATION`, `FIELD`, `VARIABLE_DEFINITION`) from the supergraph. Directives with schema-level locations (`OBJECT`, `FIELD_DEFINITION`) were unaffected. This fix skips the executable directive stripping logic for composed directives entirely, since they were explicitly requested via `@composeDirective`. diff --git a/README.md b/README.md index cf22f195..e2d49eea 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,12 @@ will continue to do so as we learn more about the Federation specification. Your feedback and bug reports are welcome and appreciated. +#### Known Behavioral Differences + +| Scenario | Apollo | Guild | +| --- | --- | --- | +| `@composeDirective` with conflicting argument types across subgraphs (e.g. `String!` vs `ID!`) | Silently picks one definition by reverse-alphabetical service name | Raises `FIELD_ARGUMENT_TYPE_MISMATCH` error | + ## Supergraph SDL Composition ✅ Done diff --git a/__tests__/supergraph/base.spec.ts b/__tests__/supergraph/base.spec.ts index a0f3f455..c64486cf 100644 --- a/__tests__/supergraph/base.spec.ts +++ b/__tests__/supergraph/base.spec.ts @@ -1,5 +1,6 @@ import { expect, test } from "vitest"; import { + assertCompositionFailure, assertCompositionSuccess, graphql, testVersions, @@ -247,6 +248,38 @@ testVersions((api, version) => { expect(result.supergraphSdl).not.toContain("directive @a(n: Int)"); }); + test("non-composed executable directive with QUERY | MUTATION is omitted if only defined within a single subgraph", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@key"]) + + directive @a(n: Int) on QUERY | MUTATION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@key"]) + + type Query { + b: Int + } + `, + }, + ]); + expect(result.supergraphSdl).not.toContain("directive @a"); + }); + test("executable directive only contains locations shared between all subgraphs", () => { const result = api.composeServices([ { @@ -450,5 +483,272 @@ testVersions((api, version) => { } `); }); + + test("composed directive with only executable locations is preserved in supergraph", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: String!) on QUERY | MUTATION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: []) + + type Query { + b: Int + } + `, + }, + ]); + + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(name: String!) on QUERY | MUTATION + `); + }); + + test("composed directive with VARIABLE_DEFINITION and FIELD locations is preserved in supergraph", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(provider: String!) on VARIABLE_DEFINITION | FIELD + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: []) + + type Query { + b: Int + } + `, + }, + ]); + + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(provider: String!) on VARIABLE_DEFINITION | FIELD + `); + }); + + test("composed directive with mixed schema and executable locations is preserved when only one subgraph defines it", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(n: Int) on FIELD | FIELD_DEFINITION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: []) + + type Query { + b: Int + } + `, + }, + ]); + + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(n: Int) on FIELD | FIELD_DEFINITION + `); + }); + + test("composed directive with only executable locations is preserved when both subgraphs define it", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: String!) on QUERY | MUTATION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: String!) on QUERY | MUTATION + + type Query { + b: Int + } + `, + }, + ]); + + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(name: String!) on QUERY | MUTATION + `); + }); + + // Apollo silently picks a winning definition by reverse-alphabetical service name + // (i.e., "b" wins over "a") when composed directive definitions conflict across subgraphs. + // Guild raises FIELD_ARGUMENT_TYPE_MISMATCH, which is stricter and arguably more correct. + test("conflicting composed directive definitions across subgraphs", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: String!) on QUERY | MUTATION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: ID!) on QUERY | MUTATION + + type Query { + b: Int + } + `, + }, + ]); + + if (api.library === "apollo") { + // Apollo silently resolves the conflict by picking one definition + // by reverse-alphabetical service name. Service "b" wins over "a", + // so the ID! type from subgraph "b" is used. + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(name: ID!) on QUERY | MUTATION + `); + } else { + // Guild correctly rejects conflicting argument types + assertCompositionFailure(result); + expect(result.errors?.[0]?.message).toContain( + 'Type of argument "@a(name:)" is incompatible across subgraphs', + ); + } + }); + + // When the type definitions are swapped between services, Apollo picks a + // different winner, confirming the resolution is based on service name sort + // order, not schema correctness. + test("conflicting composed directive definitions; swapped types changes Apollo winner", () => { + const result = api.composeServices([ + { + name: "a", + url: "http://a.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: ID!) on QUERY | MUTATION + + type Query { + a: Int + } + `, + }, + { + name: "b", + url: "http://b.com", + typeDefs: graphql` + extend schema + @link(url: "https://specs.apollo.dev/federation/${version}", import: ["@composeDirective"]) + @link(url: "https://a.dev/a/v1.0", import: ["@a"]) + @composeDirective(name: "@a") + + directive @a(name: String!) on QUERY | MUTATION + + type Query { + b: Int + } + `, + }, + ]); + + if (api.library === "apollo") { + // Now service "b" has String! instead of ID!, and Apollo picks "b" again; + // so this time String! wins, proving the winner is name-sorted, not type-based. + assertCompositionSuccess(result); + expect(result.supergraphSdl).toContainGraphQL(graphql` + directive @a(name: String!) on QUERY | MUTATION + `); + } else { + // Guild rejects regardless of which service has which type + assertCompositionFailure(result); + expect(result.errors?.[0]?.message).toContain( + 'Type of argument "@a(name:)" is incompatible across subgraphs', + ); + } + }); } }); diff --git a/src/supergraph/state.ts b/src/supergraph/state.ts index 133d5f7a..9c4de8d4 100644 --- a/src/supergraph/state.ts +++ b/src/supergraph/state.ts @@ -301,9 +301,14 @@ export function createSupergraphStateBuilder() { build() { const transformFields = createFieldsTransformer(state); - // Strip out all executable directives that are not defined or identical every supergraph + // Strip non-composed executable directives that are not defined identically in every subgraph. + // Composed directives are preserved unconditionally, they were explicitly requested via @composeDirective. for (const directiveState of state.directives.values()) { - if (!directiveState.isExecutable || !directiveState.byGraph.size) { + if ( + !directiveState.isExecutable || + !directiveState.byGraph.size || + directiveState.composed + ) { continue; } @@ -325,11 +330,6 @@ export function createSupergraphStateBuilder() { directiveState.locations.forEach((location) => { // if it is not an executable location -> remove if (!isExecutableDirectiveLocation(location)) { - // If it is a compose directive we want to retain the schema location. - if (directiveState.composed) { - return; - } - directiveState.locations.delete(location); return; }