From cb97acaa3f5557927c53aabc71537d442bfe3cbf Mon Sep 17 00:00:00 2001 From: "george.crawford" <84737+georgecrawford@users.noreply.github.com> Date: Mon, 4 Nov 2024 15:16:19 +0000 Subject: [PATCH] fix: correctly detect truly circular paths in JS objects, rather than objects with multiple references --- .../o-tracking/src/javascript/core/send.js | 12 +- .../o-tracking/src/javascript/core/store.js | 11 +- libraries/o-tracking/src/javascript/utils.js | 160 +++++++++--------- .../o-tracking/test/events/custom.test.js | 43 +++-- libraries/o-tracking/test/utils.test.js | 85 ++++------ 5 files changed, 143 insertions(+), 168 deletions(-) diff --git a/libraries/o-tracking/src/javascript/core/send.js b/libraries/o-tracking/src/javascript/core/send.js index 229a63c735..41d21b7d64 100644 --- a/libraries/o-tracking/src/javascript/core/send.js +++ b/libraries/o-tracking/src/javascript/core/send.js @@ -1,5 +1,5 @@ import {get as getSetting} from './settings.js'; -import {broadcast, is, findCircularPathsIn, containsCircularPaths, merge, addEvent, log} from '../utils.js'; +import {broadcast, is, safelyStringifyJson, merge, addEvent, log} from '../utils.js'; import {Queue} from './queue.js'; import {get as getTransport} from './transports/index.js'; @@ -68,15 +68,7 @@ function sendRequest(request, callback) { log('user_callback', user_callback); log('PreSend', request); - if (containsCircularPaths(request)) { - const errorMessage = "o-tracking does not support circular references in the analytics data.\n" + - "Please remove the circular references in the data.\n" + - "Here are the paths in the data which are circular:\n" + - JSON.stringify(findCircularPathsIn(request), undefined, 4); - throw new Error(errorMessage); - } - - const stringifiedData = JSON.stringify(request); + const stringifiedData = safelyStringifyJson(request); transport.complete(function (error) { if (is(user_callback, 'function')) { diff --git a/libraries/o-tracking/src/javascript/core/store.js b/libraries/o-tracking/src/javascript/core/store.js index 8a5435c500..eaacba1cd4 100644 --- a/libraries/o-tracking/src/javascript/core/store.js +++ b/libraries/o-tracking/src/javascript/core/store.js @@ -1,4 +1,4 @@ -import {broadcast, containsCircularPaths, decode, encode, findCircularPathsIn, is} from '../utils.js'; +import {broadcast, safelyStringifyJson, decode, encode, is} from '../utils.js'; /** * Class for storing data @@ -172,14 +172,7 @@ Store.prototype.write = function (data) { if (typeof this.data === 'string') { value = this.data; } else { - if (containsCircularPaths(this.data)) { - const errorMessage = "o-tracking does not support circular references in the analytics data.\n" + - "Please remove the circular references in the data.\n" + - "Here are the paths in the data which are circular:\n" + - JSON.stringify(findCircularPathsIn(this.data), undefined, 4); - throw new Error(errorMessage); - } - value = JSON.stringify(this.data); + value = safelyStringifyJson(this.data); } this.storage.save(this.storageKey, value); diff --git a/libraries/o-tracking/src/javascript/utils.js b/libraries/o-tracking/src/javascript/utils.js index 5217281a29..f02cde2818 100644 --- a/libraries/o-tracking/src/javascript/utils.js +++ b/libraries/o-tracking/src/javascript/utils.js @@ -222,99 +222,94 @@ function assignIfUndefined (subject, target) { } /** - * Used to find out all the paths which contain a circular reference. + * Used to find out all the paths which contain a circular reference. + * Inspired by https://github.com/douglascrockford/JSON-js/blob/master/cycle.js * - * @param {*} rootObject The object we want to search within for circular references - * @returns {string[]} Returns an array of strings, the strings are the full paths to the circular references within the rootObject + * @param {*} object The object we want to stringify, and search within for circular references + * @returns {string} The safely stringified JSON string + * @throws {Error} Error with message highlighting the paths which are circular references */ -function findCircularPathsIn(rootObject) { - const traversedValues = new WeakSet(); +function safelyStringifyJson(object) { + const traversedValues = new WeakMap(); const circularPaths = []; - function _findCircularPathsIn(currentObject, path) { - // If we already saw this object - // the rootObject contains a circular reference - // and we can stop looking any further into this currentObj - if (traversedValues.has(currentObject)) { - circularPaths.push(path); - return; - } + // By definition, JSON.stringify() will throw a TypeError + // if the object contains a circular reference + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#exceptions + try { + return JSON.stringify(object); + } catch (error) { + // ignore error + } - // Only Objects and things which inherit from Objects can contain circular references - // I.E. string/number/boolean/template literals can not contain circular references - if (currentObject instanceof Object) { - traversedValues.add(currentObject); - - // Loop through all the values of the current object and search those for circular references - for (const [key, value] of Object.entries(currentObject)) { - // No need to recurse on every value because only things which inherit - // from Objects can contain circular references - if (value instanceof Object) { - const parentObjectIsAnArray = Array.isArray(currentObject); - if (parentObjectIsAnArray) { - // Store path in bracket notation when value is an array - _findCircularPathsIn(value, `${path}[${key}]`); - } else { - // Store path in dot-notation when value is an object - _findCircularPathsIn(value, `${path}.${key}`); - } - } + function findCircularReferences(key, value, path = "") { + let oldPath; + let modifiedValue; + + if ( + typeof value === "object" + && value !== null + && !(value instanceof Boolean) + && !(value instanceof Date) + && !(value instanceof Number) + && !(value instanceof RegExp) + && !(value instanceof String) + ) { + oldPath = traversedValues.get(value); + + if (oldPath !== undefined) { + circularPaths.push(path); + + // $ref has no standardised meaning + // (see https://json-schema.org/understanding-json-schema/structuring#dollarref), + // but could be used to reconstruct circular references on the server if required + // (see the 'retrocycle' function at + // https://github.com/douglascrockford/JSON-js/blob/master/cycle.js. + return { $ref: oldPath }; } - } - } - _findCircularPathsIn(rootObject, ""); - return circularPaths; -} + traversedValues.set(value, path); + + if (Array.isArray(value)) { + modifiedValue = []; + value.forEach(function (element, i) { + modifiedValue[i] = findCircularReferences(i, element, path + "[" + i + "]"); + }); + } else { + modifiedValue = {}; + Object.keys(value).forEach(function (name) { + modifiedValue[name] = findCircularReferences( + name, + value[name], + path + "." + name + ); + }); + } -/** - * Used to find out whether an object contains a circular reference. - * - * @param {object} rootObject The object we want to search within for circular references - * @returns {boolean} Returns true if a circular reference was found, otherwise returns false - */ -function containsCircularPaths(rootObject) { - // Used to keep track of all the values the rootObject contains - const traversedValues = new WeakSet(); - - /** - * - * @param {*} currentObject The current object we want to search within for circular references - * @returns {boolean|undefined} Returns true if a circular reference was found, otherwise returns undefined - */ - function _containsCircularPaths(currentObject) { - // If we already saw this object - // the rootObject contains a circular reference - // and we can stop looking any further - if (traversedValues.has(currentObject)) { - return true; + return modifiedValue; } - // Only Objects and things which inherit from Objects can contain circular references - // I.E. string/number/boolean/template literals can not contain circular references - if (currentObject instanceof Object) { - traversedValues.add(currentObject); - // Loop through all the values of the current object and search those for circular references - for (const value of Object.values(currentObject)) { - // No need to recurse on every value because only things which inherit - // from Objects can contain circular references - if (value instanceof Object) { - if (_containsCircularPaths(value)) { - return true; - } - } - } - } + return value; } - // _containsCircularPaths returns true or undefined. - // By using Boolean we convert the undefined into false. - return Boolean( - _containsCircularPaths( - rootObject - ) - ); -} + const safeJson = JSON.stringify(object, findCircularReferences); + + // NB: by throwing an error when a circular path is encountered, the tracking payload is + // discarded. A future iteration of this feature could be to send the JSON with circular + // references replaced with `$ref: `, and to allow the server to reconstruct the + // references and process the payload. + // See https://json-schema.org/understanding-json-schema/structuring#dollarref, and + // the 'retrocycle' function at https://github.com/douglascrockford/JSON-js/blob/master/cycle.js. + if (circularPaths.length) { + const errorMessage = "AssertionError: o-tracking does not support circular references in the analytics data.\n" + + "Please remove the circular references in the data.\n" + + "Here are the paths in the data which are circular:\n" + + JSON.stringify(circularPaths, undefined, 4); + throw new Error(errorMessage); + } + + return safeJson; +}; /** * Find out whether two objects are deeply equal to each other. @@ -403,7 +398,6 @@ export { sanitise, assignIfUndefined, filterProperties, - findCircularPathsIn, - containsCircularPaths, + safelyStringifyJson, isDeepEqual }; diff --git a/libraries/o-tracking/test/events/custom.test.js b/libraries/o-tracking/test/events/custom.test.js index 5f51b6bc91..0c4331a978 100644 --- a/libraries/o-tracking/test/events/custom.test.js +++ b/libraries/o-tracking/test/events/custom.test.js @@ -92,19 +92,13 @@ describe('event', function () { const customTrackingData = {ohh: 'ahh'}; customTrackingData.circular = customTrackingData; - const errorMessage = `o-tracking does not support circular references in the analytics data. + const errorMessage = `AssertionError: o-tracking does not support circular references in the analytics data. Please remove the circular references in the data. Here are the paths in the data which are circular: [ ".context.context.circular" ]`; - const errorMessageOnIOS10 = `o-tracking does not support circular references in the analytics data. -Please remove the circular references in the data. -Here are the paths in the data which are circular: -[ - "[0].item.context.context.circular" -]`; try { trackEvent( new CustomEvent("oTracking.event", { @@ -122,11 +116,36 @@ Here are the paths in the data which are circular: proclaim.notOk("Expected function to throw an error but it did not"); } catch (error) { proclaim.isInstanceOf(error, Error); - try { - proclaim.equal(error.message, errorMessage); - } catch (testError) { - proclaim.equal(error.message, errorMessageOnIOS10); - } + proclaim.equal(error.message, errorMessage); } }); + + it('should not report a circular reference when multiple references are made to the same object', function () { + const callback = sinon.spy(); + + const contextPartOne = {key: 'part one'}; + const contextPartTwo = {key: 'part two'}; + + const sharedReference = {shared: 'shared'}; + contextPartOne.sharedReference = sharedReference; + contextPartTwo.sharedReference = sharedReference; + + trackEvent( + new CustomEvent("oTracking.event", { + detail: { + category: "slideshow", + action: "slide_viewed", + slide_number: 5, + component_id: "123456", + component_name: "custom-o-tracking", + contextPartOne, + contextPartTwo + }, + }), + callback + ); + + proclaim.ok("Expected function to not to throw an error"); + proclaim.ok(callback.called, 'Callback not called.'); + }); }); diff --git a/libraries/o-tracking/test/utils.test.js b/libraries/o-tracking/test/utils.test.js index 9d1aa6958a..0a88bfbfe9 100644 --- a/libraries/o-tracking/test/utils.test.js +++ b/libraries/o-tracking/test/utils.test.js @@ -14,8 +14,7 @@ import { sanitise, assignIfUndefined, filterProperties, - findCircularPathsIn, - containsCircularPaths, + safelyStringifyJson, guid } from '../src/javascript/utils.js'; @@ -122,28 +121,28 @@ describe('Utils', function () { }); }); - describe('findCircularPathsIn', function() { - it('should return an empty array if object contains no circular references', function() { - proclaim.deepStrictEqual(findCircularPathsIn({ a: 1, b: 2 }), []); + describe('safelyStringifyJson', function() { + it('should return a JSON string if object contains no circular references', function() { + proclaim.strictEqual(safelyStringifyJson({ a: 1, b: 2 }), '{"a":1,"b":2}'); }); - it('should return an empty array if given a string literal', function() { - proclaim.deepStrictEqual(findCircularPathsIn(''), []); + it('should return a JSON empty string if given a string literal', function() { + proclaim.strictEqual(safelyStringifyJson(''), '""'); }); - it('should return an empty array if given a number literal', function() { - proclaim.deepStrictEqual(findCircularPathsIn(137), []); + it('should return a JSON number if given a number literal', function() { + proclaim.strictEqual(safelyStringifyJson(137), '137'); }); - it('should return an empty array if given a boolean literal', function() { - proclaim.deepStrictEqual(findCircularPathsIn(true), []); + it('should return the string true if given a boolean literal', function() { + proclaim.strictEqual(safelyStringifyJson(true), 'true'); }); - it('should return an empty array if given null', function() { - proclaim.deepStrictEqual(findCircularPathsIn(null), []); + it('should return the string null if given null', function() { + proclaim.strictEqual(safelyStringifyJson(null), 'null'); }); - it('should return an array containing all the paths which are circular', function() { + it('should throw an informative error listing all the paths which are circular', function() { const rootObject = {}; rootObject.a = rootObject; rootObject.b = 2; @@ -152,46 +151,24 @@ describe('Utils', function () { rootObject.e = true; rootObject.f = [rootObject.a, 'carrot', rootObject]; rootObject.f.push(rootObject.f); - proclaim.deepStrictEqual(findCircularPathsIn(rootObject), [ - '.a', - '.f[0]', - '.f[2]', - '.f[3]' - ]); - }); - }); - - describe('containsCircularPaths', function() { - it('should return false if object contains no circular references', function() { - proclaim.isFalse(containsCircularPaths({ a: 1, b: 2 })); - }); - - it('should return false if given a string literal', function() { - proclaim.isFalse(containsCircularPaths('')); - }); - - it('should return false if given a number literal', function() { - proclaim.isFalse(containsCircularPaths(137)); - }); - - it('should return false if given a boolean literal', function() { - proclaim.isFalse(containsCircularPaths(true)); - }); - - it('should return false if given null', function() { - proclaim.isFalse(containsCircularPaths(null)); - }); - it('should return true if given an object which contains circular references', function() { - const rootObject = {}; - rootObject.a = rootObject; - rootObject.b = 2; - rootObject.c = ''; - rootObject.d = null; - rootObject.e = true; - rootObject.f = [rootObject.a, 'carrot', rootObject]; - rootObject.f.push(rootObject.f); - proclaim.isTrue(containsCircularPaths(rootObject)); + try { + safelyStringifyJson(rootObject); + proclaim.notOk("Expected function to throw an error but it did not"); + + } catch (error) { + proclaim.isInstanceOf(error, Error); + proclaim.equal(error.message, +`AssertionError: o-tracking does not support circular references in the analytics data. +Please remove the circular references in the data. +Here are the paths in the data which are circular: +[ + ".a", + ".f[0]", + ".f[2]", + ".f[3]" +]`); + } }); - }); + }) });