feat: prevent schema check approval when policy errors are present (#7185)

Author: adambenhassen

.changeset/nasty-emus-win.md

@@ -0,0 +1,6 @@
+---
+'hive': patch
+---
+
+Fix schema check approval to properly reject checks with policy errors and return descriptive error
+message instead of generic error

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

@@ -2231,3 +2231,132 @@ test.concurrent(
     });
   },
 );
+
+test.concurrent('schema policy errors prevent schema check approval', async ({ expect }) => {
+  const { createOrg, ownerToken } = await initSeed().createOwner();
+  const { createProject, organization } = await createOrg();
+  const { createTargetAccessToken, setProjectSchemaPolicy, project, target } = await createProject(
+    ProjectType.Single,
+  );
+
+  await setProjectSchemaPolicy(createPolicy(RuleInstanceSeverityLevel.Error));
+
+  const writeToken = await createTargetAccessToken({});
+  await writeToken
+    .publishSchema({
+      sdl: /* GraphQL */ `
+        type Query {
+          ping: String
+        }
+      `,
+    })
+    .then(r => r.expectNoGraphQLErrors());
+
+  const checkResult = await writeToken
+    .checkSchema(/* GraphQL */ `
+      type Query {
+        ping: Float
+      }
+    `)
+    .then(r => r.expectNoGraphQLErrors());
+
+  const check = checkResult.schemaCheck;
+
+  if (check.__typename !== 'SchemaCheckError') {
+    throw new Error(`Expected SchemaCheckError, got ${check.__typename}`);
+  }
+
+  const schemaCheckId = check.schemaCheck?.id;
+
+  if (schemaCheckId == null) {
+    throw new Error('Missing schema check id.');
+  }
+
+  const mutationResult = await execute({
+    document: ApproveFailedSchemaCheckMutation,
+    variables: {
+      input: {
+        organizationSlug: organization.slug,
+        projectSlug: project.slug,
+        targetSlug: target.slug,
+        schemaCheckId,
+      },
+    },
+    authToken: ownerToken,
+  }).then(r => r.expectNoGraphQLErrors());
+
+  expect(mutationResult).toEqual({
+    approveFailedSchemaCheck: {
+      ok: null,
+      error: {
+        message: 'Schema check has schema policy errors that must be resolved before approval.',
+      },
+    },
+  });
+});
+
+test.concurrent(
+  'schema policy errors prevent schema check approval with safe changes',
+  async ({ expect }) => {
+    const { createOrg, ownerToken } = await initSeed().createOwner();
+    const { createProject, organization } = await createOrg();
+    const { createTargetAccessToken, setProjectSchemaPolicy, project, target } =
+      await createProject(ProjectType.Single);
+
+    await setProjectSchemaPolicy(createPolicy(RuleInstanceSeverityLevel.Error));
+
+    const writeToken = await createTargetAccessToken({});
+    await writeToken
+      .publishSchema({
+        sdl: /* GraphQL */ `
+          type Query {
+            ping: String
+          }
+        `,
+      })
+      .then(r => r.expectNoGraphQLErrors());
+
+    const checkResult = await writeToken
+      .checkSchema(/* GraphQL */ `
+        type Query {
+          ping: String
+          pong: String
+        }
+      `)
+      .then(r => r.expectNoGraphQLErrors());
+
+    const check = checkResult.schemaCheck;
+
+    if (check.__typename !== 'SchemaCheckError') {
+      throw new Error(`Expected SchemaCheckError, got ${check.__typename}`);
+    }
+
+    const schemaCheckId = check.schemaCheck?.id;
+
+    if (schemaCheckId == null) {
+      throw new Error('Missing schema check id.');
+    }
+
+    const mutationResult = await execute({
+      document: ApproveFailedSchemaCheckMutation,
+      variables: {
+        input: {
+          organizationSlug: organization.slug,
+          projectSlug: project.slug,
+          targetSlug: target.slug,
+          schemaCheckId,
+        },
+      },
+      authToken: ownerToken,
+    }).then(r => r.expectNoGraphQLErrors());
+
+    expect(mutationResult).toEqual({
+      approveFailedSchemaCheck: {
+        ok: null,
+        error: {
+          message: 'Schema check has schema policy errors that must be resolved before approval.',
+        },
+      },
+    });
+  },
+);

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

@@ -955,6 +955,19 @@ export class SchemaManager {
     });
 
     if (!schemaCheck) {
+      // Re-fetch the schema check to determine why approval failed
+      const recheck = await this.storage.findSchemaCheck({
+        targetId: args.targetId,
+        schemaCheckId: args.schemaCheckId,
+      });
+
+      if (recheck?.schemaPolicyErrors !== null) {
+        return {
+          type: 'error',
+          reason: 'Schema check has schema policy errors that must be resolved before approval.',
+        } as const;
+      }
+
       return {
         type: 'error',
         reason: "Schema check doesn't exist.",

packages/services/storage/src/index.ts

@@ -4060,6 +4060,7 @@ export async function createStorage(
             "id" = ${args.schemaCheckId}
             AND "is_success" = false
             AND "schema_composition_errors" IS NULL
+            AND "schema_policy_errors" IS NULL
           RETURNING
             "id"
         `);
@@ -4078,6 +4079,7 @@ export async function createStorage(
             "id" = ${args.schemaCheckId}
             AND "is_success" = false
             AND "schema_composition_errors" IS NULL
+            AND "schema_policy_errors" IS NULL
           RETURNING
             "id"
         `);