enhance(federation/delegate): cleanup extra fields, empty inline frag…

…ments and duplicate `__typename`s (#6403) * enhance(federation/delegate): cleanup extra fields, empty inline fragments and duplicate `__typename`s * Better cleanup * Fix snapshot release * Use Guild Bot token * Fix
ardatan · Aug 1, 2024 · 3803897 · 3803897
1 parent 80aad37
commit 3803897
Show file tree

Hide file tree

Showing 82 changed files with 118 additions and 6,172 deletions.
diff --git a/.changeset/eleven-bobcats-bathe.md b/.changeset/eleven-bobcats-bathe.md
@@ -0,0 +1,6 @@
+---
+'@graphql-tools/federation': patch
+'@graphql-tools/delegate': patch
+---
+
+Cleanup extra fields, empty inline fragments and duplicate \_\_typename fields
diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml
@@ -12,17 +12,23 @@ jobs:
       githubToken: ${{ secrets.GUILD_BOT_TOKEN }}
 
   alpha:
+    permissions:
+      contents: read
+      id-token: write
     uses: the-guild-org/shared-config/.github/workflows/release-snapshot.yml@main
     if: ${{ github.event.pull_request.title != 'Upcoming Release Changes' }}
     with:
       npmTag: alpha
       buildScript: build
       nodeVersion: 18
     secrets:
-      githubToken: ${{ secrets.GITHUB_TOKEN }}
+      githubToken: ${{ secrets.GUILD_BOT_TOKEN }}
       npmToken: ${{ secrets.NODE_AUTH_TOKEN }}
 
   release-candidate:
+    permissions:
+      contents: read
+      id-token: write
     uses: the-guild-org/shared-config/.github/workflows/release-snapshot.yml@main
     if: ${{ github.event.pull_request.title == 'Upcoming Release Changes' }}
     with:
@@ -31,5 +37,5 @@ jobs:
       restoreDeletedChangesets: true
       nodeVersion: 18
     secrets:
-      githubToken: ${{ secrets.GITHUB_TOKEN }}
+      githubToken: ${{ secrets.GUILD_BOT_TOKEN }}
       npmToken: ${{ secrets.NODE_AUTH_TOKEN }}
diff --git a/package.json b/package.json
@@ -33,14 +33,14 @@
     "build:api-docs": "ts-node --transpileOnly --compiler-options='{\"module\":\"commonjs\"}' scripts/build-api-docs",
     "ci:lint": "cross-env \"ESLINT_USE_FLAT_CONFIG=false\" eslint --ext .ts . --output-file eslint_report.json --format json",
     "clean-dist": "rimraf \"packages/**/dist\" && rimraf \".bob\"",
-    "fetch-federation-tests": "ts-node --transpileOnly --compiler-options='{\"module\":\"commonjs\"}' scripts/fetch-federation-tests",
     "lint": "cross-env \"ESLINT_USE_FLAT_CONFIG=false\" eslint --ext .ts .",
     "postinstall": "patch-package && husky install",
     "prerelease": "yarn build",
     "prettier": "prettier --cache --ignore-path .prettierignore --write --list-different .",
     "prettier:check": "prettier --cache --ignore-path .prettierignore --check .",
     "release": "changeset publish",
     "test": "jest --no-watchman",
+    "test-fed-compat": "ts-node --transpileOnly --compiler-options='{\"module\":\"commonjs\"}' scripts/fetch-federation-tests && yarn test federation-compat",
     "test:leaks": "cross-env \"LEAK_TEST=1\" jest --no-watchman --detectOpenHandles --detectLeaks --logHeapUsage",
     "ts:check": "tsc --noEmit"
   },

diff --git a/packages/delegate/src/finalizeGatewayRequest.ts b/packages/delegate/src/finalizeGatewayRequest.ts
@@ -99,6 +99,15 @@ function finalizeGatewayDocument(
       );
     }
 
+    // Do not add the operation if it only asks for __typename
+    if (
+      selectionSet.selections.length === 1 &&
+      selectionSet.selections[0].kind === Kind.FIELD &&
+      selectionSet.selections[0].name.value === '__typename'
+    ) {
+      continue;
+    }
+
     newOperations.push({
       kind: Kind.OPERATION_DEFINITION,
       operation: operation.operation,
@@ -109,6 +118,17 @@ function finalizeGatewayDocument(
     });
   }
 
