From abaa4da5ab6559cc1b710698cf50d7188eff34f0 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Tue, 19 May 2015 01:05:05 -0400 Subject: [PATCH] Much better data object change detection Replace Z.DataObjects::diff() with Z.DataObjectUtilities.diff(). Instead of just returning two objects with the differing fields, the new diff() generates a changeset with operations to apply with applyChanges(), including at the array member level for collections and tags. This, combined with cached pristine copies of objects, will allow for vastly better conflict resolution, with automatic merging of non-conflicting changes. Creators currently don't show granular changes, and ordering might make it too tough to do so. Relations diffing isn't yet implemented. --- chrome/content/zotero/xpcom/data/creators.js | 10 + .../zotero/xpcom/data/dataObjectUtilities.js | 372 ++++++++++++++++++ .../content/zotero/xpcom/data/dataObjects.js | 143 ------- chrome/content/zotero/xpcom/data/tags.js | 11 + test/tests/dataObjectUtilitiesTest.js | 277 +++++++++++++ test/tests/dataObjectsTest.js | 110 ------ 6 files changed, 670 insertions(+), 253 deletions(-) create mode 100644 test/tests/dataObjectUtilitiesTest.js delete mode 100644 test/tests/dataObjectsTest.js diff --git a/chrome/content/zotero/xpcom/data/creators.js b/chrome/content/zotero/xpcom/data/creators.js index 1eb0a425c..7290f5214 100644 --- a/chrome/content/zotero/xpcom/data/creators.js +++ b/chrome/content/zotero/xpcom/data/creators.js @@ -140,6 +140,16 @@ Zotero.Creators = new function() { }); + this.equals = function (data1, data2) { + data1 = this.cleanData(data1); + data2 = this.cleanData(data2); + return data1.lastName === data2.lastName + && data1.firstName === data2.firstName + && data1.fieldMode === data2.fieldMode + && data1.creatorTypeID === data2.creatorTypeID; + }, + + this.cleanData = function (data) { // Validate data if (data.name === undefined && data.lastName === undefined) { diff --git a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js index bf0f034b2..04a63fce8 100644 --- a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js +++ b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js @@ -66,5 +66,377 @@ Zotero.DataObjectUtilities = { var objectTypePlural = this.getObjectTypePlural(objectType); var className = objectTypePlural[0].toUpperCase() + objectTypePlural.substr(1); return Zotero[className] + }, + + /** + * Determine whether two API JSON objects are equivalent + * + * @param {Object} data1 - API JSON of first object + * @param {Object} data2 - API JSON of second object + * @param {Array} [ignoreFields] - Fields to ignore + * @param {Boolean} - True if objects are the same, false if not + */ + equals: function (data1, data2, ignoreFields) { + var skipFields = {}; + for (let field of ['key', 'version'].concat(ignoreFields || [])) { + skipFields[field] = true; + } + + for (let field in data1) { + if (skipFields[field]) { + continue; + } + + let val1 = data1[field]; + let val2 = data2[field]; + let val1HasValue = val1 || val1 === 0; + let val2HasValue = val2 || val2 === 0; + + if (!val1HasValue && !val2HasValue) { + continue; + } + + let changed; + + switch (field) { + case 'creators': + case 'collections': + case 'tags': + case 'relations': + changed = this["_" + field + "Equals"](val1, val2); + if (changed) { + return true; + } + break; + + default: + changed = val1 !== val2; + if (changed) { + return true; + } + } + + skipFields[field] = true; + } + + for (let field in data2) { + // Skip ignored fields and fields we've already compared + if (skipFields[field]) { + continue; + } + + // All remaining fields don't exist in data1 + + if (data2[field] === false) { + continue; + } + + return true; + } + + return false; + }, + + _creatorsEquals: function (data1, data2) { + if (!data2 || data1.length != data2.length) return true; + for (let i = 0; i < data1.length; i++) { + if (!Zotero.Creators.equals(data1[i], data2[i])) { + return false; + } + } + return true; + }, + + _collectionsEquals: function (data1, data2) { + if (!data2 || data1.length != data2.length) return true; + let c1 = data1.concat(); + let c2 = data2.concat(); + c1.sort(); + c2.sort(); + return Zotero.Utilities.arrayEquals(c1, c2); + }, + + _tagsEquals: function (data1, data2) { + if (!data2 || data1.length != data2.length) return true; + for (let i = 0; i < data1.length; i++) { + if (!Zotero.Tags.equals(data1[i], data2[i])) { + return false; + } + } + return true; + }, + + _relationsEquals: function (data1, data2) { + if (!data2) return true; + var pred1 = Object.keys(data1); + pred1.sort(); + var pred2 = Object.keys(data2); + pred2.sort(); + if (!Zotero.Utilities.arrayEquals(pred1, pred2)) return false; + for (let i in pred1) { + if (!Zotero.Utilities.arrayEquals(pred1[i], pred2[i])) { + return false; + } + } + return true; + }, + + + /** + * Compare two API JSON objects and generate a changeset + * + * @param {Object} data1 + * @param {Object} data2 + * @param {String[]} [ignoreFields] - Fields to ignore + */ + diff: function (data1, data2, ignoreFields) { + var changeset = []; + + var skipFields = {}; + for (let field of ['key', 'version'].concat(ignoreFields || [])) { + skipFields[field] = true; + } + + for (let field in data1) { + if (skipFields[field]) { + continue; + } + + let val1 = data1[field]; + let val2 = data2[field]; + let val1HasValue = (val1 && val1 !== "") || val1 === 0; + let val2HasValue = (val2 && val2 !== "") || val2 === 0; + + if (!val1HasValue && !val2HasValue) { + continue; + } + + switch (field) { + case 'creators': + case 'collections': + case 'relations': + case 'tags': + let changes = this["_" + field + "Diff"](val1, val2); + if (changes.length) { + changeset = changeset.concat(changes); + } + break; + + default: + var changed = val1 !== val2; + if (changed) { + if (val1HasValue && !val2HasValue) { + changeset.push({ + field: field, + op: 'delete' + }); + } + else if (!val1HasValue && val2HasValue) { + changeset.push({ + field: field, + op: 'add', + value: val2 + }); + } + else { + changeset.push({ + field: field, + op: 'modify', + value: val2 + }); + } + } + } + + skipFields[field] = true; + } + + for (let field in data2) { + // Skip ignored fields and fields we've already compared + if (skipFields[field]) { + continue; + } + + // All remaining fields don't exist in data1 + + if (data2[field] === false || data2[field] === "") { + continue; + } + + changeset.push({ + field: field, + op: "add", + value: data2[field] + }); + } + + return changeset; + }, + + /** + * For creators, just determine if changed, since ordering makes a full diff too complicated + */ + _creatorsDiff: function (data1, data2) { + if (!data2 || !data2.length) { + if (!data1.length) { + return []; + } + return [{ + field: "creators", + op: "delete" + }]; + } + if (!this._creatorsEquals(data1, data2)) { + return [{ + field: "creators", + op: "modify", + value: data2 + }]; + } + return []; + }, + + _collectionsDiff: function (data1, data2) { + data2 = data2 || []; + var changeset = []; + var removed = Zotero.Utilities.arrayDiff(data1, data2); + for (let i = 0; i < removed.length; i++) { + changeset.push({ + field: "collections", + op: "member-remove", + value: removed[i] + }); + } + let added = Zotero.Utilities.arrayDiff(data2, data1); + for (let i = 0; i < added.length; i++) { + changeset.push({ + field: "collections", + op: "member-add", + value: added[i] + }); + } + return changeset; + }, + + _tagsDiff: function (data1, data2) { + data2 = data2 || []; + var changeset = []; + outer: + for (let i = 0; i < data1.length; i++) { + for (let j = 0; j < data2.length; j++) { + if (Zotero.Tags.equals(data1[i], data2[j])) { + continue outer; + } + } + changeset.push({ + field: "tags", + op: "member-remove", + value: data1[i] + }); + } + outer: + for (let i = 0; i < data2.length; i++) { + for (let j = 0; j < data1.length; j++) { + if (Zotero.Tags.equals(data2[i], data1[j])) { + continue outer; + } + } + changeset.push({ + field: "tags", + op: "member-add", + value: data2[i] + }); + } + return changeset; + }, + + _relationsDiff: function (data1, data2) { + if (!data1.length && !data2.length) { + return []; + } + throw new Error("Unimplemented"); + }, + + + /** + * Apply a set of changes generated by Zotero.DataObjectUtilities.diff() to an API JSON object + * + * @param {Object} json - API JSON object to modify + * @param {Object[]} changeset - Change instructions, as generated by .diff() + */ + applyChanges: function (json, changeset) { + for (let i = 0; i < changeset.length; i++) { + let c = changeset[i]; + if (c.op == 'delete') { + delete json[c.field]; + } + else if (c.op == 'add' || c.op == 'modify') { + json[c.field] = c.value; + } + else if (c.op == 'member-add') { + switch (c.field) { + case 'collections': + if (json[c.field].indexOf(c.value) == -1) { + json[c.field].push(c.value); + } + break; + + case 'creators': + throw new Error("Unimplemented"); + break; + + case 'relations': + throw new Error("Unimplemented"); + break; + + case 'tags': + let found = false; + for (let i = 0; i < json[c.field].length; i++) { + if (Zotero.Tags.equals(json[c.field][i], c.value)) { + found = true; + break; + } + } + if (!found) { + json[c.field].push(c.value); + } + break; + + default: + throw new Error("Unexpected field"); + } + } + else if (c.op == 'member-remove') { + switch (c.field) { + case 'collections': + let pos = json[c.field].indexOf(c.value); + if (pos == -1) { + continue; + } + json[c.field].splice(pos, 1); + break; + + case 'creators': + throw new Error("Unimplemented"); + break; + + case 'tags': + for (let i = 0; i < json[c.field].length; i++) { + if (Zotero.Tags.equals(json[c.field][i], c.value)) { + json[c.field].splice(i, 1); + break; + } + } + break; + + default: + throw new Error("Unexpected field"); + } + } + // TODO: properties + else { + throw new Error("Unimplemented"); + } + } } }; diff --git a/chrome/content/zotero/xpcom/data/dataObjects.js b/chrome/content/zotero/xpcom/data/dataObjects.js index ef57d4a12..29b98c53c 100644 --- a/chrome/content/zotero/xpcom/data/dataObjects.js +++ b/chrome/content/zotero/xpcom/data/dataObjects.js @@ -424,149 +424,6 @@ Zotero.DataObjects.prototype.unload = function () { } -/** - * @param {Object} data1 - API JSON of first object - * @param {Object} data2 - API JSON of second object - * @param {Array} [ignoreFields] - Fields to ignore - */ -Zotero.DataObjects.prototype.diff = function (data1, data2, ignoreFields) { - var diff = [{}, {}]; - var numDiffs = 0; - - var skipFields = {}; - for (let field of ['key', 'version'].concat(ignoreFields || [])) { - skipFields[field] = true; - } - - for (var field in data1) { - if (skipFields[field]) { - continue; - } - - if (!data1[field] && data1[field] !== 0 && !data2[field] && data2[field] !== 0) { - continue; - } - - switch (field) { - case 'creators': - case 'collections': - case 'tags': - case 'relations': - var changed = this["_diff" + field[0].toUpperCase() + field.substr(1)]( - data1[field], data2[field] - ); - break; - - default: - var changed = data1[field] !== data2[field]; - } - - if (changed) { - diff[0][field] = data1[field] !== false ? data1[field] : ''; - diff[1][field] = (data2[field] !== false && data2[field] !== undefined) - ? data2[field] : ''; - } - - if (changed) { - //Zotero.debug("Field " + field + " has changed"); - numDiffs++; - } - - skipFields[field] = true; - } - - for (var field in data2) { - // Skip ignored fields and fields we've already compared - if (skipFields[field]) { - continue; - } - - if (!data2[field] && data2[field] !== 0 && !data1[field] && data1[field] !== 0) { - continue; - } - - switch (field) { - case 'creators': - case 'collections': - case 'tags': - case 'relations': - var changed = this["_diff" + field[0].toUpperCase() + field.substr(1)]( - data1[field], data2[field] - ); - break; - - default: - var changed = data1[field] !== data2[field]; - } - - if (changed) { - diff[0][field] = (data1[field] !== false && data1[field] !== undefined) - ? data1[field] : ''; - diff[1][field] = data2[field] !== false ? data2[field] : ''; - } - - if (changed) { - //Zotero.debug("Field " + field + " has changed"); - numDiffs++; - } - } - - return numDiffs ? diff : false; -} - -Zotero.DataObjects.prototype._diffCreators = function (data1, data2) { - if (data1.length != data2.length) return false; - for (let i = 0; i < data1.length; i++) { - let c1 = Zotero.Creators.cleanData(data1[i]); - let c2 = Zotero.Creators.cleanData(data2[i]); - if (c1.lastName !== c2.lastName - || c1.firstName !== c2.firstName - || c1.fieldMode !== c2.fieldMode - || c1.creatorTypeID !== c2.creatorTypeID) { - return true; - } - } - return false; -} - -Zotero.DataObjects.prototype._diffCollections = function (data1, data2) { - // Child items shouldn't have collections properties, but just in case one does - if (!data2) return false; - if (data1.length != data2.length) return false; - let c1 = data1.concat(); - let c2 = data2.concat(); - c1.sort(); - c2.sort(); - return !Zotero.Utilities.arrayEquals(c1, c2); -} - -Zotero.DataObjects.prototype._diffTags = function (data1, data2) { - if (data1.length != data2.length) return false; - for (let i = 0; i < data1.length; i++) { - let t1 = data1[i]; - let t2 = data2[i]; - if (t1.tag !== t2.tag || (t1.type || 0) !== (t2.type || 0)) { - return false; - } - } - return false; -} - -Zotero.DataObjects.prototype._diffRelations = function (data1, data2) { - var pred1 = Object.keys(data1); - pred1.sort(); - var pred2 = Object.keys(data2); - pred2.sort(); - if (!Zotero.Utilities.arrayEquals(pred1, pred2)) return false; - for (let i in pred1) { - if (!Zotero.Utilities.arrayEquals(pred1[i], pred2[i])) { - return true; - } - } - return false; -} - - Zotero.DataObjects.prototype.isEditable = function (obj) { var libraryID = obj.libraryID; if (!libraryID) { diff --git a/chrome/content/zotero/xpcom/data/tags.js b/chrome/content/zotero/xpcom/data/tags.js index 3ba52e64a..3ceef1b7e 100644 --- a/chrome/content/zotero/xpcom/data/tags.js +++ b/chrome/content/zotero/xpcom/data/tags.js @@ -818,6 +818,17 @@ Zotero.Tags = new function() { } + /** + * Compare two API JSON tag objects + */ + this.equals = function (data1, data2) { + data1 = this.cleanData(data1); + data2 = this.cleanData(data2); + return data1.tag === data2.tag + && ((!data1.type && !data2.type) || data1.type === data2.type); + }, + + this.cleanData = function (data) { // Validate data if (data.tag === undefined) { diff --git a/test/tests/dataObjectUtilitiesTest.js b/test/tests/dataObjectUtilitiesTest.js new file mode 100644 index 000000000..61a1d9471 --- /dev/null +++ b/test/tests/dataObjectUtilitiesTest.js @@ -0,0 +1,277 @@ +"use strict"; + +describe("Zotero.DataObjectUtilities", function() { + describe("#diff()", function () { + it("should not show empty items as different", function* () { + var id1, id2, json1, json2; + yield Zotero.DB.executeTransaction(function* () { + var item = new Zotero.Item('book'); + id1 = yield item.save(); + item = yield Zotero.Items.getAsync(id1); + json1 = yield item.toJSON(); + + var item = new Zotero.Item('book'); + id2 = yield item.save(); + item = yield Zotero.Items.getAsync(id2); + json2 = yield item.toJSON(); + }); + + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.lengthOf(changes, 0); + + yield Zotero.Items.erase(id1, id2); + }) + + it("should not show empty strings as different", function* () { + var json1 = { + title: "" + }; + var json2 = { + title: "" + }; + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.lengthOf(changes, 0); + }) + + it("should not show empty string and undefined as different", function* () { + var json1 = { + title: "" + }; + var json2 = { + place: "" + }; + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.lengthOf(changes, 0); + }) + + it("should not show identical creators as different", function* () { + var json1 = { + creators: [ + { + name: "Center for History and New Media", + creatorType: "author" + } + ] + }; + var json2 = { + creators: [ + { + creatorType: "author", + name: "Center for History and New Media" + } + ] + }; + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.lengthOf(changes, 0); + }) + + it("should not show manual tags with or without 'type' property as different", function* () { + var json1 = { + tags: [ + { + tag: "Foo" + } + ] + }; + var json2 = { + tags: [ + { + tag: "Foo", + type: 0 + } + ] + }; + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.lengthOf(changes, 0); + }) + + it("should show tags of different types as different", function* () { + var json1 = { + tags: [ + { + tag: "Foo" + } + ] + }; + var json2 = { + tags: [ + { + tag: "Foo", + type: 1 + } + ] + }; + var changes = Zotero.DataObjectUtilities.diff(json1, json2); + assert.sameDeepMembers( + changes, + [ + { + field: "tags", + op: "member-remove", + value: { + tag: "Foo" + } + }, + { + field: "tags", + op: "member-add", + value: { + tag: "Foo", + type: 1 + } + } + ] + ); + }) + }) + + describe("#applyChanges()", function () { + it("should set added/modified field values", function* () { + var json = { + title: "A" + }; + var changes = [ + { + field: "title", + op: "add", + value: "B" + }, + { + field: "date", + op: "modify", + value: "2015-05-19" + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.equal(json.title, "B"); + assert.equal(json.date, "2015-05-19"); + }) + + it("should add a collection", function* () { + var json = { + collections: ["AAAAAAAA"] + }; + var changes = [ + { + field: "collections", + op: "member-add", + value: "BBBBBBBB" + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.sameMembers(json.collections, ["AAAAAAAA", "BBBBBBBB"]); + }) + + it("should not duplicate an existing collection", function* () { + var json = { + collections: ["AAAAAAAA"] + }; + var changes = [ + { + field: "collections", + op: "member-add", + value: "AAAAAAAA" + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.sameMembers(json.collections, ["AAAAAAAA"]); + assert.lengthOf(json.collections, 1); + }) + + it("should remove a collection", function* () { + var json = { + collections: ["AAAAAAAA"] + }; + var changes = [ + { + field: "collections", + op: "member-remove", + value: "AAAAAAAA" + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.lengthOf(json.collections, 0); + }) + + it("should add a tag", function* () { + var json = { + tags: [ + { + tag: "A" + } + ] + }; + var changes = [ + { + field: "tags", + op: "member-add", + value: { + tag: "B" + } + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.sameDeepMembers( + json.tags, + [ + { + tag: "A" + }, + { + tag: "B" + } + ] + ); + }) + + it("should not duplicate an existing tag", function* () { + var json = { + tags: [ + { + tag: "A" + } + ] + }; + var changes = [ + { + field: "tags", + op: "member-add", + value: { + tag: "A" + } + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.sameDeepMembers( + json.tags, + [ + { + tag: "A" + } + ] + ); + assert.lengthOf(json.tags, 1); + }) + + it("should remove a tag", function* () { + var json = { + tags: [ + { + tag: "A" + } + ] + }; + var changes = [ + { + field: "tags", + op: "member-remove", + value: { + tag: "A" + } + } + ]; + Zotero.DataObjectUtilities.applyChanges(json, changes); + assert.lengthOf(json.tags, 0); + }) + }) +}) diff --git a/test/tests/dataObjectsTest.js b/test/tests/dataObjectsTest.js deleted file mode 100644 index 8b4ece9e8..000000000 --- a/test/tests/dataObjectsTest.js +++ /dev/null @@ -1,110 +0,0 @@ -"use strict"; - -describe("Zotero.DataObjects", function() { - var types = ['collection', 'item', 'search']; - - describe("#diff()", function () { - it("should not show empty items as different", function* () { - var id1, id2, json1, json2; - yield Zotero.DB.executeTransaction(function* () { - var item = new Zotero.Item('book'); - id1 = yield item.save(); - item = yield Zotero.Items.getAsync(id1); - json1 = yield item.toJSON(); - - var item = new Zotero.Item('book'); - id2 = yield item.save(); - item = yield Zotero.Items.getAsync(id2); - json2 = yield item.toJSON(); - }); - - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - - yield Zotero.Items.erase(id1, id2); - }) - - it("should not show empty strings as different", function* () { - var json1 = { - title: "" - }; - var json2 = { - title: "" - }; - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - }) - - it("should not show empty string and undefined as different", function* () { - var json1 = { - title: "" - }; - var json2 = { - place: "" - }; - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - }) - - it("should not show identical creators as different", function* () { - var json1 = { - creators: [ - { - name: "Center for History and New Media", - creatorType: "author" - } - ] - }; - var json2 = { - creators: [ - { - creatorType: "author", - name: "Center for History and New Media" - } - ] - }; - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - }) - - it("should show tags of different types as different", function* () { - var json1 = { - tags: [ - { - tag: "Foo" - } - ] - }; - var json2 = { - tags: [ - { - tag: "Foo", - type: 1 - } - ] - }; - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - }) - - it("should not show manual tags as different without 'type' property", function* () { - var json1 = { - tags: [ - { - tag: "Foo" - } - ] - }; - var json2 = { - tags: [ - { - tag: "Foo", - type: 0 - } - ] - }; - var diff = Zotero.Items.diff(json1, json2); - assert.isFalse(diff); - }) - }) -})