From 644c4e592580f4007ab590dcfe460d94934668ca Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 9 Dec 2021 03:05:50 -0500 Subject: [PATCH] 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 | 6 +- .../zotero/xpcom/sync/syncAPIClient.js | 15 +++-- .../content/zotero/xpcom/sync/syncEngine.js | 66 ++++++++++++------- test/tests/syncEngineTest.js | 28 ++++++++ 4 files changed, 83 insertions(+), 32 deletions(-) diff --git a/chrome/content/zotero/xpcom/storage/zfs.js b/chrome/content/zotero/xpcom/storage/zfs.js index 19fd1212f5..a55713c73f 100644 --- a/chrome/content/zotero/xpcom/storage/zfs.js +++ b/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) { diff --git a/chrome/content/zotero/xpcom/sync/syncAPIClient.js b/chrome/content/zotero/xpcom/sync/syncAPIClient.js index 1a9a822715..6437aa4159 100644 --- a/chrome/content/zotero/xpcom/sync/syncAPIClient.js +++ b/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>} - An array of promises for batches of JSON objects - * or Errors for failures + * @return {Promise[]} - 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 + }; }) ]; }, diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 4238acaea2..fd2bf17513 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/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,20 +686,21 @@ 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]) - + " from sync queue"); - await Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys); - remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, missingKeys); - } + if (missingKeys.length) { + Zotero.debug(`Removing ${missingKeys.length} missing ` + + Zotero.Utilities.pluralize(missingKeys.length, [objectType, objectTypePlural]) + + " from sync queue"); + 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; } diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index e85c71c9cd..da9c57cbd2 100644 --- a/test/tests/syncEngineTest.js +++ b/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] + ); + }); });