Fix errors uploading remotely missing objects with local version numbers

If an object exists locally but not remotely and the local version has a
version number, that's an error. I don't think that should ever happen,
but it can if things somehow get out of sync due to other bugs.

To address, reprocess the API delete log during a full sync and then
reset the version number of all remaining local objects that don't exist
remotely (not just unmodified objects, as was the case previously) to 0
for uploading.

When remote deletions are reprocessed, delete local objects that haven't
been modified and show the conflict resolution window for any local
items that have.

Also:

- Clean up checking of last remote library version during download
  syncs
- Add Zotero.DataObjects.getAllKeys()

chrome/content/zotero/xpcom/data/dataObjects.js

@@ -222,6 +222,12 @@ Zotero.DataObjects.prototype.getLoaded = function () {
 }
 
 
+Zotero.DataObjects.prototype.getAllKeys = function (libraryID) {
+	var sql = "SELECT key FROM " + this._ZDO_table + " WHERE libraryID=?";
+	return Zotero.DB.columnQueryAsync(sql, [libraryID]);
+};
+
+
 /**
  * @deprecated - use .libraryKey
  */
@@ -794,7 +800,7 @@ Zotero.DataObjects.prototype.unload = function () {
  * Set the version of objects, efficiently
  *
  * @param {Integer[]} ids - Ids of objects to update
- * @param {Boolean} synced
+ * @param {Boolean} version
  */
 Zotero.DataObjects.prototype.updateVersion = Zotero.Promise.method(function (ids, version) {
 	if (version != parseInt(version)) {

chrome/content/zotero/xpcom/sync/syncEngine.js

@@ -73,7 +73,8 @@ Zotero.Sync.Data.Engine = function (options) {
 Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CONTINUE = 1;
 Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_CHANGES_TO_UPLOAD = 2;
 Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_NO_CHANGES_TO_UPLOAD = 3;
-Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_RESTART = 4;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_LIBRARY_UNMODIFIED = 4;
+Zotero.Sync.Data.Engine.prototype.DOWNLOAD_RESULT_RESTART = 5;
 
 Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_SUCCESS = 1;
 Zotero.Sync.Data.Engine.prototype.UPLOAD_RESULT_NOTHING_TO_UPLOAD = 2;
@@ -114,6 +115,7 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* ()
 		}
 	}
 	
+	this.downloadDelayGenerator = null;
 	var autoReset = false;
 	
 	sync:
@@ -211,17 +213,20 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 	var libraryVersion = this.library.libraryVersion;
 	var newLibraryVersion;
 	
-	this.downloadDelayGenerator = Zotero.Utilities.Internal.delayGenerator(
-		Zotero.Sync.Data.conflictDelayIntervals, 60 * 60 * 1000
-	);
-	
 	loop:
 	while (true) {
 		// Get synced settings first, since they affect how other data is displayed
-		newLibraryVersion = yield this._downloadSettings(libraryVersion);
+		let results = yield this._downloadSettings(libraryVersion);
-		if (newLibraryVersion === false) {
+		if (results.result == this.DOWNLOAD_RESULT_LIBRARY_UNMODIFIED) {
 			break;
 		}
+		else if (results.result == this.DOWNLOAD_RESULT_RESTART) {
+			yield this._onLibraryVersionChange();
+			continue loop;
+		}
+		else {
+			newLibraryVersion = results.libraryVersion;
+		}
 		
 		//
 		// Get other object types
@@ -243,6 +248,7 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 					}
 				);
 				if (result == this.DOWNLOAD_RESULT_RESTART) {
+					yield this._onLibraryVersionChange();
 					continue loop;
 				}
 			}
@@ -253,12 +259,14 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 				newLibraryVersion
 			);
 			if (result == this.DOWNLOAD_RESULT_RESTART) {
+				yield this._onLibraryVersionChange();
 				continue loop;
 			}
 		}
 		
 		let deletionsResult = yield this._downloadDeletions(libraryVersion, newLibraryVersion);
 		if (deletionsResult == this.DOWNLOAD_RESULT_RESTART) {
+			yield this._onLibraryVersionChange();
 			continue loop;
 		}
 		
