From ad13313924aee1ea346a9628643a0b8db01ac6b4 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Tue, 14 Apr 2020 05:03:56 -0400
Subject: [PATCH] Don't change existing sync cache object on 'unchanged'
 response

Previously, if an object was uploaded but the API returned 'unchanged',
the uploaded data would be written to the sync cache, which, given that
most requests are patch requests, could result in an empty or mostly
empty object being saved to the sync cache. That would cause the next
sync to treat most/all local fields as changed and either upload them
unnecessarily or trigger a conflict instead of merging changes
automatically.
---
 .../content/zotero/xpcom/sync/syncEngine.js   | 25 +++++++---
 test/tests/syncEngineTest.js                  | 50 +++++++++++++++++++
 2 files changed, 68 insertions(+), 7 deletions(-)

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());