fix(rum-oversight): unknown filter should return empty set (#9)

adobe · Oct 14, 2024 · 9554e62 · 9554e62
1 parent 719f0cd
commit 9554e62
Show file tree

Hide file tree

Showing 2 changed files with 143 additions and 96 deletions.
diff --git a/distiller.js b/distiller.js
@@ -500,7 +500,12 @@ export class DataChunks {
    * @param {string[]} skipped facets to skip
    */
   filterBundles(bundles, filterSpec, skipped = []) {
-    const existenceFilterFn = ([facetName]) => this.facetFns[facetName];
+    const existenceFilterFn = ([facetName]) => {
+      if (!this.facetFns[facetName]) {
+        throw new Error(`Unknown "${facetName}" facet in filter`);
+      }
+      return this.facetFns[facetName];
+    };
     const skipFilterFn = ([facetName]) => !skipped.includes(facetName);
     const valuesExtractorFn = (attributeName, bundle, parent) => {
       const facetValue = parent.facetFns[attributeName](bundle);
@@ -526,38 +531,44 @@ export class DataChunks {
    */
   // eslint-disable-next-line max-len
   applyFilter(bundles, filterSpec, skipFilterFn, existenceFilterFn, valuesExtractorFn, combinerExtractorFn) {
-    const filterBy = Object.entries(filterSpec)
-      .filter(skipFilterFn)
-      .filter(([, desiredValues]) => desiredValues.length)
-      .filter(existenceFilterFn);
-    return bundles.filter((bundle) => filterBy.every(([attributeName, desiredValues]) => {
-      const actualValues = valuesExtractorFn(attributeName, bundle, this);
-
-      const combiners = {
-        // if some elements match, then return true (partial inclusion)
-        some: 'some',
-        // if some elements do not match, then return true (partial exclusion)
-        none: 'some',
-        // if every element matches, then return true (full inclusion)
-        every: 'every',
-        // if every element does not match, then return true (full exclusion)
-        never: 'every',
-      };
-
-      const negators = {
-        some: (value) => value,
-        every: (value) => value,
-        none: (value) => !value,
-        never: (value) => !value,
-      };
-      // this can be some, every, or none
-      const combinerprefence = combinerExtractorFn(attributeName, this);
-
-      const combiner = combiners[combinerprefence];
-      const negator = negators[combinerprefence];
-
-      return desiredValues[combiner]((value) => negator(actualValues.includes(value)));
-    }));
+    try {
+      const filterBy = Object.entries(filterSpec)
+        .filter(skipFilterFn)
+        .filter(([, desiredValues]) => desiredValues.length)
+        .filter(existenceFilterFn);
+      return bundles.filter((bundle) => filterBy.every(([attributeName, desiredValues]) => {
+        const actualValues = valuesExtractorFn(attributeName, bundle, this);
+
+        const combiners = {
+          // if some elements match, then return true (partial inclusion)
+          some: 'some',
+          // if some elements do not match, then return true (partial exclusion)
+          none: 'some',
+          // if every element matches, then return true (full inclusion)
+          every: 'every',
+          // if every element does not match, then return true (full exclusion)
+          never: 'every',
+        };
+
+        const negators = {
+          some: (value) => value,
+          every: (value) => value,
+          none: (value) => !value,
+          never: (value) => !value,
+        };
+        // this can be some, every, or none
+        const combinerprefence = combinerExtractorFn(attributeName, this);
+
+        const combiner = combiners[combinerprefence];
+        const negator = negators[combinerprefence];
+
+        return desiredValues[combiner]((value) => negator(actualValues.includes(value)));
+      }));
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.warn(`Error while applying filter: ${error.message}`);
+      return [];
+    }
   }
 
   /**
@@ -572,7 +583,12 @@ export class DataChunks {
    * @returns {boolean} the result of the check.
    */
   hasConversion(aBundle, filterSpec, combiner) {
-    const existenceFilterFn = ([facetName]) => this.facetFns[facetName];
+    const existenceFilterFn = ([facetName]) => {
+      if (!this.facetFns[facetName]) {
+        throw new Error(`Unknown "${facetName}" facet in filter`);
+      }
+      return this.facetFns[facetName];
+    };
     const skipFilterFn = () => true;
     const valuesExtractorFn = (attributeName, bundle, parent) => {
       const facetValue = parent.facetFns[attributeName](bundle);

diff --git a/test/distiller.test.js b/test/distiller.test.js
@@ -330,6 +330,12 @@ describe('DataChunks', () => {
       id: ['one'],
     };
     assert.equal(d.filtered.length, 1);
+
+    d.filter = {
+      // trying to filter with an undefined facet
+      unknown: ['unknown'],
+    };
+    assert.equal(d.filtered.length, 0);
   });
 
   it('DataChunk.group()', () => {
@@ -686,6 +692,7 @@ describe('DataChunks', () => {
     d.load(chunks1);
 
     // define facet functions
+    d.addFacet('host', (bundle) => bundle.host);
     d.addFacet('userAgent', (bundle) => {
       const parts = bundle.userAgent.split(':');
       return parts.reduce((acc, _, i) => {
@@ -782,6 +789,7 @@ describe('DataChunks', () => {
     d.load(chunks1);
 
     // define facet functions
+    d.addFacet('host', (bundle) => bundle.host);
     d.addFacet(
       // as a convention, we use ! to indicate negation. When used in URL parameters, this
       // looks like a nice filter:  ?userAgent!=desktop
@@ -809,69 +817,70 @@ describe('DataChunks', () => {
   });
 });
 describe('DataChunks.hasConversion', () => {
+  const chunks = [
+    {
+      date: '2024-05-06',
+      rumBundles: [
+        {
+          id: 'one',
+          host: 'www.aem.live',
+          time: '2024-05-06T00:00:04.444Z',
+          timeSlot: '2024-05-06T00:00:00.000Z',
+          url: 'https://www.aem.live/developer/tutorial',
+          userAgent: 'desktop:windows',
+          weight: 100,
+          events: [
+            {
+              checkpoint: 'top',
+              target: 'visible',
+              timeDelta: 100,
+            },
+          ],
+        },
+        {
+          id: 'two',
+          host: 'www.aem.live',
+          time: '2024-05-06T00:00:04.444Z',
+          timeSlot: '2024-05-06T00:00:00.000Z',
+          url: 'https://www.aem.live/home',
+          userAgent: 'desktop',
+          weight: 100,
+          events: [
+            {
+              checkpoint: 'top',
+              target: 'hidden',
+              timeDelta: 200,
+            },
+            {
+              checkpoint: 'click',
+            },
+          ],
+        },
+        {
+          id: 'three',
+          host: 'www.aem.live',
+          time: '2024-05-06T00:00:04.444Z',
+          timeSlot: '2024-05-06T00:00:00.000Z',
+          url: 'https://www.aem.live/home',
+          userAgent: 'mobile:ios',
+          weight: 100,
+          events: [
+            {
+              checkpoint: 'top',
+              target: 'visible',
+              timeDelta: 200,
+            },
+            {
+              checkpoint: 'viewmedia',
+              target: 'some_image.png',
+            },
+          ],
+        },
+      ],
+    },
+  ];
+
   it('will tag bundles with convert and not-convert based on a filter spec', () => {
-    const chunks = [
-      {
-        date: '2024-05-06',
-        rumBundles: [
-          {
-            id: 'one',
-            host: 'www.aem.live',
-            time: '2024-05-06T00:00:04.444Z',
-            timeSlot: '2024-05-06T00:00:00.000Z',
-            url: 'https://www.aem.live/developer/tutorial',
-            userAgent: 'desktop:windows',
-            weight: 100,
-            events: [
-              {
-                checkpoint: 'top',
-                target: 'visible',
-                timeDelta: 100,
-              },
-            ],
-          },
-          {
-            id: 'two',
-            host: 'www.aem.live',
-            time: '2024-05-06T00:00:04.444Z',
-            timeSlot: '2024-05-06T00:00:00.000Z',
-            url: 'https://www.aem.live/home',
-            userAgent: 'desktop',
-            weight: 100,
-            events: [
-              {
-                checkpoint: 'top',
-                target: 'hidden',
-                timeDelta: 200,
-              },
-              {
-                checkpoint: 'click',
-              },
-            ],
-          },
-          {
-            id: 'three',
-            host: 'www.aem.live',
-            time: '2024-05-06T00:00:04.444Z',
-            timeSlot: '2024-05-06T00:00:00.000Z',
-            url: 'https://www.aem.live/home',
-            userAgent: 'mobile:ios',
-            weight: 100,
-            events: [
-              {
-                checkpoint: 'top',
-                target: 'visible',
-                timeDelta: 200,
-              },
-              {
-                checkpoint: 'viewmedia',
-                target: 'some_image.png',
-              },
-            ],
-          },
-        ],
-      },
-    ];
     const d = new DataChunks();
     d.load(chunks);
 
@@ -889,4 +898,26 @@ describe('DataChunks.hasConversion', () => {
     const notConverted = facets.find((f) => f.value === 'not-converted');
     assert.equal(notConverted?.count, 2);
   });
+
+  it('unknown facet in filter spec', () => {
+    const d = new DataChunks();
+    d.load(chunks);
+
+    const spec = {
+      facetOne: ['top'],
+      facetTwo: ['hidden'],
+      // trying to filter with an undefined facet
+      unknowFacet: ['unknown'],
+    };
+    d.addFacet('facetOne', (bundle) => bundle.events.map((e) => e.checkpoint));
+    d.addFacet('facetTwo', (bundle) => bundle.events.map((e) => e.target));
+    const facetValueFn = (bundle) => (d.hasConversion(bundle, spec) ? 'converted' : 'not-converted');
+    d.addFacet('conversion', facetValueFn);
+
+    const facets = d.facets.conversion;
+    const converted = facets.find((f) => f.value === 'converted');
+    assert.equal(converted, undefined);
+    const notConverted = facets.find((f) => f.value === 'not-converted');
+    assert.equal(notConverted?.count, 3);
+  });
 });