Check remote delete log during full sync

If an object is marked as synced locally and doesn't exist remotely but
is in the remote delete log, delete it locally

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

@@ -117,6 +117,10 @@ Zotero.Sync.APIClient.prototype = {
 	}),
 	
 	
+	/**
+	 * @return {Object|false} - An object with 'libraryVersion' and a 'deleted' array, or
+	 *     false if 'since' is earlier than the beginning of the delete log
+	 */
 	getDeleted: Zotero.Promise.coroutine(function* (libraryType, libraryTypeID, since) {
 		var params = {
 			target: "deleted",
@@ -125,7 +129,11 @@ Zotero.Sync.APIClient.prototype = {
 			since: since || 0
 		};
 		var uri = this._buildRequestURI(params);
-		var xmlhttp = yield this._makeRequest("GET", uri);
+		var xmlhttp = yield this._makeRequest("GET", uri, { successCodes: [200, 409] });
+		if (xmlhttp.status == 409) {
+			Zotero.debug(`'since' value '${since}' is earlier than the beginning of the delete log`);
+			return false;
+		}
 		var libraryVersion = xmlhttp.getResponseHeader('Last-Modified-Version');
 		if (!libraryVersion) {
 			throw new Error("Last-Modified-Version not provided");

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

@@ -918,8 +918,8 @@ Zotero.Sync.Data.Engine.prototype._upgradeCheck = Zotero.Promise.coroutine(funct
  * missing or outdated and not up-to-date in the sync cache, download them. If any local objects
  * are marked as synced but aren't available remotely, mark them as unsynced for later uploading.
  *
- * (Technically this isn't a _full_ sync on its own, because settings aren't downloaded here and
+ * (Technically this isn't a full sync on its own, because objects are only flagged for later
- * objects are only flagged for later upload.)
+ * upload.)
  *
  * @param {Object[]} [versionResults] - Objects returned from getVersions(), keyed by objectType
  * @return {Promise<Integer>} - Promise for the library version after syncing
@@ -929,6 +929,7 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 	
 	var gen;
 	var lastLibraryVersion;
+	var remoteDeleted;
 	
 	loop:
 	while (true) {
@@ -1030,14 +1031,19 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 			let syncedKeys = yield Zotero.Sync.Data.Local.getSynced(this.libraryID, objectType);
 			let remoteMissing = Zotero.Utilities.arrayDiff(syncedKeys, Object.keys(results.versions));
 			if (remoteMissing.length) {
-				Zotero.debug("Marking remotely missing synced " + objectTypePlural + " as unsynced");
+				Zotero.debug("Checking remotely missing synced " + objectTypePlural);
 				Zotero.debug(remoteMissing);
 				
-				// TODO: Check remote deleted
+				// Check remotely deleted objects
-				// If remote delete log doesn't go back far enough, add to recovered items?
+				if (!remoteDeleted) {
-				// Do we care about local timestamp of last edit?
+					let results = yield this.apiClient.getDeleted(
+						this.libraryType, this.libraryTypeID
+					);
+					remoteDeleted = results.deleted;
+				}
 				
-				let ids = [];
+				let toDelete = [];
+				let toUpload = [];
 				for (let key of remoteMissing) {
 					let id = objectsClass.getIDFromLibraryAndKey(this.libraryID, key);
 					if (!id) {
@@ -1045,11 +1051,25 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function*
 							+ " not found to mark as unsynced");
 						continue;
 					}
-					ids.push(id);
+					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, { 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");
+					yield objectsClass.updateVersion(toUpload, 0);
+					yield objectsClass.updateSynced(toUpload, false);
 				}
-				// Reset version, since old version will no longer match remote
-				yield objectsClass.updateVersion(ids, 0);
-				yield objectsClass.updateSynced(ids, false);
 			}
 			
 			// Process newly cached objects

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

@@ -387,7 +387,7 @@ Zotero.Sync.Data.Local = {
 							isNewObject = true;
 							
 							// Check if object has been deleted locally
-							if (yield this._objectInDeleteLog(objectType, libraryID, objectKey)) {
+							if (yield this.objectInDeleteLog(objectType, libraryID, objectKey)) {
 								switch (objectType) {
 								case 'item':
 									throw new Error("Unimplemented");
@@ -701,7 +701,7 @@ Zotero.Sync.Data.Local = {
 	/**
 	 * @return {Promise<Boolean>}
 	 */
-	_objectInDeleteLog: Zotero.Promise.coroutine(function* (objectType, libraryID, key) {
+	objectInDeleteLog: Zotero.Promise.coroutine(function* (objectType, libraryID, key) {
 		var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType);
 		var sql = "SELECT COUNT(*) FROM syncDeleteLog WHERE libraryID=? AND key=? "
 			+ "AND syncObjectTypeID=?";

test/tests/syncEngineTest.js

@@ -661,16 +661,16 @@ describe("Zotero.Sync.Data.Engine", function () {
 			assert.isFalse(Zotero.Items.exists(itemID));
 			
 			// Make sure objects weren't added to sync delete log
-			assert.isFalse(yield Zotero.Sync.Data.Local._objectInDeleteLog(
+			assert.isFalse(yield Zotero.Sync.Data.Local.objectInDeleteLog(
 				'setting', userLibraryID, 'tagColors'
 			));
-			assert.isFalse(yield Zotero.Sync.Data.Local._objectInDeleteLog(
+			assert.isFalse(yield Zotero.Sync.Data.Local.objectInDeleteLog(
 				'collection', userLibraryID, collectionKey
 			));
-			assert.isFalse(yield Zotero.Sync.Data.Local._objectInDeleteLog(
+			assert.isFalse(yield Zotero.Sync.Data.Local.objectInDeleteLog(
 				'search', userLibraryID, searchKey
 			));
-			assert.isFalse(yield Zotero.Sync.Data.Local._objectInDeleteLog(
+			assert.isFalse(yield Zotero.Sync.Data.Local.objectInDeleteLog(
 				'item', userLibraryID, itemKey
 			));
 		})
@@ -853,7 +853,6 @@ describe("Zotero.Sync.Data.Engine", function () {
 			var userLibraryID = Zotero.Libraries.userLibraryID;
 			({ engine, client, caller } = yield setup());
 			
-			yield Zotero.Items.erase([1, 2], { skipDeleteLog: true });
 			var types = Zotero.DataObjectUtilities.getTypes();
 			var objects = {};
 			var objectJSON = {};
@@ -862,7 +861,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 			}
 			
 			for (let type of types) {
-				// Create objects with outdated versions, which should be updated
+				// Create object with outdated version, which should be updated
 				let obj = createUnsavedDataObject(type);
 				obj.synced = true;
 				obj.version = 5;
@@ -875,7 +874,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 					name: Zotero.Utilities.randomString()
 				}));
 				
-				// Create JSON for objects that exist remotely and not locally,
+				// Create JSON for object that exists remotely and not locally,
 				// which should be downloaded
 				objectJSON[type].push(makeJSONFunctions[type]({
 					key: Zotero.DataObjectUtilities.generateKey(),
@@ -883,13 +882,21 @@ describe("Zotero.Sync.Data.Engine", function () {
 					name: Zotero.Utilities.randomString()
 				}));
 				
-				// Create objects marked as synced that don't exist remotely,
+				// Create object marked as synced that doesn't exist remotely,
 				// which should be flagged for upload
 				obj = createUnsavedDataObject(type);
 				obj.synced = true;
 				obj.version = 10;
 				yield obj.saveTx();
 				objects[type].push(obj);
+				
+				// Create object marked as synced that doesn't exist remotely but is in the
+				// remote delete log, which should be deleted locally
+				obj = createUnsavedDataObject(type);
+				obj.synced = true;
+				obj.version = 10;
+				yield obj.saveTx();
+				objects[type].push(obj);
 			}
 			
 			var headers = {
@@ -912,15 +919,17 @@ describe("Zotero.Sync.Data.Engine", function () {
 					}
 				}
 			});
+			let deletedJSON = {};
 			for (let type of types) {
-				var suffix = type == 'item' ? '&includeTrashed=1' : '';
+				let suffix = type == 'item' ? '&includeTrashed=1' : '';
+				let plural = Zotero.DataObjectUtilities.getObjectTypePlural(type);
 				
 				var json = {};
 				json[objectJSON[type][0].key] = objectJSON[type][0].version;
 				json[objectJSON[type][1].key] = objectJSON[type][1].version;
 				setResponse({
 					method: "GET",
-					url: "users/1/" + Zotero.DataObjectUtilities.getObjectTypePlural(type)
+					url: "users/1/" + plural
 						+ "?format=versions" + suffix,
 					status: 200,
 					headers: headers,
@@ -929,7 +938,7 @@ describe("Zotero.Sync.Data.Engine", function () {
 				
 				setResponse({
 					method: "GET",
-					url: "users/1/" + Zotero.DataObjectUtilities.getObjectTypePlural(type)
+					url: "users/1/" + plural
 						+ "?format=json"
 						+ "&" + type + "Key=" + objectJSON[type][0].key + "%2C" + objectJSON[type][1].key
 						+ suffix,
@@ -937,7 +946,16 @@ describe("Zotero.Sync.Data.Engine", function () {
 					headers: headers,
 					json: objectJSON[type]
 				});
+				
+				deletedJSON[plural] = [objects[type][2].key];
 			}
+			setResponse({
+				method: "GET",
+				url: "users/1/deleted?since=0",
+				status: 200,
+				headers: headers,
+				json: deletedJSON
+			});
 			yield engine._fullSync();
 			
 			// Check settings
@@ -963,6 +981,12 @@ describe("Zotero.Sync.Data.Engine", function () {
 				// JSON objects 2 should be marked as unsynced, with their version reset to 0
 				assert.equal(objects[type][1].version, 0);
 				assert.isFalse(objects[type][1].synced);
+				
+				// JSON objects 3 should be deleted and not in the delete log
+				assert.isFalse(objectsClass.getByLibraryAndKey(userLibraryID, objects[type][2].key));
+				assert.isFalse(yield Zotero.Sync.Data.Local.objectInDeleteLog(
+					type, userLibraryID, objects[type][2].key
+				));
 			}
 		})
 	})