From a1ce85decb82824d7c351f1b0656bd5153de2053 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Mon, 29 Feb 2016 04:23:00 -0500 Subject: [PATCH] Overhaul object downloading/processing during data syncs Previously, objects were first downloaded and saved to the sync cache, which was then processed separately to create/update local objects. This meant that a server bug could result in invalid data in the sync cache that would never be processed. Now, objects are saved as they're downloaded and only added to the sync cache after being successfully saved. The keys of objects that fail are added to a queue, and those objects are refetched and retried on a backoff schedule or when a new client version is installed (in case of a client bug or a client with outdated data model support). An alternative would be to save to the sync cache first and evict objects that fail and add them to the queue, but that requires more complicated logic, and it probably makes more sense just to buffer a few downloads ahead so that processing is never waiting for downloads to finish. --- chrome/content/zotero/merge.js | 9 +- .../content/zotero/xpcom/data/dataObject.js | 12 +- .../content/zotero/xpcom/data/dataObjects.js | 37 + chrome/content/zotero/xpcom/data/item.js | 5 + chrome/content/zotero/xpcom/schema.js | 52 +- chrome/content/zotero/xpcom/search.js | 10 +- .../zotero/xpcom/sync/syncAPIClient.js | 8 +- .../content/zotero/xpcom/sync/syncEngine.js | 264 ++++---- chrome/content/zotero/xpcom/sync/syncLocal.js | 632 ++++++++++++------ .../content/zotero/xpcom/sync/syncRunner.js | 2 +- resource/schema/userdata.sql | 13 +- test/tests/schemaTest.js | 26 + test/tests/storageLocalTest.js | 8 +- test/tests/syncAPIClientTest.js | 29 +- test/tests/syncEngineTest.js | 219 +++++- test/tests/syncLocalTest.js | 312 +++++++-- test/tests/syncRunnerTest.js | 98 ++- 17 files changed, 1251 insertions(+), 485 deletions(-) diff --git a/chrome/content/zotero/merge.js b/chrome/content/zotero/merge.js index ee527c4d43..a8d4a3cafa 100644 --- a/chrome/content/zotero/merge.js +++ b/chrome/content/zotero/merge.js @@ -164,12 +164,9 @@ var Zotero_Merge_Window = new function () { _merged.forEach(function (x, i, a) { // Add key x.data.key = _conflicts[i].left.key || _conflicts[i].right.key; - // If selecting right item, add back version - if (x.data && x.selected == 'right') { - x.data.version = _conflicts[i].right.version; - } - else { - delete x.data.version; + // Add back version + if (x.data) { + x.data.version = _conflicts[i][x.selected].version; } a[i] = x.data; }) diff --git a/chrome/content/zotero/xpcom/data/dataObject.js b/chrome/content/zotero/xpcom/data/dataObject.js index bb667122a2..9c6628811a 100644 --- a/chrome/content/zotero/xpcom/data/dataObject.js +++ b/chrome/content/zotero/xpcom/data/dataObject.js @@ -671,22 +671,20 @@ Zotero.DataObject.prototype._requireData = function (dataType) { * Loads data for a given data type * @param {String} dataType * @param {Boolean} reload + * @param {Promise} */ Zotero.DataObject.prototype._loadDataType = function (dataType, reload) { return this._ObjectsClass._loadDataType(dataType, this.libraryID, [this.id]); } -Zotero.DataObject.prototype.loadAllData = function (reload) { - let loadPromises = new Array(this._dataTypes.length); +Zotero.DataObject.prototype.loadAllData = Zotero.Promise.coroutine(function* (reload) { for (let i=0; i} - A promise for an object with object keys as keys and versions + * as properties + */ +Zotero.DataObjects.prototype.getObjectVersions = Zotero.Promise.coroutine(function* (libraryID, keys = null) { + var versions = {}; + + if (keys) { + yield Zotero.Utilities.Internal.forEachChunkAsync( + keys, + Zotero.DB.MAX_BOUND_PARAMETERS - 1, + Zotero.Promise.coroutine(function* (chunk) { + var sql = "SELECT key, version FROM " + this._ZDO_table + + " WHERE libraryID=? AND key IN (" + chunk.map(key => '?').join(', ') + ")"; + var rows = yield Zotero.DB.queryAsync(sql, [libraryID].concat(chunk)); + for (let i = 0; i < rows.length; i++) { + let row = rows[i]; + versions[row.key] = row.version; + } + }.bind(this)) + ); + } + else { + let sql = "SELECT key, version FROM " + this._ZDO_table + " WHERE libraryID=?"; + let rows = yield Zotero.DB.queryAsync(sql, [libraryID]); + for (let i = 0; i < rows.length; i++) { + let row = rows[i]; + versions[row.key] = row.version; + } + } + + return versions; +}); + + /** * Loads data for a given data type * @param {String} dataType diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 0915448f71..bbb5ebaf6d 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/chrome/content/zotero/xpcom/data/item.js @@ -3914,6 +3914,11 @@ Zotero.Item.prototype.fromJSON = function (json) { } let itemTypeID = Zotero.ItemTypes.getID(json.itemType); + if (!itemTypeID) { + let e = new Error(`Invalid item type '${json.itemType}'`); + e.name = "ZoteroUnknownTypeError"; + throw e; + } this.setType(itemTypeID); var isValidForType = {}; diff --git a/chrome/content/zotero/xpcom/schema.js b/chrome/content/zotero/xpcom/schema.js index 55f50505cf..bcc11154e0 100644 --- a/chrome/content/zotero/xpcom/schema.js +++ b/chrome/content/zotero/xpcom/schema.js @@ -178,6 +178,9 @@ Zotero.Schema = new function(){ } } + // Reset sync queue tries if new version + yield _checkClientVersion(); + Zotero.initializationPromise .then(1000) .then(function () { @@ -1383,6 +1386,8 @@ Zotero.Schema = new function(){ yield Zotero.DB.queryAsync(sql, [welcomeMsg, welcomeTitle]); }*/ + yield _updateLastClientVersion(); + self.dbInitialized = true; }) .catch(function (e) { @@ -1434,13 +1439,46 @@ Zotero.Schema = new function(){ } yield Zotero.DB.queryAsync( - "REPLACE INTO settings VALUES (?, ?, ?)", - ['client', 'lastCompatibleVersion', Zotero.version] + "REPLACE INTO settings VALUES ('client', 'lastCompatibleVersion', ?)", [Zotero.version] ); yield _updateDBVersion('compatibility', version); }); + function _checkClientVersion() { + return Zotero.DB.executeTransaction(function* () { + var lastVersion = yield _getLastClientVersion(); + var currentVersion = Zotero.version; + + if (currentVersion == lastVersion) { + return false; + } + + Zotero.debug(`Client version has changed from ${lastVersion} to ${currentVersion}`); + + // Retry all queued objects immediately on upgrade + yield Zotero.Sync.Data.Local.resetSyncQueueTries(); + + // Update version + yield _updateLastClientVersion(); + + return true; + }.bind(this)); + } + + + function _getLastClientVersion() { + var sql = "SELECT value FROM settings WHERE setting='client' AND key='lastVersion'"; + return Zotero.DB.valueQueryAsync(sql); + } + + + function _updateLastClientVersion() { + var sql = "REPLACE INTO settings (setting, key, value) VALUES ('client', 'lastVersion', ?)"; + return Zotero.DB.queryAsync(sql, Zotero.version); + } + + /** * Process the response from the repository * @@ -2168,7 +2206,7 @@ Zotero.Schema = new function(){ } - if (i == 81) { + else if (i == 81) { yield _updateCompatibility(2); yield Zotero.DB.queryAsync("ALTER TABLE libraries RENAME TO librariesOld"); @@ -2179,7 +2217,7 @@ Zotero.Schema = new function(){ yield Zotero.DB.queryAsync("DELETE FROM version WHERE schema LIKE ?", "storage_%"); } - if (i == 82) { + else if (i == 82) { yield Zotero.DB.queryAsync("DELETE FROM itemTypeFields WHERE itemTypeID=17 AND orderIndex BETWEEN 3 AND 9"); yield Zotero.DB.queryAsync("INSERT INTO itemTypeFields VALUES (17, 44, NULL, 3)"); yield Zotero.DB.queryAsync("INSERT INTO itemTypeFields VALUES (17, 96, NULL, 4)"); @@ -2190,13 +2228,17 @@ Zotero.Schema = new function(){ yield Zotero.DB.queryAsync("INSERT INTO itemTypeFields VALUES (17, 42, NULL, 9)"); } - if (i == 83) { + else if (i == 83) { // Feeds yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS feeds"); yield Zotero.DB.queryAsync("DROP TABLE IF EXISTS feedItems"); yield Zotero.DB.queryAsync("CREATE TABLE feeds (\n libraryID INTEGER PRIMARY KEY,\n name TEXT NOT NULL,\n url TEXT NOT NULL UNIQUE,\n lastUpdate TIMESTAMP,\n lastCheck TIMESTAMP,\n lastCheckError TEXT,\n cleanupAfter INT,\n refreshInterval INT,\n FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE\n)"); yield Zotero.DB.queryAsync("CREATE TABLE feedItems (\n itemID INTEGER PRIMARY KEY,\n guid TEXT NOT NULL UNIQUE,\n readTime TIMESTAMP,\n translatedTime TIMESTAMP,\n FOREIGN KEY (itemID) REFERENCES items(itemID) ON DELETE CASCADE\n)"); } + + else if (i == 84) { + yield Zotero.DB.queryAsync("CREATE TABLE syncQueue (\n libraryID INT NOT NULL,\n key TEXT NOT NULL,\n syncObjectTypeID INT NOT NULL,\n lastCheck TIMESTAMP,\n tries INT,\n PRIMARY KEY (libraryID, key, syncObjectTypeID),\n FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE,\n FOREIGN KEY (syncObjectTypeID) REFERENCES syncObjectTypes(syncObjectTypeID) ON DELETE CASCADE\n)"); + } } yield _updateDBVersion('userdata', toVersion); diff --git a/chrome/content/zotero/xpcom/search.js b/chrome/content/zotero/xpcom/search.js index 923ef61d1a..c0d358a3f6 100644 --- a/chrome/content/zotero/xpcom/search.js +++ b/chrome/content/zotero/xpcom/search.js @@ -279,7 +279,9 @@ Zotero.Search.prototype.addCondition = function (condition, operator, value, req this._requireData('conditions'); if (!Zotero.SearchConditions.hasOperator(condition, operator)){ - throw new Error("Invalid operator '" + operator + "' for condition " + condition); + let e = new Error("Invalid operator '" + operator + "' for condition " + condition); + e.name = "ZoteroUnknownFieldError"; + throw e; } // Shortcut to add a condition on every table -- does not return an id @@ -412,7 +414,9 @@ Zotero.Search.prototype.updateCondition = function (searchConditionID, condition } if (!Zotero.SearchConditions.hasOperator(condition, operator)){ - throw new Error("Invalid operator '" + operator + "' for condition " + condition); + let e = new Error("Invalid operator '" + operator + "' for condition " + condition); + e.name = "ZoteroUnknownFieldError"; + throw e; } var [condition, mode] = Zotero.SearchConditions.parseCondition(condition); @@ -2323,7 +2327,7 @@ Zotero.SearchConditions = new function(){ } if (!_conditions[condition]){ - var e = new Error("Invalid condition '" + condition + "' in hasOperator()"); + let e = new Error("Invalid condition '" + condition + "' in hasOperator()"); e.name = "ZoteroUnknownFieldError"; throw e; } diff --git a/chrome/content/zotero/xpcom/sync/syncAPIClient.js b/chrome/content/zotero/xpcom/sync/syncAPIClient.js index 0f054bb4bc..ca93fe33d9 100644 --- a/chrome/content/zotero/xpcom/sync/syncAPIClient.js +++ b/chrome/content/zotero/xpcom/sync/syncAPIClient.js @@ -38,6 +38,7 @@ Zotero.Sync.APIClient = function (options) { this.caller = options.caller; this.failureDelayIntervals = [2500, 5000, 10000, 20000, 40000, 60000, 120000, 240000, 300000]; + this.failureDelayMax = 60 * 60 * 1000; // 1 hour } Zotero.Sync.APIClient.prototype = { @@ -270,11 +271,10 @@ Zotero.Sync.APIClient.prototype = { }.bind(this)) // Return the error without failing the whole chain .catch(function (e) { + Zotero.logError(e); if (e instanceof Zotero.HTTP.UnexpectedStatusException && e.is4xx()) { - Zotero.logError(e); throw e; } - Zotero.logError(e); return e; }) ]; @@ -591,13 +591,13 @@ Zotero.Sync.APIClient.prototype = { if (!failureDelayGenerator) { // Keep trying for up to an hour failureDelayGenerator = Zotero.Utilities.Internal.delayGenerator( - this.failureDelayIntervals, 60 * 60 * 1000 + this.failureDelayIntervals, this.failureDelayMax ); } let keepGoing = yield failureDelayGenerator.next().value; if (!keepGoing) { Zotero.logError("Failed too many times"); - throw lastError; + throw e; } return false; } diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js index 9c77c3c1e9..da0fb18fba 100644 --- a/chrome/content/zotero/xpcom/sync/syncEngine.js +++ b/chrome/content/zotero/xpcom/sync/syncEngine.js @@ -50,27 +50,24 @@ Zotero.Sync.Data.Engine = function (options) { this.libraryID = options.libraryID; this.library = Zotero.Libraries.get(options.libraryID); this.libraryTypeID = this.library.libraryTypeID; - this.setStatus = options.setStatus || function () {}; - this.onError = options.onError || function (e) {}; - this.stopOnError = options.stopOnError; this.requests = []; this.uploadBatchSize = 25; this.uploadDeletionBatchSize = 50; this.failed = false; + this.failedItems = []; - this.options = { - setStatus: this.setStatus, - stopOnError: this.stopOnError, - onError: this.onError - } - - Components.utils.import("resource://zotero/concurrentCaller.js"); - this.syncCacheProcessor = new ConcurrentCaller({ - id: "Sync Cache Processor", - numConcurrent: 1, - logger: Zotero.debug, - stopOnError: this.stopOnError + // Options to pass through to processing functions + this.optionNames = ['setStatus', 'onError', 'stopOnError', 'background', 'firstInSession']; + this.options = {}; + this.optionNames.forEach(x => { + // Create dummy functions if not set + if (x == 'setStatus' || x == 'onError') { + this[x] = options[x] || function () {}; + } + else { + this[x] = options[x]; + } }); }; @@ -162,9 +159,6 @@ Zotero.Sync.Data.Engine.prototype.start = Zotero.Promise.coroutine(function* () } } - Zotero.debug("Waiting for sync cache to be processed"); - yield this.syncCacheProcessor.wait(); - yield Zotero.Libraries.updateLastSyncTime(this.libraryID); Zotero.debug("Done syncing " + this.library.name); @@ -213,7 +207,6 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func // for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(this.libraryID)) { this._failedCheck(); - this._processCache(objectType); // For items, fetch top-level items first // @@ -245,9 +238,6 @@ Zotero.Sync.Data.Engine.prototype._startDownload = Zotero.Promise.coroutine(func } } - // Wait for sync process to clear - yield this.syncCacheProcessor.wait(); - // // Get deleted objects // @@ -435,6 +425,7 @@ Zotero.Sync.Data.Engine.prototype._downloadSettings = Zotero.Promise.coroutine(f */ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.coroutine(function* (objectType, libraryVersion, lastLibraryVersion, delayGenerator, options = {}) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); + var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); // Get versions of all objects updated remotely since the current local library version Zotero.debug(`Checking for updated ${options.top ? 'top-level ' : ''}` @@ -468,30 +459,58 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou return -1; } + var numObjects = Object.keys(results.versions).length; - if (!numObjects) { + if (numObjects) { + Zotero.debug(numObjects + " " + (numObjects == 1 ? objectType : objectTypePlural) + + " modified since last check"); + } + else { Zotero.debug("No " + objectTypePlural + " modified remotely since last check"); + } + + // Get objects that should be retried based on the current time, unless it's top-level items mode. + // (We don't know if the queued items are top-level or not, so we do them with child items.) + let queuedKeys = []; + if (objectType != 'item' || !options.top) { + queuedKeys = yield Zotero.Sync.Data.Local.getObjectsToTryFromSyncQueue( + objectType, this.libraryID + ); + if (queuedKeys.length) { + Zotero.debug(`Refetching ${queuedKeys.length} queued ` + + (queuedKeys.length == 1 ? objectType : objectTypePlural)) + } + } + + if (!numObjects && !queuedKeys.length) { return false; } - Zotero.debug(numObjects + " " + (numObjects == 1 ? objectType : objectTypePlural) - + " modified since last check"); let keys = []; - let versions = yield Zotero.Sync.Data.Local.getLatestCacheObjectVersions( - objectType, this.libraryID, Object.keys(results.versions) + let versions = yield objectsClass.getObjectVersions( + this.libraryID, Object.keys(results.versions) ); for (let key in results.versions) { - // Skip objects that are already up-to-date in the sync cache. Generally all returned - // objects should have newer version numbers, but there are some situations, such as - // full syncs or interrupted syncs, where we may get versions for objects that are - // already up-to-date locally. + // Skip objects that are already up-to-date. Generally all returned objects should have + // newer version numbers, but there are some situations, such as full syncs or + // interrupted syncs, where we may get versions for objects that are already up-to-date + // locally. if (versions[key] == results.versions[key]) { Zotero.debug("Skipping up-to-date " + objectType + " " + this.libraryID + "/" + key); continue; } keys.push(key); } + + // In child-items mode, remove top-level items that just failed + if (objectType == 'item' && !options.top && this.failedItems.length) { + keys = Zotero.Utilities.arrayDiff(keys, this.failedItems); + } + + keys.push(...queuedKeys); + if (!keys.length) { + Zotero.debug(`No ${objectTypePlural} to download`); return false; } @@ -502,19 +521,42 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) { var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); - var failureDelayGenerator = null; var lastLength = keys.length; + var objectData = {}; + keys.forEach(key => objectData[key] = null); while (true) { this._failedCheck(); let lastError = false; + // Get data we've downloaded in a previous loop but failed to process + var json = []; + let keysToDownload = []; + for (let key in objectData) { + if (objectData[key] === null) { + keysToDownload.push(key); + } + else { + json.push(objectData[key]); + } + } + if (json.length) { + json = [json]; + } + // Add promises for batches of downloaded data for remaining keys + json.push(...this.apiClient.downloadObjects( + this.library.libraryType, + this.libraryTypeID, + objectType, + keysToDownload + )); + // TODO: localize this.setStatus( "Downloading " - + (keys.length == 1 + + (keysToDownload.length == 1 ? "1 " + objectType : Zotero.Utilities.numberFormat(keys.length, 0) + " " + objectTypePlural) + " in " + this.library.name @@ -522,65 +564,79 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu // Process batches as soon as they're available yield Zotero.Promise.map( - this.apiClient.downloadObjects( - this.library.libraryType, - this.libraryTypeID, - objectType, - keys - ), + json, function (batch) { this._failedCheck(); - Zotero.debug("MAPPING"); + Zotero.debug(`Processing batch of downloaded ${objectTypePlural} in ` + + this.library.name); + + if (!Array.isArray(batch)) { - Zotero.debug("WE GOT AN ERROR"); - Components.utils.reportError(batch); - Zotero.debug(batch, 1); this.failed = batch; lastError = batch; return; } - // Save objects to sync cache - return Zotero.Sync.Data.Local.saveCacheObjects( - objectType, this.libraryID, batch + // Save downloaded JSON for later attempts + batch.forEach(obj => { + objectData[obj.key] = obj; + }); + + // Process objects + return Zotero.Sync.Data.Local.processObjectsFromJSON( + objectType, + this.libraryID, + batch, + this._getOptions() ) - .then(function () { - let processedKeys = batch.map(item => item.key); + .then(function (results) { + let processedKeys = []; + results.forEach(x => { + // If data was processed, remove JSON + if (x.processed) { + delete objectData[x.key]; + } + // If object shouldn't be retried, mark as processed + if (x.processed || !x.retry) { + processedKeys.push(x.key); + } + }); keys = Zotero.Utilities.arrayDiff(keys, processedKeys); - - // Create/update objects as they come in - this._processCache(objectType); }.bind(this)); }.bind(this) ); - if (!keys.length) { - Zotero.debug("All " + objectTypePlural + " for library " - + this.libraryID + " saved to sync cache"); - break; - } - - // If we're not making process, delay for increasing amounts of time - // and then keep going - if (keys.length == lastLength) { - if (!failureDelayGenerator) { - // Keep trying for up to an hour - failureDelayGenerator = Zotero.Utilities.Internal.delayGenerator( - Zotero.Sync.Data.failureDelayIntervals, 60 * 60 * 1000 + if (!keys.length || keys.length == lastLength) { + // Add failed objects to sync queue + let failedKeys = Object.keys(objectData).filter(key => objectData[key]) + if (failedKeys.length) { + let objDesc = `${failedKeys.length == 1 ? objectType : objectTypePlural}`; + Zotero.debug(`Queueing ${failedKeys.length} failed ${objDesc} for later`, 2); + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue( + objectType, this.libraryID, failedKeys ); + + // Note failed item keys so child items step (if this isn't it) can skip them + if (objectType == 'item') { + this.failedItems = failedKeys; + } } - let keepGoing = yield failureDelayGenerator.next(); - if (!keepGoing) { - Zotero.logError("Failed too many times"); - throw lastError; + else { + Zotero.debug("All " + objectTypePlural + " for " + + this.library.name + " saved to database"); + + if (objectType == 'item') { + this.failedItems = []; + } } - } - else { - failureDelayGenerator = null; + return; } lastLength = keys.length; + + var remainingObjectDesc = `${keys.length == 1 ? objectType : objectTypePlural}`; + Zotero.debug(`Retrying ${keys.length} remaining ${remainingObjectDesc}`); } }); @@ -1107,7 +1163,7 @@ 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 objects are only flagged for later + * (Technically this isn't a full sync on its own, because local objects are only flagged for later * upload.) * * @param {Object[]} [versionResults] - Objects returned from getVersions(), keyed by objectType @@ -1135,9 +1191,6 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* // TODO: localize this.setStatus("Updating " + objectTypePlural + " in " + this.library.name); - // Start processing cached objects while waiting for API - this._processCache(objectType); - let results = {}; // Use provided versions if (versionResults) { @@ -1174,9 +1227,7 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); let toDownload = []; - let cacheVersions = yield Zotero.Sync.Data.Local.getLatestCacheObjectVersions( - objectType, this.libraryID - ); + let localVersions = yield objectsClass.getObjectVersions(this.libraryID); // Queue objects that are out of date or don't exist locally for (let key in results.versions) { let version = results.versions[key]; @@ -1184,31 +1235,26 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* noCache: true }); // If object already at latest version, skip - if (obj && obj.version === version) { + let localVersion = localVersions[key]; + if (localVersion && localVersion === version) { continue; } - let cacheVersion = cacheVersions[key]; - // If cache already has latest version, skip - if (cacheVersion == version) { - continue; - } - // This should never happen, but recover if it does - if (cacheVersion > version) { - Zotero.logError("Sync cache had later version than remote for " - + objectType + " " + this.libraryID + "/" + key - + " (" + cacheVersion + " > " + version + ") -- deleting"); + + // This should never happen + if (localVersion > version) { + Zotero.logError(`Local version of ${objectType} ${this.libraryID}/${key} ` + + `is later than remote! (${localVersion} > ${version})`); + // Delete cache version if it's there yield Zotero.Sync.Data.Local.deleteCacheObjectVersions( - objectType, this.libraryID, key, cacheVersion, cacheVersion + objectType, this.libraryID, key, localVersion, localVersion ); } if (obj) { - Zotero.debug(ObjectType + " " + obj.libraryKey - + " is older than version in sync cache"); + Zotero.debug(`${ObjectType} ${obj.libraryKey} is older than remote version`); } else { - Zotero.debug(ObjectType + " " + this.libraryID + "/" + key - + " in sync cache not found locally"); + Zotero.debug(`${ObjectType} ${this.libraryID}/${key} does not exist locally`); } toDownload.push(key); @@ -1263,15 +1309,10 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* yield objectsClass.updateSynced(toUpload, false); } } - - // Process newly cached objects - this._processCache(objectType); } break; } - yield this.syncCacheProcessor.wait(); - yield Zotero.Libraries.setVersion(this.libraryID, lastLibraryVersion); Zotero.debug("Done with full sync for " + this.library.name); @@ -1280,27 +1321,10 @@ Zotero.Sync.Data.Engine.prototype._fullSync = Zotero.Promise.coroutine(function* }); -/** - * Chain sync cache processing for a given object type - * - * On error, check if errors should be fatal and set the .failed flag - * - * @param {String} objectType - */ -Zotero.Sync.Data.Engine.prototype._processCache = function (objectType) { - this.syncCacheProcessor.start(function () { - this._failedCheck(); - return Zotero.Sync.Data.Local.processSyncCacheForObjectType( - this.libraryID, objectType, this.options - ) - .catch(function (e) { - Zotero.logError(e); - if (this.stopOnError) { - Zotero.debug("WE FAILED!!!"); - this.failed = e; - } - }.bind(this)); - }.bind(this)) +Zotero.Sync.Data.Engine.prototype._getOptions = function () { + var options = {}; + this.optionNames.forEach(x => options[x] = this[x]); + return options; } diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index ccf98fadee..9370d68e10 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -28,6 +28,7 @@ if (!Zotero.Sync.Data) { } Zotero.Sync.Data.Local = { + _syncQueueIntervals: [0.5, 1, 4, 16, 16, 16, 16, 16, 16, 16, 64], // hours _loginManagerHost: 'chrome://zotero', _loginManagerRealm: 'Zotero Web API', _lastSyncTime: null, @@ -383,50 +384,39 @@ Zotero.Sync.Data.Local = { }), - _checkCacheJSON: function (json) { - if (json.key === undefined) { - throw new Error("Missing 'key' property in JSON"); - } - if (json.version === undefined) { - throw new Error("Missing 'version' property in JSON"); - } - // If direct data object passed, wrap in fake response object - return json.data === undefined ? { - key: json.key, - version: json.version, - data: json - } : json; - }, - - - processSyncCache: Zotero.Promise.coroutine(function* (libraryID, options) { - for (let objectType of Zotero.DataObjectUtilities.getTypesForLibrary(libraryID)) { - yield this.processSyncCacheForObjectType(libraryID, objectType, options); - } - }), - - - processSyncCacheForObjectType: Zotero.Promise.coroutine(function* (libraryID, objectType, options = {}) { + /** + * Process downloaded JSON and update local objects + * + * @return {Promise>} - Promise for an array of objects with the following properties: + * {String} key + * {Boolean} processed + * {Object} [error] + * {Boolean} [retry] + */ + processObjectsFromJSON: Zotero.Promise.coroutine(function* (objectType, libraryID, json, options = {}) { var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType); var ObjectType = Zotero.Utilities.capitalize(objectType); var libraryName = Zotero.Libraries.getName(libraryID); - Zotero.debug("Processing " + objectTypePlural + " in sync cache for " + libraryName); + var knownErrors = [ + 'ZoteroUnknownTypeError', + 'ZoteroUnknownFieldError', + 'ZoteroMissingObjectError' + ]; + Zotero.debug("Processing " + json.length + " downloaded " + + (json.length == 1 ? objectType : objectTypePlural) + + " for " + libraryName); + + var results = []; var conflicts = []; - var numSaved = 0; - var numSkipped = 0; - var data = yield this._getUnwrittenData(libraryID, objectType); - if (!data.length) { - Zotero.debug("No unwritten " + objectTypePlural + " in sync cache"); - return; + if (!json.length) { + return results; } - Zotero.debug("Processing " + data.length + " " - + (data.length == 1 ? objectType : objectTypePlural) - + " in sync cache"); + json = json.map(o => this._checkCacheJSON(o)); if (options.setStatus) { options.setStatus("Processing " + objectTypePlural); // TODO: localize @@ -435,67 +425,79 @@ Zotero.Sync.Data.Local = { // Sort parent objects first, to avoid retries due to unmet dependencies if (objectType == 'item' || objectType == 'collection') { let parentProp = 'parent' + objectType[0].toUpperCase() + objectType.substr(1); - data.sort(function (a, b) { + json.sort(function (a, b) { if (a[parentProp] && !b[parentProp]) return 1; if (b[parentProp] && !a[parentProp]) return -1; return 0; }); } - var concurrentObjects = 5; - yield Zotero.Utilities.Internal.forEachChunkAsync( - data, - concurrentObjects, - function (chunk) { - return Zotero.DB.executeTransaction(function* () { - for (let i = 0; i < chunk.length; i++) { - let json = chunk[i]; - let jsonData = json.data; - let objectKey = json.key; - - Zotero.debug(`Processing ${objectType} ${libraryID}/${objectKey}`); - Zotero.debug(json); - - if (!jsonData) { - Zotero.logError(new Error("Missing 'data' object in JSON in sync cache for " - + objectType + " " + libraryID + "/" + objectKey)); + var batchSize = 10; + var batchCounter = 0; + try { + for (let i = 0; i < json.length; i++) { + // Batch notifier updates + if (batchCounter == 0) { + Zotero.Notifier.begin(); + } + else if (batchCounter == batchSize || i == json.length - 1) { + Zotero.Notifier.commit(); + Zotero.Notifier.begin(); + } + + let jsonObject = json[i]; + let jsonData = jsonObject.data; + let objectKey = jsonObject.key; + + let saveOptions = {}; + Object.assign(saveOptions, options); + saveOptions.isNewObject = false; + saveOptions.skipCache = false; + saveOptions.storageDetailsChanged = false; + + Zotero.debug(`Processing ${objectType} ${libraryID}/${objectKey}`); + Zotero.debug(jsonObject); + + // Skip objects with unmet dependencies + if (objectType == 'item' || objectType == 'collection') { + let parentProp = 'parent' + objectType[0].toUpperCase() + objectType.substr(1); + let parentKey = jsonData[parentProp]; + if (parentKey) { + let parentObj = yield objectsClass.getByLibraryAndKeyAsync( + libraryID, parentKey, { noCache: true } + ); + if (!parentObj) { + let error = new Error("Parent of " + objectType + " " + + libraryID + "/" + jsonData.key + " not found -- skipping"); + error.name = "ZoteroMissingObjectError"; + Zotero.debug(error.message); + results.push({ + key: objectKey, + processed: false, + error, + retry: true + }); continue; } - - // Skip objects with unmet dependencies - if (objectType == 'item' || objectType == 'collection') { - let parentProp = 'parent' + objectType[0].toUpperCase() + objectType.substr(1); - let parentKey = jsonData[parentProp]; - if (parentKey) { - let parentObj = yield objectsClass.getByLibraryAndKeyAsync( - libraryID, parentKey, { noCache: true } - ); - if (!parentObj) { - Zotero.debug("Parent of " + objectType + " " - + libraryID + "/" + jsonData.key + " not found -- skipping"); - // TEMP: Add parent to a queue, in case it's somehow missing - // after retrieving all objects? - numSkipped++; - continue; - } + } + + /*if (objectType == 'item') { + for (let j = 0; j < jsonData.collections.length; i++) { + let parentKey = jsonData.collections[j]; + let parentCollection = Zotero.Collections.getByLibraryAndKey( + libraryID, parentKey, { noCache: true } + ); + if (!parentCollection) { + // ??? } - - /*if (objectType == 'item') { - for (let j = 0; j < jsonData.collections.length; i++) { - let parentKey = jsonData.collections[j]; - let parentCollection = Zotero.Collections.getByLibraryAndKey( - libraryID, parentKey, { noCache: true } - ); - if (!parentCollection) { - // ??? - } - } - }*/ } - - let isNewObject = false; - let skipCache = false; - let storageDetailsChanged = false; + }*/ + } + + // Errors have to be thrown in order to roll back the transaction, so catch those here + // and continue + try { + yield Zotero.DB.executeTransaction(function* () { let obj = yield objectsClass.getByLibraryAndKeyAsync( libraryID, objectKey, { noCache: true } ); @@ -519,7 +521,7 @@ Zotero.Sync.Data.Local = { if (objectType == 'item' && obj.isImportedAttachment()) { if (jsonDataLocal.mtime != jsonData.mtime || jsonDataLocal.md5 != jsonData.md5) { - storageDetailsChanged = true; + saveOptions.storageDetailsChanged = true; } } @@ -535,19 +537,20 @@ Zotero.Sync.Data.Local = { if (!result.changes.length && !result.conflicts.length) { Zotero.debug("No remote changes to apply to local " + objectType + " " + obj.libraryKey); - obj.version = json.version; - obj.synced = true; - yield obj.save(); - if (objectType == 'item') { - yield this._onItemProcessed( - obj, - jsonData, - isNewObject, - storageDetailsChanged - ); + let saveResults = yield this._saveObjectFromJSON( + obj, + jsonObject, + { + skipData: true + } + ); + results.push(saveResults); + if (!saveResults.processed) { + throw saveResults.error; } - continue; + batchCounter++; + return; } if (result.conflicts.length) { @@ -564,7 +567,7 @@ Zotero.Sync.Data.Local = { changes: result.changes, conflicts: result.conflicts }); - continue; + return; } // If no conflicts, apply remote changes automatically @@ -606,7 +609,7 @@ Zotero.Sync.Data.Local = { }, right: jsonData }); - continue; + return; // Auto-restore some locally deleted objects that have changed remotely case 'collection': @@ -632,31 +635,56 @@ Zotero.Sync.Data.Local = { yield obj.loadPrimaryData(); // Don't cache new items immediately, which skips reloading after save - skipCache = true; + saveOptions.skipCache = true; } - let saved = yield this._saveObjectFromJSON( - obj, jsonData, options, { skipCache } - ); - if (saved) { - if (objectType == 'item') { - yield this._onItemProcessed( - obj, - jsonData, - isNewObject, - storageDetailsChanged - ); - } - } - - if (saved) { - numSaved++; + let saveResults = yield this._saveObjectFromJSON(obj, jsonObject, saveOptions); + results.push(saveResults); + if (!saveResults.processed) { + throw saveResults.error; } + batchCounter++; + }.bind(this)); + } + catch (e) { + // Display nicer debug line for known errors + if (knownErrors.indexOf(e.name) != -1) { + let desc = e.name + .replace(/^Zotero/, "") + // Convert "MissingObjectError" to "missing object error" + .split(/([a-z]+)/).join(' ').trim() + .replace(/([A-Z]) ([a-z]+)/g, "$1$2").toLowerCase(); + let msg = Zotero.Utilities.capitalize(desc) + " for " + + `${objectType} ${jsonObject.key} in ${Zotero.Libraries.get(libraryID).name}`; + Zotero.debug(msg, 2); + Zotero.debug(e, 2); + Components.utils.reportError(msg + ": " + e.message); } - }.bind(this)); - }.bind(this) - ); + else { + Zotero.logError(e); + } + + if (options.onError) { + options.onError(e); + } + + if (options.stopOnError) { + throw e; + } + } + } + } + catch (e) { + Zotero.Notifier.reset(); + throw e; + } + finally { + Zotero.Notifier.commit(); + } + // + // Conflict resolution + // if (conflicts.length) { // Sort conflicts by local Date Modified/Deleted conflicts.sort(function (a, b) { @@ -674,79 +702,163 @@ Zotero.Sync.Data.Local = { var mergeData = this.resolveConflicts(conflicts); if (mergeData) { Zotero.debug("Processing resolved conflicts"); - let mergeOptions = {}; - Object.assign(mergeOptions, options); - // Tell _saveObjectFromJSON not to save with 'synced' set to true - mergeOptions.saveAsChanged = true; - let concurrentObjects = 50; - yield Zotero.Utilities.Internal.forEachChunkAsync( - mergeData, - concurrentObjects, - function (chunk) { - return Zotero.DB.executeTransaction(function* () { - for (let json of chunk) { + + let batchSize = 50; + let batchCounter = 0; + try { + for (let i = 0; i < mergeData.length; i++) { + // Batch notifier updates + if (batchCounter == 0) { + Zotero.Notifier.begin(); + } + else if (batchCounter == batchSize || i == json.length - 1) { + Zotero.Notifier.commit(); + Zotero.Notifier.begin(); + } + + let json = mergeData[i]; + + let saveOptions = {}; + Object.assign(saveOptions, options); + // Tell _saveObjectFromJSON to save as unsynced + saveOptions.saveAsChanged = true; + + // Errors have to be thrown in order to roll back the transaction, so catch + // those here and continue + try { + yield Zotero.DB.executeTransaction(function* () { let obj = yield objectsClass.getByLibraryAndKeyAsync( libraryID, json.key, { noCache: true } ); // Update object with merge data if (obj) { + // Delete local object if (json.deleted) { - yield obj.erase(); - } - else { - yield this._saveObjectFromJSON(obj, json, mergeOptions); + try { + yield obj.erase(); + } + catch (e) { + results.push({ + key: json.key, + processed: false, + error: e, + retry: false + }); + throw e; + } + results.push({ + key: json.key, + processed: true + }); + return; } + + // Save merged changes below } - // Recreate deleted object - else if (!json.deleted) { + // If no local object and merge wanted a delete, we're good + else if (json.deleted) { + results.push({ + key: json.key, + processed: true + }); + return; + } + // Recreate locally deleted object + else { obj = new Zotero[ObjectType]; obj.libraryID = libraryID; obj.key = json.key; yield obj.loadPrimaryData(); - let saved = yield this._saveObjectFromJSON(obj, json, options, { - // Don't cache new items immediately, which skips reloading after save - skipCache: true - }); + // Don't cache new items immediately, + // which skips reloading after save + saveOptions.skipCache = true; } + + let saveResults = yield this._saveObjectFromJSON( + obj, json, saveOptions + ); + results.push(saveResults); + if (!saveResults.processed) { + throw saveResults.error; + } + + }.bind(this)); + } + catch (e) { + Zotero.logError(e); + + if (options.onError) { + options.onError(e); } - }.bind(this)); - }.bind(this) - ); + + if (options.stopOnError) { + throw e; + } + } + } + } + catch (e) { + Zotero.Notifier.reset(); + } + finally { + Zotero.Notifier.commit(); + } } } - // Keep retrying if we skipped any, as long as we're still making progress - if (numSkipped && numSaved != 0) { - Zotero.debug("More " + objectTypePlural + " in cache -- continuing"); - return this.processSyncCacheForObjectType(libraryID, objectType, options); - } + let processed = 0; + let skipped = 0; + results.forEach(x => x.processed ? processed++ : skipped++); - data = yield this._getUnwrittenData(libraryID, objectType); - if (data.length) { - Zotero.debug(`Skipping ${data.length} ` - + (data.length == 1 ? objectType : objectTypePlural) - + " in sync cache"); - } + Zotero.debug(`Processed ${processed} ` + + (processed == 1 ? objectType : objectTypePlural) + + (skipped ? ` and skipped ${skipped}` : "") + + " in " + libraryName); + + return results; }), - _onItemProcessed: Zotero.Promise.coroutine(function* (item, jsonData, isNewObject, storageDetailsChanged) { - // Delete older versions of the item in the cache + _checkCacheJSON: function (json) { + if (json.key === undefined) { + Zotero.debug(json, 1); + throw new Error("Missing 'key' property in JSON"); + } + if (json.version === undefined) { + Zotero.debug(json, 1); + throw new Error("Missing 'version' property in JSON"); + } + // If direct data object passed, wrap in fake response object + return json.data === undefined ? { + key: json.key, + version: json.version, + data: json + } : json; + }, + + + _onObjectProcessed: Zotero.Promise.coroutine(function* (obj, jsonObject, isNewObject, storageDetailsChanged) { + var jsonData = jsonObject.data; + + // Delete older versions of the object in the cache yield this.deleteCacheObjectVersions( - 'item', item.libraryID, jsonData.key, null, jsonData.version - 1 + obj.objectType, obj.libraryID, jsonData.key, null, jsonData.version - 1 ); + // Delete from sync queue + yield this._removeObjectFromSyncQueue(obj.objectType, obj.libraryID, jsonData.key); + // Mark updated attachments for download - if (item.isImportedAttachment()) { + if (obj.objectType == 'item' && obj.isImportedAttachment()) { // If storage changes were made (attachment mtime or hash), mark // library as requiring download if (isNewObject || storageDetailsChanged) { - Zotero.Libraries.get(item.libraryID).storageDownloadNeeded = true; + Zotero.Libraries.get(obj.libraryID).storageDownloadNeeded = true; } yield this._checkAttachmentForDownload( - item, jsonData.mtime, isNewObject + obj, jsonData.mtime, isNewObject ); } }), @@ -811,7 +923,7 @@ Zotero.Sync.Data.Local = { sql += " AND version>=?"; params.push(minVersion); } - if (maxVersion) { + if (maxVersion || maxVersion === 0) { sql += " AND version<=?"; params.push(maxVersion); } @@ -873,50 +985,44 @@ Zotero.Sync.Data.Local = { _saveObjectFromJSON: Zotero.Promise.coroutine(function* (obj, json, options) { try { - obj.fromJSON(json); + json = this._checkCacheJSON(json); + var results = { + key: json.key + }; + if (!options.skipData) { + obj.fromJSON(json.data); + } + obj.version = json.data.version; if (!options.saveAsChanged) { - obj.version = json.version; obj.synced = true; } - Zotero.debug("SAVING " + json.key + " WITH SYNCED"); - Zotero.debug(obj.version); yield obj.save({ skipEditCheck: true, skipDateModifiedUpdate: true, skipSelect: true, + skipCache: options.skipCache || false, + // Errors are logged elsewhere, so skip in DataObject.save() errorHandler: function (e) { - // Don't log expected errors - if (e.name == 'ZoteroUnknownTypeError' - && e.name == 'ZoteroUnknownFieldError' - && e.name == 'ZoteroMissingObjectError') { - return; - } - Zotero.debug(e, 1); + return; } }); + yield this.saveCacheObject(obj.objectType, obj.libraryID, json.data); + results.processed = true; + + yield this._onObjectProcessed( + obj, + json, + options.isNewObject, + options.storageDetailsChanged + ); } catch (e) { - if (e.name == 'ZoteroUnknownTypeError' - || e.name == 'ZoteroUnknownFieldError' - || e.name == 'ZoteroMissingObjectError') { - let desc = e.name - .replace(/^Zotero/, "") - // Convert "MissingObjectError" to "missing object error" - .split(/([a-z]+)/).join(' ').trim() - .replace(/([A-Z]) ([a-z]+)/g, "$1$2").toLowerCase(); - Zotero.logError("Ignoring " + desc + " for " - + obj.objectType + " " + obj.libraryKey, 2); - } - else if (options.stopOnError) { - throw e; - } - else { - Zotero.logError(e); - options.onError(e); - } - return false; + // For now, allow sync to proceed after all errors + results.processed = false; + results.error = e; + results.retry = false; } - return true; + return results; }), @@ -1094,26 +1200,6 @@ Zotero.Sync.Data.Local = { }, - /** - * @return {Promise} A promise for an array of JSON objects - */ - _getUnwrittenData: function (libraryID, objectType, max) { - var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(objectType); - // The MAX(version) ensures we get the data from the most recent version of the object, - // thanks to SQLite 3.7.11 (http://www.sqlite.org/releaselog/3_7_11.html) - var sql = "SELECT data, MAX(SC.version) AS version FROM syncCache SC " - + "LEFT JOIN " + objectsClass.table + " O " - + "USING (libraryID, key) " - + "WHERE SC.libraryID=? AND " - + "syncObjectTypeID IN (SELECT syncObjectTypeID FROM " - + "syncObjectTypes WHERE name='" + objectType + "') " - // If saved version doesn't have a version or is less than the cache version - + "AND IFNULL(O.version, 0) < SC.version " - + "GROUP BY SC.libraryID, SC.key"; - return Zotero.DB.queryAsync(sql, [libraryID]).map(row => JSON.parse(row.data)); - }, - - markObjectAsSynced: Zotero.Promise.method(function (obj) { obj.synced = true; return obj.saveTx({ @@ -1176,5 +1262,127 @@ Zotero.Sync.Data.Local = { }) ); }.bind(this)); + }, + + + addObjectsToSyncQueue: Zotero.Promise.coroutine(function* (objectType, libraryID, keys) { + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + var now = Zotero.Date.getUnixTimestamp(); + + // Default to first try + var keyTries = {}; + keys.forEach(key => keyTries[key] = 0); + + // Check current try counts + var sql = "SELECT key, tries FROM syncQueue WHERE "; + yield Zotero.Utilities.Internal.forEachChunkAsync( + keys, + Math.floor(Zotero.DB.MAX_BOUND_PARAMETERS / 3), + Zotero.Promise.coroutine(function* (chunk) { + var params = chunk.reduce( + (arr, key) => arr.concat([libraryID, key, syncObjectTypeID]), [] + ); + var rows = yield Zotero.DB.queryAsync( + sql + Array(chunk.length) + .fill('(libraryID=? AND key=? AND syncObjectTypeID=?)') + .join(' OR '), + params + ); + for (let row of rows) { + keyTries[row.key] = row.tries + 1; // increment current count + } + }) + ); + + // Insert or update + yield Zotero.DB.executeTransaction(function* () { + var sql = "INSERT OR REPLACE INTO syncQueue " + + "(libraryID, key, syncObjectTypeID, lastCheck, tries) VALUES "; + return Zotero.Utilities.Internal.forEachChunkAsync( + keys, + Math.floor(Zotero.DB.MAX_BOUND_PARAMETERS / 3), + function (chunk) { + var params = chunk.reduce( + (arr, key) => arr.concat( + [libraryID, key, syncObjectTypeID, now, keyTries[key]] + ), [] + ); + return Zotero.DB.queryAsync( + sql + Array(chunk.length).fill('(?, ?, ?, ?, ?)').join(', '), params + ); + } + ); + }.bind(this)); + }), + + + getObjectsFromSyncQueue: function (objectType, libraryID) { + return Zotero.DB.columnQueryAsync( + "SELECT key FROM syncQueue WHERE libraryID=? AND " + + "syncObjectTypeID IN (SELECT syncObjectTypeID FROM syncObjectTypes WHERE name=?)", + [libraryID, objectType] + ); + }, + + + getObjectsToTryFromSyncQueue: Zotero.Promise.coroutine(function* (objectType, libraryID) { + var rows = yield Zotero.DB.queryAsync( + "SELECT key, lastCheck, tries FROM syncQueue WHERE libraryID=? AND " + + "syncObjectTypeID IN (SELECT syncObjectTypeID FROM syncObjectTypes WHERE name=?)", + [libraryID, objectType] + ); + var keysToTry = []; + for (let row of rows) { + let interval = this._syncQueueIntervals[row.tries]; + // Keep using last interval if beyond + if (!interval) { + interval = this._syncQueueIntervals[this._syncQueueIntervals.length - 1]; + } + let nextCheck = row.lastCheck + interval * 60 * 60; + if (nextCheck <= Zotero.Date.getUnixTimestamp()) { + keysToTry.push(row.key); + } + } + return keysToTry; + }), + + + removeObjectsFromSyncQueue: function (objectType, libraryID, keys) { + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + var sql = "DELETE FROM syncQueue WHERE libraryID=? AND syncObjectTypeID=? AND key IN ("; + return Zotero.DB.executeTransaction(function* () { + return Zotero.Utilities.Internal.forEachChunkAsync( + keys, + Zotero.DB.MAX_BOUND_PARAMETERS - 2, + Zotero.Promise.coroutine(function* (chunk) { + var params = [libraryID, syncObjectTypeID].concat(chunk); + return Zotero.DB.queryAsync( + sql + Array(chunk.length).fill('?').join(',') + ")", params + ); + }) + ); + }.bind(this)); + }, + + + _removeObjectFromSyncQueue: function (objectType, libraryID, key) { + return Zotero.DB.queryAsync( + "DELETE FROM syncQueue WHERE libraryID=? AND key=? AND syncObjectTypeID=?", + [ + libraryID, + key, + Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType) + ] + ); + }, + + + resetSyncQueue: function () { + return Zotero.DB.queryAsync("DELETE FROM syncQueue"); + }, + + + resetSyncQueueTries: function () { + return Zotero.DB.queryAsync("UPDATE syncQueue SET tries=0"); } } diff --git a/chrome/content/zotero/xpcom/sync/syncRunner.js b/chrome/content/zotero/xpcom/sync/syncRunner.js index baefc82594..d6a45e0900 100644 --- a/chrome/content/zotero/xpcom/sync/syncRunner.js +++ b/chrome/content/zotero/xpcom/sync/syncRunner.js @@ -31,7 +31,7 @@ if (!Zotero.Sync) { // Initialized as Zotero.Sync.Runner in zotero.js Zotero.Sync.Runner_Module = function (options = {}) { - const stopOnError = true; + const stopOnError = false; Zotero.defineProperty(this, 'background', { get: () => _background }); Zotero.defineProperty(this, 'lastSyncStatus', { get: () => _lastSyncStatus }); diff --git a/resource/schema/userdata.sql b/resource/schema/userdata.sql index c388b6b6aa..d17fa29941 100644 --- a/resource/schema/userdata.sql +++ b/resource/schema/userdata.sql @@ -1,4 +1,4 @@ --- 83 +-- 84 -- Copyright (c) 2009 Center for History and New Media -- George Mason University, Fairfax, Virginia, USA @@ -328,6 +328,17 @@ CREATE TABLE syncDeleteLog ( FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE ); +CREATE TABLE syncQueue ( + libraryID INT NOT NULL, + key TEXT NOT NULL, + syncObjectTypeID INT NOT NULL, + lastCheck TIMESTAMP, + tries INT, + PRIMARY KEY (libraryID, key, syncObjectTypeID), + FOREIGN KEY (libraryID) REFERENCES libraries(libraryID) ON DELETE CASCADE, + FOREIGN KEY (syncObjectTypeID) REFERENCES syncObjectTypes(syncObjectTypeID) ON DELETE CASCADE +); + CREATE TABLE storageDeleteLog ( libraryID INT NOT NULL, key TEXT NOT NULL, diff --git a/test/tests/schemaTest.js b/test/tests/schemaTest.js index dc1dcfcb35..06e9f407ca 100644 --- a/test/tests/schemaTest.js +++ b/test/tests/schemaTest.js @@ -1,4 +1,30 @@ describe("Zotero.Schema", function() { + describe("#initializeSchema()", function () { + it("should set last client version", function* () { + yield resetDB({ + thisArg: this, + skipBundledFiles: true + }); + + var sql = "SELECT value FROM settings WHERE setting='client' AND key='lastVersion'"; + var lastVersion = yield Zotero.DB.valueQueryAsync(sql); + yield assert.eventually.equal(Zotero.DB.valueQueryAsync(sql), Zotero.version); + }); + }); + + describe("#updateSchema()", function () { + it("should set last client version", function* () { + var sql = "REPLACE INTO settings (setting, key, value) VALUES ('client', 'lastVersion', ?)"; + return Zotero.DB.queryAsync(sql, "5.0old"); + + yield Zotero.Schema.updateSchema(); + + var sql = "SELECT value FROM settings WHERE setting='client' AND key='lastVersion'"; + var lastVersion = yield Zotero.DB.valueQueryAsync(sql); + yield assert.eventually.equal(Zotero.DB.valueQueryAsync(sql), Zotero.version); + }); + }); + describe("#integrityCheck()", function () { before(function* () { yield resetDB({ diff --git a/test/tests/storageLocalTest.js b/test/tests/storageLocalTest.js index 741941a209..d051c08d22 100644 --- a/test/tests/storageLocalTest.js +++ b/test/tests/storageLocalTest.js @@ -144,12 +144,8 @@ describe("Zotero.Sync.Storage.Local", function () { md5, mtime }; - yield Zotero.Sync.Data.Local.saveCacheObjects( - 'item', libraryID, [json] - ); - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, 'item', { stopOnError: true } - ); + yield Zotero.Sync.Data.Local.processObjectsFromJSON('item', libraryID, [json]); + var item = yield Zotero.Items.getByLibraryAndKeyAsync(libraryID, key); yield Zotero.Sync.Storage.Local.processDownload({ item, diff --git a/test/tests/syncAPIClientTest.js b/test/tests/syncAPIClientTest.js index ef0b5c808b..bdf21c2127 100644 --- a/test/tests/syncAPIClientTest.js +++ b/test/tests/syncAPIClientTest.js @@ -12,6 +12,10 @@ describe("Zotero.Sync.APIClient", function () { } before(function () { + Zotero.HTTP.mock = sinon.FakeXMLHttpRequest; + }); + + beforeEach(function () { Components.utils.import("resource://zotero/concurrentCaller.js"); var caller = new ConcurrentCaller(1); caller.setLogger(msg => Zotero.debug(msg)); @@ -24,17 +28,13 @@ describe("Zotero.Sync.APIClient", function () { } }; - Zotero.HTTP.mock = sinon.FakeXMLHttpRequest; - client = new Zotero.Sync.APIClient({ baseURL, apiVersion: ZOTERO_CONFIG.API_VERSION, apiKey, caller }) - }) - - beforeEach(function () { + server = sinon.fakeServer.create(); server.autoRespond = true; }) @@ -44,16 +44,29 @@ describe("Zotero.Sync.APIClient", function () { }) describe("#_checkConnection()", function () { - it("should catch an error with an empty response", function* () { + var spy; + + beforeEach(function () { + client.failureDelayIntervals = [10]; + client.failureDelayMax = 15; + }); + afterEach(function () { + if (spy) { + spy.restore(); + } + }); + + it("should retry on 500 error", function* () { setResponse({ method: "GET", url: "error", status: 500, text: "" }) + var spy = sinon.spy(Zotero.HTTP, "request"); var e = yield getPromiseError(client.makeRequest("GET", baseURL + "error")); - assert.ok(e); - assert.isTrue(e.message.startsWith(Zotero.getString('sync.error.emptyResponseServer'))); + assert.instanceOf(e, Zotero.HTTP.UnexpectedStatusException); + assert.isTrue(spy.calledTwice); }) it("should catch an interrupted connection", function* () { diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js index 019060b7d1..2fc2ba5ee1 100644 --- a/test/tests/syncEngineTest.js +++ b/test/tests/syncEngineTest.js @@ -46,7 +46,8 @@ describe("Zotero.Sync.Data.Engine", function () { data: { key: options.key, version: options.version, - name: options.name + name: options.name, + parentCollection: options.parentCollection } }; } @@ -93,6 +94,14 @@ describe("Zotero.Sync.Data.Engine", function () { item: makeItemJSON }; + var assertInCache = Zotero.Promise.coroutine(function* (obj) { + var cacheObject = yield Zotero.Sync.Data.Local.getCacheObject( + obj.objectType, obj.libraryID, obj.key, obj.version + ); + assert.isObject(cacheObject); + assert.propertyVal(cacheObject, 'key', obj.key); + }); + // // Tests // @@ -250,23 +259,27 @@ describe("Zotero.Sync.Data.Engine", function () { assert.equal(obj.name, 'A'); assert.equal(obj.version, 1); assert.isTrue(obj.synced); + yield assertInCache(obj); obj = yield Zotero.Searches.getByLibraryAndKeyAsync(userLibraryID, "AAAAAAAA"); assert.equal(obj.name, 'A'); assert.equal(obj.version, 2); assert.isTrue(obj.synced); + yield assertInCache(obj); obj = yield Zotero.Items.getByLibraryAndKeyAsync(userLibraryID, "AAAAAAAA"); assert.equal(obj.getField('title'), 'A'); assert.equal(obj.version, 3); assert.isTrue(obj.synced); var parentItemID = obj.id; + yield assertInCache(obj); obj = yield Zotero.Items.getByLibraryAndKeyAsync(userLibraryID, "BBBBBBBB"); assert.equal(obj.getNote(), 'This is a note.'); assert.equal(obj.parentItemID, parentItemID); assert.equal(obj.version, 3); assert.isTrue(obj.synced); + yield assertInCache(obj); }) it("should upload new full items and subsequent patches", function* () { @@ -651,10 +664,211 @@ describe("Zotero.Sync.Data.Engine", function () { }); yield engine.start(); }) + + it("should ignore errors when saving downloaded objects", function* () { + ({ engine, client, caller } = yield setup()); + engine.stopOnError = false; + + var headers = { + "Last-Modified-Version": 3 + }; + setResponse({ + method: "GET", + url: "users/1/settings", + status: 200, + headers: headers, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/collections?format=versions", + status: 200, + headers: headers, + json: { + "AAAAAAAA": 1, + "BBBBBBBB": 1, + "CCCCCCCC": 1 + } + }); + setResponse({ + method: "GET", + url: "users/1/searches?format=versions", + status: 200, + headers: headers, + json: { + "DDDDDDDD": 2, + "EEEEEEEE": 2, + "FFFFFFFF": 2 + } + }); + setResponse({ + method: "GET", + url: "users/1/items/top?format=versions&includeTrashed=1", + status: 200, + headers: headers, + json: { + "GGGGGGGG": 3, + "HHHHHHHH": 3 + } + }); + setResponse({ + method: "GET", + url: "users/1/items?format=versions&includeTrashed=1", + status: 200, + headers: headers, + json: { + "GGGGGGGG": 3, + "HHHHHHHH": 3, + "JJJJJJJJ": 3 + } + }); + setResponse({ + method: "GET", + url: "users/1/collections?format=json&collectionKey=AAAAAAAA%2CBBBBBBBB%2CCCCCCCCC", + status: 200, + headers: headers, + json: [ + makeCollectionJSON({ + key: "AAAAAAAA", + version: 1, + name: "A" + }), + makeCollectionJSON({ + key: "BBBBBBBB", + version: 1, + name: "B", + // Missing parent -- collection should be queued + parentCollection: "ZZZZZZZZ" + }), + makeCollectionJSON({ + key: "CCCCCCCC", + version: 1, + name: "C", + // Unknown field -- should be ignored + unknownField: 5 + }) + ] + }); + setResponse({ + method: "GET", + url: "users/1/searches?format=json&searchKey=DDDDDDDD%2CEEEEEEEE%2CFFFFFFFF", + status: 200, + headers: headers, + json: [ + makeSearchJSON({ + key: "DDDDDDDD", + version: 2, + name: "D", + conditions: [ + { + condition: "title", + operator: "is", + value: "a" + } + ] + }), + makeSearchJSON({ + key: "EEEEEEEE", + version: 2, + name: "E", + conditions: [ + { + // Unknown search condition -- search should be queued + condition: "unknownCondition", + operator: "is", + value: "a" + } + ] + }), + makeSearchJSON({ + key: "FFFFFFFF", + version: 2, + name: "F", + conditions: [ + { + condition: "title", + // Unknown search operator -- search should be queued + operator: "unknownOperator", + value: "a" + } + ] + }) + ] + }); + setResponse({ + method: "GET", + url: "users/1/items?format=json&itemKey=GGGGGGGG%2CHHHHHHHH&includeTrashed=1", + status: 200, + headers: headers, + json: [ + makeItemJSON({ + key: "GGGGGGGG", + version: 3, + itemType: "book", + title: "G", + // Unknown item field -- should be ignored + unknownField: "B" + }), + makeItemJSON({ + key: "HHHHHHHH", + version: 3, + // Unknown item type -- item should be queued + itemType: "unknownItemType", + title: "H" + }) + ] + }); + setResponse({ + method: "GET", + url: "users/1/items?format=json&itemKey=JJJJJJJJ&includeTrashed=1", + status: 200, + headers: headers, + json: [ + makeItemJSON({ + key: "JJJJJJJJ", + version: 3, + itemType: "note", + // Parent that couldn't be saved -- item should be queued + parentItem: "HHHHHHHH", + note: "This is a note." + }) + ] + }); + setResponse({ + method: "GET", + url: "users/1/deleted?since=0", + status: 200, + headers: headers, + json: {} + }); + var spy = sinon.spy(engine, "onError"); + yield engine.start(); + + var userLibraryID = Zotero.Libraries.userLibraryID; + + // Library version should have been updated + assert.equal(Zotero.Libraries.getVersion(userLibraryID), 3); + + // Check for saved objects + yield assert.eventually.ok(Zotero.Collections.getByLibraryAndKeyAsync(userLibraryID, "AAAAAAAA")); + yield assert.eventually.ok(Zotero.Searches.getByLibraryAndKeyAsync(userLibraryID, "DDDDDDDD")); + yield assert.eventually.ok(Zotero.Items.getByLibraryAndKeyAsync(userLibraryID, "GGGGGGGG")); + + var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('collection', userLibraryID); + assert.sameMembers(keys, ['BBBBBBBB']); + + var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('search', userLibraryID); + assert.sameMembers(keys, ['EEEEEEEE', 'FFFFFFFF']); + + var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue('item', userLibraryID); + assert.sameMembers(keys, ['HHHHHHHH', 'JJJJJJJJ']); + + assert.equal(spy.callCount, 3); + }); }) describe("#_startDownload()", function () { - it("shouldn't redownload objects already in the cache", function* () { + it("shouldn't redownload objects that are already up to date", function* () { var userLibraryID = Zotero.Libraries.userLibraryID; //yield Zotero.Libraries.setVersion(userLibraryID, 5); ({ engine, client, caller } = yield setup()); @@ -1230,6 +1444,7 @@ describe("Zotero.Sync.Data.Engine", function () { let obj = objectsClass.getByLibraryAndKey(userLibraryID, objectJSON[type][0].key); assert.equal(obj.version, 20); assert.isTrue(obj.synced); + yield assertInCache(obj); // JSON objects 2 should be marked as unsynced, with their version reset to 0 assert.equal(objects[type][1].version, 0); diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index 3f5b82c7b5..d7fbe2124d 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -89,10 +89,10 @@ describe("Zotero.Sync.Data.Local", function() { }) - describe("#processSyncCacheForObjectType()", function () { + describe("#processObjectsFromJSON()", function () { var types = Zotero.DataObjectUtilities.getTypes(); - before(function* () { + beforeEach(function* () { yield resetDB({ thisArg: this, skipBundledFiles: true @@ -113,11 +113,8 @@ describe("Zotero.Sync.Data.Local", function() { version: 10, data: data }; - yield Zotero.Sync.Data.Local.saveCacheObjects( - type, libraryID, [json] - ); - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, [json], { stopOnError: true } ); let localObj = objectsClass.getByLibraryAndKey(libraryID, obj.key); assert.equal(localObj.version, 10); @@ -148,12 +145,8 @@ describe("Zotero.Sync.Data.Local", function() { version: 10, data: data }; - yield Zotero.Sync.Data.Local.saveCacheObjects( - type, libraryID, [json] - ); - - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, [json], { stopOnError: true } ); assert.equal(obj.version, 10); assert.equal(obj.getField('title'), changedTitle); @@ -164,23 +157,19 @@ describe("Zotero.Sync.Data.Local", function() { var libraryID = Zotero.Libraries.userLibraryID; for (let type of types) { + let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); + + // Save original version let obj = yield createDataObject(type, { version: 5 }); let data = obj.toJSON(); yield Zotero.Sync.Data.Local.saveCacheObjects( type, libraryID, [data] ); - } - - for (let type of types) { - let objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); - let obj = yield createDataObject(type, { version: 10 }); - let data = obj.toJSON(); - yield Zotero.Sync.Data.Local.saveCacheObjects( - type, libraryID, [data] - ); - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + // Save newer version + data.version = 10; + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, [data], { stopOnError: true } ); let localObj = objectsClass.getByLibraryAndKey(libraryID, obj.key); @@ -195,7 +184,36 @@ describe("Zotero.Sync.Data.Local", function() { "should have only latest version of " + type + " in cache" ); } - }) + }); + + it("should delete object from sync queue after processing", function* () { + var objectType = 'item'; + var libraryID = Zotero.Libraries.userLibraryID; + var key = Zotero.DataObjectUtilities.generateKey(); + + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(objectType, libraryID, [key]); + + var versions = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, libraryID); + assert.include(versions, key); + + var json = { + key, + version: 10, + data: { + key, + version: 10, + itemType: "book", + title: "Test" + } + }; + + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + objectType, libraryID, [json], { stopOnError: true } + ); + + var versions = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, libraryID); + assert.notInclude(versions, key); + }); it("should mark new attachment items for download", function* () { var libraryID = Zotero.Libraries.userLibraryID; @@ -216,11 +234,8 @@ describe("Zotero.Sync.Data.Local", function() { } }; - yield Zotero.Sync.Data.Local.saveCacheObjects( - 'item', Zotero.Libraries.userLibraryID, [json] - ); - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, 'item', { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + 'item', libraryID, [json], { stopOnError: true } ); var item = Zotero.Items.getByLibraryAndKey(libraryID, key); assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD); @@ -247,12 +262,8 @@ describe("Zotero.Sync.Data.Local", function() { json.data.version = 10; json.data.md5 = '57f8a4fda823187b91e1191487b87fe6'; json.data.mtime = new Date().getTime() + 10000; - yield Zotero.Sync.Data.Local.saveCacheObjects( - 'item', Zotero.Libraries.userLibraryID, [json] - ); - - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, 'item', { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + 'item', libraryID, [json], { stopOnError: true } ); assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD); @@ -284,17 +295,188 @@ describe("Zotero.Sync.Data.Local", function() { json.data.version = 10; json.data.md5 = '57f8a4fda823187b91e1191487b87fe6'; json.data.mtime = new Date().getTime() + 10000; - yield Zotero.Sync.Data.Local.saveCacheObjects('item', libraryID, [json]); - - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, 'item', { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + 'item', libraryID, [json], { stopOnError: true } ); assert.equal(item.getField('title'), newTitle); assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_DOWNLOAD); }) + + it("should roll back partial object changes on error", function* () { + var libraryID = Zotero.Libraries.publicationsLibraryID; + var key1 = "AAAAAAAA"; + var key2 = "BBBBBBBB"; + var json = [ + { + key: key1, + version: 1, + data: { + key: key1, + version: 1, + itemType: "book", + title: "Test A" + } + }, + { + key: key2, + version: 1, + data: { + key: key2, + version: 1, + itemType: "journalArticle", + title: "Test B", + deleted: true // Not allowed in My Publications + } + } + ]; + yield Zotero.Sync.Data.Local.processObjectsFromJSON('item', libraryID, json); + + // Shouldn't roll back the successful item + yield assert.eventually.equal(Zotero.DB.valueQueryAsync( + "SELECT COUNT(*) FROM items WHERE libraryID=? AND key=?", [libraryID, key1] + ), 1); + // Should rollback the unsuccessful item + yield assert.eventually.equal(Zotero.DB.valueQueryAsync( + "SELECT COUNT(*) FROM items WHERE libraryID=? AND key=?", [libraryID, key2] + ), 0); + }); }) + describe("Sync Queue", function () { + var lib1, lib2; + + before(function* () { + lib1 = Zotero.Libraries.userLibraryID; + lib2 = Zotero.Libraries.publicationsLibraryID; + }); + + beforeEach(function* () { + yield Zotero.DB.queryAsync("DELETE FROM syncQueue"); + }); + + after(function* () { + yield Zotero.DB.queryAsync("DELETE FROM syncQueue"); + }); + + describe("#addObjectsToSyncQueue()", function () { + it("should add new objects and update lastCheck and tries for existing objects", function* () { + var objectType = 'item'; + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + var now = Zotero.Date.getUnixTimestamp(); + var key1 = Zotero.DataObjectUtilities.generateKey(); + var key2 = Zotero.DataObjectUtilities.generateKey(); + var key3 = Zotero.DataObjectUtilities.generateKey(); + var key4 = Zotero.DataObjectUtilities.generateKey(); + yield Zotero.DB.queryAsync( + "INSERT INTO syncQueue (libraryID, key, syncObjectTypeID, lastCheck, tries) " + + "VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)", + [ + lib1, key1, syncObjectTypeID, now - 3700, 0, + lib1, key2, syncObjectTypeID, now - 7000, 1, + lib2, key3, syncObjectTypeID, now - 86400, 2 + ] + ); + + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(objectType, lib1, [key1, key2]); + yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(objectType, lib2, [key4]); + + var sql = "SELECT lastCheck, tries FROM syncQueue WHERE libraryID=? " + + `AND syncObjectTypeID=${syncObjectTypeID} AND key=?`; + var row; + // key1 + row = yield Zotero.DB.rowQueryAsync(sql, [lib1, key1]); + assert.approximately(row.lastCheck, now, 1); + assert.equal(row.tries, 1); + // key2 + row = yield Zotero.DB.rowQueryAsync(sql, [lib1, key2]); + assert.approximately(row.lastCheck, now, 1); + assert.equal(row.tries, 2); + // key3 + row = yield Zotero.DB.rowQueryAsync(sql, [lib2, key3]); + assert.equal(row.lastCheck, now - 86400); + assert.equal(row.tries, 2); + // key4 + row = yield Zotero.DB.rowQueryAsync(sql, [lib2, key4]); + assert.approximately(row.lastCheck, now, 1); + assert.equal(row.tries, 0); + }); + }); + + describe("#getObjectsToTryFromSyncQueue()", function () { + it("should get objects that should be retried", function* () { + var objectType = 'item'; + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + var now = Zotero.Date.getUnixTimestamp(); + var key1 = Zotero.DataObjectUtilities.generateKey(); + var key2 = Zotero.DataObjectUtilities.generateKey(); + var key3 = Zotero.DataObjectUtilities.generateKey(); + var key4 = Zotero.DataObjectUtilities.generateKey(); + yield Zotero.DB.queryAsync( + "INSERT INTO syncQueue (libraryID, key, syncObjectTypeID, lastCheck, tries) " + + "VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)", + [ + lib1, key1, syncObjectTypeID, now - (30 * 60) - 10, 0, // more than half an hour, so should be retried + lib1, key2, syncObjectTypeID, now - (16 * 60 * 60) + 10, 4, // less than 16 hours, shouldn't be retried + lib2, key3, syncObjectTypeID, now - 86400 * 7, 20 // more than 64 hours, so should be retried + ] + ); + + var keys = yield Zotero.Sync.Data.Local.getObjectsToTryFromSyncQueue('item', lib1); + assert.sameMembers(keys, [key1]); + var keys = yield Zotero.Sync.Data.Local.getObjectsToTryFromSyncQueue('item', lib2); + assert.sameMembers(keys, [key3]); + }); + }); + + describe("#removeObjectsFromSyncQueue()", function () { + it("should remove objects from the sync queue", function* () { + var objectType = 'item'; + var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType); + var now = Zotero.Date.getUnixTimestamp(); + var key1 = Zotero.DataObjectUtilities.generateKey(); + var key2 = Zotero.DataObjectUtilities.generateKey(); + var key3 = Zotero.DataObjectUtilities.generateKey(); + yield Zotero.DB.queryAsync( + "INSERT INTO syncQueue (libraryID, key, syncObjectTypeID, lastCheck, tries) " + + "VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)", + [ + lib1, key1, syncObjectTypeID, now, 0, + lib1, key2, syncObjectTypeID, now, 4, + lib2, key3, syncObjectTypeID, now, 20 + ] + ); + + yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue('item', lib1, [key1]); + + var sql = "SELECT COUNT(*) FROM syncQueue WHERE libraryID=? " + + `AND syncObjectTypeID=${syncObjectTypeID} AND key=?`; + assert.notOk(yield Zotero.DB.valueQueryAsync(sql, [lib1, key1])); + assert.ok(yield Zotero.DB.valueQueryAsync(sql, [lib1, key2])); + assert.ok(yield Zotero.DB.valueQueryAsync(sql, [lib2, key3])); + }) + }); + + describe("#resetSyncQueueTries", function () { + var spy; + + after(function () { + if (spy) { + spy.restore(); + } + }) + + it("should be run on version upgrade", function* () { + var sql = "REPLACE INTO settings (setting, key, value) VALUES ('client', 'lastVersion', ?)"; + yield Zotero.DB.queryAsync(sql, "5.0foo"); + + spy = sinon.spy(Zotero.Sync.Data.Local, "resetSyncQueueTries"); + yield Zotero.Schema.updateSchema(); + assert.ok(spy.called); + }); + }); + }); + describe("Conflict Resolution", function () { beforeEach(function* () { yield Zotero.DB.queryAsync("DELETE FROM syncCache"); @@ -311,6 +493,7 @@ describe("Zotero.Sync.Data.Local", function() { var objects = []; var values = []; var dateAdded = Date.now() - 86400000; + var downloadedJSON = []; for (let i = 0; i < 2; i++) { values.push({ left: {}, @@ -339,14 +522,15 @@ describe("Zotero.Sync.Data.Local", function() { }; yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); - // Create new version in cache, simulating a download - json.version = jsonData.version = 15; + // Create updated JSON, simulating a download values[i].right.title = jsonData.title = Zotero.Utilities.randomString(); - yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); + values[i].right.version = json.version = jsonData.version = 15; + downloadedJSON.push(json); // Modify object locally yield modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true }); values[i].left.title = obj.getField('title'); + values[i].left.version = obj.getField('version'); } waitForWindow('chrome://zotero/content/merge.xul', function (dialog) { @@ -373,12 +557,14 @@ describe("Zotero.Sync.Data.Local", function() { } wizard.getButton('finish').click(); }) - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, downloadedJSON, { stopOnError: true } ); assert.equal(objects[0].getField('title'), values[0].right.title); assert.equal(objects[1].getField('title'), values[1].left.title); + assert.equal(objects[0].getField('version'), values[0].right.version); + assert.equal(objects[1].getField('version'), values[1].left.version); }) it("should resolve all remaining conflicts with one side", function* () { @@ -388,6 +574,7 @@ describe("Zotero.Sync.Data.Local", function() { var objects = []; var values = []; + var downloadedJSON = []; var dateAdded = Date.now() - 86400000; for (let i = 0; i < 3; i++) { values.push({ @@ -418,13 +605,15 @@ describe("Zotero.Sync.Data.Local", function() { yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); // Create new version in cache, simulating a download - json.version = jsonData.version = 15; values[i].right.title = jsonData.title = Zotero.Utilities.randomString(); + values[i].right.version = json.version = jsonData.version = 15; yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); + downloadedJSON.push(json); // Modify object locally yield modifyDataObject(obj, undefined, { skipDateModifiedUpdate: true }); values[i].left.title = obj.getField('title'); + values[i].left.version = obj.getField('version'); } waitForWindow('chrome://zotero/content/merge.xul', function (dialog) { @@ -460,13 +649,16 @@ describe("Zotero.Sync.Data.Local", function() { } wizard.getButton('finish').click(); }) - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, downloadedJSON, { stopOnError: true } ); assert.equal(objects[0].getField('title'), values[0].right.title); + assert.equal(objects[0].getField('version'), values[0].right.version); assert.equal(objects[1].getField('title'), values[1].left.title); + assert.equal(objects[1].getField('version'), values[1].left.version); assert.equal(objects[2].getField('title'), values[2].left.title); + assert.equal(objects[2].getField('version'), values[2].left.version); }) it("should handle local item deletion, keeping deletion", function* () { @@ -475,6 +667,8 @@ describe("Zotero.Sync.Data.Local", function() { var type = 'item'; var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); + var downloadedJSON = []; + // Create object, generate JSON, and delete var obj = yield createDataObject(type, { version: 10 }); var jsonData = obj.toJSON(); @@ -491,7 +685,7 @@ describe("Zotero.Sync.Data.Local", function() { // Create new version in cache, simulating a download json.version = jsonData.version = 15; jsonData.title = Zotero.Utilities.randomString(); - yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); + downloadedJSON.push(json); var windowOpened = false; waitForWindow('chrome://zotero/content/merge.xul', function (dialog) { @@ -508,8 +702,8 @@ describe("Zotero.Sync.Data.Local", function() { mergeGroup.leftpane.pane.click(); wizard.getButton('finish').click(); }) - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, downloadedJSON, { stopOnError: true } ); assert.isTrue(windowOpened); @@ -523,6 +717,8 @@ describe("Zotero.Sync.Data.Local", function() { var type = 'item'; var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); + var downloadedJSON = []; + // Create object, generate JSON, and delete var obj = yield createDataObject(type, { version: 10 }); var jsonData = obj.toJSON(); @@ -538,7 +734,7 @@ describe("Zotero.Sync.Data.Local", function() { // Create new version in cache, simulating a download json.version = jsonData.version = 15; jsonData.title = Zotero.Utilities.randomString(); - yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); + downloadedJSON.push(json); waitForWindow('chrome://zotero/content/merge.xul', function (dialog) { var doc = dialog.document; @@ -551,8 +747,8 @@ describe("Zotero.Sync.Data.Local", function() { assert.equal(mergeGroup.rightpane.getAttribute('selected'), 'true'); wizard.getButton('finish').click(); }) - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, downloadedJSON, { stopOnError: true } ); obj = objectsClass.getByLibraryAndKey(libraryID, key); @@ -562,9 +758,9 @@ describe("Zotero.Sync.Data.Local", function() { it("should handle note conflict", function* () { var libraryID = Zotero.Libraries.userLibraryID; - var type = 'item'; var objectsClass = Zotero.DataObjectUtilities.getObjectsClassForObjectType(type); + var downloadedJSON = []; var noteText1 = "