+  if (!newOperations.length) {
+    throw createGraphQLError(
+      'Failed to create a gateway request. The request must contain at least one operation.',
+      {
+        extensions: {
+          [CRITICAL_ERROR]: true,
+        },
+      },
+    );
+  }
+
   const newDocument: DocumentNode = {
     kind: Kind.DOCUMENT,
     definitions: [...newOperations, ...newFragments],
@@ -141,31 +161,6 @@ export function finalizeGatewayRequest<TContext>(
     operations,
   );
 
-  // Fail if the query is only the root __typename field
-
-  if (
-    newDocument.definitions.length === 1 &&
-    newDocument.definitions[0].kind === Kind.OPERATION_DEFINITION
-  ) {
-    const operation = newDocument.definitions[0] as OperationDefinitionNode;
-    if (
-      operation.selectionSet.selections.length === 1 &&
-      operation.selectionSet.selections[0].kind === Kind.FIELD
-    ) {
-      const field = operation.selectionSet.selections[0] as any;
-      if (field.name.value === '__typename' && operation.operation === 'query') {
-        throw createGraphQLError(
-          'Failed to create a gateway request. The query must contain at least one selection.',
-          {
-            extensions: {
-              [CRITICAL_ERROR]: true,
-            },
-          },
-        );
-      }
-    }
-  }
-
   const newVariables: Record<string, any> = {};
   if (variables != null) {
     for (const variableName of usedVariables) {
@@ -176,13 +171,77 @@ export function finalizeGatewayRequest<TContext>(
     }
   }
 
+  let cleanedUpDocument = newDocument;
+
+  // TODO: Optimize this internally later
+  cleanedUpDocument = visit(newDocument, {
+    // Cleanup extra __typename fields
+    SelectionSet: {
+      leave(node) {
+        const { hasTypeNameField, selections } = filterTypenameFields(node.selections);
+        if (hasTypeNameField) {
+          selections.unshift({
+            kind: Kind.FIELD,
+            name: {
+              kind: Kind.NAME,
+              value: '__typename',
+            },
+          });
+        }
+        return {
+          ...node,
+          selections,
+        };
+      },
+    },
+    // Cleanup empty inline fragments
+    InlineFragment: {
+      leave(node) {
+        // No need __typename in inline fragment
+        const { selections } = filterTypenameFields(node.selectionSet.selections);
+        if (selections.length === 0) {
+          return null;
+        }
+        return {
+          ...node,
+          selectionSet: {
+            ...node.selectionSet,
+            selections,
+          },
+        };
+      },
+    },
+  });
+
   return {
     ...originalRequest,
-    document: newDocument,
+    document: cleanedUpDocument,
     variables: newVariables,
   };
 }
 
+function isTypeNameField(selection: SelectionNode): boolean {
+  return selection.kind === Kind.FIELD && !selection.alias && selection.name.value === '__typename';
+}
+
+function filterTypenameFields(selections: readonly SelectionNode[]): {
+  hasTypeNameField: boolean;
+  selections: SelectionNode[];
+} {
+  let hasTypeNameField = false;
+  const filteredSelections = selections.filter(selection => {
+    if (isTypeNameField(selection)) {
+      hasTypeNameField = true;
+      return false;
+    }
+    return true;
+  });
+  return {
+    hasTypeNameField,
+    selections: filteredSelections,
+  };
+}
+
 function addVariablesToRootFields(
   targetSchema: GraphQLSchema,
   operations: Array<OperationDefinitionNode>,

diff --git a/packages/federation/test/federation-compatibility.test.ts b/packages/federation/test/federation-compatibility.test.ts
@@ -21,7 +21,12 @@ import {
 } from '@graphql-tools/utils';
 import { getStitchedSchemaFromSupergraphSdl } from '../src/supergraph';
 
-describe.skip('Federation Compatibility', () => {
+describe('Federation Compatibility', () => {
+  if (!existsSync(join(__dirname, 'fixtures', 'federation-compatibility'))) {
+    console.warn('Make sure you fetched the fixtures from the API first');
+    it.skip('skipping tests', () => {});
+    return;
+  }
   const fixturesDir = join(__dirname, 'fixtures', 'federation-compatibility');
   readdirSync(fixturesDir).forEach(supergraphName => {
     const supergraphFixturesDir = join(fixturesDir, supergraphName);

diff --git a/packages/federation/test/fixtures/.gitignore b/packages/federation/test/fixtures/.gitignore
@@ -0,0 +1 @@
+federation-compatibility