fix: check request threshold against total request count

.changeset/nine-meals-raise.md

@@ -0,0 +1,5 @@
+---
+'hive': patch
+---
+
+Use sum instead of max of top request counts for breaking changes calculation

integration-tests/tests/api/target/usage.spec.ts

@@ -2509,6 +2509,135 @@ test.concurrent(
   },
 );
 
+test.concurrent(
+  'test threshold when using conditional breaking change "REQUEST_COUNT" detection, across multiple operations',
+  async ({ expect }) => {
+    const { createOrg } = await initSeed().createOwner();
+    const { createProject } = await createOrg();
+    const { createTargetAccessToken, toggleTargetValidation, updateTargetValidationSettings } =
+      await createProject(ProjectType.Single);
+    const token = await createTargetAccessToken({});
+    await toggleTargetValidation(true);
+    await updateTargetValidationSettings({
+      excludedClients: [],
+      requestCount: 2,
+      percentage: 0,
+      breakingChangeFormula: BreakingChangeFormula.RequestCount,
+    });
+
+    const sdl = /* GraphQL */ `
+      type Query {
+        a: String
+        b: String
+        c: String
+      }
+    `;
+
+    const queryA = parse(/* GraphQL */ `
+      query {
+        a
+      }
+    `);
+    const queryB = parse(/* GraphQL */ `
+      query {
+        a
+        b
+      }
+    `);
+
+    function collectA() {
+      client.collectUsage()(
+        {
+          document: queryA,
+          schema,
+          contextValue: {
+            request,
+          },
+        },
+        {},
+      );
+    }
+    function collectB() {
+      client.collectUsage()(
+        {
+          document: queryB,
+          schema,
+          contextValue: {
+            request,
+          },
+        },
+        {},
+      );
+    }
+
+    const schema = buildASTSchema(parse(sdl));
+
+    const schemaPublishResult = await token
+      .publishSchema({
+        sdl,
+        author: 'Kamil',
+        commit: 'initial',
+      })
+      .then(res => res.expectNoGraphQLErrors());
+
+    expect(schemaPublishResult.schemaPublish.__typename).toEqual('SchemaPublishSuccess');
+
+    const usageAddress = await getServiceHost('usage', 8081);
+
+    const client = createHive({
+      enabled: true,
+      token: token.secret,
+      usage: true,
+      debug: false,
+      agent: {
+        logger: createLogger('debug'),
+        maxSize: 1,
+      },
+      selfHosting: {
+        usageEndpoint: 'http://' + usageAddress,
+        graphqlEndpoint: 'http://noop/',
+        applicationUrl: 'http://noop/',
+      },
+    });
+
+    const request = new Request('http://localhost:4000/graphql', {
+      method: 'POST',
+      headers: {
+        'x-graphql-client-name': 'integration-tests',
+        'x-graphql-client-version': '6.6.6',
+      },
+    });
+
+    collectA();
+    collectB();
+
+    await waitFor(8000);
+
+    // try to remove `Query.a`
+    const above = await token
+      .checkSchema(/* GraphQL */ `
+        type Query {
+          b: String
+          c: String
+        }
+      `)
+      .then(r => r.expectNoGraphQLErrors());
+
+    if (above.schemaCheck.__typename !== 'SchemaCheckError') {
+      throw new Error(`Expected SchemaCheckError, got ${above.schemaCheck.__typename}`);
+    }
+
+    expect(above.schemaCheck.errors).toEqual({
+      nodes: [
+        {
+          message: "Field 'a' was removed from object type 'Query'",
+        },
+      ],
+      total: 1,
+    });
+  },
+);
+
 test.concurrent(
   'subscription operation is used for conditional breaking change detection',
   async ({ expect }) => {

packages/services/api/src/modules/operations/providers/operations-reader.ts

@@ -181,25 +181,48 @@ export class OperationsReader {
     }).then(r => r[this.makeId({ type, field, argument })]);
   }
 
