Fix missing usedOverridden on non-external key field

the-guild-org · Jan 4, 2024 · 220dfc0 · 220dfc0
1 parent fdba937
commit 220dfc0
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 8 deletions.
diff --git a/.changeset/tidy-rules-relax.md b/.changeset/tidy-rules-relax.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Fix missing usedOverridden on non-external key field
diff --git a/__tests__/composition.spec.ts b/__tests__/composition.spec.ts
@@ -6810,6 +6810,54 @@ testImplementations(api => {
     `);
   });
 
+  test('join__field(usedOverridden: true) on a field that is a key field but not external', () => {
+    const result = api.composeServices([
+      {
+        name: 'a',
+        typeDefs: parse(/* GraphQL */ `
+          extend schema
+            @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key", "@override"])
+
+          type User @key(fields: "userId") {
+            userId: ID! @override(from: "b")
+            age: Int! @override(from: "b")
+          }
+
+          type Query {
+            a: String
+          }
+        `),
+      },
+      {
+        name: 'b',
+        typeDefs: parse(/* GraphQL */ `
+          extend schema
+            @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key", "@override"])
+
+          type User @key(fields: "userId") {
+            userId: ID!
+            age: Int!
+          }
+
+          type Query {
+            b: String
+          }
+        `),
+      },
+    ]);
+
+    assertCompositionSuccess(result);
+
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type User @join__type(graph: A, key: "userId") @join__type(graph: B, key: "userId") {
+        age: Int! @join__field(graph: A, override: "b")
+        userId: ID!
+          @join__field(graph: A, override: "b")
+          @join__field(graph: B, usedOverridden: true)
+      }
+    `);
+  });
+
   test('deduplicates directives', () => {
     const result = api.composeServices([
       {

diff --git a/src/supergraph/composition/object-type.ts b/src/supergraph/composition/object-type.ts
@@ -336,6 +336,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
             const override = meta.override ?? undefined;
             const usedOverridden = provideUsedOverriddenValue(
               field.name,
+              meta,
               overridesMap,
               fieldNamesOfImplementedInterfaces,
               graphId,
@@ -441,6 +442,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                 // it's not part of any @override(from:) and it's not used by any interface
                 !provideUsedOverriddenValue(
                   field.name,
+                  fieldInGraph,
                   overridesMap,
                   fieldNamesOfImplementedInterfaces,
                   graphId,
@@ -475,6 +477,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                   override: meta.override ?? undefined,
                   usedOverridden: provideUsedOverriddenValue(
                     field.name,
+                    meta,
                     overridesMap,
                     fieldNamesOfImplementedInterfaces,
                     graphId,
@@ -513,6 +516,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                   const isOverridden = overriddenGraphs.includes(graphId);
                   const needsToPrintUsedOverridden = provideUsedOverriddenValue(
                     field.name,
+                    f,
                     overridesMap,
                     fieldNamesOfImplementedInterfaces,
                     graphId,
@@ -536,6 +540,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                     override: meta.override ?? undefined,
                     usedOverridden: provideUsedOverriddenValue(
                       field.name,
+                      meta,
                       overridesMap,
                       fieldNamesOfImplementedInterfaces,
                       graphId,
@@ -567,6 +572,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                     (meta.shareable && !overriddenGraphs.includes(graphId)) ||
                     provideUsedOverriddenValue(
                       field.name,
+                      meta,
                       overridesMap,
                       fieldNamesOfImplementedInterfaces,
                       graphId,
@@ -578,6 +584,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
                   override: meta.override ?? undefined,
                   usedOverridden: provideUsedOverriddenValue(
                     field.name,
+                    meta,
                     overridesMap,
                     fieldNamesOfImplementedInterfaces,
                     graphId,
@@ -680,6 +687,7 @@ export function objectTypeBuilder(): TypeBuilder<ObjectType, ObjectTypeState> {
 
 function provideUsedOverriddenValue(
   fieldName: string,
+  fieldStateInGraph: FieldStateInGraph,
   overridesMap: {
     // @override(from: KEY): <where directive was used>
     [originalGraphId: string]: string;
@@ -691,18 +699,14 @@ function provideUsedOverriddenValue(
 ): boolean {
   const inGraphs = fieldNamesOfImplementedInterfaces[fieldName];
   const hasMatchingInterfaceFieldInGraph: boolean = inGraphs && inGraphs.has(graphId);
-
-  if (!hasMatchingInterfaceFieldInGraph) {
-    return false;
-  }
-
+  const isUsedAsNonExternalKey = fieldStateInGraph.usedAsKey && !fieldStateInGraph.external;
   const hasOverride = typeof overridesMap[graphId] === 'string';
 
-  if (!hasOverride) {
-    return false;
+  if (hasOverride && (isUsedAsNonExternalKey || hasMatchingInterfaceFieldInGraph)) {
+    return true;
   }
 
-  return true;
+  return false;
 }
 
 export type ObjectTypeState = {