From cd5e805b9e06652191134ec18db57b51f611b3eb Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 4 May 2016 01:05:09 -0400 Subject: [PATCH] Sync logic improvements - Cancel sync when cancelling conflict resolution window - Don't try to upload unsynced objects if present in sync queue --- .../content/zotero/xpcom/sync/syncEngine.js | 128 ++++++++++++------ test/tests/syncEngineTest.js | 121 ++++++++++++++++- 2 files changed, 200 insertions(+), 49 deletions(-) diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 90c87eb1f..d739747d9 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -71,6 +71,12 @@ Zotero.Sync.Data.Engine = function (options) { }); }; +Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CONTINUE = 1; +Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD = 2; +Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD = 3; +Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_RESTART = 4; +Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CANCEL = 5; + Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_SUCCESS = 1; Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_NOTHING_TO_UPLOAD = 2; Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_LIBRARY_CONFLICT = 3; @@ -113,7 +119,8 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* () sync: while (true) { let uploadResult = yield this._startUpload(); - Zotero.debug("UPLOAD RESULT WITH " + uploadResult); + let downloadResult; + Zotero.debug("Upload result is " + uploadResult, 4); switch (uploadResult) { // If upload succeeded, we're done @@ -132,16 +139,21 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* () yield this._fullSync(); break; - // If conflict, start at beginning with downloads case this.UPLOAD_RESULT_NOTHING_TO_UPLOAD: - let localChanges = yield this._startDownload(); - if (!localChanges) { + downloadResult = yield this._startDownload(); + Zotero.debug("Download result is " + downloadResult, 4); + if (downloadResult == this.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD) { + break; + } + break sync; + + // If conflict, start at beginning with downloads + case this.UPLOAD_RESULT_LIBRARY_CONFLICT: + downloadResult = yield this._startDownload(); + Zotero.debug("Download result is " + downloadResult, 4); + if (downloadResult == this.DOWNLOAD_RESULT_CANCEL) { break sync; } - break; - - case this.UPLOAD_RESULT_LIBRARY_CONFLICT: - yield this._startDownload(); if (!gen) { var gen = Zotero.Utilities.Internal.delayGenerator( @@ -183,7 +195,7 @@ Zotero.Sync.Data.Engine.prototype.stop = function () { /** * Download updated objects from API and save to local cache * - * @return {Boolean} True if an upload is needed, false otherwise + * @return {Promise} - A download result code (this.DOWNLOAD_RESULT_*) */ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(function* () { var localChanges = false; @@ -222,9 +234,12 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func top: true } ); - if (result == -1) { + if (result == this.DOWNLOAD_RESULT_RESTART) { continue loop; } + else if (result == this.DOWNLOAD_RESULT_CANCEL) { + return this.DOWNLOAD_RESULT_CANCEL; + } } let result = yield this._downloadUpdatedObjects( @@ -233,15 +248,18 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func lastLibraryVersion, gen ); - if (result == -1) { + if (result == this.DOWNLOAD_RESULT_RESTART) { continue loop; } + else if (result == this.DOWNLOAD_RESULT_CANCEL) { + return this.DOWNLOAD_RESULT_CANCEL; + } } // // Get deleted objects // - results = yield this.apiClient.getDeleted( + let results = yield this.apiClient.getDeleted( this.library.libraryType, this.libraryTypeID, libraryVersion @@ -327,31 +345,33 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func return 0; }); var mergeData = Zotero.Sync.Data.Local.showConflictResolutionWindow(conflicts); - if (mergeData) { - let concurrentObjects = 50; - yield Zotero.Utilities.Internal.forEachChunkAsync( - mergeData, - concurrentObjects, - function (chunk) { - return Zotero.DB.executeTransaction(function* () { - for (let json of chunk) { - if (!json.deleted) continue; - let obj = objectsClass.getByLibraryAndKey( - this.libraryID, json.key - ); - if (!obj) { - Zotero.logError("Remotely deleted " + objectType - + " didn't exist after conflict resolution"); - continue; - } - yield obj.erase({ - skipEditCheck: true - }); - } - }.bind(this)); - }.bind(this) - ); + if (!mergeData) { + Zotero.debug("Cancelling sync"); + return this.DOWNLOAD_RESULT_CANCEL; } + let concurrentObjects = 50; + yield Zotero.Utilities.Internal.forEachChunkAsync( + mergeData, + concurrentObjects, + function (chunk) { + return Zotero.DB.executeTransaction(function* () { + for (let json of chunk) { + if (!json.deleted) continue; + let obj = objectsClass.getByLibraryAndKey( + this.libraryID, json.key + ); + if (!obj) { + Zotero.logError("Remotely deleted " + objectType + + " didn't exist after conflict resolution"); + continue; + } + yield obj.erase({ + skipEditCheck: true + }); + } + }.bind(this)); + }.bind(this) + ); } if (toDelete.length) { @@ -377,7 +397,9 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func yield Zotero.Libraries.setVersion(this.libraryID, lastLibraryVersion); } - return localChanges; + return localChanges + ? this.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD + : this.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD; }); @@ -420,8 +442,9 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f /** - * @return {Boolean|Integer} - True if objects downloaded, false if none, or -1 to restart sync - * if library version has changed + * Get versions of objects updated remotely since the last sync time and kick off object downloading + * + * @return {Promise} - A download result code (this.DOWNLOAD_RESULT_*) */ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, libraryVersion, lastLibraryVersion, delayGenerator, options = {}) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); @@ -456,7 +479,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou if (!keepGoing) { throw new Error("Could not update " + this.library.name + " -- library in use"); } - return -1; + return this.DOWNLOAD_RESULT_RESTART; } @@ -511,14 +534,19 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou if (!keys.length) { Zotero.debug(`No ${objectTypePlural} to download`); - return false; + return this.DOWNLOAD_RESULT_CONTINUE; } - yield this._downloadObjects(objectType, keys); - return true; + return this._downloadObjects(objectType, keys); }); +/** + * Download data for specified objects from the API and run processing on them, and show the conflict + * resolution window if necessary + * + * @return {Promise} - A download result code (this.DOWNLOAD_RESULT_*) + */ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); @@ -653,8 +681,13 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu ); // Keys can be unprocessed if conflict resolution is cancelled let keys = results.filter(x => x.processed).map(x => x.key); + if (!keys.length) { + return this.DOWNLOAD_RESULT_CANCEL; + } yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, keys); } + + return this.DOWNLOAD_RESULT_CONTINUE; }); @@ -693,9 +726,18 @@ Zotero.Sync.Data.Engine.prototype._startUpload = Zotero.Promise.coroutine(functi // Get unsynced local objects for each object type for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(this.libraryID)) { let objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); + let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); // New/modified objects let ids = yield Zotero.Sync.Data.Local.getUnsynced(objectType, this.libraryID); + + // Skip objects in sync queue, because they might have unresolved conflicts. + // The queue only has keys, so we have to convert to keys and back. + let unsyncedKeys = ids.map(id => objectsClass.getLibraryAndKeyFromID(id).key); + let queueKeys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, this.libraryID); + unsyncedKeys = Zotero.Utilities.arrayDiff(unsyncedKeys, queueKeys); + ids = unsyncedKeys.map(key => objectsClass.getIDFromLibraryAndKey(this.libraryID, key)); + if (ids.length) { Zotero.debug(ids.length + " " + (ids.length == 1 ? objectType : objectTypePlural) diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index d8f96bb2b..cca9ddc3f 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -1543,8 +1543,9 @@ describe("Zotero.Sync.Data.Engine", function () { }) it("should handle cancellation of conflict resolution window", function* () { - var userLibraryID = Zotero.Libraries.userLibraryID; - yield Zotero.Libraries.setVersion(userLibraryID, 5); + var library = Zotero.Libraries.userLibrary; + library.libraryVersion = 5; + yield library.saveTx(); ({ engine, client, caller } = yield setup()); var item = yield createDataObject('item'); @@ -1630,17 +1631,124 @@ describe("Zotero.Sync.Data.Engine", function () { var wizard = doc.documentElement; wizard.getButton('cancel').click(); }) - yield engine._startDownload(); + var downloadResult = yield engine._startDownload(); + assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL); // Non-conflicted item should be saved - assert.ok(Zotero.Items.getIDFromLibraryAndKey(userLibraryID, "AAAAAAAA")); + assert.ok(Zotero.Items.getIDFromLibraryAndKey(library.id, "AAAAAAAA")); // Conflicted item should be skipped and in queue assert.isFalse(Zotero.Items.exists(itemID)); - var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', userLibraryID); + var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', library.id); assert.sameMembers(keys, [itemKey]); + + // Library version should not have advanced + assert.equal(library.libraryVersion, 5); }); - }) + + + /** + * The CR window for remote deletions is triggered separately, so test separately + */ + it("should handle cancellation of remote deletion conflict resolution window", function* () { + var library = Zotero.Libraries.userLibrary; + library.libraryVersion = 5; + yield library.saveTx(); + ({ engine, client, caller } = yield setup()); + + // Create local unsynced items + var item = createUnsavedDataObject('item'); + item.setField('title', 'A'); + item.synced = false; + var itemID1 = yield item.saveTx(); + var itemKey1 = item.key; + + item = createUnsavedDataObject('item'); + item.setField('title', 'B'); + item.synced = false; + var itemID2 = yield item.saveTx(); + var itemKey2 = item.key; + + var headers = { + "Last-Modified-Version": 6 + }; + setResponse({ + method: "GET", + url: "users/1/settings?since=5", + status: 200, + headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/collections?format=versions&since=5", + status: 200, + headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/searches?format=versions&since=5", + status: 200, + headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&since=5&includeTrashed=1", + status: 200, + headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/items?format=versions&since=5&includeTrashed=1", + status: 200, + headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/deleted?since=5", + status: 200, + headers, + json: { + settings: [], + collections: [], + searches: [], + items: [itemKey1, itemKey2] + } + }); + + waitForWindow('chrome://zotero/content/merge.xul', function (dialog) { + var doc = dialog.document; + var wizard = doc.documentElement; + wizard.getButton('cancel').click(); + }) + var downloadResult = yield engine._startDownload(); + assert.equal(downloadResult, engine.DOWNLOAD_RESULT_CANCEL); + + // Conflicted items should still exists + assert.isTrue(Zotero.Items.exists(itemID1)); + assert.isTrue(Zotero.Items.exists(itemID2)); + + // Library version should not have advanced + assert.equal(library.libraryVersion, 5); + }); + }); + + + describe("#_startUpload()", function () { + it("shouldn't upload unsynced objects if present in sync queue", function* () { + ({ engine, client, caller } = yield setup()); + var libraryID = Zotero.Libraries.userLibraryID; + var objectType = 'item'; + var obj = yield createDataObject(objectType); + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(objectType, libraryID, [obj.key]); + var result = yield engine._startUpload(); + assert.equal(result, engine.UPLOAD_RESULT_NOTHING_TO_UPLOAD); + }); + }); describe("Conflict Resolution", function () { @@ -1852,6 +1960,7 @@ describe("Zotero.Sync.Data.Engine", function () { assert.lengthOf(keys, 0); }) + // Note: Conflicts with remote deletions are handled in _startDownload() it("should handle local item deletion, keeping deletion", function* () { var libraryID = Zotero.Libraries.userLibraryID; ({ engine, client, caller } = yield setup());