chore: Use canonical JSON for summarizer indexing. (#862)

Author: kinyoklion

packages/shared/common/__tests__/Context.test.ts

@@ -1,7 +1,7 @@
 import * as fs from 'node:fs';
 import * as path from 'node:path';
 
-import { canonicalize } from '../../src/json/canonicalize';
+import { canonicalize } from '../../../src/internal/json/canonicalize';
 
 // Get the test file pairs
 const testInputDir = path.join(__dirname, 'testdata', 'input');

packages/shared/common/__tests__/json/canonicalize.test.ts renamed to packages/shared/common/__tests__/internal/json/canonicalize.test.ts

@@ -1,7 +1,6 @@
 /* eslint-disable no-underscore-dangle */
 // eslint-disable-next-line max-classes-per-file
 import type {
-  Crypto,
   LDContext,
   LDContextCommon,
   LDMultiKindContext,
@@ -10,7 +9,7 @@ import type {
 } from './api';
 import AttributeReference from './AttributeReference';
 import { isLegacyUser, isMultiKind, isSingleKind } from './internal/context';
-import { canonicalize } from './json/canonicalize';
+import { canonicalize } from './internal/json/canonicalize';
 import { TypeValidators } from './validators';
 
 // The general strategy for the context is to transform the passed in context
@@ -172,6 +171,8 @@ export default class Context {
 
   private _privateAttributeReferences?: Record<string, AttributeReference[]>;
 
+  private _cachedCanonicalJson?: string;
+
   public readonly kind: string;
 
   /**
@@ -468,26 +469,25 @@ export default class Context {
     return this._wasLegacy;
   }
 
-  public async hash(crypto: Crypto): Promise<string | undefined> {
+  /**
+   * Get the serialized canonical JSON for this context. This is not filtered for use in events.
+   *
+   * This method will cache the result.
+   *
+   * @returns The serialized canonical JSON or undefined if it cannot be serialized.
+   */
+  public canonicalUnfilteredJson(): string | undefined {
     if (!this.valid) {
       return undefined;
     }
-
+    if (this._cachedCanonicalJson) {
+      return this._cachedCanonicalJson;
+    }
     try {
-      const canonicalized = canonicalize(this);
-
-      const hasher = crypto.createHash('sha256');
-      hasher.update(canonicalized);
-
-      if (hasher.digest) {
-        return hasher.digest('hex');
-      }
-
-      // The hasher must have either digest or asyncDigest.
-      const digest = await hasher.asyncDigest!('hex');
-      return digest;
+      this._cachedCanonicalJson = canonicalize(Context.toLDContext(this));
     } catch {
-      return undefined;
+      // Indicated by undefined being returned.
     }
+    return this._cachedCanonicalJson;
   }
 }

packages/shared/common/__tests__/json/testdata/LICENSE.txt renamed to packages/shared/common/__tests__/internal/json/testdata/LICENSE.txt

@@ -154,11 +154,7 @@ export default class EventProcessor implements LDEventProcessor {
     );
 
     if (summariesPerContext) {
-      this._summarizer = new MultiEventSummarizer(
-        clientContext.platform.crypto,
-        this._contextFilter,
-        this._logger,
-      );
+      this._summarizer = new MultiEventSummarizer(this._contextFilter, this._logger);
     } else {
       this._summarizer = new EventSummarizer();
     }
@@ -232,7 +228,7 @@ export default class EventProcessor implements LDEventProcessor {
     this._queue = [];
 
     if (isMultiEventSummarizer(this._summarizer)) {
-      const summaries = await this._summarizer.getSummaries();
+      const summaries = this._summarizer.getSummaries();
 
       summaries.forEach((summary) => {
         if (Object.keys(summary.features).length) {

packages/shared/common/__tests__/json/testdata/VENDOR.txt renamed to packages/shared/common/__tests__/internal/json/testdata/VENDOR.txt

@@ -45,7 +45,7 @@ export interface LDMultiEventSummarizer {
    * Gets the current summary of processed events.
    * @returns A summary of all processed feature flag events
    */
-  getSummaries(): Promise<SummarizedFlagsEvent[]>;
+  getSummaries(): SummarizedFlagsEvent[];
 }
 
 /**

packages/shared/common/__tests__/json/testdata/input/arrays.json renamed to packages/shared/common/__tests__/internal/json/testdata/input/arrays.json

@@ -1,16 +1,12 @@
-import { setupAsyncCrypto, setupCrypto } from '../../../__tests__/setupCrypto';
 import { LDLogger } from '../../api';
 import Context from '../../Context';
 import ContextFilter from '../../ContextFilter';
 import InputEvalEvent from './InputEvalEvent';
 import InputIdentifyEvent from './InputIdentifyEvent';
 import MultiEventSummarizer from './MultiEventSummarizer';
 
-// Test with both sync and async crypto implementations
-describe.each([
-  ['sync', setupCrypto()],
-  ['async', setupAsyncCrypto()],
-])('with mocked crypto and hasher/%s', (_name, crypto) => {
+// Test with both sync and crypto implementations
+describe('given a mock logger and an event summarizer instance', () => {
   let logger: LDLogger;
   let summarizer: MultiEventSummarizer;
 
@@ -22,42 +18,42 @@ describe.each([
       info: jest.fn(),
       debug: jest.fn(),
     };
-    summarizer = new MultiEventSummarizer(crypto, contextFilter, logger);
+    summarizer = new MultiEventSummarizer(contextFilter, logger);
   });
 
-  it('creates new summarizer for new context hash', async () => {
+  it('creates new summarizer for new context hash', () => {
     const context = Context.fromLDContext({ kind: 'user', key: 'user1' });
     const event = new InputEvalEvent(true, context, 'flag-key', 'value', 'default', 1, 0, true);
 
     summarizer.summarizeEvent(event);
 
-    const summaries = await summarizer.getSummaries();
+    const summaries = summarizer.getSummaries();
     expect(summaries).toHaveLength(1);
   });
 
-  it('uses existing summarizer for same context hash', async () => {
+  it('uses existing summarizer for same context hash', () => {
     const context = Context.fromLDContext({ kind: 'user', key: 'user1' });
     const event1 = new InputEvalEvent(true, context, 'flag-key', 'value1', 'default', 1, 0, true);
     const event2 = new InputEvalEvent(true, context, 'flag-key', 'value2', 'default', 1, 0, true);
 
     summarizer.summarizeEvent(event1);
     summarizer.summarizeEvent(event2);
 
-    const summaries = await summarizer.getSummaries();
+    const summaries = summarizer.getSummaries();
     expect(summaries).toHaveLength(1);
   });
 
-  it('ignores non-feature events', async () => {
+  it('ignores non-feature events', () => {
     const context = Context.fromLDContext({ kind: 'user', key: 'user1' });
     const event = new InputIdentifyEvent(context);
 
     summarizer.summarizeEvent(event);
 
-    const summaries = await summarizer.getSummaries();
+    const summaries = summarizer.getSummaries();
     expect(summaries).toHaveLength(0);
   });
 
-  it('handles multiple different contexts', async () => {
+  it('handles multiple different contexts', () => {
     const context1 = Context.fromLDContext({ kind: 'user', key: 'user1' });
     const context2 = Context.fromLDContext({ kind: 'user', key: 'user2' });
     const event1 = new InputEvalEvent(true, context1, 'flag-key', 'value1', 'default', 1, 0, true);
@@ -66,23 +62,23 @@ describe.each([
     summarizer.summarizeEvent(event1);
     summarizer.summarizeEvent(event2);
 
-    const summaries = await summarizer.getSummaries();
+    const summaries = summarizer.getSummaries();
     expect(summaries).toHaveLength(2);
   });
 
-  it('automatically clears summaries when summarized', async () => {
+  it('automatically clears summaries when summarized', () => {
     const context = Context.fromLDContext({ kind: 'user', key: 'user1' });
     const event = new InputEvalEvent(true, context, 'flag-key', 'value', 'default', 1, 0, true);
 
     summarizer.summarizeEvent(event);
 
-    const summariesA = await summarizer.getSummaries();
-    const summariesB = await summarizer.getSummaries();
+    const summariesA = summarizer.getSummaries();
+    const summariesB = summarizer.getSummaries();
     expect(summariesA).toHaveLength(1);
     expect(summariesB).toHaveLength(0);
   });
 
-  it('logs error when context cannot be hashed', async () => {
+  it('logs error when context cannot be hashed', () => {
     const a: any = {};
     const b: any = { a };
     a.b = b;
@@ -91,10 +87,10 @@ describe.each([
     const event = new InputEvalEvent(true, context, 'flag-key', 'value', 'default', 1, 0, true);
 
     summarizer.summarizeEvent(event);
-    await summarizer.getSummaries();
+    summarizer.getSummaries();
     expect(logger.error).toHaveBeenCalledTimes(1);
     expect(logger.error).toHaveBeenCalledWith(
-      'Unable to hash context, likely the context contains a cycle.',
+      'Unable to serialize context, likely the context contains a cycle.',
     );
   });
 });

packages/shared/common/__tests__/json/testdata/input/french.json renamed to packages/shared/common/__tests__/internal/json/testdata/input/french.json

@@ -1,4 +1,4 @@
-import { Crypto, LDLogger } from '../../api';
+import { LDLogger } from '../../api';
 import ContextFilter from '../../ContextFilter';
 import EventSummarizer from './EventSummarizer';
 import { isFeature } from './guards';
@@ -7,55 +7,34 @@ import { LDMultiEventSummarizer, SummarizedFlagsEvent } from './LDEventSummarize
 
 export default class MultiEventSummarizer implements LDMultiEventSummarizer {
   constructor(
-    private readonly _crypto: Crypto,
     private readonly _contextFilter: ContextFilter,
     private readonly _logger?: LDLogger,
   ) {}
   private _summarizers: Record<string, EventSummarizer> = {};
-  private _pendingPromises: Promise<void>[] = [];
 
   summarizeEvent(event: InputEvent) {
-    // The event is summarized asynchronously, but the promise is created synchronously, this means that all events
-    // which have been requested to be summarized will be in the next flush.
-    const promise = (async () => {
-      if (isFeature(event)) {
-        const hash = await event.context.hash(this._crypto);
-        if (!hash) {
-          if (event.context.valid) {
-            // The context appeared valid, but it could not be hashed.
-            // This is likely because of a cycle in the data.
-            this._logger?.error('Unable to hash context, likely the context contains a cycle.');
-          }
-          return;
+    if (isFeature(event)) {
+      const key = event.context.canonicalUnfilteredJson();
+      if (!key) {
+        if (event.context.valid) {
+          // The context appeared valid, but it could not be hashed.
+          // This is likely because of a cycle in the data.
+          this._logger?.error('Unable to serialize context, likely the context contains a cycle.');
         }
-        // It is important that async operations do not happen between checking that the summarizer
-        // exists and having it summarize the event.
-        // If it did, then that event could be lost.
-        let summarizer = this._summarizers[hash];
-        if (!summarizer) {
-          this._summarizers[hash] = new EventSummarizer(true, this._contextFilter);
-          summarizer = this._summarizers[hash];
-        }
-
-        summarizer.summarizeEvent(event);
+        return;
       }
-    })();
-    this._pendingPromises.push(promise);
-    promise.finally(() => {
-      const index = this._pendingPromises.indexOf(promise);
-      if (index !== -1) {
-        this._pendingPromises.splice(index, 1);
+
+      let summarizer = this._summarizers[key];
+      if (!summarizer) {
+        this._summarizers[key] = new EventSummarizer(true, this._contextFilter);
+        summarizer = this._summarizers[key];
       }
-    });
-  }
 
-  async getSummaries(): Promise<SummarizedFlagsEvent[]> {
-    // Wait for any pending summarizations to complete
-    // Additional tasks queued while waiting will not be waited for.
-    await Promise.all([...this._pendingPromises]);
+      summarizer.summarizeEvent(event);
+    }
+  }
 
-    // It is important not to put any async operations between caching the summarizers and clearing them.
-    // If we did then summerizers added during the async operation would be lost.
+  getSummaries(): SummarizedFlagsEvent[] {
     const summarizersToFlush = this._summarizers;
     this._summarizers = {};
     return Object.values(summarizersToFlush).map((summarizer) => summarizer.getSummary());

packages/shared/common/__tests__/json/testdata/input/structures.json renamed to packages/shared/common/__tests__/internal/json/testdata/input/structures.json

@@ -5,3 +5,4 @@ export * from './events';
 export * from './fdv2';
 export * from './metadata';
 export * from './plugins';
+export * from './json';

packages/shared/common/__tests__/json/testdata/input/unicode.json renamed to packages/shared/common/__tests__/internal/json/testdata/input/unicode.json

@@ -0,0 +1 @@
+export * from './canonicalize';