A lot of fixes (#36)

- Visit every field in provides and requires directives - Fix unnecessary join__field(override:) on Query fields when it points to non-existing subgraph - Deduplicate composed directives - Remove duplicated link spec definitions - Drop unused fields marked with @external only in a single type in Fed v1
the-guild-org · Jan 4, 2024 · fdba937 · fdba937
1 parent cc6a995
commit fdba937
Show file tree

Hide file tree

Showing 13 changed files with 595 additions and 244 deletions.
diff --git a/.changeset/cold-keys-dream.md b/.changeset/cold-keys-dream.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Visit every field in provides and requires directives
diff --git a/.changeset/heavy-steaks-agree.md b/.changeset/heavy-steaks-agree.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Fix unnecessary join\_\_field(override:) on Query fields when it points to non-existing subgraph
diff --git a/.changeset/rich-sheep-hide.md b/.changeset/rich-sheep-hide.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Deduplicate composed directives
diff --git a/.changeset/silly-parents-smash.md b/.changeset/silly-parents-smash.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Remove duplicated link spec definitions
diff --git a/.changeset/thin-vans-chew.md b/.changeset/thin-vans-chew.md
@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+Drop unused fields marked with @external only in a single type in Fed v1
diff --git a/__tests__/composition.spec.ts b/__tests__/composition.spec.ts
@@ -6661,4 +6661,215 @@ testImplementations(api => {
       }
     `);
   });
+
+  test('print join__field external the field is required in a deeply nested selection set', () => {
+    const result = api.composeServices([
+      {
+        name: 'a',
+        typeDefs: parse(/* GraphQL */ `
+          type Query {
+            a: String
+          }
+
+          type User {
+            id: ID!
+            age: Int!
+            name: String!
+          }
+        `),
+      },
+      {
+        name: 'b',
+        typeDefs: parse(/* GraphQL */ `
+          type Query {
+            b: String
+          }
+
+          type Book {
+            author: User @requires(fields: "author { name }")
+          }
+
+          extend type User {
+            name: String! @external
+          }
+        `),
+      },
+    ]);
+
+    assertCompositionSuccess(result);
+
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type User @join__type(graph: A) @join__type(graph: B) {
+        name: String! @join__field(external: true, graph: B) @join__field(graph: A)
+        id: ID! @join__field(graph: A)
+        age: Int! @join__field(graph: A)
+      }
+    `);
+  });
+
+  test('Query field with @override that points to non-existing subgraph', () => {
+    let result = api.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: ["@override"])
+
+          type Query {
+            a: String @override(from: "non-existing")
+          }
+        `),
+      },
+    ]);
+    assertCompositionSuccess(result);
+
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type Query @join__type(graph: A) {
+        a: String
+      }
+    `);
+
+    result = api.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: ["@override"])
+
+          type Query {
+            a: String @override(from: "non-existing")
+          }
+        `),
+      },
+      {
+        name: 'b',
+        typeDefs: parse(/* GraphQL */ `
+          extend schema
+            @link(url: "https://specs.apollo.dev/link/v1.0")
+            @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@override"])
+
+          type Query {
+            b: String
+          }
+        `),
+      },
+    ]);
+    assertCompositionSuccess(result);
+
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type Query @join__type(graph: A) @join__type(graph: B) {
+        a: String @join__field(graph: A, override: "non-existing")
+        b: String @join__field(graph: B)
+      }
+    `);
+  });
+
+  test('drop unused external fields from Federation v1 subgraphs', () => {
+    const result = api.composeServices([
+      {
+        name: 'a',
+        typeDefs: parse(/* GraphQL */ `
+          type User @key(fields: "id") {
+            id: ID!
+            name: String @external
+            age: Int!
+          }
+
+          type Query {
+            a: String
+          }
+        `),
+      },
+      {
+        name: 'b',
+        typeDefs: parse(/* GraphQL */ `
+          type User @key(fields: "id") {
+            id: ID!
+            age: Int! @external
+            birthday: String @requires(fields: "age")
+          }
+
+          type Query {
+            b: String
+          }
+        `),
+      },
+    ]);
+
+    assertCompositionSuccess(result);
+
+    // No User.name, it's dropped
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type User @join__type(graph: A, key: "id") @join__type(graph: B, key: "id") {
+        id: ID!
+        age: Int! @join__field(external: true, graph: B) @join__field(graph: A)
+        birthday: String @join__field(graph: B, requires: "age")
+      }
+    `);
+  });
+
+  test('deduplicates directives', () => {
+    const result = api.composeServices([
+      {
+        name: 'a',
+        typeDefs: parse(/* GraphQL */ `
+          extend schema
+            @link(
+              url: "https://specs.apollo.dev/federation/v2.3"
+              import: ["@key", "@composeDirective"]
+            )
+            @link(url: "https://myspecs.dev/lowercase/v1.0", import: ["@lowercase"])
+            @composeDirective(name: "@lowercase")
+
+          directive @lowercase on FIELD_DEFINITION
+
+          type User @key(fields: "id") {
+            id: ID! @lowercase
+            age: Int!
+          }
+
+          type Query {
+            a: String
+          }
+        `),
+      },
+      {
+        name: 'b',
+        typeDefs: parse(/* GraphQL */ `
+          extend schema
+            @link(
+              url: "https://specs.apollo.dev/federation/v2.3"
+              import: ["@key", "@external", "@requires", "@composeDirective"]
+            )
+            @link(url: "https://myspecs.dev/lowercase/v1.0", import: ["@lowercase"])
+            @composeDirective(name: "@lowercase")
+
+          directive @lowercase on FIELD_DEFINITION
+
+          type User @key(fields: "id") {
+            id: ID! @lowercase
+            age: Int! @external
+            birthday: String @requires(fields: "age")
+          }
+
+          type Query {
+            b: String
+          }
+        `),
+      },
+    ]);
+
+    assertCompositionSuccess(result);
+
+    // No User.name, it's dropped
+    expect(result.supergraphSdl).toContainGraphQL(/* GraphQL */ `
+      type User @join__type(graph: A, key: "id") @join__type(graph: B, key: "id") {
+        id: ID! @lowercase
+        age: Int! @join__field(external: true, graph: B) @join__field(graph: A)
+        birthday: String @join__field(graph: B, requires: "age")
+      }
+    `);
+  });
 });
diff --git a/__tests__/subgraph/link-directive.spec.ts b/__tests__/subgraph/link-directive.spec.ts
@@ -1,5 +1,5 @@
 import { expect, test } from 'vitest';
-import { graphql, testVersions } from '../shared/testkit.js';
+import { assertCompositionSuccess, graphql, testVersions } from '../shared/testkit.js';
 
 testVersions((api, version) => {
   test('INVALID_LINK_IDENTIFIER', () => {
@@ -286,4 +286,42 @@ testVersions((api, version) => {
       }),
     );
   });
+
+  test('remove duplicated link spec definitions', () => {
+    assertCompositionSuccess(
+      api.composeServices([
+        {
+          name: 'users',
+          typeDefs: graphql`
+            schema
+              @link(url: "https://specs.apollo.dev/link/v1.0")
+              @link(
+                url: "https://specs.apollo.dev/federation/${version}"
+                import: ["@shareable"]
+              ) {
+              query: Query
+            }
+
+            directive @link(
+              url: String!
+              as: String
+              for: link__Purpose
+              import: [link__Import]
+            ) repeatable on SCHEMA
+
+            scalar link__Import
+
+            enum link__Purpose {
+              SECURITY
+              EXECUTION
+            }
+
+            type Query {
+              foo: String
+            }
+          `,
+        },
+      ]),
+    );
+  });
 });
diff --git a/src/subgraph/helpers.ts b/src/subgraph/helpers.ts
@@ -248,7 +248,9 @@ export function visitFields({
       break;
     }
 
-    if (interceptDirective && selection.directives?.length) {
+    const isTypename = selection.name.value === '__typename';
+
+    if (!isTypename && interceptDirective && selection.directives?.length) {
       for (const directive of selection.directives) {
         interceptDirective({
           directiveName: directive.name.value,
@@ -257,29 +259,31 @@ export function visitFields({
       }
     }
 
-    context.markAsUsed(
-      'fields',
-      typeDefinition.kind,
-      typeDefinition.name.value,
-      selectionFieldDef.name.value,
-    );
+    if (!isTypename) {
+      context.markAsUsed(
+        'fields',
+        typeDefinition.kind,
+        typeDefinition.name.value,
+        selectionFieldDef.name.value,
+      );
+    }
 
-    if (interceptField) {
+    if (!isTypename && interceptField) {
       interceptField({
         typeDefinition,
         fieldName: selection.name.value,
       });
     }
 
-    if (selectionFieldDef.arguments?.length && interceptArguments) {
+    if (!isTypename && selectionFieldDef.arguments?.length && interceptArguments) {
       interceptArguments({
         typeDefinition,
         fieldName: selection.name.value,
       });
       continue;
     }
 
-    if (interceptNonExternalField || interceptExternalField) {
+    if (!isTypename && (interceptNonExternalField || interceptExternalField)) {
       const isExternal = selectionFieldDef.directives?.some(d =>
         context.isAvailableFederationDirective('external', d),
       );
@@ -318,6 +322,7 @@ export function visitFields({
     }
 
     if (
+      !isTypename &&
       interceptInterfaceType &&
       (innerTypeDef.kind === Kind.INTERFACE_TYPE_DEFINITION ||
         innerTypeDef.kind === Kind.INTERFACE_TYPE_EXTENSION)
@@ -345,9 +350,13 @@ export function visitFields({
       context,
       selectionSet: innerSelection,
       typeDefinition: innerTypeDef,
+      interceptField,
       interceptArguments,
       interceptUnknownField,
       interceptInterfaceType,
+      interceptDirective,
+      interceptExternalField,
+      interceptFieldWithMissingSelectionSet,
     });
   }
 }

diff --git a/src/subgraph/validation/rules/elements/provides.ts b/src/subgraph/validation/rules/elements/provides.ts
@@ -262,10 +262,12 @@ export function ProvidesRules(context: SubgraphValidationContext): ASTVisitor {
                       info.typeDefinition.kind === Kind.OBJECT_TYPE_DEFINITION ||
                       info.typeDefinition.kind === Kind.OBJECT_TYPE_EXTENSION
                     ) {
-                      context.stateBuilder.objectType.field.markAsProvided(
-                        info.typeDefinition.name.value,
-                        info.fieldName,
-                      );
+                      if (info.fieldName !== '__typename') {
+                        context.stateBuilder.objectType.field.markAsProvided(
+                          info.typeDefinition.name.value,
+                          info.fieldName,
+                        );
+                      }
                     }
                   },
                 });