Skip to content

Commit ab06518

Browse files
jdollejdolle
authored
fix: check request threshold against total request count (#6358)
1 parent ba43c93 commit ab06518

File tree

4 files changed

+234
-43
lines changed

4 files changed

+234
-43
lines changed

‎.changeset/nine-meals-raise.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'hive': patch
3+
---
4+
5+
Use sum instead of max of top request counts for breaking changes calculation

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

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2509,6 +2509,135 @@ test.concurrent(
25092509
},
25102510
);
25112511

2512+
test.concurrent(
2513+
'test threshold when using conditional breaking change "REQUEST_COUNT" detection, across multiple operations',
2514+
async ({ expect }) => {
2515+
const { createOrg } = await initSeed().createOwner();
2516+
const { createProject } = await createOrg();
2517+
const { createTargetAccessToken, toggleTargetValidation, updateTargetValidationSettings } =
2518+
await createProject(ProjectType.Single);
2519+
const token = await createTargetAccessToken({});
2520+
await toggleTargetValidation(true);
2521+
await updateTargetValidationSettings({
2522+
excludedClients: [],
2523+
requestCount: 2,
2524+
percentage: 0,
2525+
breakingChangeFormula: BreakingChangeFormula.RequestCount,
2526+
});
2527+
2528+
const sdl = /* GraphQL */ `
2529+
type Query {
2530+
a: String
2531+
b: String
2532+
c: String
2533+
}
2534+
`;
2535+
2536+
const queryA = parse(/* GraphQL */ `
2537+
query {
2538+
a
2539+
}
2540+
`);
2541+
const queryB = parse(/* GraphQL */ `
2542+
query {
2543+
a
2544+
b
2545+
}
2546+
`);
2547+
2548+
function collectA() {
2549+
client.collectUsage()(
2550+
{
2551+
document: queryA,
2552+
schema,
2553+
contextValue: {
2554+
request,
2555+
},
2556+
},
2557+
{},
2558+
);
2559+
}
2560+
function collectB() {
2561+
client.collectUsage()(
2562+
{
2563+
document: queryB,
2564+
schema,
2565+
contextValue: {
2566+
request,
2567+
},
2568+
},
2569+
{},
2570+
);
2571+
}
2572+
2573+
const schema = buildASTSchema(parse(sdl));
2574+
2575+
const schemaPublishResult = await token
2576+
.publishSchema({
2577+
sdl,
2578+
author: 'Kamil',
2579+
commit: 'initial',
2580+
})
2581+
.then(res => res.expectNoGraphQLErrors());
2582+
2583+
expect(schemaPublishResult.schemaPublish.__typename).toEqual('SchemaPublishSuccess');
2584+
2585+
const usageAddress = await getServiceHost('usage', 8081);
2586+
2587+
const client = createHive({
2588+
enabled: true,
2589+
token: token.secret,
2590+
usage: true,
2591+
debug: false,
2592+
agent: {
2593+
logger: createLogger('debug'),
2594+
maxSize: 1,
2595+
},
2596+
selfHosting: {
2597+
usageEndpoint: 'http://' + usageAddress,
2598+
graphqlEndpoint: 'http://noop/',
2599+
applicationUrl: 'http://noop/',
2600+
},
2601+
});
2602+
2603+
const request = new Request('http://localhost:4000/graphql', {
2604+
method: 'POST',
2605+
headers: {
2606+
'x-graphql-client-name': 'integration-tests',
2607+
'x-graphql-client-version': '6.6.6',
2608+
},
2609+
});
2610+
2611+
collectA();
2612+
collectB();
2613+
2614+
await waitFor(8000);
2615+
2616+
// try to remove `Query.a`
2617+
const above = await token
2618+
.checkSchema(/* GraphQL */ `
2619+
type Query {
2620+
b: String
2621+
c: String
2622+
}
2623+
`)
2624+
.then(r => r.expectNoGraphQLErrors());
2625+
2626+
if (above.schemaCheck.__typename !== 'SchemaCheckError') {
2627+
throw new Error(`Expected SchemaCheckError, got ${above.schemaCheck.__typename}`);
2628+
}
2629+
2630+
expect(above.schemaCheck.errors).toEqual({
2631+
nodes: [
2632+
{
2633+
message: "Field 'a' was removed from object type 'Query'",
2634+
},
2635+
],
2636+
total: 1,
2637+
});
2638+
},
2639+
);
2640+
25122641
test.concurrent(
25132642
'subscription operation is used for conditional breaking change detection',
25142643
async ({ expect }) => {

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

Lines changed: 70 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -181,25 +181,48 @@ export class OperationsReader {
181181
}).then(r => r[this.makeId({ type, field, argument })]);
182182
}
183183

