From e36ea0f6556eb1ffe4ece33c4e4819fd5831906e Mon Sep 17 00:00:00 2001 From: Karl Pauls Date: Mon, 14 Oct 2024 12:47:47 +0200 Subject: [PATCH] fix(rum-oversight): unknown filter should return empty set --- distiller.js | 84 +++++++++++++--------- test/distiller.test.js | 155 ++++++++++++++++++++++++----------------- 2 files changed, 143 insertions(+), 96 deletions(-) diff --git a/distiller.js b/distiller.js index f006af8..fbab585 100644 --- 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 index e128b8d..d464e6a 100644 --- 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); + }); });