From f05b98ba20082175a75a8aef5b51c372fb6f99a2 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 14 Jan 2016 01:50:13 -0500 Subject: [PATCH] Fetch top-level items before other items when syncing --- .../zotero/xpcom/sync/syncAPIClient.js | 4 + .../content/zotero/xpcom/sync/syncEngine.js | 143 +++++++++++------- chrome/content/zotero/xpcom/sync/syncLocal.js | 3 +- test/tests/syncEngineTest.js | 64 +++++++- 4 files changed, 160 insertions(+), 54 deletions(-) diff --git a/chrome/content/zotero/xpcom/sync/syncAPIClient.js b/chrome/content/zotero/xpcom/sync/syncAPIClient.js index 07623fe83..911a5ae3e 100644 --- a/chrome/content/zotero/xpcom/sync/syncAPIClient.js +++ b/chrome/content/zotero/xpcom/sync/syncAPIClient.js @@ -170,6 +170,10 @@ Zotero.Sync.APIClient.prototype = { format: 'versions' }; if (queryParams) { + if (queryParams.top) { + params.target += "/top"; + delete queryParams.top; + } for (let i in queryParams) { params[i] = queryParams[i]; } diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 86017c91a..23b1242ae 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -215,64 +215,33 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func this._failedCheck(); this._processCache(objectType); - let objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); - let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); - - // Get versions of all objects updated remotely since the current local library version - Zotero.debug("Checking for updated " + objectTypePlural + " in " + this.library.name); - let results = yield this.apiClient.getVersions( - this.library.libraryType, - this.libraryTypeID, - objectType, - libraryVersion ? { since: libraryVersion } : undefined - ); - - Zotero.debug("VERSIONS:"); - Zotero.debug(JSON.stringify(results)); - - if (lastLibraryVersion) { - // If something else modified the remote library while we were getting updates, - // wait for increasing amounts of time before trying again, and then start from - // the beginning - if (lastLibraryVersion != results.libraryVersion) { - Zotero.logError("Library version changed since last download -- restarting sync"); - let keepGoing = yield gen.next(); - if (!keepGoing) { - throw new Error("Could not update " + this.library.name + " -- library in use"); + // For items, fetch top-level items first + // + // The next run will then see the same items in the non-top versions request, + // but they'll have been downloaded already and will be skipped. + if (objectType == 'item') { + let result = yield this._downloadUpdatedObjects( + objectType, + libraryVersion, + lastLibraryVersion, + gen, + { + top: true } + ); + if (result == -1) { continue loop; } } - else { - lastLibraryVersion = results.libraryVersion; - } - var numObjects = Object.keys(results.versions).length; - if (!numObjects) { - Zotero.debug("No " + objectTypePlural + " modified remotely since last check"); - continue; - } - Zotero.debug(numObjects + " " + (numObjects == 1 ? objectType : objectTypePlural) - + " modified since last check"); - - let keys = []; - let versions = yield Zotero.Sync.Data.Local.getLatestCacheObjectVersions( - objectType, this.libraryID, Object.keys(results.versions) + let result = yield this._downloadUpdatedObjects( + objectType, + libraryVersion, + lastLibraryVersion, + gen ); - for (let key in results.versions) { - // Skip objects that are already up-to-date in the sync cache. Generally all returned - // objects should have newer version numbers, but there are some situations, such as - // full syncs or interrupted syncs, where we may get versions for objects that are - // already up-to-date locally. - if (versions[key] == results.versions[key]) { - Zotero.debug("Skipping up-to-date " + objectType + " " + this.libraryID + "/" + key); - continue; - } - keys.push(key); - } - - if (keys.length) { - yield this._downloadObjects(objectType, keys); + if (result == -1) { + continue loop; } } @@ -460,6 +429,76 @@ 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 + */ +Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, libraryVersion, lastLibraryVersion, delayGenerator, options = {}) { + var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); + + // Get versions of all objects updated remotely since the current local library version + Zotero.debug("Checking for updated " + objectTypePlural + " in " + this.library.name); + var queryParams = {}; + if (libraryVersion) { + queryParams.since = libraryVersion; + } + if (options.top) { + queryParams.top = true; + } + var results = yield this.apiClient.getVersions( + this.library.libraryType, + this.libraryTypeID, + objectType, + queryParams + ); + + Zotero.debug("VERSIONS:"); + Zotero.debug(JSON.stringify(results)); + + // If something else modified the remote library while we were getting updates, + // wait for increasing amounts of time before trying again, and then start from + // the beginning + if (lastLibraryVersion != results.libraryVersion) { + Zotero.logError("Library version changed since last download -- restarting sync"); + let keepGoing = yield delayGenerator.next(); + if (!keepGoing) { + throw new Error("Could not update " + this.library.name + " -- library in use"); + } + return -1; + } + + var numObjects = Object.keys(results.versions).length; + if (!numObjects) { + Zotero.debug("No " + objectTypePlural + " modified remotely since last check"); + return false; + } + Zotero.debug(numObjects + " " + (numObjects == 1 ? objectType : objectTypePlural) + + " modified since last check"); + + let keys = []; + let versions = yield Zotero.Sync.Data.Local.getLatestCacheObjectVersions( + objectType, this.libraryID, Object.keys(results.versions) + ); + for (let key in results.versions) { + // Skip objects that are already up-to-date in the sync cache. Generally all returned + // objects should have newer version numbers, but there are some situations, such as + // full syncs or interrupted syncs, where we may get versions for objects that are + // already up-to-date locally. + if (versions[key] == results.versions[key]) { + Zotero.debug("Skipping up-to-date " + objectType + " " + this.libraryID + "/" + key); + continue; + } + keys.push(key); + } + if (!keys.length) { + return false; + } + + yield this._downloadObjects(objectType, keys); + return true; +}); + + Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); var failureDelayGenerator = null; diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 087f3e278..09457c70a 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -453,7 +453,8 @@ Zotero.Sync.Data.Local = { let jsonData = json.data; let objectKey = json.key; - Zotero.debug(`Processing ${objectType} ${libraryID}/${objectKey}`); + Zotero.debug(`Processing ${objectType} ${libraryID}/${objectKey} ` + + "from sync cache"); Zotero.debug(json); if (!jsonData) { diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 9fda696a1..5b91dc00d 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -152,13 +152,23 @@ describe("Zotero.Sync.Data.Engine", function () { }); setResponse({ method: "GET", - url: "users/1/items?format=versions&includeTrashed=1", + url: "users/1/items/top?format=versions&includeTrashed=1", status: 200, headers: headers, json: { "AAAAAAAA": 3 } }); + setResponse({ + method: "GET", + url: "users/1/items?format=versions&includeTrashed=1", + status: 200, + headers: headers, + json: { + "AAAAAAAA": 3, + "BBBBBBBB": 3 + } + }); setResponse({ method: "GET", url: "users/1/collections?format=json&collectionKey=AAAAAAAA", @@ -199,6 +209,21 @@ describe("Zotero.Sync.Data.Engine", function () { }) ] }); + setResponse({ + method: "GET", + url: "users/1/items?format=json&itemKey=BBBBBBBB&includeTrashed=1", + status: 200, + headers: headers, + json: [ + makeItemJSON({ + key: "BBBBBBBB", + version: 3, + itemType: "note", + parentItem: "AAAAAAAA", + note: "This is a note." + }) + ] + }); setResponse({ method: "GET", url: "users/1/deleted?since=0", @@ -235,6 +260,13 @@ describe("Zotero.Sync.Data.Engine", function () { assert.equal(obj.getField('title'), 'A'); assert.equal(obj.version, 3); assert.isTrue(obj.synced); + var parentItemID = obj.id; + + obj = yield Zotero.Items.getByLibraryAndKeyAsync(userLibraryID, "BBBBBBBB"); + assert.equal(obj.getNote(), 'This is a note.'); + assert.equal(obj.parentItemID, parentItemID); + assert.equal(obj.version, 3); + assert.isTrue(obj.synced); }) it("should upload new full items and subsequent patches", function* () { @@ -680,6 +712,15 @@ describe("Zotero.Sync.Data.Engine", function () { }); json = {}; json[objects.item.key] = 5; + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&includeTrashed=1", + status: 200, + headers: headers, + json: json + }); + json = {}; + json[objects.item.key] = 5; setResponse({ method: "GET", url: "users/1/items?format=versions&includeTrashed=1", @@ -751,6 +792,13 @@ describe("Zotero.Sync.Data.Engine", function () { headers: headers, json: {} }); + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&since=5&includeTrashed=1", + status: 200, + headers: headers, + json: {} + }); setResponse({ method: "GET", url: "users/1/deleted?since=5", @@ -826,6 +874,13 @@ describe("Zotero.Sync.Data.Engine", function () { headers: headers, json: {} }); + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&since=5&includeTrashed=1", + status: 200, + headers: headers, + json: {} + }); setResponse({ method: "GET", url: "users/1/items?format=versions&since=5&includeTrashed=1", @@ -895,6 +950,13 @@ describe("Zotero.Sync.Data.Engine", function () { headers: headers, json: {} }); + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&since=5&includeTrashed=1", + status: 200, + headers: headers, + json: {} + }); setResponse({ method: "GET", url: "users/1/items?format=versions&since=5&includeTrashed=1",