Generate a full list of changes on schema check, and add an argument to filter change lists

.changeset/eight-views-design.md

@@ -0,0 +1,6 @@
+---
+'hive': minor
+---
+
+adjust change capture and resolvers to optionally provide a full list or the simplified list of
+changes

packages/migrations/test/2024.01.26T00.00.01.schema-check-purging.test.ts

@@ -83,6 +83,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -166,6 +167,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       await storage.createSchemaCheck({
@@ -192,6 +194,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -275,6 +278,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       await storage.createSchemaCheck({
@@ -301,6 +305,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -394,6 +399,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       await storage.createSchemaCheck({
@@ -420,6 +426,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -571,6 +578,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       await storage.createSchemaCheck({
@@ -597,6 +605,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -745,6 +754,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let sdlStoreCount = await db.oneFirst<number>(sql`SELECT count(*) as total FROM sdl_store`);
@@ -856,6 +866,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(
@@ -1016,6 +1027,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       await storage.createSchemaCheck({
@@ -1061,6 +1073,7 @@ describe('schema check purging', async () => {
         schemaPolicyWarnings: null,
         conditionalBreakingChangeMetadata: null,
         serviceUrl: null,
+        schemaProposalId: null,
       });
 
       let schemaCheckCount = await db.oneFirst<number>(

packages/services/api/src/modules/schema/providers/inspector.ts

@@ -10,7 +10,7 @@ import {
   type GraphQLSchema,
 } from 'graphql';
 import { Injectable, Scope } from 'graphql-modules';
-import { Change, ChangeType, diff, DiffRule, TypeOfChangeType } from '@graphql-inspector/core';
+import { Change, ChangeType, diff, Rule, TypeOfChangeType } from '@graphql-inspector/core';
 import { traceFn } from '@hive/service-common';
 import { HiveSchemaChangeModel } from '@hive/storage';
 import { Logger } from '../../shared/providers/logger';
@@ -31,10 +31,10 @@ export class Inspector {
       'hive.diff.changes.count': result.length,
     }),
   })
-  async diff(existing: GraphQLSchema, incoming: GraphQLSchema) {
+  async diff(existing: GraphQLSchema, incoming: GraphQLSchema, rules?: Rule[]) {
     this.logger.debug('Comparing Schemas');
 
-    const changes = await diff(existing, incoming, [DiffRule.simplifyChanges]);
+    const changes = await diff(existing, incoming, rules);
 
     return changes
       .filter(dropTrimmedDescriptionChangedChange)

packages/services/api/src/modules/schema/providers/models/composite.ts

@@ -78,6 +78,7 @@ export class CompositeModel {
             existingSdl: contract.latestValidVersion?.compositeSchemaSdl ?? null,
             incomingSdl: contractCompositionResult?.result?.fullSchemaSdl ?? null,
             failDiffOnDangerousChange: args.failDiffOnDangerousChange,
+            filterNestedChanges: true,
           }),
         };
       }),
@@ -103,6 +104,7 @@ export class CompositeModel {
     conditionalBreakingChangeDiffConfig,
     contracts,
     failDiffOnDangerousChange,
+    filterNestedChanges,
   }: {
     input: {
       sdl: string;
@@ -136,6 +138,7 @@ export class CompositeModel {
         approvedChanges: Map<string, SchemaChangeType> | null;
       }
     > | null;
+    filterNestedChanges: boolean;
   }): Promise<SchemaCheckResult> {
     const incoming: PushedCompositeSchema = {
       kind: 'composite',
@@ -228,6 +231,7 @@ export class CompositeModel {
           compositionCheck.result?.fullSchemaSdl ?? compositionCheck.reason?.fullSchemaSdl ?? null,
         conditionalBreakingChangeConfig: conditionalBreakingChangeDiffConfig,
         failDiffOnDangerousChange,
+        filterNestedChanges,
       }),
       this.checks.policyCheck({
         selector,
@@ -487,6 +491,7 @@ export class CompositeModel {
       existingSdl: previousVersionSdl,
       incomingSdl: compositionCheck.result?.fullSchemaSdl ?? null,
       failDiffOnDangerousChange,
+      filterNestedChanges: true, // filter because publish is never associated to schema proposals in this way.
     });
 
     const contractChecks = await this.getContractChecks({
@@ -652,6 +657,7 @@ export class CompositeModel {
       existingSdl: previousVersionSdl,
       incomingSdl: compositionCheck.result?.fullSchemaSdl ?? null,
       failDiffOnDangerousChange,
+      filterNestedChanges: true, // filter because deletes are never associated with schema proposals in this way.
     });
 
     const contractChecks = await this.getContractChecks({

packages/services/api/src/modules/schema/providers/models/single.ts

@@ -43,6 +43,7 @@ export class SingleModel {
     approvedChanges,
     conditionalBreakingChangeDiffConfig,
     failDiffOnDangerousChange,
+    filterNestedChanges,
   }: {
     input: {
       sdl: string;
@@ -68,6 +69,7 @@ export class SingleModel {
     approvedChanges: Map<string, SchemaChangeType>;
     conditionalBreakingChangeDiffConfig: null | ConditionalBreakingChangeDiffConfig;
     failDiffOnDangerousChange: boolean;
+    filterNestedChanges: boolean;
   }): Promise<SchemaCheckResult> {
     const incoming: SingleSchema = {
       kind: 'single',
@@ -130,6 +132,7 @@ export class SingleModel {
         existingSdl: previousVersionSdl,
         incomingSdl: compositionCheck.result?.fullSchemaSdl ?? null,
         failDiffOnDangerousChange,
+        filterNestedChanges,
       }),
       this.checks.policyCheck({
         selector,
@@ -267,6 +270,7 @@ export class SingleModel {
         existingSdl: previousVersionSdl,
         incomingSdl: compositionCheck.result?.fullSchemaSdl ?? null,
         failDiffOnDangerousChange,
+        filterNestedChanges: true, // publish is never associated with schema proposals in this way. So always show the minimal changeset.
       }),
     ]);
 

packages/services/api/src/modules/schema/providers/registry-checks.ts

@@ -2,7 +2,7 @@ import { URL } from 'node:url';
 import { type GraphQLSchema } from 'graphql';
 import { Injectable, Scope } from 'graphql-modules';
 import hashObject from 'object-hash';
-import { ChangeType, CriticalityLevel, TypeOfChangeType } from '@graphql-inspector/core';
+import { ChangeType, CriticalityLevel, DiffRule, TypeOfChangeType } from '@graphql-inspector/core';
 import type { CheckPolicyResponse } from '@hive/policy';
 import type { CompositionFailureError, ContractsInputType } from '@hive/schema';
 import { traceFn } from '@hive/service-common';
@@ -426,6 +426,11 @@ export class RegistryChecks {
     /** Settings for fetching conditional breaking changes. */
     conditionalBreakingChangeConfig: null | ConditionalBreakingChangeDiffConfig;
     failDiffOnDangerousChange: null | boolean;
+    /**
+     * Set to true to reduce the number of changes to only what's relevant to the user.
+     * Use false for schema proposals in order to capture every single change record for the patch function.
+     */
+    filterNestedChanges: boolean;
   }) {
     let existingSchema: GraphQLSchema | null = null;
     let incomingSchema: GraphQLSchema | null = null;
@@ -463,7 +468,11 @@ export class RegistryChecks {
       } satisfies CheckResult;
     }
 
-    let inspectorChanges = await this.inspector.diff(existingSchema, incomingSchema);
+    let inspectorChanges = await this.inspector.diff(
+      existingSchema,
+      incomingSchema,
+      args.filterNestedChanges ? [DiffRule.simplifyChanges] : [],
+    );
 
     // Filter out federation specific changes as they are not relevant for the schema diff and were in previous schema versions by accident.
     if (args.filterOutFederationChanges === true) {

packages/services/api/src/modules/schema/providers/schema-check-manager.ts

@@ -48,7 +48,12 @@ export class SchemaCheckManager {
       return null;
     }
 
-    return [...(schemaCheck.breakingSchemaChanges ?? []), ...(schemaCheck.safeSchemaChanges ?? [])];
+    const changes = [
+      ...(schemaCheck.breakingSchemaChanges ?? []),
+      ...(schemaCheck.safeSchemaChanges ?? []),
+    ];
+
+    return changes;
   }
 
   getBreakingSchemaChanges(schemaCheck: SchemaCheck) {

packages/services/api/src/modules/schema/providers/schema-publisher.ts

@@ -492,7 +492,6 @@ export class SchemaPublisher {
       }
 
       if (github != null) {
-        // @todo should proposals do anything special here?
         const result = await this.createGithubCheckRunStartForSchemaCheck({
           organization,
           project,
@@ -623,6 +622,7 @@ export class SchemaPublisher {
           conditionalBreakingChangeDiffConfig:
             conditionalBreakingChangeConfiguration?.conditionalBreakingChangeDiffConfig ?? null,
           failDiffOnDangerousChange,
+          filterNestedChanges: !input.schemaProposalId,
         });
         break;
       case ProjectType.FEDERATION:
@@ -668,6 +668,7 @@ export class SchemaPublisher {
           conditionalBreakingChangeDiffConfig:
             conditionalBreakingChangeConfiguration?.conditionalBreakingChangeDiffConfig ?? null,
           failDiffOnDangerousChange,
+          filterNestedChanges: !input.schemaProposalId,
         });
         break;
       default:
@@ -732,7 +733,7 @@ export class SchemaPublisher {
             breakingSchemaChanges: contract.schemaChanges?.breaking ?? null,
             safeSchemaChanges: contract.schemaChanges?.safe ?? null,
           })) ?? null,
-        schemaProposalId: input.schemaProposalId,
+        schemaProposalId: input.schemaProposalId ?? null,
       });
       this.logger.info('created failed schema check. (schemaCheckId=%s)', schemaCheck.id);
     } else if (checkResult.conclusion === SchemaCheckConclusion.Success) {
@@ -779,7 +780,7 @@ export class SchemaPublisher {
             breakingSchemaChanges: contract.schemaChanges?.breaking ?? null,
             safeSchemaChanges: contract.schemaChanges?.safe ?? null,
           })) ?? null,
-        schemaProposalId: input.schemaProposalId,
+        schemaProposalId: input.schemaProposalId ?? null,
       });
       this.logger.info('created successful schema check. (schemaCheckId=%s)', schemaCheck.id);
     } else if (checkResult.conclusion === SchemaCheckConclusion.Skip) {
@@ -858,7 +859,7 @@ export class SchemaPublisher {
               })),
             )
           : null,
-        schemaProposalId: input.schemaProposalId,
+        schemaProposalId: input.schemaProposalId ?? null,
       });
       this.logger.info('created skipped schema check. (schemaCheckId=%s)', schemaCheck.id);
     }

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

