fix: prevent putDimensions from storing duplicate dimensions

Conditions previously checked for any dimension set which did not match
before adding the input dimension set.

This would allow different-sized dimension sets from being mistaken
as non-duplicate and being added (see unit test for example).

New conditions check for any dimension set which does match
before skipping adding the input dimension set to the collection.

[TESTING]

Unit test updated to address edge case;
multiple dimension sets of variable size are added and checked.

Author: Aaron Michael Lamb

src/logger/MetricsContext.ts

@@ -58,22 +58,22 @@ export class MetricsContext {
     dimensions?: Array<Record<string, string>>,
     defaultDimensions?: Record<string, string>,
     shouldUseDefaultDimensions?: boolean,
-    timestamp?: Date | number
+    timestamp?: Date | number,
   ) {
-    this.namespace = namespace || Configuration.namespace
+    this.namespace = namespace || Configuration.namespace;
     this.properties = properties || {};
     this.dimensions = dimensions || [];
     this.timestamp = timestamp;
     this.meta.Timestamp = MetricsContext.resolveMetaTimestamp(timestamp);
     this.defaultDimensions = defaultDimensions || {};
     if (shouldUseDefaultDimensions != undefined) {
-      this.shouldUseDefaultDimensions = shouldUseDefaultDimensions
+      this.shouldUseDefaultDimensions = shouldUseDefaultDimensions;
     }
   }
 
   private static resolveMetaTimestamp(timestamp?: Date | number): number {
     if (timestamp instanceof Date) {
-      return timestamp.getTime()
+      return timestamp.getTime();
     } else if (timestamp) {
       return timestamp;
     } else {
@@ -111,33 +111,24 @@ export class MetricsContext {
    * @param dimensions
    */
   public putDimensions(incomingDimensionSet: Record<string, string>): void {
-    if (this.dimensions.length === 0) {
-      this.dimensions.push(incomingDimensionSet);
-      return;
-    }
-
-    for (let i = 0; i < this.dimensions.length; i++) {
-      const existingDimensionSet = this.dimensions[i];
+    const incomingDimensionSetKeys = Object.keys(incomingDimensionSet);
 
-      // check for duplicate dimensions when putting
-      // this is an O(n^2) operation, but since we never expect to have more than
-      // 10 dimensions, this is acceptable for almost all cases.
-      // This makes re-using loggers much easier.
+    // This operation is O(n^2), but acceptable given sets are capped at 10 dimensions
+    const doesDimensionSetExist = this.dimensions.some(existingDimensionSet => {
       const existingDimensionSetKeys = Object.keys(existingDimensionSet);
-      const incomingDimensionSetKeys = Object.keys(incomingDimensionSet);
       if (existingDimensionSetKeys.length !== incomingDimensionSetKeys.length) {
-        this.dimensions.push(incomingDimensionSet);
-        return;
+        return false;
       }
+      return existingDimensionSetKeys.every(existingDimensionSetKey =>
+        incomingDimensionSetKeys.includes(existingDimensionSetKey),
+      );
+    });
 
-      for (let j = 0; j < existingDimensionSetKeys.length; j++) {
-        if (!incomingDimensionSetKeys.includes(existingDimensionSetKeys[j])) {
-          // we're done now because we know that the dimensions keys are not identical
-          this.dimensions.push(incomingDimensionSet);
-          return;
-        }
-      }
+    if (doesDimensionSetExist) {
+      return;
     }
+
+    this.dimensions.push(incomingDimensionSet);
   }
 
   /**
@@ -196,7 +187,7 @@ export class MetricsContext {
       Object.assign([], this.dimensions),
       this.defaultDimensions,
       this.shouldUseDefaultDimensions,
-      this.timestamp
+      this.timestamp,
     );
   }
 }

src/logger/__tests__/MetricsContext.test.ts

@@ -16,7 +16,7 @@ test('can set property', () => {
   expect(actualValue).toBe(expectedValue);
 });
 
-test('putDimension adds key to dimension and sets the dimension as a property', () => {
+test('putDimensions adds key to dimension and sets the dimension as a property', () => {
   // arrange
   const context = MetricsContext.empty();
   const dimension = faker.random.word();
@@ -29,51 +29,46 @@ test('putDimension adds key to dimension and sets the dimension as a property',
   expect(context.getDimensions()[0]).toStrictEqual(expectedDimension);
 });
 
-test('putDimension will not duplicate dimensions', () => {
+test('putDimensions accepts multiple unique dimension sets', () => {
   // arrange
   const context = MetricsContext.empty();
-  const dimension = faker.random.word();
-  const expectedDimension = { dimension };
+  const expectedDimension1 = { d1: faker.random.word(), d2: faker.random.word() };
+  const expectedDimension2 = { d2: faker.random.word(), d3: faker.random.word() };
 
   // act
-  context.putDimensions({ dimension });
-  context.putDimensions({ dimension });
+  context.putDimensions(expectedDimension1);
+  context.putDimensions(expectedDimension2);
 
   // assert
-  expect(context.getDimensions().length).toBe(1);
-  expect(context.getDimensions()[0]).toStrictEqual(expectedDimension);
+  expect(context.getDimensions().length).toBe(2);
+  expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1);
+  expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2);
 });
 
-test('putDimension will not duplicate dimensions, multiple in different order', () => {
+test('putDimensions will not duplicate dimensions', () => {
   // arrange
   const context = MetricsContext.empty();
   const dimension1 = faker.random.word();
   const dimension2 = faker.random.word();
-  const expectedDimension = { dimension1, dimension2 };
+  const expectedDimension1 = { dimension1 };
+  const expectedDimension2 = { dimension1, dimension2 };
+  const expectedDimension3 = { dimension2 };
 
   // act
+  context.putDimensions({ dimension1 });
   context.putDimensions({ dimension1, dimension2 });
   context.putDimensions({ dimension2, dimension1 });
+  context.putDimensions({ dimension2 });
+  context.putDimensions({ dimension1 });
+  context.putDimensions({ dimension1, dimension2 });
+  context.putDimensions({ dimension2, dimension1 });
+  context.putDimensions({ dimension2 });
 
   // assert
-  expect(context.getDimensions().length).toBe(1);
-  expect(context.getDimensions()[0]).toStrictEqual(expectedDimension);
-});
-
-test('putDimension accepts multiple unique dimension sets', () => {
-  // arrange
-  const context = MetricsContext.empty();
-  const expectedDimension1 = { d1: faker.random.word(), d2: faker.random.word() };
-  const expectedDimension2 = { d2: faker.random.word(), d3: faker.random.word() };
-
-  // act
-  context.putDimensions(expectedDimension1);
-  context.putDimensions(expectedDimension2);
-
-  // assert
-  expect(context.getDimensions().length).toBe(2);
+  expect(context.getDimensions().length).toBe(3);
   expect(context.getDimensions()[0]).toStrictEqual(expectedDimension1);
   expect(context.getDimensions()[1]).toStrictEqual(expectedDimension2);
+  expect(context.getDimensions()[2]).toStrictEqual(expectedDimension3);
 });
 
 test('getDimensions returns default dimensions if custom dimensions not set', () => {
@@ -201,5 +196,5 @@ test('createCopyWithContext copies shouldUseDefaultDimensions', () => {
 
   // assert
   expect(newContext).not.toBe(context);
-  expect(newContext.getDimensions()).toEqual([])
+  expect(newContext.getDimensions()).toEqual([]);
 });