A

"; var noteText2 = "

B

"; @@ -586,7 +782,7 @@ describe("Zotero.Sync.Data.Local", function() { // Create new version in cache, simulating a download json.version = jsonData.version = 15; json.data.note = noteText2; - yield Zotero.Sync.Data.Local.saveCacheObjects(type, libraryID, [json]); + downloadedJSON.push(json); // Delete object locally obj.setNote(noteText1); @@ -600,8 +796,8 @@ describe("Zotero.Sync.Data.Local", function() { assert.equal(mergeGroup.rightpane.getAttribute('selected'), 'true'); wizard.getButton('finish').click(); }) - yield Zotero.Sync.Data.Local.processSyncCacheForObjectType( - libraryID, type, { stopOnError: true } + yield Zotero.Sync.Data.Local.processObjectsFromJSON( + type, libraryID, downloadedJSON, { stopOnError: true } ); obj = objectsClass.getByLibraryAndKey(libraryID, key); diff --git a/test/tests/syncRunnerTest.js b/test/tests/syncRunnerTest.js index 9725a22325..68e232c7d2 100644 --- a/test/tests/syncRunnerTest.js +++ b/test/tests/syncRunnerTest.js @@ -105,14 +105,31 @@ describe("Zotero.Sync.Runner", function () { // // Helper functions // - var setup = Zotero.Promise.coroutine(function* (options = {}) { - yield Zotero.DB.queryAsync("DELETE FROM settings WHERE setting='account'"); - yield Zotero.Users.init(); + function setResponse(response) { + setHTTPResponse(server, baseURL, response, responses); + } + + + // + // Tests + // + beforeEach(function* () { + yield resetDB({ + thisArg: this, + skipBundledFiles: true + }); - var runner = new Zotero.Sync.Runner_Module({ baseURL, apiKey }); + userLibraryID = Zotero.Libraries.userLibraryID; + publicationsLibraryID = Zotero.Libraries.publicationsLibraryID; + + Zotero.HTTP.mock = sinon.FakeXMLHttpRequest; + server = sinon.fakeServer.create(); + server.autoRespond = true; + + runner = new Zotero.Sync.Runner_Module({ baseURL, apiKey }); Components.utils.import("resource://zotero/concurrentCaller.js"); - var caller = new ConcurrentCaller(1); + caller = new ConcurrentCaller(1); caller.setLogger(msg => Zotero.debug(msg)); caller.stopOnError = true; caller.onError = function (e) { @@ -126,29 +143,6 @@ describe("Zotero.Sync.Runner", function () { } }; - return { runner, caller }; - }) - - function setResponse(response) { - setHTTPResponse(server, baseURL, response, responses); - } - - - // - // Tests - // - before(function* () { - userLibraryID = Zotero.Libraries.userLibraryID; - publicationsLibraryID = Zotero.Libraries.publicationsLibraryID; - }) - beforeEach(function* () { - Zotero.HTTP.mock = sinon.FakeXMLHttpRequest; - - server = sinon.fakeServer.create(); - server.autoRespond = true; - - ({ runner, caller } = yield setup()); - yield Zotero.Users.setCurrentUserID(1); yield Zotero.Users.setCurrentUsername("A"); }) @@ -404,27 +398,7 @@ describe("Zotero.Sync.Runner", function () { }) describe("#sync()", function () { - var spy; - - before(function* () { - yield resetDB({ - thisArg: this, - skipBundledFiles: true - }); - - yield Zotero.Libraries.init(); - }) - - afterEach(function () { - if (spy) { - spy.restore(); - } - }); - it("should perform a sync across all libraries and update library versions", function* () { - yield Zotero.Users.setCurrentUserID(1); - yield Zotero.Users.setCurrentUsername("A"); - setResponse('keyInfo.fullAccess'); setResponse('userGroups.groupVersions'); setResponse('groups.ownerGroup'); @@ -675,11 +649,11 @@ describe("Zotero.Sync.Runner", function () { // Check local library versions assert.equal( - Zotero.Libraries.getVersion(Zotero.Libraries.userLibraryID), + Zotero.Libraries.getVersion(userLibraryID), 5 ); assert.equal( - Zotero.Libraries.getVersion(Zotero.Libraries.publicationsLibraryID), + Zotero.Libraries.getVersion(publicationsLibraryID), 10 ); assert.equal( @@ -699,8 +673,9 @@ describe("Zotero.Sync.Runner", function () { it("should show the sync error icon on error", function* () { - yield Zotero.Users.setCurrentUserID(1); - yield Zotero.Users.setCurrentUsername("A"); + let pubLib = Zotero.Libraries.get(publicationsLibraryID); + pubLib.libraryVersion = 5; + yield pubLib.save(); setResponse('keyInfo.fullAccess'); setResponse('userGroups.groupVersionsEmpty'); @@ -716,6 +691,25 @@ describe("Zotero.Sync.Runner", function () { INVALID: true // TODO: Find a cleaner error } }); + // No publications changes + setResponse({ + method: "GET", + url: "users/1/publications/settings?since=5", + status: 304, + headers: { + "Last-Modified-Version": 5 + }, + json: {} + }); + setResponse({ + method: "GET", + url: "users/1/publications/fulltext", + status: 200, + headers: { + "Last-Modified-Version": 5 + }, + json: {} + }); spy = sinon.spy(runner, "updateIcons"); yield runner.sync();