From 1d2c705e139a81ade46171e0612273ff911f9d8d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 6 Apr 2016 15:56:28 +0100 Subject: [PATCH 1/2] Fix a bug where we recreated sync filters Fix the object comparison used for client filters (JSON.stringify is non-deterministic) --- lib/sync.js | 11 +++--- lib/utils.js | 86 +++++++++++++++++++++++++++++++++++++++++ spec/unit/utils.spec.js | 68 ++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 5 deletions(-) diff --git a/lib/sync.js b/lib/sync.js index 7416e8408..edc0ec78d 100644 --- a/lib/sync.js +++ b/lib/sync.js @@ -821,16 +821,17 @@ SyncApi.prototype._getOrCreateFilter = function(filterName, filter) { promise = client.getFilter(client.credentials.userId, filterId, true ).then(function(existingFilter) { - var oldStr = JSON.stringify(existingFilter.getDefinition()); - var newStr = JSON.stringify(filter.getDefinition()); + var oldDef = existingFilter.getDefinition(); + var newDef = filter.getDefinition(); - if (oldStr == newStr) { + if (utils.deepCompare(oldDef, newDef)) { // super, just use that. - debuglog("Using existing filter ID %s: %s", filterId, oldStr); + debuglog("Using existing filter ID %s: %s", filterId, + JSON.stringify(oldDef)); return q(filterId); } debuglog("Existing filter ID %s: %s; new filter: %s", - filterId, oldStr, newStr); + filterId, JSON.stringify(oldDef), JSON.stringify(newDef)); return; }); } diff --git a/lib/utils.js b/lib/utils.js index 397c702f8..7b60020ec 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -246,6 +246,92 @@ module.exports.deepCopy = function(obj) { return JSON.parse(JSON.stringify(obj)); }; +/** + * Compare two objects for equality. The objects MUST NOT have circular references. + * + * @param {Object} x The first object to compare. + * @param {Object} y The second object to compare. + * + * @return {boolean} true if the two objects are equal + */ +var deepCompare = module.exports.deepCompare = function(x, y) { + // Inspired by + // http://stackoverflow.com/questions/1068834/object-comparison-in-javascript#1144249 + + // Compare primitives and functions. + // Also check if both arguments link to the same object. + if (x === y) { + return true; + } + + if (typeof x !== typeof y) { + return false; + } + + // special-case NaN (since NaN !== NaN) + if (typeof x === 'number' && isNaN(x) && isNaN(y)) { + return true; + } + + // special-case null (since typeof null == 'object', but null.constructor + // throws) + if (x === null || y === null) { + return x === y; + } + + // everything else is either an unequal primitive, or an object + if (!(x instanceof Object)) { + return false; + } + + // check they are the same type of object + if (x.constructor !== y.constructor || x.prototype !== y.prototype) { + return false; + } + + // special-casing for some special types of object + if (x instanceof RegExp || x instanceof Date) { + return x.toString() === y.toString(); + } + + // the object algorithm works for Array, but it's sub-optimal. + if (x instanceof Array) { + if (x.length !== y.length) { + return false; + } + + for (var i = 0; i < x.length; i++) { + if (deepCompare(x[i], y[i])) { + return false; + } + } + } else { + // disable jshint "The body of a for in should be wrapped in an if + // statement" + /* jshint -W089 */ + + // check that all of y's direct keys are in x + var p; + for (p in y) { + if (y.hasOwnProperty(p) !== x.hasOwnProperty(p)) { + return false; + } + } + + // finally, compare each of x's keys with y + for (p in y) { + if (y.hasOwnProperty(p) !== x.hasOwnProperty(p)) { + return false; + } + if (!deepCompare(x[p], y[p])) { + return false; + } + } + } + /* jshint +W089 */ + return true; +}; + /** * Run polyfills to add Array.map and Array.filter if they are missing. diff --git a/spec/unit/utils.spec.js b/spec/unit/utils.spec.js index d8be6ceb1..49ac2693b 100644 --- a/spec/unit/utils.spec.js +++ b/spec/unit/utils.spec.js @@ -132,4 +132,72 @@ describe("utils", function() { }, ["foo"]); }).not.toThrow(); }); }); + + describe("deepCompare", function() { + var assert = { + isTrue: function(x) { expect(x).toBe(true); }, + isFalse: function(x) { expect(x).toBe(false); }, + }; + + it("should handle primitives", function() { + assert.isTrue(utils.deepCompare(null, null)); + assert.isFalse(utils.deepCompare(null, undefined)); + assert.isTrue(utils.deepCompare("hi", "hi")); + assert.isTrue(utils.deepCompare(5, 5)); + assert.isFalse(utils.deepCompare(5, 10)); + }); + + it("should handle regexps", function() { + assert.isTrue(utils.deepCompare(/abc/, /abc/)); + assert.isFalse(utils.deepCompare(/abc/, /123/)); + var r = /abc/; + assert.isTrue(utils.deepCompare(r, r)); + }); + + it("should handle dates", function() { + assert.isTrue(utils.deepCompare(new Date("2011-03-31"), + new Date("2011-03-31"))); + assert.isFalse(utils.deepCompare(new Date("2011-03-31"), + new Date("1970-01-01"))); + }); + + it("should handle arrays", function() { + assert.isTrue(utils.deepCompare([], [])); + assert.isTrue(utils.deepCompare([1, 2], [1, 2])); + assert.isFalse(utils.deepCompare([1, 2], [2, 1])); + assert.isFalse(utils.deepCompare([1, 2], [1, 2, 3])); + }); + + it("should handle simple objects", function() { + assert.isTrue(utils.deepCompare({}, {})); + assert.isTrue(utils.deepCompare({a: 1, b: 2}, {a: 1, b: 2})); + assert.isTrue(utils.deepCompare({a: 1, b: 2}, {b: 2, a: 1})); + assert.isFalse(utils.deepCompare({a: 1, b: 2}, {a: 1, b: 3})); + + assert.isTrue(utils.deepCompare({1: {name: "mhc", age: 28}, + 2: {name: "arb", age: 26}}, + {1: {name: "mhc", age: 28}, + 2: {name: "arb", age: 26}})); + + assert.isFalse(utils.deepCompare({1: {name: "mhc", age: 28}, + 2: {name: "arb", age: 26}}, + {1: {name: "mhc", age: 28}, + 2: {name: "arb", age: 27}})); + + assert.isFalse(utils.deepCompare({}, null)); + assert.isFalse(utils.deepCompare({}, undefined)); + }); + + it("should handle functions", function() { + // no two different function is equal really, they capture their + // context variables so even if they have same toString(), they + // won't have same functionality + var func = function(x) { return true; }; + var func2 = function(x) { return true; }; + assert.isTrue(utils.deepCompare(func, func)); + assert.isFalse(utils.deepCompare(func, func2)); + assert.isTrue(utils.deepCompare({ a: { b: func } }, { a: { b: func } })); + assert.isFalse(utils.deepCompare({ a: { b: func } }, { a: { b: func2 } })); + }); + }); }); From 384c4748002c7f5c5ad83a2cb87e174640b3dfbe Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 6 Apr 2016 18:14:34 +0100 Subject: [PATCH 2/2] fix failure of deepCompare to compare arrays what a difference a character can make --- lib/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/utils.js b/lib/utils.js index 7b60020ec..d593de98a 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -301,7 +301,7 @@ var deepCompare = module.exports.deepCompare = function(x, y) { } for (var i = 0; i < x.length; i++) { - if (deepCompare(x[i], y[i])) { + if (!deepCompare(x[i], y[i])) { return false; } }