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.

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,
+						// If a cache object exists, there shouldn't be any need to update it, since
-						// since objects are uploaded in batches and we only get the final version, but it
+						// nothing was changed on the server, including the version.
-						// 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
+						// If there's no cache object, we can save the uploaded object, which we know
-						// server.
+						// matches what's on the server. The version number won't necessarily reflect
-						jsonBatch[index].version = libraryVersion;
+						// the actual version of the object on the server, since objects are uploaded
-						toCache.push(jsonBatch[index]);
+						// 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++;

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