From 4600318ad70b3ba699ac76f1a4f533e7d11de3dd Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 22 Jul 2015 05:21:32 -0400 Subject: [PATCH] Support 'successful' property in upload response Save uploaded data to cache, and update local object if necessary (which it mostly shouldn't be except for invalid characters and HTML filtering in notes) Also add some upload and JSON tests --- .../content/zotero/xpcom/data/collection.js | 35 ++- .../content/zotero/xpcom/data/dataObject.js | 41 +++ .../zotero/xpcom/data/dataObjectUtilities.js | 71 +++-- .../content/zotero/xpcom/data/dataObjects.js | 3 +- chrome/content/zotero/xpcom/data/item.js | 62 +--- chrome/content/zotero/xpcom/search.js | 15 +- .../zotero/xpcom/sync/syncAPIClient.js | 2 - .../content/zotero/xpcom/sync/syncEngine.js | 50 +++- chrome/content/zotero/xpcom/sync/syncLocal.js | 27 +- test/content/support.js | 44 ++- test/tests/itemTest.js | 138 +++++---- test/tests/syncEngineTest.js | 277 ++++++++++++++++-- 12 files changed, 578 insertions(+), 187 deletions(-) diff --git a/chrome/content/zotero/xpcom/data/collection.js b/chrome/content/zotero/xpcom/data/collection.js index 719aba9900..8c4e11718b 100644 --- a/chrome/content/zotero/xpcom/data/collection.js +++ b/chrome/content/zotero/xpcom/data/collection.js @@ -678,14 +678,43 @@ Zotero.Collection.prototype.fromJSON = Zotero.Promise.coroutine(function* (json) }); -Zotero.Collection.prototype.toJSON = Zotero.Promise.coroutine(function* (options) { - var obj = {}; +Zotero.Collection.prototype.toResponseJSON = Zotero.Promise.coroutine(function* (options = {}) { + var json = yield this.constructor._super.prototype.toResponseJSON.apply(this, options); + + // TODO: library block? + + // creatorSummary + var firstCreator = this.getField('firstCreator'); + if (firstCreator) { + json.meta.creatorSummary = firstCreator; + } + // parsedDate + var parsedDate = Zotero.Date.multipartToSQL(this.getField('date', true, true)); + if (parsedDate) { + // 0000? + json.meta.parsedDate = parsedDate; + } + // numChildren + if (this.isRegularItem()) { + json.meta.numChildren = this.numChildren(); + } + return json; +}) + + +Zotero.Collection.prototype.toJSON = Zotero.Promise.coroutine(function* (options = {}) { + var env = this._preToJSON(options); + var mode = env.mode; + + var obj = env.obj = {}; obj.key = this.key; obj.version = this.version; + obj.name = this.name; obj.parentCollection = this.parentKey ? this.parentKey : false; obj.relations = {}; // TEMP - return obj; + + return this._postToJSON(env); }); diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index b5d3c97d3d..e186ec1de7 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -1190,6 +1190,47 @@ Zotero.DataObject.prototype._finalizeErase = function (env) { } } + +Zotero.DataObject.prototype.toResponseJSON = Zotero.Promise.coroutine(function* (options) { + // TODO: library block? + + return { + key: this.key, + version: this.version, + meta: {}, + data: yield this.toJSON(options) + }; +}); + + +Zotero.DataObject.prototype._preToJSON = function (options) { + var env = { options }; + env.mode = options.mode || 'new'; + if (env.mode == 'patch') { + if (!options.patchBase) { + throw new Error("Cannot use patch mode if patchBase not provided"); + } + } + else if (options.patchBase) { + if (options.mode) { + Zotero.debug("Zotero.Item.toJSON: ignoring provided patchBase in " + env.mode + " mode", 2); + } + // If patchBase provided and no explicit mode, use 'patch' + else { + env.mode = 'patch'; + } + } + return env; +} + +Zotero.DataObject.prototype._postToJSON = function (env) { + if (env.mode == 'patch') { + env.obj = Zotero.DataObjectUtilities.patch(env.options.patchBase, env.obj); + } + return env.obj; +} + + /** * Generates data object key * @return {String} key diff --git a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js index 15af32b1d7..51a88426c9 100644 --- a/chrome/content/zotero/xpcom/data/dataObjectUtilities.js +++ b/chrome/content/zotero/xpcom/data/dataObjectUtilities.js @@ -101,6 +101,42 @@ Zotero.DataObjectUtilities = { return Zotero[className] }, + + patch: function (base, obj) { + var target = {}; + Object.assign(target, obj); + + for (let i in base) { + switch (i) { + case 'key': + case 'version': + case 'dateModified': + continue; + } + + // If field from base exists in the new version, delete it if it's the same + if (i in target) { + if (!this._fieldChanged(i, base[i], target[i])) { + delete target[i]; + } + } + // If field from base doesn't exist in new version, clear it + else { + switch (i) { + case 'deleted': + target[i] = false; + break; + + default: + target[i] = ''; + } + } + } + + return target; + }, + + /** * Determine whether two API JSON objects are equivalent * @@ -129,24 +165,9 @@ Zotero.DataObjectUtilities = { continue; } - let changed; - - switch (field) { - case 'creators': - case 'collections': - case 'tags': - case 'relations': - changed = this["_" + field + "Changed"](val1, val2); - if (changed) { - return true; - } - break; - - default: - changed = val1 !== val2; - if (changed) { - return true; - } + let changed = this._fieldChanged(field, val1, val2); + if (changed) { + return true; } skipFields[field] = true; @@ -170,6 +191,20 @@ Zotero.DataObjectUtilities = { return false; }, + _fieldChanged: function (fieldName, field1, field2) { + switch (fieldName) { + case 'collections': + case 'conditions': + case 'creators': + case 'tags': + case 'relations': + return this["_" + fieldName + "Changed"](field1, field2); + + default: + return field1 !== field2; + } + }, + _creatorsChanged: function (data1, data2) { if (!data2 || data1.length != data2.length) return true; for (let i = 0; i < data1.length; i++) { diff --git a/chrome/content/zotero/xpcom/data/dataObjects.js b/chrome/content/zotero/xpcom/data/dataObjects.js index c6f423b222..52d196d554 100644 --- a/chrome/content/zotero/xpcom/data/dataObjects.js +++ b/chrome/content/zotero/xpcom/data/dataObjects.js @@ -534,7 +534,6 @@ Zotero.DataObjects.prototype._load = Zotero.Promise.coroutine(function* (library return loaded; } - // getPrimaryDataSQL() should use "O" for the primary table alias var sql = this.primaryDataSQL; var params = []; if (libraryID !== false) { @@ -551,7 +550,7 @@ Zotero.DataObjects.prototype._load = Zotero.Promise.coroutine(function* (library params, { onRow: function (row) { - var id = row.getResultByIndex(this._ZDO_id); + var id = row.getResultByName(this._ZDO_id); var columns = Object.keys(this._primaryDataSQLParts); var rowObj = {}; for (let i=0; i { + if (o.key === undefined) { + throw new Error("Missing 'key' property in JSON"); + } + if (o.version === undefined) { + throw new Error("Missing 'version' property in JSON"); + } + // If direct data object passed, wrap in fake response object + return o.data === undefined ? { + key: o.key, + version: o.version, + data: o + } : o; + }); + Zotero.debug("Saving to sync cache:"); Zotero.debug(jsonArray); @@ -174,12 +195,6 @@ Zotero.Sync.Data.Local = { var params = []; for (let i = 0; i < chunk.length; i++) { let o = chunk[i]; - if (o.key === undefined) { - throw new Error("Missing 'key' property in JSON"); - } - if (o.version === undefined) { - throw new Error("Missing 'version' property in JSON"); - } params.push(libraryID, o.key, syncObjectTypeID, o.version, JSON.stringify(o)); } return Zotero.DB.queryAsync( diff --git a/test/content/support.js b/test/content/support.js index 4401b55acf..9f1955dc24 100644 --- a/test/content/support.js +++ b/test/content/support.js @@ -260,12 +260,24 @@ var createGroup = Zotero.Promise.coroutine(function* (props) { // // Data objects // -function createUnsavedDataObject(objectType, params) { +/** + * @param {String} objectType - 'collection', 'item', 'search' + * @param {Object} [params] + * @param {Integer} [params.libraryID] + * @param {String} [params.itemType] - Item type + * @param {String} [params.title] - Item title + * @param {Boolean} [params.setTitle] - Assign a random item title + * @param {String} [params.name] - Collection/search name + * @param {Integer} [params.parentID] + * @param {String} [params.parentKey] + * @param {Boolean} [params.synced] + * @param {Integer} [params.version] + */ +function createUnsavedDataObject(objectType, params = {}) { if (!objectType) { throw new Error("Object type not provided"); } - params = params || {}; if (objectType == 'item') { var param = params.itemType || 'book'; } @@ -275,14 +287,14 @@ function createUnsavedDataObject(objectType, params) { } switch (objectType) { case 'item': - if (params.title) { - obj.setField('title', params.title); + if (params.title !== undefined || params.setTitle) { + obj.setField('title', params.title !== undefined ? params.title : Zotero.Utilities.randomString()); } break; case 'collection': case 'search': - obj.name = params.name !== undefined ? params.name : "Test"; + obj.name = params.name !== undefined ? params.name : Zotero.Utilities.randomString(); break; } var allowedParams = ['parentID', 'parentKey', 'synced', 'version']; @@ -294,12 +306,32 @@ function createUnsavedDataObject(objectType, params) { return obj; } -var createDataObject = Zotero.Promise.coroutine(function* (objectType, params, saveOptions) { +var createDataObject = Zotero.Promise.coroutine(function* (objectType, params = {}, saveOptions) { var obj = createUnsavedDataObject(objectType, params); yield obj.saveTx(saveOptions); return obj; }); +function getNameProperty(objectType) { + return objectType == 'item' ? 'title' : 'name'; +} + +var modifyDataObject = Zotero.Promise.coroutine(function* (obj, params = {}) { + switch (obj.objectType) { + case 'item': + yield obj.loadItemData(); + obj.setField( + 'title', + params.title !== undefined ? params.title : Zotero.Utilities.randomString() + ); + break; + + default: + obj.name = params.name !== undefined ? params.name : Zotero.Utilities.randomString(); + } + return obj.saveTx(); +}); + /** * Return a promise for the error thrown by a promise, or false if none */ diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index ec55a79779..4cd180d77e 100644 --- a/test/tests/itemTest.js +++ b/test/tests/itemTest.js @@ -709,56 +709,96 @@ describe("Zotero.Item", function () { }) describe("#toJSON()", function () { - it("should output only fields with values in default mode", function* () { - var itemType = "book"; - var title = "Test"; - - var item = new Zotero.Item(itemType); - item.setField("title", title); - var id = yield item.saveTx(); - item = yield Zotero.Items.getAsync(id); - var json = yield item.toJSON(); - - assert.equal(json.itemType, itemType); - assert.equal(json.title, title); - assert.isUndefined(json.date); - assert.isUndefined(json.numPages); - }) - - it("should output all fields in 'full' mode", function* () { - var itemType = "book"; - var title = "Test"; - - var item = new Zotero.Item(itemType); - item.setField("title", title); - var id = yield item.saveTx(); - item = yield Zotero.Items.getAsync(id); - var json = yield item.toJSON({ mode: 'full' }); - assert.equal(json.title, title); - assert.equal(json.date, ""); - assert.equal(json.numPages, ""); - }) - - it("should output only fields that differ in 'patch' mode", function* () { - var itemType = "book"; - var title = "Test"; - var date = "2015-05-12"; - - var item = new Zotero.Item(itemType); - item.setField("title", title); - var id = yield item.saveTx(); - item = yield Zotero.Items.getAsync(id); - var patchBase = yield item.toJSON(); - - item.setField("date", date); - yield item.saveTx(); - var json = yield item.toJSON({ - patchBase: patchBase + describe("default mode", function () { + it("should output only fields with values", function* () { + var itemType = "book"; + var title = "Test"; + + var item = new Zotero.Item(itemType); + item.setField("title", title); + var id = yield item.saveTx(); + item = yield Zotero.Items.getAsync(id); + var json = yield item.toJSON(); + + assert.equal(json.itemType, itemType); + assert.equal(json.title, title); + assert.isUndefined(json.date); + assert.isUndefined(json.numPages); + }) + }) + + describe("'full' mode", function () { + it("should output all fields", function* () { + var itemType = "book"; + var title = "Test"; + + var item = new Zotero.Item(itemType); + item.setField("title", title); + var id = yield item.saveTx(); + item = yield Zotero.Items.getAsync(id); + var json = yield item.toJSON({ mode: 'full' }); + assert.equal(json.title, title); + assert.equal(json.date, ""); + assert.equal(json.numPages, ""); + }) + }) + + describe("'patch' mode", function () { + it("should output only fields that differ", function* () { + var itemType = "book"; + var title = "Test"; + var date = "2015-05-12"; + + var item = new Zotero.Item(itemType); + item.setField("title", title); + var id = yield item.saveTx(); + item = yield Zotero.Items.getAsync(id); + var patchBase = yield item.toJSON(); + + item.setField("date", date); + yield item.saveTx(); + var json = yield item.toJSON({ + patchBase: patchBase + }) + assert.isUndefined(json.itemType); + assert.isUndefined(json.title); + assert.equal(json.date, date); + assert.isUndefined(json.numPages); + assert.isUndefined(json.deleted); + assert.isUndefined(json.creators); + assert.isUndefined(json.relations); + assert.isUndefined(json.tags); + }) + + it("should include changed 'deleted' field", function* () { + // True to false + var item = new Zotero.Item('book'); + item.deleted = true; + var id = yield item.saveTx(); + item = yield Zotero.Items.getAsync(id); + var patchBase = yield item.toJSON(); + + item.deleted = false; + var json = yield item.toJSON({ + patchBase: patchBase + }) + assert.isUndefined(json.title); + assert.isFalse(json.deleted); + + // False to true + var item = new Zotero.Item('book'); + item.deleted = false; + var id = yield item.saveTx(); + item = yield Zotero.Items.getAsync(id); + var patchBase = yield item.toJSON(); + + item.deleted = true; + var json = yield item.toJSON({ + patchBase: patchBase + }) + assert.isUndefined(json.title); + assert.isTrue(json.deleted); }) - assert.isUndefined(json.itemType); - assert.isUndefined(json.title); - assert.equal(json.date, date); - assert.isUndefined(json.numPages); }) }) diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 1a954f5664..6e5a04d352 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -122,39 +122,9 @@ describe("Zotero.Sync.Data.Engine", function () { }) describe("Syncing", function () { - it("should perform a sync for a new library", function* () { + it("should download items into a new library", function* () { ({ engine, client, caller } = yield setup()); - server.respond(function (req) { - if (req.method == "POST" && req.url == baseURL + "users/1/items") { - let ifUnmodifiedSince = req.requestHeaders["If-Unmodified-Since-Version"]; - if (ifUnmodifiedSince == 0) { - req.respond(412, {}, "Library has been modified since specified version"); - return; - } - - if (ifUnmodifiedSince == 3) { - let json = JSON.parse(req.requestBody); - req.respond( - 200, - { - "Content-Type": "application/json", - "Last-Modified-Version": 3 - }, - JSON.stringify({ - success: { - "0": json[0].key, - "1": json[1].key - }, - unchanged: {}, - failed: {} - }) - ); - return; - } - } - }) - var headers = { "Last-Modified-Version": 3 }; @@ -280,6 +250,251 @@ describe("Zotero.Sync.Data.Engine", function () { assert.isTrue(obj.synced); }) + it("should upload new full items and subsequent patches", function* () { + ({ engine, client, caller } = yield setup()); + + var libraryID = Zotero.Libraries.userLibraryID; + var lastLibraryVersion = 5; + yield Zotero.Libraries.setVersion(libraryID, lastLibraryVersion); + + var types = Zotero.DataObjectUtilities.getTypes(); + var objects = {}; + var objectResponseJSON = {}; + var objectVersions = {}; + for (let type of types) { + objects[type] = [yield createDataObject(type, { setTitle: true })]; + objectVersions[type] = {}; + objectResponseJSON[type] = yield Zotero.Promise.all(objects[type].map(o => o.toResponseJSON())); + } + + server.respond(function (req) { + if (req.method == "POST") { + assert.equal( + req.requestHeaders["If-Unmodified-Since-Version"], lastLibraryVersion + ); + + for (let type of types) { + let typePlural = Zotero.DataObjectUtilities.getObjectTypePlural(type); + if (req.url == baseURL + "users/1/" + typePlural) { + let json = JSON.parse(req.requestBody); + assert.lengthOf(json, 1); + assert.equal(json[0].key, objects[type][0].key); + assert.equal(json[0].version, 0); + if (type == 'item') { + assert.equal(json[0].title, objects[type][0].getField('title')); + } + else { + assert.equal(json[0].name, objects[type][0].name); + } + let objectJSON = objectResponseJSON[type][0]; + objectJSON.version = ++lastLibraryVersion; + objectJSON.data.version = lastLibraryVersion; + req.respond( + 200, + { + "Content-Type": "application/json", + "Last-Modified-Version": lastLibraryVersion + }, + JSON.stringify({ + successful: { + "0": objectJSON + }, + unchanged: {}, + failed: {} + }) + ); + objectVersions[type][objects[type][0].key] = lastLibraryVersion; + return; + } + } + } + }) + + yield engine.start(); + + assert.equal(Zotero.Libraries.getVersion(libraryID), lastLibraryVersion); + for (let type of types) { + // Make sure objects were set to the correct version and marked as synced + assert.lengthOf((yield Zotero.Sync.Data.Local.getUnsynced(libraryID, type)), 0); + let key = objects[type][0].key; + let version = objects[type][0].version; + assert.equal(version, objectVersions[type][key]); + // Make sure uploaded objects were added to cache + let cached = yield Zotero.Sync.Data.Local.getCacheObject(type, libraryID, key, version); + assert.typeOf(cached, 'object'); + assert.equal(cached.key, key); + assert.equal(cached.version, version); + + yield modifyDataObject(objects[type][0]); + } + + ({ engine, client, caller } = yield setup()); + + server.respond(function (req) { + if (req.method == "POST") { + assert.equal( + req.requestHeaders["If-Unmodified-Since-Version"], lastLibraryVersion + ); + + for (let type of types) { + let typePlural = Zotero.DataObjectUtilities.getObjectTypePlural(type); + if (req.url == baseURL + "users/1/" + typePlural) { + let json = JSON.parse(req.requestBody); + assert.lengthOf(json, 1); + let j = json[0]; + let o = objects[type][0]; + assert.equal(j.key, o.key); + assert.equal(j.version, objectVersions[type][o.key]); + if (type == 'item') { + assert.equal(j.title, o.getField('title')); + } + else { + assert.equal(j.name, o.name); + } + + // Verify PATCH semantics instead of POST (i.e., only changed fields) + let changedFieldsExpected = ['key', 'version']; + if (type == 'item') { + changedFieldsExpected.push('title', 'dateModified'); + } + else { + changedFieldsExpected.push('name'); + } + let changedFields = Object.keys(j); + assert.lengthOf( + changedFields, changedFieldsExpected.length, "same " + type + " length" + ); + assert.sameMembers( + changedFields, changedFieldsExpected, "same " + type + " members" + ); + let objectJSON = objectResponseJSON[type][0]; + objectJSON.version = ++lastLibraryVersion; + objectJSON.data.version = lastLibraryVersion; + req.respond( + 200, + { + "Content-Type": "application/json", + "Last-Modified-Version": lastLibraryVersion + }, + JSON.stringify({ + successful: { + "0": objectJSON + }, + unchanged: {}, + failed: {} + }) + ); + objectVersions[type][o.key] = lastLibraryVersion; + return; + } + } + } + }) + + yield engine.start(); + + assert.equal(Zotero.Libraries.getVersion(libraryID), lastLibraryVersion); + for (let type of types) { + // Make sure objects were set to the correct version and marked as synced + assert.lengthOf((yield Zotero.Sync.Data.Local.getUnsynced(libraryID, type)), 0); + let o = objects[type][0]; + let key = o.key; + let version = o.version; + assert.equal(version, objectVersions[type][key]); + // Make sure uploaded objects were added to cache + let cached = yield Zotero.Sync.Data.Local.getCacheObject(type, libraryID, key, version); + assert.typeOf(cached, 'object'); + assert.equal(cached.key, key); + assert.equal(cached.version, version); + + switch (type) { + case 'collection': + assert.isFalse(cached.data.parentCollection); + break; + + case 'item': + assert.equal(cached.data.dateAdded, Zotero.Date.sqlToISO8601(o.dateAdded)); + break; + + case 'search': + assert.typeOf(cached.data.conditions, 'object'); + break; + } + } + }) + + it("should update local objects with remotely saved version after uploading if necessary", function* () { + ({ engine, client, caller } = yield setup()); + + var libraryID = Zotero.Libraries.userLibraryID; + var lastLibraryVersion = 5; + yield Zotero.Libraries.setVersion(libraryID, lastLibraryVersion); + + var types = Zotero.DataObjectUtilities.getTypes(); + var objects = {}; + var objectResponseJSON = {}; + var objectNames = {}; + for (let type of types) { + objects[type] = [yield createDataObject(type, { setTitle: true })]; + objectNames[type] = {}; + objectResponseJSON[type] = yield Zotero.Promise.all(objects[type].map(o => o.toResponseJSON())); + } + + server.respond(function (req) { + if (req.method == "POST") { + assert.equal( + req.requestHeaders["If-Unmodified-Since-Version"], lastLibraryVersion + ); + + for (let type of types) { + let typePlural = Zotero.DataObjectUtilities.getObjectTypePlural(type); + if (req.url == baseURL + "users/1/" + typePlural) { + let key = objects[type][0].key; + let objectJSON = objectResponseJSON[type][0]; + objectJSON.version = ++lastLibraryVersion; + objectJSON.data.version = lastLibraryVersion; + let prop = type == 'item' ? 'title' : 'name'; + objectNames[type][key] = objectJSON.data[prop] = Zotero.Utilities.randomString(); + req.respond( + 200, + { + "Content-Type": "application/json", + "Last-Modified-Version": lastLibraryVersion + }, + JSON.stringify({ + successful: { + "0": objectJSON + }, + unchanged: {}, + failed: {} + }) + ); + return; + } + } + } + }) + + yield engine.start(); + + assert.equal(Zotero.Libraries.getVersion(libraryID), lastLibraryVersion); + for (let type of types) { + // Make sure local objects were updated with new metadata and marked as synced + assert.lengthOf((yield Zotero.Sync.Data.Local.getUnsynced(libraryID, type)), 0); + let o = objects[type][0]; + let key = o.key; + let version = o.version; + let name = objectNames[type][key]; + if (type == 'item') { + yield o.loadItemData(); + assert.equal(name, o.getField('title')); + } + else { + assert.equal(name, o.name); + } + } + }) + it("should make only one request if in sync", function* () { yield Zotero.Libraries.setVersion(Zotero.Libraries.userLibraryID, 5); ({ engine, client, caller } = yield setup());