Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Correctly detect truly circular paths in JS objects, rather than objects with multiple references #1862

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Correctly detect truly circular paths in JS objects, rather than objects with multiple references #1862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions libraries/o-tracking/src/javascript/core/send.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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')) {
Expand Down
11 changes: 2 additions & 9 deletions libraries/o-tracking/src/javascript/core/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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);
Expand Down
160 changes: 77 additions & 83 deletions libraries/o-tracking/src/javascript/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I think this particular line now only applies to the contained function ;)

* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: This function doesn't quite do this - should we change the name?

Right now it does three things:

  1. Try to JSON.stringify the object. If this succeeds, the object contains no circular references, and is returned.
  2. If the object failed to stringify, a safe-encoding routine is run, recording circular references.
  3. If circular references were found, an error is thrown.

So really the only reason for (2) is so that (3) can report the circular reference paths. And it also means this function is not safely stringifying the json - instead it's designed to error.

I think I'd be in favour of actually safely stringifying the data! I don't think we need to go as far as using the JSON schema's support, as it seems unlikely that we'd ever have a server try to reconstruct the original data. Instead I think we could replace it with a human-readable path reference, so the tracking data isn't lost; potentially also paired with a console.error - or perhaps instead an actual thrown error in a timeout, so that global error handlers can also catch/report?

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) {
Comment on lines +258 to +260
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: much like the old function, I think this isn't actually detecting only circular references, is it? It's detecting object values seen before, by putting every object as a key in a weakmap and then using references if those objects are encountered again.

Your use of json.stringify() in a try/catch is hiding this from the tests I think 😁

(That said, I think the new function is a significant improvement over the old one because it has much better skipping of built-in types including null. But I think if we're relying on a list of circular references in the error, it won't be...)

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 };
georgecrawford marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

_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.
Expand Down Expand Up @@ -403,7 +398,6 @@ export {
sanitise,
assignIfUndefined,
filterProperties,
findCircularPathsIn,
containsCircularPaths,
safelyStringifyJson,
isDeepEqual
};
43 changes: 31 additions & 12 deletions libraries/o-tracking/test/events/custom.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", {
Expand All @@ -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.');
});
});
Loading
Loading