@@ -276,11 +284,19 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func
 
 
 /**
+ * Download settings modified since the given version
+ *
+ * Unlike the other download methods, this method, which runs first in the main download process,
+ * returns an object rather than just a download result code. It does this so it can return the
+ * current library version from the API to pass to later methods, allowing them to restart the download
+ * process if there was a remote change.
+ *
  * @param {Integer} since - Last-known library version; get changes since this version
- * @return {Integer|Boolean} - Library version returned from server, or false if no changes since
+ * @param {Integer} [newLibraryVersion] - Newest library version seen in this sync process; if newer
- *                             specified version
+ *     version is seen, restart the sync
+ * @return {Object} - Object with 'result' (DOWNLOAD_RESULT_*) and 'libraryVersion'
  */
-Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(function* (since) {
+Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(function* (since, newLibraryVersion) {
 	let results = yield this.apiClient.getSettings(
 		this.library.libraryType,
 		this.libraryTypeID,
@@ -291,7 +307,15 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f
 	if (results === false) {
 		Zotero.debug("Library " + this.libraryID + " hasn't been modified "
 			+ "-- skipping further object downloads");
-		return false;
+		return {
+			result: this.DOWNLOAD_RESULT_LIBRARY_UNMODIFIED
+		};
+	}
+	if (newLibraryVersion !== undefined && newLibraryVersion != results.libraryVersion) {
+		return {
+			result: this.DOWNLOAD_RESULT_RESTART,
+			libraryVersion: results.libraryVersion
+		};
 	}
 	var numObjects = Object.keys(results.settings).length;
 	if (numObjects) {
@@ -309,7 +333,10 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f
 	else {
 		Zotero.debug("No settings modified remotely since last check");
 	}
-	return results.libraryVersion;
+	return {
+		result: this.DOWNLOAD_RESULT_CONTINUE,
+		libraryVersion: results.libraryVersion
+	};
 })
 
 
@@ -351,7 +378,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
 	// wait for increasing amounts of time before trying again, and then start from
 	// the beginning
 	if (newLibraryVersion != results.libraryVersion) {
-		return this._onLibraryVersionChange();
+		return this.DOWNLOAD_RESULT_RESTART;
 	}
 	
 	
@@ -598,8 +625,8 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
  * Get deleted objects from the API and process them
  *
  * @param {Integer} since - Last-known library version; get changes sinces this version
- * @param {Integer} newLibraryVersion - Newest library version seen in this sync process; if newer version
+ * @param {Integer} [newLibraryVersion] - Newest library version seen in this sync process; if newer
- *     is seen, restart the sync
+ *     version is seen, restart the sync
  * @return {Promise<Integer>} - A download result code (this.DOWNLOAD_RESULT_*)
  */
 Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(function* (since, newLibraryVersion) {
@@ -608,8 +635,8 @@ Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(
 		this.libraryTypeID,
 		since
 	);
-	if (newLibraryVersion != results.libraryVersion) {
+	if (newLibraryVersion !== undefined && newLibraryVersion != results.libraryVersion) {
-		return this._onLibraryVersionChange();
+		return this.DOWNLOAD_RESULT_RESTART;
 	}
 	
 	var numObjects = Object.keys(results.deleted).reduce((n, k) => n + results.deleted[k].length, 0);
@@ -672,8 +699,6 @@ Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(
 					}
 				});
 			}
-			
-			// Ignore deletion if collection/search changed locally
 		}
 		
 		if (conflicts.length) {
@@ -741,11 +766,17 @@ Zotero.Sync.Data.Engine.prototype._downloadDeletions = Zotero.Promise.coroutine(
  */
 Zotero.Sync.Data.Engine.prototype._onLibraryVersionChange = Zotero.Promise.coroutine(function* (mode) {
 	Zotero.logError("Library version changed since last download -- restarting sync");
+	
+	if (!this.downloadDelayGenerator) {
+		this.downloadDelayGenerator = Zotero.Utilities.Internal.delayGenerator(
+			Zotero.Sync.Data.conflictDelayIntervals, 60 * 60 * 1000
+		);
+	}
+	
 	let keepGoing = yield this.downloadDelayGenerator.next().value;
 	if (!keepGoing) {
 		throw new Error("Could not update " + this.library.name + " -- library in use");
 	}
-	return this.DOWNLOAD_RESULT_RESTART;
 });
 
 
@@ -1296,19 +1327,33 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 	
 	var gen;
 	var lastLibraryVersion;
-	var remoteDeleted;
 	
 	loop:
 	while (true) {
-		// Get synced settings
+		// Reprocess all deletions available from API
-		lastLibraryVersion = yield this._downloadSettings();
+		let result = yield this._downloadDeletions(0, lastLibraryVersion);
+		if (result == this.DOWNLOAD_RESULT_RESTART) {
+			yield this._onLibraryVersionChange();
+			continue loop;
+		}
 		
-		// Get other object types
+		// Get synced settings
+		results = yield this._downloadSettings(0, lastLibraryVersion);
+		if (results.result == this.DOWNLOAD_RESULT_RESTART) {
+			yield this._onLibraryVersionChange();
+			continue loop;
+		}
+		else {
+			lastLibraryVersion = results.libraryVersion;
+		}
+		
+		// Get object types
 		for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(this.libraryID)) {
 			this._failedCheck();
 			
 			let objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
 			let ObjectType = Zotero.Utilities.capitalize(objectType);
+			let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
 			
 			// TODO: localize
 			this.setStatus("Updating " + objectTypePlural + " in " + this.library.name);
@@ -1326,28 +1371,11 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 					objectType
 				);
 			}
-			if (lastLibraryVersion) {
+			if (lastLibraryVersion != results.libraryVersion) {
-				// If something else modified the remote library while we were getting updates,
+				yield this._onLibraryVersionChange();
-				// wait for increasing amounts of time before trying again, and then start from
+				continue loop;
-				// the first object type
-				if (lastLibraryVersion != results.libraryVersion) {
-					if (!gen) {
-						gen = Zotero.Utilities.Internal.delayGenerator(
-							Zotero.Sync.Data.conflictDelayIntervals, 60 * 60 * 1000
-						);
-					}
-					let keepGoing = yield gen.next().value;
-					if (!keepGoing) {
-						throw new Error("Could not update " + this.library.name + " -- library in use");
-					}
-					continue loop;
-				}
-			}
-			else {
-				lastLibraryVersion = results.libraryVersion;
 			}
 			
-			let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType);
 			let toDownload = [];
 			let localVersions = yield objectsClass.getObjectVersions(this.libraryID);
 			// Queue objects that are out of date or don't exist locally
@@ -1388,50 +1416,22 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 				Zotero.debug(`No missing/outdated ${objectTypePlural} to download`);
 			}
 			
-			// Mark synced objects that don't exist remotely as unsynced
+			// Mark local objects that don't exist remotely as unsynced and version 0
-			let syncedKeys = yield Zotero.Sync.Data.Local.getSynced(objectType, this.libraryID);
+			let allKeys = yield objectsClass.getAllKeys(this.libraryID);
-			let remoteMissing = Zotero.Utilities.arrayDiff(syncedKeys, Object.keys(results.versions));
+			let remoteMissing = Zotero.Utilities.arrayDiff(allKeys, Object.keys(results.versions));
 			if (remoteMissing.length) {
-				Zotero.debug("Checking remotely missing synced " + objectTypePlural);
+				Zotero.debug("Checking remotely missing " + objectTypePlural);
 				Zotero.debug(remoteMissing);
 				
-				// Check remotely deleted objects
+				let toUpload = remoteMissing.map(
-				if (!remoteDeleted) {
+					key => objectsClass.getIDFromLibraryAndKey(this.libraryID, key)
-					let results = yield this.apiClient.getDeleted(
+				// Remove any objects deleted since getAllKeys() call
-						this.library.libraryType, this.libraryTypeID
+				).filter(id => id);
-					);
-					remoteDeleted = results.deleted;
-				}
 				
-				let toDelete = [];
-				let toUpload = [];
-				for (let key of remoteMissing) {
-					let id = objectsClass.getIDFromLibraryAndKey(this.libraryID, key);
-					if (!id) {
-						Zotero.logError(`ObjectType ${key} not found to mark as unsynced`);
-						continue;
-					}
-					if (remoteDeleted[objectTypePlural].indexOf(key) != -1) {
-						toDelete.push(id);
-						continue;
-					}
-					toUpload.push(id);
-				}
-				// Delete local objects that were deleted remotely
-				if (toDelete.length) {
-					Zotero.debug("Deleting remotely deleted synced " + objectTypePlural);
-					yield objectsClass.erase(
-						toDelete,
-						{
-							skipEditCheck: true,
-							skipDeleteLog: true
-						}
-					);
-				}
 				// For remotely missing objects that exist locally, reset version, since old
 				// version will no longer match remote, and mark for upload
 				if (toUpload.length) {
-					Zotero.debug(`Marking remotely missing synced ${objectTypePlural} as unsynced`);
+					Zotero.debug(`Marking remotely missing ${objectTypePlural} as unsynced`);
 					yield objectsClass.updateVersion(toUpload, 0);
 					yield objectsClass.updateSynced(toUpload, false);
 				}

test/tests/syncEngineTest.js

@@ -3051,6 +3051,104 @@ describe("Zotero.Sync.Data.Engine", function () {
 					type, userLibraryID, objects[type][2].key
 				));
 			}
-		})
+		});
+		
+		it("should reprocess remote deletions", function* () {
+			var userLibraryID = Zotero.Libraries.userLibraryID;
+			({ engine, client, caller } = yield setup());
+			
+			var types = Zotero.DataObjectUtilities.getTypes();
+			var objects = {};
+			var objectIDs = {};
+			
+			for (let type of types) {
+				// Create object marked as synced that's in the remote delete log, which should be
+				// deleted locally
+				let obj = createUnsavedDataObject(type);
+				obj.synced = true;
+				obj.version = 5;
+				yield obj.saveTx();
+				objects[type] = [obj];
+				objectIDs[type] = [obj.id];
+				
+				// Create object marked as unsynced that's in the remote delete log, which should
+				// trigger a conflict in the case of items and otherwise reset version to 0
+				obj = createUnsavedDataObject(type);
+				obj.synced = false;
+				obj.version = 5;
+				yield obj.saveTx();
+				objects[type].push(obj);
+				objectIDs[type].push(obj.id);
+			}
+			
+			var headers = {
+				"Last-Modified-Version": 20
+			}
+			setResponse({
+				method: "GET",
+				url: "users/1/settings",
+				status: 200,
+				headers,
+				json: {}
+			});
+			let deletedJSON = {};
+			for (let type of types) {
+				let suffix = type == 'item' ? '&includeTrashed=1' : '';
+				let plural = Zotero.DataObjectUtilities.getObjectTypePlural(type);
+				setResponse({
+					method: "GET",
+					url: "users/1/" + plural + "?format=versions" + suffix,
+					status: 200,
+					headers,
+					json: {}
+				});
+				deletedJSON[plural] = objects[type].map(o => o.key);
+			}
+			setResponse({
+				method: "GET",
+				url: "users/1/deleted?since=0",
+				status: 200,
+				headers: headers,
+				json: deletedJSON
+			});
+			
+			// Apply remote deletions
+			var shown = false;
+			waitForWindow('chrome://zotero/content/merge.xul', function (dialog) {
+				shown = true;
+				var doc = dialog.document;
+				var wizard = doc.documentElement;
+				var mergeGroup = wizard.getElementsByTagName('zoteromergegroup')[0];
+				
+				// Should be one conflict for each object type; select local
+				var numConflicts = Object.keys(objects).length;
+				for (let i = 0; i < numConflicts; i++) {
+					assert.equal(mergeGroup.leftpane.getAttribute('selected'), 'true');
+					
+					if (i < numConflicts - 1) {
+						wizard.getButton('next').click();
+					}
+					else {
+						wizard.getButton('finish').click();
+					}
+				}
+			});
+			
+			yield engine._fullSync();
+			assert.ok(shown);
+			
+			// Check objects
+			for (let type of types) {
+				let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type);
+				
+				// Objects 0 should be deleted
+				assert.isFalse(objectsClass.exists(objectIDs[type][0]));
+				
+				// Objects 1 should be marked for reupload
+				assert.isTrue(objectsClass.exists(objectIDs[type][1]));
+				assert.strictEqual(objects[type][1].version, 0);
+				assert.strictEqual(objects[type][1].synced, false);
+			}
+		});
 	})
 })