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.)
This commit is contained in:
Dan Stillman
parent
7ccf781add
commit
7c25093ca2
3 changed files with 199 additions and 28 deletions
|
|
@ -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;
|
||||
});
|
||||
}),
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue