From 3983eb7637da34025c5cdae802b9f1fe54972293 Mon Sep 17 00:00:00 2001 From: Kamil Kisiela Date: Thu, 22 Sep 2022 13:22:35 +0200 Subject: [PATCH] Improve #379 and add more tests (#397) --- .../libraries/client/src/internal/types.ts | 2 +- .../libraries/client/src/internal/usage.ts | 44 ++++++++++++------- .../libraries/client/src/internal/utils.ts | 27 +++++++----- .../client/tests/usage-collector.spec.ts | 4 +- packages/libraries/client/tests/utils.spec.ts | 43 ++++++++++++++++++ 5 files changed, 90 insertions(+), 30 deletions(-) create mode 100644 packages/libraries/client/tests/utils.spec.ts diff --git a/packages/libraries/client/src/internal/types.ts b/packages/libraries/client/src/internal/types.ts index ceeae72b6..9ff8e9cbf 100644 --- a/packages/libraries/client/src/internal/types.ts +++ b/packages/libraries/client/src/internal/types.ts @@ -66,7 +66,7 @@ export interface HiveUsagePluginOptions { */ sampleRate?: number; /** - * Enables collecting Input fields usage based on the variables passed to the operation. + * (Experimental) Enables collecting Input fields usage based on the variables passed to the operation. * * Default: false */ diff --git a/packages/libraries/client/src/internal/usage.ts b/packages/libraries/client/src/internal/usage.ts index 6f34a8dca..4351dfe0e 100644 --- a/packages/libraries/client/src/internal/usage.ts +++ b/packages/libraries/client/src/internal/usage.ts @@ -158,7 +158,7 @@ export function createUsage(pluginOptions: HivePluginOptions): UsageCollector { ttl: options.ttl, processVariables: options.processVariables ?? false, }); - const { key, value: info } = collect(document, args.variableValues); + const { key, value: info } = collect(document, args.variableValues ?? null); agent.capture({ key, @@ -196,7 +196,7 @@ export function createCollector({ schema, max, ttl, - processVariables, + processVariables = false, }: { schema: GraphQLSchema; max?: number; @@ -205,9 +205,15 @@ export function createCollector({ }) { const typeInfo = new TypeInfo(schema); - function collect(doc: DocumentNode, variables: ExecutionArgs['variableValues']): CacheResult { + function collect( + doc: DocumentNode, + variables: { + [key: string]: unknown; + } | null + ): CacheResult { const entries = new Set(); const collected_entire_named_types = new Set(); + const shouldAnalyzeVariableValues = processVariables === true && variables !== null; function markAsUsed(id: string) { if (!entries.has(id)) { @@ -249,11 +255,16 @@ export function createCollector({ if (node.value.kind === Kind.ENUM) { // Collect only a specific enum value collectInputType(inputTypeName, node.value.value); - } else if ( - node.value.kind !== Kind.OBJECT && - node.value.kind !== Kind.LIST && - node.value.kind !== Kind.VARIABLE - ) { + } else if (node.value.kind !== Kind.OBJECT && node.value.kind !== Kind.LIST) { + // When processing of variables is enabled, + // we want to skip collecting full input types of variables + // and only collect specific fields. + // That's why the following condition is added. + // Otherwise we would mark entire input types as used, and not granular fields. + if (node.value.kind === Kind.VARIABLE && shouldAnalyzeVariableValues) { + return; + } + collectInputType(inputTypeName); } } @@ -326,13 +337,17 @@ export function createCollector({ }, VariableDefinition(node) { const inputType = typeInfo.getInputType()!; - if (!variables) { - collectInputType(resolveTypeName(inputType)); - } else { + + if (shouldAnalyzeVariableValues) { + // Granular collection of variable values is enabled const variableName = node.variable.name.value; const variableValue = variables[variableName]; const namedType = unwrapType(inputType); + collectVariable(namedType, variableValue); + } else { + // Collect the entire type without processing the variables + collectInputType(resolveTypeName(inputType)); } }, Argument(node) { @@ -386,12 +401,7 @@ export function createCollector({ }; } - return cache( - (doc: DocumentNode, variables: ExecutionArgs['variableValues']) => - collect(doc, processVariables ? variables : undefined), - cacheDocumentKey, - LRU(max, ttl) - ); + return cache(collect, cacheDocumentKey, LRU(max, ttl)); } function resolveTypeName(inputType: GraphQLType): string { diff --git a/packages/libraries/client/src/internal/utils.ts b/packages/libraries/client/src/internal/utils.ts index d9754cf3e..2171d74de 100644 --- a/packages/libraries/client/src/internal/utils.ts +++ b/packages/libraries/client/src/internal/utils.ts @@ -52,18 +52,25 @@ export function cache( }; } -export function cacheDocumentKey(doc: T, variables?: V) { - const cacheKeySource: { doc: T; variables?: string } = { doc }; - if (variables) { - cacheKeySource['variables'] = JSON.stringify(variables, (key, value) => { - if ((value && typeof value === 'object' && Object.keys(value).length) || (Array.isArray(value) && value.length)) { - return value; - } +export function cacheDocumentKey(doc: T, variables: V | null) { + const hasher = createHash('md5').update(JSON.stringify(doc)); - return ''; - }); + if (variables) { + hasher.update( + JSON.stringify(variables, (_, value) => { + if ( + (value && typeof value === 'object' && Object.keys(value).length) || + (Array.isArray(value) && value.length) + ) { + return value; + } + + return ''; + }) + ); } - return createHash('md5').update(JSON.stringify(cacheKeySource)).digest('hex'); + + return hasher.digest('hex'); } const HR_TO_NS = 1e9; diff --git a/packages/libraries/client/tests/usage-collector.spec.ts b/packages/libraries/client/tests/usage-collector.spec.ts index 10a5b6c94..e005b028b 100644 --- a/packages/libraries/client/tests/usage-collector.spec.ts +++ b/packages/libraries/client/tests/usage-collector.spec.ts @@ -239,7 +239,7 @@ test('collect all input fields when `processVariables` has not been passed and i expect(info.fields).toContain(`PaginationInput.offset`); }); -test('collect used-only input fields if input is passed as a variable', async () => { +test('(processVariables: true) collect used-only input fields', async () => { const collect = createCollector({ schema, max: 1, @@ -267,7 +267,7 @@ test('collect used-only input fields if input is passed as a variable', async () expect(info.fields).not.toContain(`PaginationInput.offset`); }); -test('collect used-only input fields if input array is passed as a variable', async () => { +test('(processVariables: true) collect used-only input type fields from an array', async () => { const collect = createCollector({ schema, max: 1, diff --git a/packages/libraries/client/tests/utils.spec.ts b/packages/libraries/client/tests/utils.spec.ts new file mode 100644 index 000000000..50c9ebac1 --- /dev/null +++ b/packages/libraries/client/tests/utils.spec.ts @@ -0,0 +1,43 @@ +import { cacheDocumentKey } from '../src/internal/utils'; + +test('produce identical hash for the same document and the same keys but different values in variables', () => { + const left = cacheDocumentKey('doc', { a: true }); + const right = cacheDocumentKey('doc', { a: false }); + expect(left).toEqual(right); +}); + +test('produce identical hash for the same document but with an empty array', () => { + const left = cacheDocumentKey('doc', { a: [] }); + const right = cacheDocumentKey('doc', { a: [] }); + expect(left).toEqual(right); +}); + +test('produce identical hash for the same document but with and without an empty array', () => { + const left = cacheDocumentKey('doc', { a: [] }); + const right = cacheDocumentKey('doc', { a: null }); + expect(left).toEqual(right); +}); + +test('produce identical hash for the same document but with an array of primitive values', () => { + const left = cacheDocumentKey('doc', { a: [1, 2, 3] }); + const right = cacheDocumentKey('doc', { a: [4, 5, 6] }); + expect(left).toEqual(right); +}); + +test('produce different hash for the same document but with different keys in variables', () => { + const left = cacheDocumentKey('doc', { a: true }); + const right = cacheDocumentKey('doc', { b: true }); + expect(left).not.toEqual(right); +}); + +test('produce different hash for the same document but with and without variables', () => { + const left = cacheDocumentKey('doc', { a: true }); + const right = cacheDocumentKey('doc', null); + expect(left).not.toEqual(right); +}); + +test('produce different hash for the same document but with and without variables (empty object)', () => { + const left = cacheDocumentKey('doc', { a: true }); + const right = cacheDocumentKey('doc', {}); + expect(left).not.toEqual(right); +});