-  private async countFields({
-    fields,
-    target,
+  countCoordinate = batchBy<
+    {
+      schemaCoordinate: string;
+      targetIds: readonly string[];
+      period: DateRange;
+      operations?: readonly string[];
+      excludedClients?: readonly string[] | null;
+    },
+    Record<string, number>
+  >(
+    item =>
+      `${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.operations?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}`,
+    async items => {
+      const schemaCoordinates = items.map(item => item.schemaCoordinate);
+      return await this.countCoordinates({
+        targetIds: items[0].targetIds,
+        excludedClients: items[0].excludedClients,
+        period: items[0].period,
+        operations: items[0].operations,
+        schemaCoordinates,
+      }).then(result =>
+        items.map(item =>
+          Promise.resolve({ [item.schemaCoordinate]: result[item.schemaCoordinate] }),
+        ),
+      );
+    },
+  );
+
+  public async countCoordinates({
+    schemaCoordinates,
+    targetIds,
     period,
     operations,
     excludedClients,
   }: {
-    fields: ReadonlyArray<{
-      type: string;
-      field?: string | null;
-      argument?: string | null;
-    }>;
-    target: string | readonly string[];
+    schemaCoordinates: readonly string[];
+    targetIds: string | readonly string[];
     period: DateRange;
     operations?: readonly string[];
     excludedClients?: readonly string[] | null;
-  }): Promise<Record<string, number>> {
-    const coordinates = fields.map(selector => this.makeId(selector));
-    const conditions = [sql`(coordinate IN (${sql.array(coordinates, 'String')}))`];
+  }) {
+    const conditions = [sql`(coordinate IN (${sql.array(schemaCoordinates, 'String')}))`];
 
     if (Array.isArray(excludedClients) && excludedClients.length > 0) {
       // Eliminate coordinates fetched by excluded clients.
@@ -216,7 +239,7 @@ export class OperationsReader {
                 'String',
               )})) as non_excluded_clients_total
             FROM clients_daily ${this.createFilter({
-              target,
+              target: targetIds,
               period,
             })}
             GROUP BY hash
