From 7c25093ca2286b39afe38d4a0a937ded444adc5e Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Fri, 3 Mar 2017 04:09:37 -0500 Subject: [PATCH] Fix various error propagation issues during translation As noted in 27cb099c82, import translators should be rewritten to return a promise from doImport() and wait for promises from successive item.complete() calls. They should then be marked as minVersion: "5.0" to be handled properly by this new code. (But this tries to account, albeit with somewhat worse behavior, for translators that haven't been rewritten and sandboxes without Promise (which is currently the case with child sandboxes in the client).) (Oh, and I haven't tested this at all in the connectors.) --- .../zotero/xpcom/translation/translate.js | 56 ++++--- .../xpcom/translation/translate_firefox.js | 16 +- test/tests/translateTest.js | 155 ++++++++++++++++++ 3 files changed, 199 insertions(+), 28 deletions(-) diff --git a/chrome/content/zotero/xpcom/translation/translate.js b/chrome/content/zotero/xpcom/translation/translate.js index d4394b5334..3bae3e7007 100644 --- a/chrome/content/zotero/xpcom/translation/translate.js +++ b/chrome/content/zotero/xpcom/translation/translate.js @@ -81,6 +81,9 @@ Zotero.Translate.Sandbox = { * @param {SandboxItem} An item created using the Zotero.Item class from the sandbox */ _itemDone: function (translate, item) { + var asyncTranslator = translate.translator[0].minVersion + && parseInt(translate.translator[0].minVersion.match(/^[0-9]+/)[0]) >= 5; + var run = function (resolve) { Zotero.debug("Translate: Saving item"); @@ -142,15 +145,7 @@ Zotero.Translate.Sandbox = { item = translate._sandboxManager.copyObject(item); item.complete = oldItem.complete; } - let maybePromise = translate._runHandler("itemDone", item, item); - // DEBUG: Is this ever necessary? - if (maybePromise) { - return resolve ? maybePromise.then(resolve) : maybePromise; - } - if (resolve) { - resolve(); - } - return; + return translate._runHandler("itemDone", item, item); } // We use this within the connector to keep track of items as they are saved @@ -206,12 +201,11 @@ Zotero.Translate.Sandbox = { // Fire itemSaving event translate._runHandler("itemSaving", item); - translate._savingItems++; - // For synchronous import (when Promise isn't available in the sandbox) and web - // translators, we queue saves - if (!resolve || translate instanceof Zotero.Translate.Web) { + // For synchronous import (when Promise isn't available in the sandbox or the do* + // function doesn't use it) and web translators, queue saves + if (!resolve || translate instanceof Zotero.Translate.Web || !asyncTranslator) { Zotero.debug("Translate: Saving via queue"); translate.saveQueue.push(item); if (resolve) { @@ -221,7 +215,9 @@ Zotero.Translate.Sandbox = { // For async import, save items immediately else { Zotero.debug("Translate: Saving now"); - translate._saveItems([item]).then(resolve); + translate.incrementAsyncProcesses("Zotero.Translate#_saveItems()"); + return translate._saveItems([item]) + .then(() => translate.decrementAsyncProcesses("Zotero.Translate#_saveItems()")); } }; @@ -233,7 +229,20 @@ Zotero.Translate.Sandbox = { return new translate._sandboxManager.sandbox.Promise(function (resolve, reject) { try { - run(resolve); + let maybePromise = run(resolve); + if (maybePromise) { + maybePromise + .then(resolve) + .catch(function (e) { + // Fix wrapping error from sandbox when error is thrown from _saveItems() + if (Zotero.isFx) { + reject(translate._sandboxManager.copyObject(e)); + } + else { + reject(e); + } + }); + } } catch (e) { reject(e); @@ -1368,12 +1377,7 @@ Zotero.Translate.Base.prototype = { // item.complete() calls) if (maybePromise) { maybePromise - .then( - () => this.decrementAsyncProcesses("Zotero.Translate#translate()") - ) - .catch((e) => { - this.complete(false, e) - }); + .then(() => this.decrementAsyncProcesses("Zotero.Translate#translate()")) return; } } catch (e) { @@ -1536,7 +1540,9 @@ Zotero.Translate.Base.prototype = { if(returnValue) { if(this.saveQueue.length) { this._waitingForSave = true; - this._saveItems(this.saveQueue).then(() => this.saveQueue = []); + this._saveItems(this.saveQueue) + .catch(e => this._runHandler("error", e)) + .finally(() => this.saveQueue = []); return; } this._debug("Translation successful"); @@ -1656,11 +1662,11 @@ Zotero.Translate.Base.prototype = { this.newItems = this.newItems.concat(newItems); this._checkIfDone(); }.bind(this)) - .catch(function(e) { + .catch((e) => { this._savingItems -= items.length; - Zotero.logError(e); this.complete(false, e); - }.bind(this)); + throw e; + }); }), /** diff --git a/chrome/content/zotero/xpcom/translation/translate_firefox.js b/chrome/content/zotero/xpcom/translation/translate_firefox.js index 88a3b6ac46..06c35cbade 100644 --- a/chrome/content/zotero/xpcom/translation/translate_firefox.js +++ b/chrome/content/zotero/xpcom/translation/translate_firefox.js @@ -519,8 +519,9 @@ Zotero.Translate.SandboxManager.prototype = { "_canCopy":function(obj) { if(typeof obj !== "object" || obj === null) return false; - if((obj.constructor.name !== "Object" && obj.constructor.name !== "Array") || - "__exposedProps__" in obj || (obj.wrappedJSObject && obj.wrappedJSObject.__wrappingManager)) { + if (!["Object", "Array", "Error"].includes(obj.constructor.name) + || "__exposedProps__" in obj + || (obj.wrappedJSObject && obj.wrappedJSObject.__wrappingManager)) { return false; } return true; @@ -534,7 +535,16 @@ Zotero.Translate.SandboxManager.prototype = { "copyObject":function(obj, wm) { if(!this._canCopy(obj)) return obj; if(!wm) wm = new WeakMap(); - var obj2 = (obj.constructor.name === "Array" ? this.sandbox.Array() : this.sandbox.Object()); + switch (obj.constructor.name) { + case 'Array': + case 'Error': + var obj2 = this.sandbox[obj.constructor.name](); + break; + + default: + var obj2 = this.sandbox.Object(); + break; + } var wobj2 = obj2.wrappedJSObject ? obj2.wrappedJSObject : obj2; for(var i in obj) { if(!obj.hasOwnProperty(i)) continue; diff --git a/test/tests/translateTest.js b/test/tests/translateTest.js index 26bfb84aee..ea87178031 100644 --- a/test/tests/translateTest.js +++ b/test/tests/translateTest.js @@ -764,6 +764,161 @@ describe("Zotero.Translate", function() { }); }); }); + + + describe("Error Handling", function () { + it("should propagate saveItems() errors from synchronous doImport()", function* () { + var items = [ + { + // Invalid object + }, + { + itemType: "book", + title: "B" + } + ]; + + var added = 0; + var notifierID = Zotero.Notifier.registerObserver({ + notify: function (event, type, ids, extraData) { + added++; + } + }, ['item']); + + var translation = new Zotero.Translate.Import(); + translation.setString(""); + translation.setTranslator(buildDummyTranslator( + "import", + "function detectImport() {}" + + "function doImport() {" + + " var json = JSON.parse('" + JSON.stringify(items).replace(/['\\]/g, "\\$&") + "');" + + " for (let o of json) {" + + " let item = new Zotero.Item;" + + " for (let field in o) { item[field] = o[field]; }" + + " item.complete();" + + " }" + + "}" + )); + var e = yield getPromiseError(translation.translate()); + Zotero.Notifier.unregisterObserver(notifierID); + assert.ok(e); + + // Saving should be stopped without any saved items + assert.equal(added, 0); + assert.equal(translation._savingItems, 0); + assert.equal(translation._runningAsyncProcesses, 0); + assert.isNull(translation._currentState); + }); + + it("should propagate saveItems() errors from asynchronous doImport()", function* () { + var items = [ + { + // Invalid object + }, + { + itemType: "book", + title: "B" + } + ]; + + var added = 0; + var notifierID = Zotero.Notifier.registerObserver({ + notify: function (event, type, ids, extraData) { + added++; + } + }, ['item']); + + var translation = new Zotero.Translate.Import(); + translation.setString(""); + translation.setTranslator(buildDummyTranslator( + "import", + "function detectImport() {}" + + "function doImport() {" + + " var json = JSON.parse('" + JSON.stringify(items).replace(/['\\]/g, "\\$&") + "');" + + " return new Promise(function (resolve, reject) {" + + " function next() {" + + " var data = json.shift();" + + " if (!data) {" + + " resolve();" + + " return;" + + " }" + + " var item = new Zotero.Item;" + + " for (let field in data) { item[field] = data[field]; }" + + " item.complete().then(next).catch(reject);" + + " }" + + " next();" + + " });" + + "}", + { + minVersion: "5.0" + } + )); + var e = yield getPromiseError(translation.translate()); + Zotero.Notifier.unregisterObserver(notifierID); + assert.ok(e); + + // Saving should be stopped without any saved items + assert.equal(added, 0); + assert.equal(translation._savingItems, 0); + assert.equal(translation._runningAsyncProcesses, 0); + assert.isNull(translation._currentState); + }); + + it("should propagate errors from saveItems with synchronous doSearch()", function* () { + var stub = sinon.stub(Zotero.Translate.ItemSaver.prototype, "saveItems"); + stub.returns(Zotero.Promise.reject(new Error("Save error"))); + + var translation = new Zotero.Translate.Search(); + translation.setTranslator(buildDummyTranslator( + "search", + "function detectSearch() {}" + + "function doSearch() {" + + " var item = new Zotero.Item('journalArticle');" + + " item.itemType = 'book';" + + " item.title = 'A';" + + " item.complete();" + + "}" + )); + translation.setSearch({ itemType: "journalArticle", DOI: "10.111/Test"}); + var e = yield getPromiseError(translation.translate({ + libraryID: Zotero.Libraries.userLibraryID, + saveAttachments: false + })); + assert.ok(e); + + stub.restore(); + }); + + it("should propagate errors from saveItems() with asynchronous doSearch()", function* () { + var stub = sinon.stub(Zotero.Translate.ItemSaver.prototype, "saveItems"); + stub.returns(Zotero.Promise.reject(new Error("Save error"))); + + var translation = new Zotero.Translate.Search(); + translation.setTranslator(buildDummyTranslator( + "search", + "function detectSearch() {}" + + "function doSearch() {" + + " var item = new Zotero.Item('journalArticle');" + + " item.itemType = 'book';" + + " item.title = 'A';" + + " return new Promise(function (resolve, reject) {" + + " item.complete().then(next).catch(reject);" + + " });" + + "}", + { + minVersion: "5.0" + } + )); + translation.setSearch({ itemType: "journalArticle", DOI: "10.111/Test"}); + var e = yield getPromiseError(translation.translate({ + libraryID: Zotero.Libraries.userLibraryID, + saveAttachments: false + })); + assert.ok(e); + + stub.restore(); + }); + }); }); describe("Zotero.Translate.ItemGetter", function() {