@@ -174,9 +174,11 @@ export class SchemaVersionHelper {
     }
 
     if (schemaVersion.hasPersistedSchemaChanges) {
-      const changes = await this.storage.getSchemaChangesForVersion({
-        versionId: schemaVersion.id,
-      });
+      const changes: null | Array<SchemaChangeType> = await this.storage.getSchemaChangesForVersion(
+        {
+          versionId: schemaVersion.id,
+        },
+      );
 
       const safeChanges: Array<SchemaChangeType> = [];
       const breakingChanges: Array<SchemaChangeType> = [];
@@ -241,6 +243,7 @@ export class SchemaVersionHelper {
       filterOutFederationChanges: project.type === ProjectType.FEDERATION,
       conditionalBreakingChangeConfig: null,
       failDiffOnDangerousChange,
+      filterNestedChanges: true,
     });
 
     if (diffCheck.status === 'skipped') {

packages/services/storage/src/schema-change-model.ts

@@ -1012,25 +1012,25 @@ export const ObjectTypeInterfaceRemovedModel = implement<ObjectTypeInterfaceRemo
 export const SchemaQueryTypeChangedModel = implement<SchemaQueryTypeChangedChange>().with({
   type: SchemaQueryTypeChangedLiteral,
   meta: z.object({
-    oldQueryTypeName: z.string(),
-    newQueryTypeName: z.string(),
+    oldQueryTypeName: z.string().nullable(),
+    newQueryTypeName: z.string().nullable(),
   }),
 });
 
 export const SchemaMutationTypeChangedModel = implement<SchemaMutationTypeChangedChange>().with({
   type: SchemaMutationTypeChangedLiteral,
   meta: z.object({
-    oldMutationTypeName: z.string(),
-    newMutationTypeName: z.string(),
+    oldMutationTypeName: z.string().nullable(),
+    newMutationTypeName: z.string().nullable(),
   }),
 });
 
 export const SchemaSubscriptionTypeChangedModel =
   implement<SchemaSubscriptionTypeChangedChange>().with({
     type: SchemaSubscriptionTypeChangedLiteral,
     meta: z.object({
-      oldSubscriptionTypeName: z.string(),
-      newSubscriptionTypeName: z.string(),
+      oldSubscriptionTypeName: z.string().nullable(),
+      newSubscriptionTypeName: z.string().nullable(),
     }),
   });
 
@@ -1414,6 +1414,7 @@ const SchemaCheckSharedOutputFields = {
   serviceUrl: z.string().nullable(),
   targetId: z.string(),
   schemaVersionId: z.string().nullable(),
+  schemaProposalId: z.string().nullable(),
   meta: z
     .object({
       author: z.string(),