diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 620d5b32ea..0f3ebebe1f 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -1240,6 +1240,7 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func } } + // Successful if (state == 'successful') { // Update local object with saved data if necessary, as long as it hasn't // changed locally since the upload @@ -1253,14 +1254,24 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func } toCache.push(current); } + // Unchanged else { - // This won't necessarily reflect the actual version of the object on the server, - // since objects are uploaded in batches and we only get the final version, but it - // will guarantee that the item won't be redownloaded unnecessarily in the case of - // a full sync, because the version will be higher than whatever version is on the - // server. - jsonBatch[index].version = libraryVersion; - toCache.push(jsonBatch[index]); + // If a cache object exists, there shouldn't be any need to update it, since + // nothing was changed on the server, including the version. + // + // If there's no cache object, we can save the uploaded object, which we know + // matches what's on the server. The version number won't necessarily reflect + // the actual version of the object on the server, since objects are uploaded + // in batches and we only get the final version, but it will guarantee that + // the object won't be redownloaded unnecessarily in the case of a full sync, + // because the version will be higher than whatever version is on the server. + let hasCacheObject = !!Zotero.Sync.Data.Local.getCacheObject( + objectType, obj.libraryID, obj.key, obj.version + ); + if (!hasCacheObject) { + jsonBatch[index].version = libraryVersion; + toCache.push(jsonBatch[index]); + } } numSuccessful++; diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 9d5fe4841a..f202546f32 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -801,6 +801,56 @@ describe("Zotero.Sync.Data.Engine", function () { }); + it("shouldn't update existing cache object after upload on 'unchanged' response", async function () { + ({ engine, client, caller } = await setup()); + + var library = Zotero.Libraries.userLibrary; + var lastLibraryVersion = 5; + library.libraryVersion = lastLibraryVersion; + await library.saveTx(); + + var item = await createDataObject('item', { version: 1, title: "A" }); + var json = item.toJSON(); + // Save current version to cache so the patch object is empty, as if the item had been + // added to a collection and removed from it (such that even dateModified didn't change) + await Zotero.Sync.Data.Local.saveCacheObjects('item', library.id, [json]); + + server.respond(function (req) { + if (req.method == "POST" && req.url == baseURL + "users/1/items") { + let json = JSON.parse(req.requestBody); + req.respond( + 200, + { + "Content-Type": "application/json", + "Last-Modified-Version": ++lastLibraryVersion + }, + JSON.stringify({ + successful: {}, + unchanged: { + "0": item.key + }, + failed: {} + }) + ); + return; + } + }); + + await engine.start(); + + // Check data in cache + var version = await Zotero.Sync.Data.Local.getLatestCacheObjectVersion( + 'item', library.id, item.key + ); + assert.equal(version, 1); + json = await Zotero.Sync.Data.Local.getCacheObject( + 'item', library.id, item.key, 1 + ); + assert.propertyVal(json.data, 'itemType', 'book'); + assert.propertyVal(json.data, 'title', 'A'); + }); + + it("should upload child collection after parent collection", function* () { ({ engine, client, caller } = yield setup());