diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 1be4dd19b7..a9c2414dd2 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -436,6 +436,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou } keys.push(...queuedKeys); + keys = Zotero.Utilities.arrayUnique(keys); if (!keys.length) { Zotero.debug(`No ${objectTypePlural} to download`); @@ -455,6 +456,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); + var remainingKeys = [...keys]; var lastLength = keys.length; var objectData = {}; keys.forEach(key => objectData[key] = null); @@ -462,8 +464,6 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu while (true) { this._failedCheck(); - let lastError = false; - // Get data we've downloaded in a previous loop but failed to process var json = []; let keysToDownload = []; @@ -491,7 +491,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu "Downloading " + (keysToDownload.length == 1 ? "1 " + objectType - : Zotero.Utilities.numberFormat(keys.length, 0) + " " + objectTypePlural) + : Zotero.Utilities.numberFormat(remainingKeys.length, 0) + " " + objectTypePlural) + " in " + this.library.name ); @@ -510,7 +510,6 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu if (!Array.isArray(batch)) { this.failed = batch; - lastError = batch; return; } @@ -564,7 +563,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu conflictResults.push(x); } }); - keys = Zotero.Utilities.arrayDiff(keys, processedKeys); + remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, processedKeys); conflicts.push(...conflictResults); }.bind(this)), { @@ -572,12 +571,28 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu } ); - if (!keys.length || keys.length == lastLength) { + this._failedCheck(); + + // If all requests were successful, such that we had a chance to see all keys, remove keys we + // didn't see from the sync queue so they don't keep being retried forever + if (!this.failed) { + let missingKeys = keys.filter(key => objectData[key] === null); + if (missingKeys.length) { + Zotero.debug(`Removing ${missingKeys.length} missing ` + + Zotero.Utilities.pluralize(missingKeys.length, [objectType, objectTypePlural]) + + " from sync queue"); + yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys); + remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, missingKeys); + } + } + + if (!remainingKeys.length || remainingKeys.length == lastLength) { // Add failed objects to sync queue - let failedKeys = Object.keys(objectData).filter(key => objectData[key]) + let failedKeys = keys.filter(key => objectData[key]); if (failedKeys.length) { - let objDesc = `${failedKeys.length == 1 ? objectType : objectTypePlural}`; - Zotero.debug(`Queueing ${failedKeys.length} failed ${objDesc} for later`, 2); + Zotero.debug(`Queueing ${failedKeys.length} failed ` + + Zotero.Utilities.pluralize(failedKeys.length, [objectType, objectTypePlural]) + + " for later", 2); yield Zotero.Sync.Data.Local.addObjectsToSyncQueue( objectType, this.libraryID, failedKeys ); @@ -598,10 +613,10 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu break; } - lastLength = keys.length; + lastLength = remainingKeys.length; - var remainingObjectDesc = `${keys.length == 1 ? objectType : objectTypePlural}`; - Zotero.debug(`Retrying ${keys.length} remaining ${remainingObjectDesc}`); + Zotero.debug(`Retrying ${remainingKeys.length} remaining ` + + Zotero.Utilities.pluralize(remainingKeys, [objectType, objectTypePlural])); } // Show conflict resolution window diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 309dccec62..a8bbd905e1 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -1692,10 +1692,10 @@ Zotero.Sync.Data.Local = { getObjectsToTryFromSyncQueue: Zotero.Promise.coroutine(function* (objectType, libraryID) { + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); var rows = yield Zotero.DB.queryAsync( - "SELECT key, lastCheck, tries FROM syncQueue WHERE libraryID=? AND " - + "syncObjectTypeID IN (SELECT syncObjectTypeID FROM syncObjectTypes WHERE name=?)", - [libraryID, objectType] + "SELECT key, lastCheck, tries FROM syncQueue WHERE libraryID=? AND syncObjectTypeID=?", + [libraryID, syncObjectTypeID] ); var keysToTry = []; for (let row of rows) { diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 48719f5ade..e2b9ad18ba 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -2056,6 +2056,88 @@ describe("Zotero.Sync.Data.Engine", function () { }); + describe("#_downloadUpdatedObjects()", function () { + it("should include objects in sync queue", function* () { + ({ engine, client, caller } = yield setup()); + + var libraryID = Zotero.Libraries.userLibraryID; + var objectType = 'collection'; + var objectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue( + objectType, libraryID, ["BBBBBBBB", "CCCCCCCC"] + ); + yield Zotero.DB.queryAsync( + "UPDATE syncQueue SET lastCheck=lastCheck-3600 " + + "WHERE syncObjectTypeID=? AND libraryID=? AND key IN (?, ?)", + [objectTypeID, libraryID, 'BBBBBBBB', 'CCCCCCCC'] + ); + + var headers = { + "Last-Modified-Version": 5 + }; + setResponse({ + method: "GET", + url: "users/1/collections?format=versions&since=1", + status: 200, + headers, + json: { + AAAAAAAA: 5, + BBBBBBBB: 5 + } + }); + + var stub = sinon.stub(engine, "_downloadObjects"); + + yield engine._downloadUpdatedObjects(objectType, 1, 5); + + assert.ok(stub.calledWith("collection", ["AAAAAAAA", "BBBBBBBB", "CCCCCCCC"])); + stub.restore(); + }); + }); + + + describe("#_downloadObjects()", function () { + it("should remove object from sync queue if missing from response", function* () { + ({ engine, client, caller } = yield setup({ + stopOnError: false + })); + var libraryID = Zotero.Libraries.userLibraryID; + var objectType = 'collection'; + var objectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue( + objectType, libraryID, ["BBBBBBBB", "CCCCCCCC"] + ); + + var headers = { + "Last-Modified-Version": 5 + }; + setResponse({ + method: "GET", + url: "users/1/collections?format=json&collectionKey=AAAAAAAA%2CBBBBBBBB%2CCCCCCCCC", + status: 200, + headers, + json: [ + makeCollectionJSON({ + key: "AAAAAAAA", + version: 5, + name: "A" + }), + makeCollectionJSON({ + key: "BBBBBBBB", + version: 5 + // Missing 'name', which causes a save error + }) + ] + }); + yield engine._downloadObjects(objectType, ["AAAAAAAA", "BBBBBBBB", "CCCCCCCC"]); + + // Missing object should have been removed, but invalid object should remain + var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, libraryID); + assert.sameMembers(keys, ['BBBBBBBB']); + }); + }); + + describe("#_startUpload()", function () { it("shouldn't upload unsynced objects if present in sync queue", function* () { ({ engine, client, caller } = yield setup());