From d4d4f1d299547076a5e493dbfe9fb0ccbe3ef6eb Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Tue, 16 Sep 2025 17:32:14 +0200 Subject: [PATCH 1/4] test: `@override` edge case --- __tests__/composition.spec.ts | 105 ++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 19 deletions(-) diff --git a/__tests__/composition.spec.ts b/__tests__/composition.spec.ts index cafab7c3..89a0dc13 100644 --- a/__tests__/composition.spec.ts +++ b/__tests__/composition.spec.ts @@ -248,6 +248,73 @@ testImplementations((api) => { `); }); + test("@override(from:) no usedOverridden in case the overriden field is @external", () => { + const result = composeServices([ + { + name: "a", + typeDefs: parse(/* GraphQL */ ` + extend schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link( + url: "https://specs.apollo.dev/federation/v2.3" + import: ["@tag", "@external", "@shareable", "@key", "@override"] + ) + + type Query { + ping: String + } + + interface Base { + id: String! + items: [String] + } + + type Object implements Base @key(fields: "id") @shareable { + id: String! + items: [String] @override(from: "b") + } + `), + }, + { + name: "b", + typeDefs: parse(/* GraphQL */ ` + extend schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link( + url: "https://specs.apollo.dev/federation/v2.0" + import: ["@tag", "@external", "@shareable", "@key"] + ) + + type Query { + pong: String + } + + interface Base { + id: String! + items: [String] + } + + type Object implements Base @key(fields: "id") @shareable { + id: String! + items: [String] @external + } + `), + }, + ]); + + expect(result.errors).toEqual(undefined); + const line = result + .supergraphSdl!.split("\n") + .find((line) => line.includes("items: [String] @join__field")); + + expect(line).toContain('@join__field(graph: A, override: "b")'); + expect(line).toContain("@join__field(graph: B, external: true)"); + // The usedOverridden should not appear + expect(line).not.toContain( + "@join__field(graph: B, usedOverridden: true, external: true)", + ); + }); + test("@join__field(external: true) when field is overridden", () => { let result = composeServices([ { @@ -7382,8 +7449,8 @@ testImplementations((api) => { typeDefs: parse(/* GraphQL */ ` extend schema @link( - url: "https://specs.apollo.dev/federation/v2.3" - import: ["@key", "@shareable"] + url: "https://specs.apollo.dev/federation/v2.3" + import: ["@key", "@shareable"] ) type Note @key(fields: "id") @shareable { id: ID! @@ -7391,16 +7458,16 @@ testImplementations((api) => { author: User } type User @key(fields: "id") { - id: ID! - name: String + id: ID! + name: String } - type PrivateNote @key(fields: "id") @shareable { - id: ID! - note: Note - } - type Query { - note: Note @shareable - privateNote: PrivateNote @shareable + type PrivateNote @key(fields: "id") @shareable { + id: ID! + note: Note + } + type Query { + note: Note @shareable + privateNote: PrivateNote @shareable } `), }, @@ -7425,9 +7492,9 @@ testImplementations((api) => { result = api.composeServices([ { name: "foo", - typeDefs: parse(/* GraphQL */ ` + typeDefs: parse(/* GraphQL */ ` extend schema - @link( + @link( url: "https://specs.apollo.dev/federation/v2.3" import: ["@key", "@external", "@provides", "@shareable"] ) @@ -7442,8 +7509,8 @@ testImplementations((api) => { type PrivateNote @key(fields: "id") @shareable { id: ID! note: Note @provides(fields: "name author { id }") - } - type Query { + } + type Query { note: Note @shareable privateNote: PrivateNote @shareable } @@ -7461,8 +7528,8 @@ testImplementations((api) => { id: ID! name: String author: User - } - type User @key(fields: "id") { + } + type User @key(fields: "id") { id: ID! name: String } @@ -7490,7 +7557,7 @@ testImplementations((api) => { type Note @key(fields: "id") @shareable { id: ID! tag: String - } + } `), }, ]); @@ -7510,7 +7577,7 @@ testImplementations((api) => { @join__field(external: true, graph: FOO) @join__field(graph: BAR) tag: String @join__field(graph: BAZ) - } + } `); }); From 1257300f80197301bff837d36e3e68fffe8e27a4 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Wed, 17 Sep 2025 09:13:35 +0200 Subject: [PATCH 2/4] fix: implementation --- __tests__/composition.spec.ts | 2 +- src/supergraph/composition/object-type.ts | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/__tests__/composition.spec.ts b/__tests__/composition.spec.ts index 89a0dc13..7bd6fc95 100644 --- a/__tests__/composition.spec.ts +++ b/__tests__/composition.spec.ts @@ -248,7 +248,7 @@ testImplementations((api) => { `); }); - test("@override(from:) no usedOverridden in case the overriden field is @external", () => { + test("@override(from:) no usedOverridden in case the overriden field is @external but has matching interface fields", () => { const result = composeServices([ { name: "a", diff --git a/src/supergraph/composition/object-type.ts b/src/supergraph/composition/object-type.ts index c7058488..3339c550 100644 --- a/src/supergraph/composition/object-type.ts +++ b/src/supergraph/composition/object-type.ts @@ -18,6 +18,7 @@ import { createObjectTypeNode, JoinFieldAST } from "./ast.js"; import type { Key, MapByGraph, TypeBuilder } from "./common.js"; import { convertToConst } from "./common.js"; import { InterfaceTypeFieldState } from "./interface-type.js"; +import { isExternal } from "node:util/types"; export function isRealExtension( meta: ObjectTypeStateInGraph, @@ -687,6 +688,16 @@ export function objectTypeBuilder(): TypeBuilder { ); const isRequired = f.required === true; + const inGraphs = + fieldNamesOfImplementedInterfaces[field.name]; + const hasMatchingInterfaceFieldInGraph: boolean = + inGraphs && inGraphs.has(graphId); + + // Apparently we need to keep it if it has a interface definition... + if (hasMatchingInterfaceFieldInGraph) { + return true; + } + return ( (isExternal && isRequired) || needsToPrintOverrideLabel || @@ -1014,6 +1025,10 @@ function provideUsedOverriddenValue( const isOverridden = field.override && graphNameToId(field.override) === graphId; + if (isOverridden && fieldStateInGraph.external) { + return false; + } + if ( isOverridden && (isUsedAsNonExternalKey || hasMatchingInterfaceFieldInGraph) From 6f7a46e16a759a5d2583c05c67f0146ae40a0ce6 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Wed, 17 Sep 2025 09:15:17 +0200 Subject: [PATCH 3/4] changeset --- .changeset/funny-cups-vanish.md | 5 +++++ src/supergraph/composition/object-type.ts | 1 - 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 .changeset/funny-cups-vanish.md diff --git a/.changeset/funny-cups-vanish.md b/.changeset/funny-cups-vanish.md new file mode 100644 index 00000000..c7c3751c --- /dev/null +++ b/.changeset/funny-cups-vanish.md @@ -0,0 +1,5 @@ +--- +"@theguild/federation-composition": patch +--- + +Do not add `usedOverridden: true to `@override` usage in case the overriden field is`@external` but has a matching interface field. diff --git a/src/supergraph/composition/object-type.ts b/src/supergraph/composition/object-type.ts index 3339c550..af915c35 100644 --- a/src/supergraph/composition/object-type.ts +++ b/src/supergraph/composition/object-type.ts @@ -18,7 +18,6 @@ import { createObjectTypeNode, JoinFieldAST } from "./ast.js"; import type { Key, MapByGraph, TypeBuilder } from "./common.js"; import { convertToConst } from "./common.js"; import { InterfaceTypeFieldState } from "./interface-type.js"; -import { isExternal } from "node:util/types"; export function isRealExtension( meta: ObjectTypeStateInGraph, From 7d4e20ef40d1e08e06121afd1ab75de9e65e51e7 Mon Sep 17 00:00:00 2001 From: Laurin Quast Date: Wed, 17 Sep 2025 09:17:44 +0200 Subject: [PATCH 4/4] Fix formatting in funny-cups-vanish.md --- .changeset/funny-cups-vanish.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/funny-cups-vanish.md b/.changeset/funny-cups-vanish.md index c7c3751c..4ee43351 100644 --- a/.changeset/funny-cups-vanish.md +++ b/.changeset/funny-cups-vanish.md @@ -2,4 +2,4 @@ "@theguild/federation-composition": patch --- -Do not add `usedOverridden: true to `@override` usage in case the overriden field is`@external` but has a matching interface field. +Do not add `usedOverridden: true` to `@override` usage in case the overriden field is`@external` but has a matching interface field.