From 8cf8e2618adc33ff34fc04b2e5fb99afb84b9708 Mon Sep 17 00:00:00 2001 From: Alan Plum Date: Tue, 1 Mar 2016 17:54:06 +0100 Subject: [PATCH] Clean up stacktraces Manual cherrypick of 6593f3d. Fixes #1564, #1565, #1744. --- CHANGELOG | 2 + js/actions/_admin/foxx/app.js | 7 +- js/client/modules/org/arangodb/arangosh.js | 4 +- .../modules/org/arangodb/foxx/manager.js | 7 +- js/common/bootstrap/modules.js | 73 +++++++---------- js/common/bootstrap/modules/internal.js | 19 +++-- js/common/tests/shell-errors.js | 4 +- .../tests/shell-foxx-manager-install-spec.js | 24 +++--- js/server/modules/org/arangodb/actions.js | 1 + .../modules/org/arangodb/foxx/manager.js | 82 ++++++------------- .../modules/org/arangodb/foxx/routing.js | 5 +- .../modules/org/arangodb/foxx/service.js | 21 +---- lib/V8/v8-utils.cpp | 6 -- 13 files changed, 94 insertions(+), 161 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a829a3bcf6..6b2b1c1873 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,8 @@ v2.8.4 (XXXX-XX-XX) * global modules are no longer incorrectly resolved outside the ArangoDB JavaScript directory or the Foxx service's root directory (issue #1577) +* improved error messages from Foxx and JavaScript (issues #1564, #1565, #1744) + v2.8.3 (2016-02-22) ------------------- diff --git a/js/actions/_admin/foxx/app.js b/js/actions/_admin/foxx/app.js index 454cf95061..2ccace5ca2 100644 --- a/js/actions/_admin/foxx/app.js +++ b/js/actions/_admin/foxx/app.js @@ -283,12 +283,7 @@ actions.defineHttp({ var name = body.name; var mount = body.mount; var options = body.options; - try { - var result = foxxManager.runScript(name, mount, options); - return result; - } catch (e) { - throw e.cause || e; - } + return foxxManager.runScript(name, mount, options); } }) }); diff --git a/js/client/modules/org/arangodb/arangosh.js b/js/client/modules/org/arangodb/arangosh.js index bb21519609..6664008f63 100644 --- a/js/client/modules/org/arangodb/arangosh.js +++ b/js/client/modules/org/arangodb/arangosh.js @@ -101,7 +101,9 @@ exports.checkRequestResult = function (requestResult) { throw new TypeError(requestResult.errorMessage); } - throw new ArangoError(requestResult); + const error = new ArangoError(requestResult); + error.message = requestResult.message; + throw error; } // remove the property from the original object diff --git a/js/client/modules/org/arangodb/foxx/manager.js b/js/client/modules/org/arangodb/foxx/manager.js index 391d6bfa55..468ae8285e 100644 --- a/js/client/modules/org/arangodb/foxx/manager.js +++ b/js/client/modules/org/arangodb/foxx/manager.js @@ -717,12 +717,7 @@ var run = function (args) { return 0; } catch (err) { - if (err instanceof ArangoError) { - printf("%s\n", err.errorMessage); - } - else { - printf("%s\n", err.message); - } + arangodb.print(String(err)); return 1; } diff --git a/js/common/bootstrap/modules.js b/js/common/bootstrap/modules.js index 8a8f602d60..1ec2210c85 100644 --- a/js/common/bootstrap/modules.js +++ b/js/common/bootstrap/modules.js @@ -416,9 +416,11 @@ Module._load = function(request, parent, isMain) { if (match) { dbModule = Module._resolveDbModule(match[3]); if (!dbModule) { - var err = new Error("Cannot find module '" + request + "'"); - err.code = 'MODULE_NOT_FOUND'; - throw err; + throw new internal.ArangoError({ + errorNum: internal.errors.ERROR_MODULE_NOT_FOUND.code, + errorMessage: internal.errors.ERROR_MODULE_NOT_FOUND.message + + '\nFile: ' + request + }); } } else { try { @@ -500,9 +502,11 @@ Module._resolveFilename = function(request, parent) { // look up the filename first, since that's the cache key. var filename = Module._findPath(request, paths); if (!filename) { - var err = new Error("Cannot find module '" + request + "'"); - err.code = 'MODULE_NOT_FOUND'; - throw err; + throw new internal.ArangoError({ + errorNum: internal.errors.ERROR_MODULE_NOT_FOUND.code, + errorMessage: internal.errors.ERROR_MODULE_NOT_FOUND.message + + '\nFile: ' + request + }); } return filename; }; @@ -516,7 +520,22 @@ Module.prototype.load = function(filename) { var extension = path.extname(filename) || '.js'; if (!Module._extensions[extension]) extension = '.js'; - Module._extensions[extension](this, filename); + + try { + Module._extensions[extension](this, filename); + } catch (e) { + if (e.errorNum !== internal.errors.ERROR_MODULE_FAILURE.code) { + const error = new internal.ArangoError({ + errorNum: internal.errors.ERROR_MODULE_FAILURE.code, + errorMessage: internal.errors.ERROR_MODULE_FAILURE.message + + '\nFile: ' + filename + + '\nCause: ' + e + }); + error.cause = e; + throw error; + } + throw e; + } this.loaded = true; }; @@ -551,15 +570,6 @@ Module.prototype._compile = function(content, filename) { content = this.preprocess(content, filename); } - // test for parse errors first and fail early if a parse error detected - if (!internal.parse(content, filename)) { - throw new internal.ArangoError({ - errorNum: internal.errors.ERROR_MODULE_SYNTAX_ERROR.code, - errorMessage: internal.errors.ERROR_MODULE_SYNTAX_ERROR.message - + '\nFile: ' + filename - }); - } - this.filename = filename; var args = this.context; @@ -567,38 +577,15 @@ Module.prototype._compile = function(content, filename) { // Do not use Function constructor or line numbers will be wrong var wrapper = `(function (${keys.join(', ')}) {${content}\n})`; - var fn; - try { - fn = internal.executeScript(wrapper, undefined, filename); - } catch (e) { - console.errorLines(e.stack || String(e)); - let err = new internal.ArangoError({ - errorNum: internal.errors.ERROR_SYNTAX_ERROR_IN_SCRIPT.code, - errorMessage: internal.errors.ERROR_SYNTAX_ERROR_IN_SCRIPT.message - + '\nFile: ' + filename - }); - err.cause = e; - throw err; - } + var fn = internal.executeScript(wrapper, undefined, filename); if (typeof fn !== 'function') { throw new TypeError('Expected internal.executeScript to return a function, not ' + typeof fn); } - try { - fn.apply(args.exports, keys.map(function (key) { - return args[key]; - })); - } catch (e) { - console.errorLines(e.stack || String(e)); - let err = new internal.ArangoError({ - errorNum: internal.errors.ERROR_MODULE_FAILURE.code, - errorMessage: internal.errors.ERROR_MODULE_FAILURE.message - + '\nFile: ' + filename - }); - err.cause = e; - throw err; - } + fn.apply(args.exports, keys.map(function (key) { + return args[key]; + })); }; diff --git a/js/common/bootstrap/modules/internal.js b/js/common/bootstrap/modules/internal.js index 4919d4c207..b39e22e2e2 100644 --- a/js/common/bootstrap/modules/internal.js +++ b/js/common/bootstrap/modules/internal.js @@ -58,22 +58,27 @@ else { this.errorNum = error.errorNum; this.errorMessage = error.errorMessage; } - - this.message = this.toString(); }; exports.ArangoError.prototype = new Error(); } +Object.defineProperty(exports.ArangoError.prototype, 'message', { + configurable: true, + enumerable: true, + get() { + return this.errorMessage; + } +}); + +exports.ArangoError.prototype.name = 'ArangoError'; + exports.ArangoError.prototype._PRINT = function (context) { - context.output += this.toString(); + context.output += '[' + this.toString() + ']'; }; exports.ArangoError.prototype.toString = function() { - var errorNum = this.errorNum; - var errorMessage = this.errorMessage || this.message; - - return '[ArangoError ' + errorNum + ': ' + errorMessage + ']'; + return `${this.name} ${this.errorNum}: ${this.message}`; }; //////////////////////////////////////////////////////////////////////////////// diff --git a/js/common/tests/shell-errors.js b/js/common/tests/shell-errors.js index 62752bb6e5..0a911f74df 100644 --- a/js/common/tests/shell-errors.js +++ b/js/common/tests/shell-errors.js @@ -161,7 +161,7 @@ function ErrorsSuite () { fail(); } catch (err) { - assertEqual("[ArangoError " + e.code + ": " + e.message + "]", err.toString()); + assertEqual("ArangoError " + e.code + ": " + e.message, err.toString()); } }, @@ -177,7 +177,7 @@ function ErrorsSuite () { fail(); } catch (err) { - assertEqual("[ArangoError " + e.code + ": " + e.message + ": did not find document]", err.toString()); + assertEqual("ArangoError " + e.code + ": " + e.message + ": did not find document", err.toString()); } } diff --git a/js/common/tests/shell-foxx-manager-install-spec.js b/js/common/tests/shell-foxx-manager-install-spec.js index 7b98c7888d..30205865d3 100644 --- a/js/common/tests/shell-foxx-manager-install-spec.js +++ b/js/common/tests/shell-foxx-manager-install-spec.js @@ -118,7 +118,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-controller-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -136,7 +136,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-controller-path"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -145,7 +145,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "broken-controller-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -154,7 +154,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "broken-exports-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -163,7 +163,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "broken-setup-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -172,7 +172,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-exports-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -181,7 +181,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-exports-path"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -190,7 +190,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-setup-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_FAILED_TO_EXECUTE_SCRIPT, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -199,7 +199,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "malformed-setup-path"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -208,7 +208,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "missing-controller-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -217,7 +217,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "missing-exports-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); @@ -226,7 +226,7 @@ describe("Foxx Manager install", function() { FoxxManager.install(fs.join(basePath, "missing-setup-file"), "/unittest/broken"); expect(true).toBeFalsy("Managed to install broken application"); } catch(e) { - validateError(errors.ERROR_SYS_ERROR, e); + validateError(errors.ERROR_MODULE_FAILURE, e); } }); }); diff --git a/js/server/modules/org/arangodb/actions.js b/js/server/modules/org/arangodb/actions.js index 094b5717b5..3c27e6bc2a 100644 --- a/js/server/modules/org/arangodb/actions.js +++ b/js/server/modules/org/arangodb/actions.js @@ -134,6 +134,7 @@ function createCallbackFromActionCallbackString (callback, parentModule, route) try { actionModule._compile(`module.exports = ${callback}`, route.name); } catch (e) { + console.errorLines(e.stack); return notImplementedFunction(route, util.format( "could not generate callback for '%s'", callback diff --git a/js/server/modules/org/arangodb/foxx/manager.js b/js/server/modules/org/arangodb/foxx/manager.js index 9d098a0a56..a8f5177f5f 100644 --- a/js/server/modules/org/arangodb/foxx/manager.js +++ b/js/server/modules/org/arangodb/foxx/manager.js @@ -411,32 +411,35 @@ function checkManifest(filename, manifest) { /// All errors are handled including file not found. Returns undefined if manifest is invalid //////////////////////////////////////////////////////////////////////////////// -function validateManifestFile(file) { +function validateManifestFile(filename) { var mf, msg; - if (!fs.exists(file)) { - msg = `Cannot find manifest file "${file}"`; - console.errorLines(msg); + if (!fs.exists(filename)) { + msg = `Cannot find manifest file "${filename}"`; throwFileNotFound(msg); } try { - mf = JSON.parse(fs.read(file)); - } catch (err) { - let details = String(err.stack || err); - msg = `Cannot parse app manifest "${file}": ${details}`; - console.errorLines(msg); - throw new ArangoError({ + mf = JSON.parse(fs.read(filename)); + } catch (e) { + const error = new ArangoError({ errorNum: errors.ERROR_MALFORMED_MANIFEST_FILE.code, - errorMessage: msg + errorMessage: errors.ERROR_MALFORMED_MANIFEST_FILE.message + + '\nFile: ' + filename + + '\nCause: ' + e }); + error.cause = e; + throw error; } try { - checkManifest(file, mf); - } catch (err) { - console.errorLines(`Manifest file "${file}" is invalid:\n${err.errorMessage}`); - if (err.stack) { - console.errorLines(err.stack); - } - throw err; + checkManifest(filename, mf); + } catch (e) { + const error = new ArangoError({ + errorNum: errors.ERROR_INVALID_APPLICATION_MANIFEST.code, + errorMessage: errors.ERROR_INVALID_APPLICATION_MANIFEST.message + + '\nFile: ' + filename + + '\nCause: ' + e + }); + error.cause = e; + throw error; } return mf; } @@ -495,24 +498,15 @@ function computeAppPath(mount) { //////////////////////////////////////////////////////////////////////////////// function executeAppScript(scriptName, app, argv) { - var readableName = utils.getReadableName(scriptName); var scripts = app.manifest.scripts; // Only run setup/teardown scripts if they exist if (scripts[scriptName] || (scriptName !== 'setup' && scriptName !== 'teardown')) { - try { - return app.run(scripts[scriptName], { - appContext: { - argv: argv ? (Array.isArray(argv) ? argv : [argv]) : [] - } - }); - } catch (e) { - if (!(e.cause || e).statusCode) { - let details = String((e.cause || e).stack || e.cause || e); - console.errorLines(`Running script "${readableName}" not possible for mount "${app.mount}":\n${details}`); + return app.run(scripts[scriptName], { + appContext: { + argv: argv ? (Array.isArray(argv) ? argv : [argv]) : [] } - throw e; - } + }); } } @@ -625,10 +619,10 @@ function installAppFromGenerator(targetPath, options) { invalidOptions.push('options.collectionNames has to be an array.'); } if (invalidOptions.length > 0) { - console.log(invalidOptions); throw new ArangoError({ errorNum: errors.ERROR_INVALID_FOXX_OPTIONS.code, - errorMessage: JSON.stringify(invalidOptions, undefined, 2) + errorMessage: errors.ERROR_INVALID_FOXX_OPTIONS.message + + '\nOptions: ' + JSON.stringify(invalidOptions, undefined, 2) }); } options.path = targetPath; @@ -986,8 +980,6 @@ function _validateApp(appInfo) { routeApp(tmp, true); exportApp(tmp); } - } catch (e) { - throw e; } finally { fs.removeDirectoryRecursive(tempPath, true); } @@ -1053,20 +1045,6 @@ function _install(appInfo, mount, options, runSetup) { } catch (err) { console.errorLines(err.stack); } - if (e instanceof ArangoError) { - if (e.errorNum === errors.ERROR_MODULE_SYNTAX_ERROR.code) { - throw _.extend(new ArangoError({ - errorNum: errors.ERROR_SYNTAX_ERROR_IN_SCRIPT.code, - errorMessage: errors.ERROR_SYNTAX_ERROR_IN_SCRIPT.message - }), {stack: e.stack}); - } - if (e.errorNum === errors.ERROR_MODULE_FAILURE.code) { - throw _.extend(new ArangoError({ - errorNum: errors.ERROR_FAILED_TO_EXECUTE_SCRIPT.code, - errorMessage: errors.ERROR_FAILED_TO_EXECUTE_SCRIPT.message - }), {stack: e.stack}); - } - } throw e; } return app; @@ -1155,12 +1133,6 @@ function _uninstall(mount, options) { } var collection = utils.getStorage(); var targetPath = computeAppPath(mount, true); - if (!fs.exists(targetPath) && !options.force) { - throw new ArangoError({ - errorNum: errors.ERROR_NO_FOXX_FOUND.code, - errorMessage: errors.ERROR_NO_FOXX_FOUND.message - }); - } delete appCache[dbname][mount]; if (!options.__clusterDistribution) { try { diff --git a/js/server/modules/org/arangodb/foxx/routing.js b/js/server/modules/org/arangodb/foxx/routing.js index 749794b811..f616bd937c 100644 --- a/js/server/modules/org/arangodb/foxx/routing.js +++ b/js/server/modules/org/arangodb/foxx/routing.js @@ -604,10 +604,7 @@ var routeApp = function (app, isInstallProcess) { // return the new routes return routes; } catch (e) { - console.error("Cannot compute Foxx application routes: %s", String(e)); - if (e.hasOwnProperty("stack")) { - console.errorLines(e.stack); - } + console.errorLines(`Cannot compute Foxx application routes:\n${e.stack}`); if (isInstallProcess) { throw e; } diff --git a/js/server/modules/org/arangodb/foxx/service.js b/js/server/modules/org/arangodb/foxx/service.js index 052881c3dc..8df8dfb4fd 100644 --- a/js/server/modules/org/arangodb/foxx/service.js +++ b/js/server/modules/org/arangodb/foxx/service.js @@ -1,7 +1,5 @@ 'use strict'; const _ = require('underscore'); -const ArangoError = require('org/arangodb').ArangoError; -const errors = require('org/arangodb').errors; const internal = require('internal'); const assert = require('assert'); const Module = require('module'); @@ -374,23 +372,8 @@ class FoxxService { }); } - try { - module.load(filename); - return module.exports; - } catch(e) { - if (e instanceof ArangoError) { - e.errorMessage += "\n(app relative include paths not supported anymore) \nFile: " + filename; - throw e; - } - var err = new ArangoError({ - errorNum: errors.ERROR_FAILED_TO_EXECUTE_SCRIPT.code, - errorMessage: errors.ERROR_FAILED_TO_EXECUTE_SCRIPT.message - + '\nFile: ' + filename - }); - err.stack = e.stack; - err.cause = e; - throw err; - } + module.load(filename); + return module.exports; } get exports() { diff --git a/lib/V8/v8-utils.cpp b/lib/V8/v8-utils.cpp index 4ec46692d9..045901aa78 100644 --- a/lib/V8/v8-utils.cpp +++ b/lib/V8/v8-utils.cpp @@ -4013,12 +4013,6 @@ void TRI_LogV8Exception (v8::Isolate* isolate, LOG_ERROR("!%s", l.c_str()); } - - TRI_Utf8ValueNFC stacktrace(TRI_UNKNOWN_MEM_ZONE, tryCatch->StackTrace()); - - if (*stacktrace && stacktrace.length() > 0) { - LOG_ERROR("stacktrace: %s", *stacktrace); - } } }