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() {