Add objects from failed download requests to sync queue

Previously only individual objects from successful requests that
couldn't be processed for some reason would be added to the queue.

`Sync.APIClient.downloadObjects()` now returns clearer and more
consistent results. It now returns an array of promises for objects with
a `keys` array of requested keys and either a `json` array of returned
API JSON objects or an `error` Error, depending on whether the request
succeeded or failed. This makes it easier to detect remotely missing
objects and request failures.

chrome/content/zotero/xpcom/storage/zfs.js

@@ -567,14 +567,14 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = {
 			
 			// Get updated item metadata
 			let library = Zotero.Libraries.get(item.libraryID);
-			let json = yield this.apiClient.downloadObjects(
+			let { json, error } = yield this.apiClient.downloadObjects(
 				library.libraryType,
 				library.libraryTypeID,
 				'item',
 				[item.key]
 			)[0];
-			if (!Array.isArray(json)) {
-				Zotero.logError(json);
+			if (error) {
+				Zotero.logError(error);
 				throw new Error(Zotero.Sync.Storage.defaultError);
 			}
 			if (json.length > 1) {

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

@@ -259,8 +259,9 @@ Zotero.Sync.APIClient.prototype = {
 	 * @param {Integer} libraryTypeID - userID or groupID
 	 * @param {String} objectType - 'collection', 'item', 'search'
 	 * @param {String[]} objectKeys - Keys of objects to request
-	 * @return {Array<Promise<Object[]|Error[]>>} - An array of promises for batches of JSON objects
-	 *     or Errors for failures
+	 * @return {Promise<Object>[]} - An array of promises for objects with JSON data as
+	 *     { keys: String[], json: Object[] } or objects with errors as
+	 *     { keys: String[], error: Error }
 	 */
 	downloadObjects: function (libraryType, libraryTypeID, objectType, objectKeys) {
 		if (!objectKeys.length) {
@@ -309,7 +310,10 @@ Zotero.Sync.APIClient.prototype = {
 		return [
 			this.makeRequest("GET", uri)
 			.then(function (xmlhttp) {
-				return this._parseJSON(xmlhttp.responseText)
+				return {
+					keys: objectKeys,
+					json: this._parseJSON(xmlhttp.responseText)
+				};
 			}.bind(this))
 			// Return the error without failing the whole chain
 			.catch(function (e) {
@@ -317,7 +321,10 @@ Zotero.Sync.APIClient.prototype = {
 				if (e instanceof Zotero.HTTP.UnexpectedStatusException && e.is4xx()) {
 					throw e;
 				}
-				return e;
+				return {
+					keys: objectKeys,
+					error: e
+				};
 			})
 		];
 	},

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

@@ -546,21 +546,26 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType,
 		this._statusCheck();
 		
 		// Get data we've downloaded in a previous loop but failed to process
-		var json = [];
+		var results = [];
 		let keysToDownload = [];
+		var keysToReprocess = [];
 		for (let key in objectData) {
 			if (objectData[key] === null) {
 				keysToDownload.push(key);
 			}
 			else {
-				json.push(objectData[key]);
+				keysToReprocess.push(key);
+				
 			}
 		}
-		if (json.length) {
-			json = [json];
+		if (keysToReprocess) {
+			results.push({
+				keys: keysToReprocess,
+				json: keysToReprocess.map(key => objectData[key])
+			});
 		}
 		// Add promises for batches of downloaded data for remaining keys
-		json.push(...this.apiClient.downloadObjects(
+		results.push(...this.apiClient.downloadObjects(
 			this.library.libraryType,
 			this.libraryTypeID,
 			objectType,
@@ -576,33 +581,43 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType,
 			+ " in " + this.library.name
 		);
 		
+		var missingKeys = [];
 		var conflicts = [];
 		var restored = [];
 		var num = 0;
 		
 		// Process batches of object data as they're available, one at a time
 		await Zotero.Promise.map(
-			json,
-			async function (batch) {
+			results,
+			async function ({ keys: batchKeys, json, error }) {
 				this._statusCheck();
 				
 				Zotero.debug(`Processing batch of downloaded ${objectTypePlural} in ${this.library.name}`);
 				
-				if (!Array.isArray(batch)) {
-					this.failed = batch;
+				if (error) {
+					this.failed = error;
 					return;
 				}
 				
 				// Save downloaded JSON for later attempts
-				batch.forEach(obj => {
+				var seenKeys = new Set();
+				json.forEach(obj => {
 					objectData[obj.key] = obj;
+					seenKeys.add(obj.key);
 				});
+				// If a key we requested wasn't returned, it's missing remotely
+				for (let key of batchKeys) {
+					if (!seenKeys.has(key)) {
+						missingKeys.push(key);
+						objectData[key] = false;
+					}
+				}
 				
 				// Process objects
 				let results = await Zotero.Sync.Data.Local.processObjectsFromJSON(
 					objectType,
 					this.libraryID,
-					batch,
+					json,
 					this._getOptions({
 						onObjectProcessed: () => {
 							num++;
@@ -627,7 +642,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType,
 							else {
 								size = 50;
 							}
-							return Math.min(size, batch.length);
+							return Math.min(size, json.length);
 						}
 					})
 				);
@@ -671,8 +686,6 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType,
 		
 		// If all requests were successful, such that we had a chance to see all keys, remove keys we
 		// didn't see from the sync queue so they don't keep being retried forever
-		if (!this.failed) {
-			let missingKeys = keys.filter(key => objectData[key] === null);
 		if (missingKeys.length) {
 			Zotero.debug(`Removing ${missingKeys.length} missing `
 				+ Zotero.Utilities.pluralize(missingKeys.length, [objectType, objectTypePlural])
@@ -680,11 +693,14 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = async function (objectType,
 			await Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys);
 			remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, missingKeys);
 		}
-		}
 		
 		if (!remainingKeys.length || remainingKeys.length == lastLength) {
 			// Add failed objects to sync queue
-			let failedKeys = keys.filter(key => objectData[key]);
+			//
+			// Object failed if it's still present in the object data (since successfully processed
+			// objects are removed) and it's not false (meaning it was missing from the request,
+			// which isn't a failure)
+			let failedKeys = keys.filter(key => objectData[key] !== undefined && objectData[key] !== false);
 			if (failedKeys.length) {
 				Zotero.debug(`Queueing ${failedKeys.length} failed `
 					+ Zotero.Utilities.pluralize(failedKeys.length, [objectType, objectTypePlural])
@@ -1464,12 +1480,12 @@ Zotero.Sync.Data.Engine.prototype._updateGroupItemUsers = async function () {
 	
 	Zotero.debug(`Updating item users in ${this.library.name}`);
 	
-	var jsonItems = await this.apiClient.downloadObjects(
+	var { json: jsonItems, error } = await this.apiClient.downloadObjects(
 		this.library.libraryType, this.libraryTypeID, 'item', keys
 	)[0];
 	
-	if (!Array.isArray(jsonItems)) {
-		Zotero.logError(e);
+	if (error) {
+		Zotero.logError(error);
 		return;
 	}
 	

test/tests/syncEngineTest.js

@@ -3005,6 +3005,34 @@ describe("Zotero.Sync.Data.Engine", function () {
 				await Zotero.Sync.Data.Local.getDateDeleted('item', libraryID, item.key)
 			);
 		});
+		
+		
+		it("should add object from failed download request to sync queue", async function () {
+			({ engine, client, caller } = await setup({
+				stopOnError: false
+			}));
+			var libraryID = Zotero.Libraries.userLibraryID;
+			
+			var item = await createDataObject('item');
+			var itemKey = item.key;
+			
+			var headers = {
+				"Last-Modified-Version": 5
+			};
+			setResponse({
+				method: "GET",
+				url: `users/1/items?itemKey=${itemKey}&includeTrashed=1`,
+				status: 0,
+				headers,
+				body: ""
+			});
+			await engine._downloadObjects('item', [itemKey]);
+			
+			assert.sameMembers(
+				await Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', libraryID),
+				[itemKey]
+			);
+		});
 	});