Merge pull request #123 from matrix-org/rav/no_recreate_filter

Fix a bug where we recreated sync filters

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 oldDef = existingFilter.getDefinition();
-            var newStr = JSON.stringify(filter.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;
         });
     }

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.

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 } }));
+        });
+    });
 });