184-
private async countFields({
185-
fields,
186-
target,
184+
countCoordinate = batchBy<
185+
{
186+
schemaCoordinate: string;
187+
targetIds: readonly string[];
188+
period: DateRange;
189+
operations?: readonly string[];
190+
excludedClients?: readonly string[] | null;
191+
},
192+
Record<string, number>
193+
>(
194+
item =>
195+
`${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.operations?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}`,
196+
async items => {
197+
const schemaCoordinates = items.map(item => item.schemaCoordinate);
198+
return await this.countCoordinates({
199+
targetIds: items[0].targetIds,
200+
excludedClients: items[0].excludedClients,
201+
period: items[0].period,
202+
operations: items[0].operations,
203+
schemaCoordinates,
204+
}).then(result =>
205+
items.map(item =>
206+
Promise.resolve({ [item.schemaCoordinate]: result[item.schemaCoordinate] }),
207+
),
208+
);
209+
},
210+
);
211+
212+
public async countCoordinates({
213+
schemaCoordinates,
214+
targetIds,
187215
period,
188216
operations,
189217
excludedClients,
190218
}: {
191-
fields: ReadonlyArray<{
192-
type: string;
193-
field?: string | null;
194-
argument?: string | null;
195-
}>;
196-
target: string | readonly string[];
219+
schemaCoordinates: readonly string[];
220+
targetIds: string | readonly string[];
197221
period: DateRange;
198222
operations?: readonly string[];
199223
excludedClients?: readonly string[] | null;
200-
}): Promise<Record<string, number>> {
201-
const coordinates = fields.map(selector => this.makeId(selector));
202-
const conditions = [sql`(coordinate IN (${sql.array(coordinates, 'String')}))`];
224+
}) {
225+
const conditions = [sql`(coordinate IN (${sql.array(schemaCoordinates, 'String')}))`];
203226

204227
if (Array.isArray(excludedClients) && excludedClients.length > 0) {
205228
// Eliminate coordinates fetched by excluded clients.
@@ -216,7 +239,7 @@ export class OperationsReader {
216239
'String',
217240
)})) as non_excluded_clients_total
218241
FROM clients_daily ${this.createFilter({
219-
target,
242+
target: targetIds,
220243
period,
221244
})}
222245
GROUP BY hash
@@ -235,7 +258,7 @@ export class OperationsReader {
235258
sum(total) as total
236259
FROM coordinates_daily
237260
${this.createFilter({
238-
target,
261+
target: targetIds,
239262
period,
240263
operations,
241264
extra: conditions,
@@ -251,17 +274,42 @@ export class OperationsReader {
251274
stats[row.coordinate] = ensureNumber(row.total);
252275
}
253276

254-
for (const selector of fields) {
255-
const key = this.makeId(selector);
256-
257-
if (typeof stats[key] !== 'number') {
258-
stats[key] = 0;
277+
for (const coordinate of schemaCoordinates) {
278+
if (typeof stats[coordinate] !== 'number') {
279+
stats[coordinate] = 0;
259280
}
260281
}
261282

262283
return stats;
263284
}
264285

286+
private async countFields({
287+
fields,
288+
target,
289+
period,
290+
operations,
291+
excludedClients,
292+
}: {
293+
fields: ReadonlyArray<{
294+
type: string;
295+
field?: string | null;
296+
argument?: string | null;
297+
}>;
298+
target: string | readonly string[];
299+
period: DateRange;
300+
operations?: readonly string[];
301+
excludedClients?: readonly string[] | null;
302+
}): Promise<Record<string, number>> {
303+
const schemaCoordinates = fields.map(selector => this.makeId(selector));
304+
return this.countCoordinates({
305+
schemaCoordinates,
306+
targetIds: target,
307+
period,
308+
operations,
309+
excludedClients,
310+
});
311+
}
312+
265313
async hasCollectedOperations({
266314
target,
267315
}: {
@@ -919,7 +967,6 @@ export class OperationsReader {
919967
excludedClients: null | readonly string[];
920968
period: DateRange;
921969
schemaCoordinates: string[];
922-
requestCountThreshold: number;
923970
}) {
924971
const RecordArrayType = z.array(
925972
z.object({
@@ -980,7 +1027,6 @@ export class OperationsReader {
9801027
AND "coordinates_daily"."timestamp" >= toDateTime(${formatDate(args.period.from)}, 'UTC')
9811028
AND "coordinates_daily"."timestamp" <= toDateTime(${formatDate(args.period.to)}, 'UTC')
9821029
AND "coordinates_daily"."coordinate" IN (${sql.longArray(args.schemaCoordinates, 'String')})
983-
HAVING "total" >= ${String(args.requestCountThreshold)}
9841030
ORDER BY
9851031
"total" DESC,
9861032
"coordinates_daily"."hash" DESC
@@ -998,7 +1044,7 @@ export class OperationsReader {
9981044
SELECT
9991045
"operation_collection_details"."name",
10001046
"operation_collection_details"."hash"
1001-
FROM
1047+
FROM
10021048
"operation_collection_details"
10031049
PREWHERE
10041050
"operation_collection_details"."target" IN (${sql.array(args.targetIds, 'String')})
@@ -1049,7 +1095,6 @@ export class OperationsReader {
10491095
excludedClients: null | readonly string[];
10501096
period: DateRange;
10511097
schemaCoordinate: string;
1052-
requestCountThreshold: number;
10531098
},
10541099
Array<{
10551100
hash: string;
@@ -1058,14 +1103,13 @@ export class OperationsReader {
10581103
}> | null
10591104
>(
10601105
item =>
1061-
`${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}-${item.requestCountThreshold}`,
1106+
`${item.targetIds.join(',')}-${item.excludedClients?.join(',') ?? ''}-${item.period.from.toISOString()}-${item.period.to.toISOString()}`,
10621107
async items => {
10631108
const schemaCoordinates = items.map(item => item.schemaCoordinate);
10641109
return await this._getTopOperationsForSchemaCoordinates({
10651110
targetIds: items[0].targetIds,
10661111
excludedClients: items[0].excludedClients,
10671112
period: items[0].period,
1068-
requestCountThreshold: items[0].requestCountThreshold,
10691113
schemaCoordinates,
10701114
}).then(result => result.map(result => Promise.resolve(result)));
10711115
},
@@ -1578,7 +1622,7 @@ export class OperationsReader {
15781622
FROM ${aggregationTableName('operations')}
15791623
${this.createFilter({ target: targets, period: roundedPeriod })}
15801624
GROUP BY target, date
1581-
ORDER BY
1625+
ORDER BY
15821626
target,
15831627
date
15841628
WITH FILL

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

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -482,35 +482,48 @@ export class RegistryChecks {
482482
return;
483483
}
484484

485-
// We need to run both the affected operations an affected clients query.
486-
// 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.
487-
488-
const topAffectedClients = await this.operationsReader.getTopClientsForSchemaCoordinate({
485+
const totalRequestCounts = await this.operationsReader.countCoordinate({
489486
targetIds: settings.targetIds,
490487
excludedClients: settings.excludedClientNames,
491488
period: settings.period,
492489
schemaCoordinate: change.breakingChangeSchemaCoordinate,
493490
});
494-
495-
if (topAffectedClients) {
496-
const topAffectedOperations =
497-
await this.operationsReader.getTopOperationsForSchemaCoordinate({
491+
const isBreaking =
492+
totalRequestCounts[change.breakingChangeSchemaCoordinate] >=
493+
Math.max(settings.requestCountThreshold, 1);
494+
if (isBreaking) {
495+
// We need to run both the affected operations an affected clients query.
496+
// Since the affected clients query is lighter it makes more sense to run it first and skip running
497+
// the operations query if no clients are affected, as it will also yield zero results in that case.
498+
499+
const topAffectedClients = await this.operationsReader.getTopClientsForSchemaCoordinate(
500+
{
498501
targetIds: settings.targetIds,
499502
excludedClients: settings.excludedClientNames,
500503
period: settings.period,
501-
requestCountThreshold: settings.requestCountThreshold,
502504
schemaCoordinate: change.breakingChangeSchemaCoordinate,
503-
});
504-
505-
if (topAffectedOperations) {
506-
change.usageStatistics = {
507-
topAffectedOperations,
508-
topAffectedClients,
509-
};
505+
},
506+
);
507+
508+
if (topAffectedClients) {
509+
const topAffectedOperations =
510+
await this.operationsReader.getTopOperationsForSchemaCoordinate({
511+
targetIds: settings.targetIds,
512+
excludedClients: settings.excludedClientNames,
513+
period: settings.period,
514+
schemaCoordinate: change.breakingChangeSchemaCoordinate,
515+
});
516+
517+
if (topAffectedOperations) {
518+
change.usageStatistics = {
519+
topAffectedOperations,
520+
topAffectedClients,
521+
};
522+
}
510523
}
511524
}
512525

513-
change.isSafeBasedOnUsage = change.usageStatistics === null;
526+
change.isSafeBasedOnUsage = !isBreaking;
514527
}),
515528
);
516529
} else {

0 commit comments

Comments
 (0)