@@ -235,7 +258,7 @@ export class OperationsReader {
               sum(total) as total
             FROM coordinates_daily
             ${this.createFilter({
-              target,
+              target: targetIds,
               period,
               operations,
               extra: conditions,
@@ -251,17 +274,42 @@ export class OperationsReader {
       stats[row.coordinate] = ensureNumber(row.total);
     }
 
-    for (const selector of fields) {
-      const key = this.makeId(selector);
-
-      if (typeof stats[key] !== 'number') {
-        stats[key] = 0;
+    for (const coordinate of schemaCoordinates) {
+      if (typeof stats[coordinate] !== 'number') {
+        stats[coordinate] = 0;
       }
     }
 
     return stats;
   }
 
+  private async countFields({
+    fields,
+    target,
+    period,
+    operations,
+    excludedClients,
+  }: {
+    fields: ReadonlyArray<{
+      type: string;
+      field?: string | null;
+      argument?: string | null;
+    }>;
+    target: string | readonly string[];
+    period: DateRange;
+    operations?: readonly string[];
+    excludedClients?: readonly string[] | null;
+  }): Promise<Record<string, number>> {
+    const schemaCoordinates = fields.map(selector => this.makeId(selector));
+    return this.countCoordinates({
+      schemaCoordinates,
+      targetIds: target,
+      period,
+      operations,
+      excludedClients,
+    });
+  }
+
   async hasCollectedOperations({
     target,
   }: {
@@ -919,7 +967,6 @@ export class OperationsReader {
     excludedClients: null | readonly string[];
     period: DateRange;
     schemaCoordinates: string[];
-    requestCountThreshold: number;
   }) {
     const RecordArrayType = z.array(
       z.object({
@@ -980,7 +1027,6 @@ export class OperationsReader {
               AND "coordinates_daily"."timestamp" >= toDateTime(${formatDate(args.period.from)}, 'UTC')
               AND "coordinates_daily"."timestamp" <= toDateTime(${formatDate(args.period.to)}, 'UTC')
               AND "coordinates_daily"."coordinate" IN (${sql.longArray(args.schemaCoordinates, 'String')})
-            HAVING "total" >= ${String(args.requestCountThreshold)}
             ORDER BY
               "total" DESC,
               "coordinates_daily"."hash" DESC
@@ -998,7 +1044,7 @@ export class OperationsReader {
             SELECT
               "operation_collection_details"."name",
               "operation_collection_details"."hash"
-            FROM
+            FROM 
               "operation_collection_details"
             PREWHERE
               "operation_collection_details"."target" IN (${sql.array(args.targetIds, 'String')})
@@ -1049,7 +1095,6 @@ export class OperationsReader {
       excludedClients: null | readonly string[];
       period: DateRange;
       schemaCoordinate: string;
-      requestCountThreshold: number;
     },
     Array<{
       hash: string;
@@ -1058,14 +1103,13 @@ export class OperationsReader {
     }> | null
   >(
     item =>
-      `${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}-${item.requestCountThreshold}`,
+      `${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}`,
     async items => {
       const schemaCoordinates = items.map(item => item.schemaCoordinate);
       return await this._getTopOperationsForSchemaCoordinates({
         targetIds: items[0].targetIds,
         excludedClients: items[0].excludedClients,
         period: items[0].period,
-        requestCountThreshold: items[0].requestCountThreshold,
         schemaCoordinates,
       }).then(result => result.map(result => Promise.resolve(result)));
     },
@@ -1578,7 +1622,7 @@ export class OperationsReader {
                 FROM ${aggregationTableName('operations')}
                 ${this.createFilter({ target: targets, period: roundedPeriod })}
                 GROUP BY target, date
-                ORDER BY
+                ORDER BY 
                   target,
                   date
                     WITH FILL

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

@@ -482,35 +482,48 @@ export class RegistryChecks {
             return;
           }
 
-          // We need to run both the affected operations an affected clients query.
-          // Since the affected clients query is lighter it makes more sense to run it first and skip running the operations query if no clients are affected, as it will also yield zero results in that case.
-
-          const topAffectedClients = await this.operationsReader.getTopClientsForSchemaCoordinate({
+          const totalRequestCounts = await this.operationsReader.countCoordinate({
             targetIds: settings.targetIds,
             excludedClients: settings.excludedClientNames,
             period: settings.period,
             schemaCoordinate: change.breakingChangeSchemaCoordinate,
           });
-
-          if (topAffectedClients) {
-            const topAffectedOperations =
-              await this.operationsReader.getTopOperationsForSchemaCoordinate({
+          const isBreaking =
+            totalRequestCounts[change.breakingChangeSchemaCoordinate] >=
+            Math.max(settings.requestCountThreshold, 1);
+          if (isBreaking) {
+            // We need to run both the affected operations an affected clients query.
+            // Since the affected clients query is lighter it makes more sense to run it first and skip running
+            // the operations query if no clients are affected, as it will also yield zero results in that case.
+
+            const topAffectedClients = await this.operationsReader.getTopClientsForSchemaCoordinate(
+              {
                 targetIds: settings.targetIds,
                 excludedClients: settings.excludedClientNames,
                 period: settings.period,
-                requestCountThreshold: settings.requestCountThreshold,
                 schemaCoordinate: change.breakingChangeSchemaCoordinate,
-              });
-
-            if (topAffectedOperations) {
-              change.usageStatistics = {
-                topAffectedOperations,
-                topAffectedClients,
-              };
+              },
+            );
+
+            if (topAffectedClients) {
+              const topAffectedOperations =
+                await this.operationsReader.getTopOperationsForSchemaCoordinate({
+                  targetIds: settings.targetIds,
+                  excludedClients: settings.excludedClientNames,
+                  period: settings.period,
+                  schemaCoordinate: change.breakingChangeSchemaCoordinate,
+                });
+
+              if (topAffectedOperations) {
+                change.usageStatistics = {
+                  topAffectedOperations,
+                  topAffectedClients,
+                };
+              }
             }
           }
 
-          change.isSafeBasedOnUsage = change.usageStatistics === null;
+          change.isSafeBasedOnUsage = !isBreaking;
         }),
       );
     } else {