Ensure type extension fields are properly considered in unused schema detection (#6586)

Author: kamilkisiela

.changeset/wet-panthers-notice.md

@@ -0,0 +1,5 @@
+---
+'hive': patch
+---
+
+Corrected an issue where fields from type extensions were not marked as unused when appropriate

integration-tests/tests/api/schema/unused.spec.ts

@@ -0,0 +1,161 @@
+import { addDays, formatISO } from 'date-fns';
+import { ProjectType } from 'testkit/gql/graphql';
+import { waitFor } from '../../../testkit/flow';
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { graphql } from '../../../testkit/gql';
+import { execute } from '../../../testkit/graphql';
+import { initSeed } from '../../../testkit/seed';
+
+const IntegrationTestsUnusedSchemaQuery = graphql(/* GraphQL */ `
+  query IntegrationTestsUnusedSchema(
+    $usageInput: UnusedSchemaExplorerUsageInput!
+    $targetRef: TargetReferenceInput!
+  ) {
+    latestValidVersion(target: $targetRef) {
+      unusedSchema(usage: $usageInput) {
+        types {
+          __typename
+          ... on GraphQLObjectType {
+            name
+            fields {
+              name
+              usage {
+                isUsed
+              }
+            }
+          }
+        }
+      }
+    }
+  }
+`);
+
+test.concurrent(
+  'a field from a type extension should be a part of unused schema if unused',
+  async ({ expect }) => {
+    const { createOrg } = await initSeed().createOwner();
+    const { createProject } = await createOrg();
+    const { createTargetAccessToken, target } = await createProject(ProjectType.Single);
+
+    // Create a token with write rights
+    const writeToken = await createTargetAccessToken({});
+
+    // Publish schema with write rights
+    const publishResult = await writeToken
+      .publishSchema({
+        sdl: /* GraphQL */ `
+          type Query {
+            user: User
+          }
+
+          type User {
+            id: ID!
+          }
+
+          extend type User {
+            name: String
+          }
+        `,
+      })
+      .then(r => r.expectNoGraphQLErrors());
+    // Schema publish should be successful
+    expect(publishResult.schemaPublish.__typename).toBe('SchemaPublishSuccess');
+
+    const period = {
+      from: formatISO(addDays(new Date(), -7)),
+      to: formatISO(addDays(new Date(), 1)),
+    };
+
+    const firstQuery = await execute({
+      document: IntegrationTestsUnusedSchemaQuery,
+      variables: {
+        targetRef: {
+          byId: target.id,
+        },
+        usageInput: {
+          period,
+        },
+      },
+      authToken: writeToken.secret,
+    }).then(r => r.expectNoGraphQLErrors());
+
+    expect(firstQuery.latestValidVersion?.unusedSchema?.types).toHaveLength(2);
+
+    let userType = firstQuery.latestValidVersion?.unusedSchema?.types.find(t =>
+      'name' in t ? t.name === 'User' : false,
+    );
+
+    if (!userType) {
+      throw new Error('User type not found');
+    }
+
+    if (userType.__typename !== 'GraphQLObjectType') {
+      throw new Error('User type is not an object type');
+    }
+
+    let idField = userType.fields.find(f => f.name === 'id');
+    let nameField = userType.fields.find(f => f.name === 'name');
+
+    expect(idField?.usage.isUsed).toEqual(false);
+    expect(nameField, 'User.name should exist').toBeDefined();
+    expect(nameField?.usage.isUsed, 'User.name should be unused').toEqual(false);
+
+    // mark name field as used
+    const collectResult = await writeToken.collectUsage({
+      size: 1,
+      map: {
+        op1: {
+          operation: 'query UserName { user { name } }',
+          operationName: 'UserName',
+          fields: ['Query', 'Query.user', 'User', 'User.name'],
+        },
+      },
+      operations: [
+        {
+          operationMapKey: 'op1',
+          timestamp: Date.now(),
+          execution: {
+            ok: true,
+            duration: 200_000_000,
+            errorsTotal: 0,
+          },
+        },
+      ],
+    });
+    expect(collectResult.status).toEqual(200);
+    await waitFor(8000);
+
+    const secondQuery = await execute({
+      document: IntegrationTestsUnusedSchemaQuery,
+      variables: {
+        targetRef: {
+          byId: target.id,
+        },
+        usageInput: {
+          period,
+        },
+      },
+      authToken: writeToken.secret,
+    }).then(r => r.expectNoGraphQLErrors());
+
+    expect(secondQuery.latestValidVersion?.unusedSchema?.types).toHaveLength(1);
+
+    userType = secondQuery.latestValidVersion?.unusedSchema?.types.find(t =>
+      'name' in t ? t.name === 'User' : false,
+    );
+
+    if (!userType) {
+      throw new Error('User type not found');
+    }
+
+    if (userType.__typename !== 'GraphQLObjectType') {
+      throw new Error('User type is not an object type');
+    }
+
+    idField = userType.fields.find(f => f.name === 'id');
+    nameField = userType.fields.find(f => f.name === 'name');
+
+    expect(idField?.usage.isUsed).toEqual(false);
+    expect(nameField, 'User.name should not be used').toEqual(undefined);
+  },
+);

packages/services/api/package.json

@@ -18,6 +18,7 @@
     "@graphql-hive/core": "workspace:*",
     "@graphql-hive/federation-link-utils": "workspace:*",
     "@graphql-inspector/core": "5.1.0-alpha-20231208113249-34700c8a",
+    "@graphql-tools/merge": "9.0.22",
     "@hive/cdn-script": "workspace:*",
     "@hive/emails": "workspace:*",
     "@hive/schema": "workspace:*",

packages/services/api/src/modules/schema/providers/schema-version-helper.ts

@@ -1,7 +1,8 @@
 import type { SchemaVersionMapper as SchemaVersion } from '../module.graphql.mappers';
-import { print } from 'graphql';
+import { isTypeSystemExtensionNode, print } from 'graphql';
 import { Injectable, Scope } from 'graphql-modules';
 import { CriticalityLevel } from '@graphql-inspector/core';
+import { mergeTypeDefs } from '@graphql-tools/merge';
 import { traceFn } from '@hive/service-common';
 import type { SchemaChangeType } from '@hive/storage';
 import {
@@ -334,39 +335,68 @@ export class SchemaVersionHelper {
 
   /**
    * There's a possibility that the composite schema SDL contains parts of the supergraph spec.
+   *
+   *
    * This is a problem because we want to show the public schema to the user, and the supergraph spec is not part of that.
    * This may happen when composite schema was produced with an old version of `transformSupergraphToPublicSchema`
    * or when supergraph sdl contained something new.
    *
    * This function will check if the SDL contains supergraph spec and if it does, it will transform it to public schema.
+   *
+   * ---
+   *
+   * There's also a possibility that the composite schema contains type extensions.
+   * This is a problem, because other parts of the system may expect it to be clean from type extensions.
+   *
+   * This function will check for type system extensions and merge them into matching definitions.
    */
-  private autoFixCompositeSchemaSdl(sdl: string, versionId: string) {
+  private autoFixCompositeSchemaSdl(sdl: string, versionId: string): string {
     const isFederationV1Output = sdl.includes('@core');
+    // Poor's man check for type extensions to avoid parsing the SDL if it's not necessary.
+    // Checks if the `extend` keyword is followed by a space or a newline and it's not a part of a word.
+    const hasPotentiallyTypeExtensions = /\bextend(?=[\s\n])/.test(sdl);
+
     /**
      * If the SDL is clean from Supergraph spec or it's an output of @apollo/federation, we don't need to transform it.
      * We ignore @apollo/federation, because we never really transformed the output of it to public schema.
      * Doing so might be a breaking change for some users (like: removed join__Graph type).
      */
-
-    if (isFederationV1Output || !containsSupergraphSpec(sdl)) {
-      return sdl;
+    if (!isFederationV1Output && containsSupergraphSpec(sdl)) {
+      this.logger.warn(
+        'Composite schema SDL contains supergraph spec, transforming to public schema (versionId: %s)',
+        versionId,
+      );
+
+      const transformedSdl = print(
+        transformSupergraphToPublicSchema(parseGraphQLSource(sdl, 'autoFixCompositeSchemaSdl')),
+      );
+
+      this.logger.debug(
+        transformedSdl === sdl
+          ? 'Transformation did not change the original SDL'
+          : 'Transformation changed the original SDL',
+      );
+
+      return transformedSdl;
     }
 
-    this.logger.warn(
-      'Composite schema SDL contains supergraph spec, transforming to public schema (versionId: %s)',
-      versionId,
-    );
+    /**
+     * If the SDL has type extensions, we need to merge them into matching definitions.
+     */
+    if (hasPotentiallyTypeExtensions) {
+      const schemaAst = parseGraphQLSource(sdl, 'autoFixCompositeSchemaSdl');
+      const hasTypeExtensions = schemaAst.definitions.some(isTypeSystemExtensionNode);
 
-    const transformedSdl = print(
-      transformSupergraphToPublicSchema(parseGraphQLSource(sdl, 'autoFixCompositeSchemaSdl')),
-    );
+      if (!hasTypeExtensions) {
+        return sdl;
+      }
 
-    this.logger.debug(
-      transformedSdl === sdl
-        ? 'Transformation did not change the original SDL'
-        : 'Transformation changed the original SDL',
-    );
+      this.logger.warn(
+        'Composite schema AST contains type extensions, merging them into matching definitions',
+      );
+      return print(mergeTypeDefs(schemaAst));
+    }
 
-    return transformedSdl;
+    return sdl;
   }
 }

packages/services/schema/package.json

@@ -12,6 +12,7 @@
     "@apollo/federation": "0.38.1",
     "@graphql-hive/external-composition": "workspace:*",
     "@graphql-hive/federation-link-utils": "workspace:*",
+    "@graphql-tools/merge": "9.0.22",
     "@graphql-tools/stitch": "9.4.13",
     "@graphql-tools/stitching-directives": "3.1.24",
     "@hive/service-common": "workspace:*",

packages/services/schema/src/orchestrators.ts

@@ -5,6 +5,7 @@ import {
   buildASTSchema,
   concatAST,
   GraphQLError,
+  isTypeSystemExtensionNode,
   Kind,
   parse,
   print,
@@ -14,6 +15,7 @@ import {
 } from 'graphql';
 import { validateSDL } from 'graphql/validation/validate.js';
 import { extractLinkImplementations } from '@graphql-hive/federation-link-utils';
+import { mergeTypeDefs } from '@graphql-tools/merge';
 import { stitchSchemas } from '@graphql-tools/stitch';
 import { stitchingDirectives } from '@graphql-tools/stitching-directives';
 import type { ServiceLogger } from '@hive/service-common';
@@ -496,12 +498,18 @@ function createSingle(): Orchestrator {
   return {
     async composeAndValidate(schemas) {
       const schema = schemas[0];
-      const schemaAst = parse(schema.raw);
+      let schemaAst = parse(schema.raw);
+
+      // If the schema contains type system extension nodes, merge them into the schema.
+      // We don't want to show many type extension of User, we want to show single User type.
+      if (schemaAst.definitions.some(isTypeSystemExtensionNode)) {
+        schemaAst = mergeTypeDefs(schemaAst);
+      }
       const errors = validateSingleSDL(schemaAst);
 
       return {
         errors,
-        sdl: print(trimDescriptions(parse(schema.raw))),
+        sdl: print(trimDescriptions(schemaAst)),
         supergraph: null,
         contracts: null,
         tags: null,