diff --git a/.gitignore b/.gitignore index b4fecda..783b5fa 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ npm-debug.log node_modules/ coverage/ run/ +.nyc_output/ diff --git a/agent.js b/agent.js index 99b34c2..d6c4988 100644 --- a/agent.js +++ b/agent.js @@ -1,8 +1,13 @@ 'use strict'; -module.exports = agent => { - // should watch error event - agent.on('error', err => { - agent.coreLogger.error(err); - }); +module.exports = class { + constructor(agent) { + this.agent = agent; + } + configDidLoad() { + // should watch error event + this.agent.on('error', err => { + this.agent.coreLogger.error(err); + }); + } }; diff --git a/app.js b/app.js index ceb67cf..310509d 100644 --- a/app.js +++ b/app.js @@ -11,140 +11,151 @@ const { accepts, } = require('./lib/utils'); -module.exports = app => { - // logging error - const config = app.config.onerror; - const viewTemplate = fs.readFileSync(config.templatePath, 'utf8'); - - app.on('error', (err, ctx) => { - ctx = ctx || app.createAnonymousContext(); - if (config.appErrorFilter && !config.appErrorFilter(err, ctx)) return; - - const status = detectStatus(err); - // 5xx - if (status >= 500) { - try { - ctx.logger.error(err); - } catch (ex) { - app.logger.error(err); - app.logger.error(ex); - } - return; +module.exports = class { + constructor(app) { + this.app = app; + } + configDidLoad() { + const app = this.app; + + const config = app.config.onerror; + const viewTemplate = fs.readFileSync(config.templatePath, 'utf8'); + + // push biz error middleware + if (config.errorHandler.enable === true) { + app.config.coreMiddleware.push('eggOnerrorHandler'); } - // 4xx - try { - ctx.logger.warn(err); - } catch (ex) { - app.logger.warn(err); - app.logger.error(ex); - } - }); + app.on('error', (err, ctx) => { + ctx = ctx || app.createAnonymousContext(); + if (config.appErrorFilter && !config.appErrorFilter(err, ctx)) return; - const errorOptions = { - // support customize accepts function - accepts() { - const fn = config.accepts || accepts; - return fn(this); - }, - - html(err) { const status = detectStatus(err); - const errorPageUrl = typeof config.errorPageUrl === 'function' - ? config.errorPageUrl(err, this) - : config.errorPageUrl; - - // keep the real response status - this.realStatus = status; - // don't respond any error message in production env - if (isProd(app)) { - // 5xx - if (status >= 500) { - if (errorPageUrl) { - const statusQuery = - (errorPageUrl.indexOf('?') > 0 ? '&' : '?') + - `real_status=${status}`; - return this.redirect(errorPageUrl + statusQuery); - } - this.status = 500; - this.body = `

Internal Server Error, real status: ${status}

`; - return; + // 5xx + if (status >= 500) { + try { + ctx.logger.error(err); + } catch (ex) { + app.logger.error(err); + app.logger.error(ex); } - // 4xx - this.status = status; - this.body = `

${status} ${http.STATUS_CODES[status]}

`; - return; - } - // show simple error format for unittest - if (app.config.env === 'unittest') { - this.status = status; - this.body = `${err.name}: ${err.message}\n${err.stack}`; return; } - const errorView = new ErrorView(this, err, viewTemplate); - this.body = errorView.toHTML(); - }, + // 4xx + try { + ctx.logger.warn(err); + } catch (ex) { + app.logger.warn(err); + app.logger.error(ex); + } + }); + + const errorOptions = { + // support customize accepts function + accepts(...args) { + const fn = config.accepts || accepts; + return fn(this, ...args); + }, + + html(err) { + const status = detectStatus(err); + const errorPageUrl = typeof config.errorPageUrl === 'function' + ? config.errorPageUrl(err, this) + : config.errorPageUrl; + + // keep the real response status + this.realStatus = status; + // don't respond any error message in production env + if (isProd(app)) { + // 5xx + if (status >= 500) { + if (errorPageUrl) { + const statusQuery = + (errorPageUrl.includes('?') ? '&' : '?') + + `real_status=${status}`; + return this.redirect(errorPageUrl + statusQuery); + } + this.status = 500; + this.body = `

Internal Server Error, real status: ${status}

`; + return; + } + // 4xx + this.status = status; + this.body = `

${status} ${http.STATUS_CODES[status]}

`; + return; + } + // show simple error format for unittest + if (app.config.env === 'unittest') { + this.status = status; + this.body = `${err.name}: ${err.message}\n${err.stack}`; + return; + } - json(err) { - const status = detectStatus(err); - let errorJson = {}; + const errorView = new ErrorView(this, err, viewTemplate); + this.body = errorView.toHTML(); + }, - this.status = status; - const code = err.code || err.type; - const message = detectErrorMessage(this, err); + json(err) { + const status = detectStatus(err); + let errorJson = {}; - if (isProd(app)) { - // 5xx server side error - if (status >= 500) { - errorJson = { - code, - // don't respond any error message in production env - message: http.STATUS_CODES[status], - }; + this.status = status; + const code = err.code || err.type; + const message = detectErrorMessage(this, err); + + if (isProd(app)) { + // 5xx server side error + if (status >= 500) { + errorJson = { + code, + // don't respond any error message in production env + message: http.STATUS_CODES[status], + }; + } else { + // 4xx client side error + // addition `errors` + errorJson = { + code, + message, + errors: err.errors, + }; + } } else { - // 4xx client side error - // addition `errors` errorJson = { code, message, errors: err.errors, }; - } - } else { - errorJson = { - code, - message, - errors: err.errors, - }; - - if (status >= 500) { - // provide detail error stack in local env - errorJson.stack = err.stack; - errorJson.name = err.name; - for (const key in err) { - if (!errorJson[key]) { - errorJson[key] = err[key]; + + if (status >= 500) { + // provide detail error stack in local env + errorJson.stack = err.stack; + errorJson.name = err.name; + for (const key in err) { + if (!errorJson[key]) { + errorJson[key] = err[key]; + } } } } - } - this.body = errorJson; - }, + this.body = errorJson; + }, - js(err) { - errorOptions.json.call(this, err, this); + js(err) { + errorOptions.json.call(this, err, this); - if (this.createJsonpBody) { - this.createJsonpBody(this.body); - } - }, - }; - - // support customize error response - [ 'all', 'html', 'json', 'text', 'js' ].forEach(type => { - if (config[type]) errorOptions[type] = config[type]; - }); - onerror(app, errorOptions); + if (this.createJsonpBody) { + this.createJsonpBody(this.body); + } + }, + }; + + // support customize error response + [ 'all', 'html', 'json', 'text', 'js' ].forEach(type => { + if (config[type]) errorOptions[type] = config[type]; + }); + onerror(app, errorOptions); + } }; diff --git a/app/middleware/egg_onerror_handler.js b/app/middleware/egg_onerror_handler.js new file mode 100644 index 0000000..03c54eb --- /dev/null +++ b/app/middleware/egg_onerror_handler.js @@ -0,0 +1,64 @@ +'use strict'; + +const { EggError, ErrorType, InternalServerError } = require('egg-errors'); +const accepts = require('accepts'); + +class UnknownError extends InternalServerError { + constructor(message) { + super(message); + this.message = message; + this.code = 'UNKNOWN_ERROR'; + } +} + +module.exports = (_, app) => { + const errorHandler = app.config.onerror.errorHandler; + const acceptContentType = app.config.onerror.accepts || + ((ctx, ...args) => accepts(ctx.req).type(args)); + + return async function bizErrorHandler(ctx, next) { + try { + await next(); + } catch (e) { + let err = e; + const errorType = EggError.getType(err); + switch (errorType) { + case ErrorType.ERROR: break; + + // if error is an EggError Exception, it will pring error log + case ErrorType.EXCEPTION: { + ctx.logger.error(err); + // force set error status + err.status = 500; + break; + } + + // if error is not recognized by EggError, + // it will be converted to UnknownError + case ErrorType.BUILTIN: { + err = UnknownError.from(err); + ctx.logger.error(err); + break; + } + + // getType not work + default: + } + + // handle the error + const contentType = acceptContentType(ctx, 'html', 'text', 'json'); + + if (contentType === 'json' && errorHandler.json) { + await errorHandler.json(ctx, err); + } else if (contentType === 'text' && errorHandler.text) { + await errorHandler.text(ctx, err); + } else if (contentType === 'html' && errorHandler.html) { + await errorHandler.html(ctx, err); + } else if (errorHandler.any) { + await errorHandler.any(ctx, err); + } else { + throw err; + } + } + }; +}; diff --git a/config/config.default.js b/config/config.default.js index ddee862..2ea0a89 100644 --- a/config/config.default.js +++ b/config/config.default.js @@ -2,6 +2,8 @@ const path = require('path'); exports.onerror = { + // function that can customize which content type will be accept + accepts: null, // 5xx error will redirect to ${errorPageUrl} // won't redirect in local env errorPageUrl: '', @@ -11,4 +13,12 @@ exports.onerror = { appErrorFilter: null, // default template path templatePath: path.join(__dirname, '../lib/onerror_page.mustache'), + // handler your error response + errorHandler: { + enable: false, + json: null, + text: null, + html: null, + any: null, + }, }; diff --git a/package.json b/package.json index 9a3ba98..e98dce2 100644 --- a/package.json +++ b/package.json @@ -24,19 +24,21 @@ "onerror" ], "dependencies": { + "accepts": "^1.3.5", "cookie": "^0.3.1", - "koa-onerror": "^4.0.0", - "mustache": "^2.3.0", + "egg-errors": "^2.1.0", + "koa-onerror": "^4.1.0", + "mustache": "^3.0.1", "stack-trace": "^0.0.10" }, "devDependencies": { - "autod": "^3.0.0", + "autod": "^3.0.1", "egg": "next", - "egg-bin": "^4.3.5", - "egg-ci": "^1.8.0", - "egg-mock": "^3.13.1", - "eslint": "^4.11.0", - "eslint-config-egg": "^5.1.1", + "egg-bin": "^4.9.0", + "egg-ci": "^1.11.0", + "egg-mock": "^3.21.0", + "eslint": "^5.11.1", + "eslint-config-egg": "^7.1.0", "pedding": "^1.1.0", "rimraf": "^2.6.2" }, diff --git a/test/error_handler.test.js b/test/error_handler.test.js new file mode 100644 index 0000000..bc48432 --- /dev/null +++ b/test/error_handler.test.js @@ -0,0 +1,286 @@ +'use strict'; + +const mm = require('egg-mock'); +const assert = require('assert'); + +describe('test/error_handler.test.js', () => { + describe('default error handler', () => { + let app; + before(() => { + app = mm.app({ + baseDir: 'any-error-handler', + }); + return app.ready(); + }); + after(() => app.close()); + + describe('error', () => { + it('should handler error', async () => { + await app.httpRequest() + .get('/error') + .set('Accept', 'application/json') + .expect({ + code: 'UNKNOWN_ERROR', + message: 'any error', + }) + .expect(500); + + await app.httpRequest() + .get('/error') + .set('Accept', 'text/plain') + .expect({ + code: 'UNKNOWN_ERROR', + message: 'any error', + }) + .expect(500); + + await app.httpRequest() + .get('/error') + .set('Accept', 'text/html') + .expect({ + code: 'UNKNOWN_ERROR', + message: 'any error', + }) + .expect(500); + + await app.httpRequest() + .get('/error') + .expect({ + code: 'UNKNOWN_ERROR', + message: 'any error', + }) + .expect(500); + }); + + }); + + describe('egg error', () => { + it('should handler egg error', async () => { + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'application/json') + .expect({ + code: 'CUSTOM_ERROR', + message: 'any egg error', + }) + .expect(422); + + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'text/plain') + .expect({ + code: 'CUSTOM_ERROR', + message: 'any egg error', + }) + .expect(422); + + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'text/html') + .expect({ + code: 'CUSTOM_ERROR', + message: 'any egg error', + }) + .expect(422); + + await app.httpRequest() + .get('/egg-error') + .expect({ + code: 'CUSTOM_ERROR', + message: 'any egg error', + }) + .expect(422); + }); + }); + + describe('egg exception', () => { + it('should handler egg exception', async () => { + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'application/json') + .expect({ + code: 'CUSTOM_EXCEPTION', + message: 'any egg exception', + }) + .expect(500); + + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'text/plain') + .expect({ + code: 'CUSTOM_EXCEPTION', + message: 'any egg exception', + }) + .expect(500); + + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'text/html') + .expect({ + code: 'CUSTOM_EXCEPTION', + message: 'any egg exception', + }) + .expect(500); + + await app.httpRequest() + .get('/egg-exception') + .expect({ + code: 'CUSTOM_EXCEPTION', + message: 'any egg exception', + }) + .expect(500); + }); + }); + + }); + + describe('custom error handler', () => { + let app; + before(() => { + app = mm.app({ + baseDir: 'custom-error-handler', + }); + return app.ready(); + }); + after(() => app.close()); + + describe('error', () => { + it('should handler error when accept json', async () => { + await app.httpRequest() + .get('/error') + .set('Accept', 'application/json') + .expect({ + code: 'UNKNOWN_ERROR', + message: 'custom error', + }) + .expect(500); + }); + + it('should handler error when accept text', async () => { + await app.httpRequest() + .get('/error') + .set('Accept', 'text/plain') + .expect('custom error') + .expect(500); + }); + + it('should handler error when accept html', async () => { + await app.httpRequest() + .get('/error') + .set('Accept', 'text/html') + .expect('

UNKNOWN_ERROR

\n
custom error
') + .expect(500); + }); + + it('should handler error without accept', async () => { + await app.httpRequest() + .get('/error') + .expect('

UNKNOWN_ERROR

\n
custom error
') + .expect(500); + }); + }); + + describe('egg error', () => { + it('should handler egg error when accept json', async () => { + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'application/json') + .expect({ + code: 'CUSTOM_ERROR', + message: 'custom egg error', + }) + .expect(422); + }); + + it('should handler egg error when accept text', async () => { + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'text/plain') + .expect('custom egg error') + .expect(422); + }); + + it('should handler egg error when accept html', async () => { + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'text/html') + .expect('

CUSTOM_ERROR

\n
custom egg error
') + .expect(422); + }); + + it('should handler egg error without accept', async () => { + await app.httpRequest() + .get('/egg-error') + .set('Accept', 'text/html') + .expect('

CUSTOM_ERROR

\n
custom egg error
') + .expect(422); + }); + }); + + describe('egg exception', () => { + it('should handler egg exception when accept json', async () => { + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'application/json') + .expect({ + code: 'CUSTOM_EXCEPTION', + message: 'custom egg exception', + }) + .expect(500); + }); + + it('should handler egg exception when accept text', async () => { + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'text/plain') + .expect('custom egg exception') + .expect(500); + }); + + it('should handler egg exception when accept html', async () => { + await app.httpRequest() + .get('/egg-exception') + .set('Accept', 'text/html') + .expect('

CUSTOM_EXCEPTION

\n
custom egg exception
') + .expect(500); + }); + + it('should handler egg exception without accept', async () => { + await app.httpRequest() + .get('/egg-exception') + .expect('

CUSTOM_EXCEPTION

\n
custom egg exception
') + .expect(500); + }); + }); + }); + + describe('unmatch error handler', () => { + let app; + before(() => { + app = mm.app({ + baseDir: 'unmatch-error-handler', + }); + return app.ready(); + }); + after(() => app.close()); + + it('should throw when accept json', async () => { + const res = await app.httpRequest() + .get('/error') + .set('Accept', 'application/json') + .expect(500); + assert(res.body.code === 'UNKNOWN_ERROR'); + assert(res.body.message === 'error'); + }); + + it('should throw when accept html', async () => { + const res = await app.httpRequest() + .get('/error') + .set('Accept', 'text/html') + .expect(500); + + assert(res.text.includes('UnknownError: error')); + }); + }); + +}); diff --git a/test/fixtures/any-error-handler/app/router.js b/test/fixtures/any-error-handler/app/router.js new file mode 100644 index 0000000..1a2a6a3 --- /dev/null +++ b/test/fixtures/any-error-handler/app/router.js @@ -0,0 +1,33 @@ +'use strict'; + +const { EggError, EggException } = require('egg-errors'); + +class CustomError extends EggError { + constructor(message) { + super(message); + this.code = 'CUSTOM_ERROR'; + this.status = 422; + } +} + +class CustomException extends EggException { + constructor(message) { + super(message); + this.code = 'CUSTOM_EXCEPTION'; + this.status = 422; + } +} + +module.exports = app => { + app.get('/error', async () => { + throw new Error('error'); + }); + + app.get('/egg-error', async () => { + throw new CustomError('egg error'); + }); + + app.get('/egg-exception', async () => { + throw new CustomException('egg exception'); + }); +}; diff --git a/test/fixtures/any-error-handler/config/config.default.js b/test/fixtures/any-error-handler/config/config.default.js new file mode 100644 index 0000000..621b74d --- /dev/null +++ b/test/fixtures/any-error-handler/config/config.default.js @@ -0,0 +1,16 @@ +'use strict'; + +exports.onerror = { + errorHandler: { + enable: true, + any: (ctx, err) => { + ctx.status = err.status; + ctx.body = { + code: err.code, + message: 'any ' + err.message, + }; + }, + }, +}; + +exports.keys = 'foo,bar'; diff --git a/test/fixtures/any-error-handler/package.json b/test/fixtures/any-error-handler/package.json new file mode 100644 index 0000000..8c431a5 --- /dev/null +++ b/test/fixtures/any-error-handler/package.json @@ -0,0 +1,3 @@ +{ + "name": "egg-onerror" +} diff --git a/test/fixtures/custom-error-handler/app/router.js b/test/fixtures/custom-error-handler/app/router.js new file mode 100644 index 0000000..1a2a6a3 --- /dev/null +++ b/test/fixtures/custom-error-handler/app/router.js @@ -0,0 +1,33 @@ +'use strict'; + +const { EggError, EggException } = require('egg-errors'); + +class CustomError extends EggError { + constructor(message) { + super(message); + this.code = 'CUSTOM_ERROR'; + this.status = 422; + } +} + +class CustomException extends EggException { + constructor(message) { + super(message); + this.code = 'CUSTOM_EXCEPTION'; + this.status = 422; + } +} + +module.exports = app => { + app.get('/error', async () => { + throw new Error('error'); + }); + + app.get('/egg-error', async () => { + throw new CustomError('egg error'); + }); + + app.get('/egg-exception', async () => { + throw new CustomException('egg exception'); + }); +}; diff --git a/test/fixtures/custom-error-handler/config/config.default.js b/test/fixtures/custom-error-handler/config/config.default.js new file mode 100644 index 0000000..85d39ee --- /dev/null +++ b/test/fixtures/custom-error-handler/config/config.default.js @@ -0,0 +1,25 @@ +'use strict'; + +exports.onerror = { + errorHandler: { + enable: true, + json: (ctx, err) => { + ctx.status = err.status || 500; + ctx.body = { + code: err.code, + message: 'custom ' + err.message, + }; + }, + text: (ctx, err) => { + ctx.status = err.status || 500; + ctx.body = 'custom ' + err.message; + }, + html: (ctx, err) => { + ctx.status = err.status || 500; + ctx.body = `

${err.code}

\n
custom ${err.message}
`; + }, + any: null, + }, +}; + +exports.keys = 'foo,bar'; diff --git a/test/fixtures/custom-error-handler/package.json b/test/fixtures/custom-error-handler/package.json new file mode 100644 index 0000000..8c431a5 --- /dev/null +++ b/test/fixtures/custom-error-handler/package.json @@ -0,0 +1,3 @@ +{ + "name": "egg-onerror" +} diff --git a/test/fixtures/onerror-biz/app/controller/home.js b/test/fixtures/onerror-biz/app/controller/home.js new file mode 100644 index 0000000..f24aa10 --- /dev/null +++ b/test/fixtures/onerror-biz/app/controller/home.js @@ -0,0 +1,22 @@ +'use strict'; + +const { Controller } = require('egg'); +const { EggError, EggException } = require('egg-errors'); + + +class HomeController extends Controller { + async throwError() { + throw new Error('error'); + } + + async throwEggError() { + throw new EggError('egg error'); + } + + async throwEggException() { + throw new EggException('egg exception'); + } + +} + +module.exports = HomeController; diff --git a/test/fixtures/onerror-biz/app/router.js b/test/fixtures/onerror-biz/app/router.js new file mode 100644 index 0000000..54a563e --- /dev/null +++ b/test/fixtures/onerror-biz/app/router.js @@ -0,0 +1,7 @@ +'use strict'; + +module.exports = app => { + app.get('/error', app.controller.home.throwError); + app.get('/eggerror', app.controller.home.throwEggError); + app.post('/eggexception', app.controller.home.throwEggException); +}; diff --git a/test/fixtures/onerror-biz/config/config.default.js b/test/fixtures/onerror-biz/config/config.default.js new file mode 100644 index 0000000..e6269cc --- /dev/null +++ b/test/fixtures/onerror-biz/config/config.default.js @@ -0,0 +1,37 @@ +'use strict'; + +exports.logger = { + level: 'NONE', + consoleLevel: 'NONE', +}; + +exports.keys = 'foo,bar'; + +exports.security = { + csrf: false, +}; + +exports.onerror = { + accepts(ctx, ...args) { + console.log(ctx.type, ctx.headers); + const a = ctx.accepts(...args); + console.log(a, ...args); + return a; + }, + application: { + formatJSON(err) { + return { + message: err.message, + code: err.code, + status: err.status, + data: err.data, + }; + }, + formatText(err) { + return err.message; + }, + formatHtml(err) { + return '

' + err.message + '

'; + }, + }, +}; diff --git a/test/fixtures/onerror-biz/package.json b/test/fixtures/onerror-biz/package.json new file mode 100644 index 0000000..2e0400c --- /dev/null +++ b/test/fixtures/onerror-biz/package.json @@ -0,0 +1,3 @@ +{ + "name": "onerror" +} diff --git a/test/fixtures/unmatch-error-handler/app/router.js b/test/fixtures/unmatch-error-handler/app/router.js new file mode 100644 index 0000000..3b36cd4 --- /dev/null +++ b/test/fixtures/unmatch-error-handler/app/router.js @@ -0,0 +1,7 @@ +'use strict'; + +module.exports = app => { + app.get('/error', async () => { + throw new Error('error'); + }); +}; diff --git a/test/fixtures/unmatch-error-handler/config/config.default.js b/test/fixtures/unmatch-error-handler/config/config.default.js new file mode 100644 index 0000000..3309290 --- /dev/null +++ b/test/fixtures/unmatch-error-handler/config/config.default.js @@ -0,0 +1,9 @@ +'use strict'; + +exports.onerror = { + errorHandler: { + enable: true, + }, +}; + +exports.keys = 'foo,bar'; diff --git a/test/fixtures/unmatch-error-handler/package.json b/test/fixtures/unmatch-error-handler/package.json new file mode 100644 index 0000000..8c431a5 --- /dev/null +++ b/test/fixtures/unmatch-error-handler/package.json @@ -0,0 +1,3 @@ +{ + "name": "egg-onerror" +} diff --git a/test/onerror.test.js b/test/onerror.test.js index 239d27a..c7f0b87 100644 --- a/test/onerror.test.js +++ b/test/onerror.test.js @@ -473,4 +473,5 @@ describe('test/onerror.test.js', () => { }); }); + });