Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add biz handler #29

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const {
} = require('./lib/utils');

module.exports = app => {
app.config.coreMiddleware.push('eggOnerrorHandler');
popomore marked this conversation as resolved.
Show resolved Hide resolved

// logging error
const config = app.config.onerror;
const viewTemplate = fs.readFileSync(config.templatePath, 'utf8');
Expand Down Expand Up @@ -43,9 +45,9 @@ module.exports = app => {

const errorOptions = {
// support customize accepts function
accepts() {
accepts(...args) {
const fn = config.accepts || accepts;
return fn(this);
return fn(this, ...args);
},

html(err) {
Expand Down
60 changes: 60 additions & 0 deletions app/middleware/egg_onerror_handler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'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 acceptFn = app.config.onerror.accepts ||
popomore marked this conversation as resolved.
Show resolved Hide resolved
((ctx, ...args) => accepts(ctx.req).type(args));

return async function onerrorBizHandler(ctx, next) {
popomore marked this conversation as resolved.
Show resolved Hide resolved
try {
await next();
} catch (e) {
let err = e;
const errorType = EggError.getType(err);
switch (errorType) {
case ErrorType.ERROR: break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

又看了一次代码,还是很难理解,日志什么时候该打,什么时候该使用 EggError,什么时候用 EXCEPTION 。。。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error 是用于终端的,比如检验错误,验权,不需要打 error。exception 是系统错误,常见的 500,或者底层抛的错,需要打 error 日志。针对响应则用 errorHandler 统一处理。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

先整理个文章出来吧,这种经验实践类的功能看代码老是跟不上思路。。。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我理解的是 ERROR 是类似业务处理报错,是业务上预期的错误,EXCEPTION 是异常,非预期的程序错误,所以需要打印日志。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果是终端的,那么应该使用 ClientError 会更好,不用思考就明白了。

// 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:
throw err;
}

// handle the error
const contentType = acceptFn(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);
}
}
};
};
20 changes: 20 additions & 0 deletions config/config.default.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,24 @@ exports.onerror = {
appErrorFilter: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

上面的 // 5xx error will redirect to ${errorPageUrl} 注释都需要修改一下了。

// default template path
templatePath: path.join(__dirname, '../lib/onerror_page.mustache'),
// handler your error response
errorHandler: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里重新实现了一下

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉这个应该是一个配置,而是约定在 app/onerror.js 文件入口。

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我感觉这里比较简单,写配置比较容易想到,约定文件太多也不是非常好。配置还有一个好处是有 dump。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

egg 的文档将使用说明写起来看看没问题就可以合并发布了。

enable: false,
json: (ctx, err) => {
ctx.status = err.status || 500;
ctx.body = {
code: err.code,
message: err.message,
};
},
text: (ctx, err) => {
ctx.status = err.status || 500;
ctx.body = err.message;
},
html: (ctx, err) => {
ctx.status = err.status || 500;
ctx.body = `<h2>${err.code}</h2>\n<div>${err.message}</div>`;
},
any: null,
},
};
18 changes: 10 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改为 ^2

"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"
},
Expand Down
130 changes: 130 additions & 0 deletions test/error_handler.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
'use strict';

// const fs = require('fs');
// const pedding = require('pedding');
const mm = require('egg-mock');
// const rimraf = require('rimraf');
// const path = require('path');
// const assert = require('assert');

describe.only('test/error_handler.test.js', () => {
describe.only('default error handler', () => {
let app;
before(() => {
app = mm.app({
baseDir: 'default-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: 'error',
})
.expect(500);
});

it('should handler error when accept text', async () => {
await app.httpRequest()
.get('/error')
.set('Accept', 'text/plain')
.expect('error')
.expect(500);
});

it('should handler error when accept html', async () => {
await app.httpRequest()
.get('/error')
.set('Accept', 'text/html')
.expect('<h2>UNKNOWN_ERROR</h2>\n<div>error</div>')
.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: 'egg error',
})
.expect(422);
});

it('should handler egg error when accept text', async () => {
await app.httpRequest()
.get('/egg-error')
.set('Accept', 'text/plain')
.expect('egg error')
.expect(422);
});

it('should handler egg error when accept html', async () => {
await app.httpRequest()
.get('/egg-error')
.set('Accept', 'text/html')
.expect('<h2>CUSTOM_ERROR</h2>\n<div>egg error</div>')
.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: 'egg exception',
})
.expect(500);
});

it('should handler egg exception when accept text', async () => {
await app.httpRequest()
.get('/egg-exception')
.set('Accept', 'text/plain')
.expect('egg exception')
.expect(500);
});

it('should handler egg exception when accept html', async () => {
await app.httpRequest()
.get('/egg-exception')
.set('Accept', 'text/html')
.expect('<h2>CUSTOM_EXCEPTION</h2>\n<div>egg exception</div>')
.expect(500);
});
});

});

describe('default error handler', () => {
let app;
before(() => {
app = mm.app({
baseDir: 'custom-error-handler',
});
return app.ready();
});
after(() => app.close());

it('should format error', async () => {
await app.httpRequest()
.get('/error')
.set('Accept', 'application/json')
.expect({

})
.expect(200);
});
});
});
33 changes: 33 additions & 0 deletions test/fixtures/default-error-handler/app/router.js
Original file line number Diff line number Diff line change
@@ -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');
});
};
9 changes: 9 additions & 0 deletions test/fixtures/default-error-handler/config/config.default.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

exports.onerror = {
errorHandler: {
enable: true,
},
};

exports.keys = 'foo,bar';
3 changes: 3 additions & 0 deletions test/fixtures/default-error-handler/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "egg-onerror"
}
22 changes: 22 additions & 0 deletions test/fixtures/onerror-biz/app/controller/home.js
Original file line number Diff line number Diff line change
@@ -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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx.throw([status], [msg], [properties])ctx.assert(value, [status], [msg], [properties]) 这 2 个对应的用法也可以加下

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个还是不要改原生了?就作为 BUILDIN 错误好了

}

async throwEggError() {
throw new EggError('egg error');
}

async throwEggException() {
throw new EggException('egg exception');
}

}

module.exports = HomeController;
7 changes: 7 additions & 0 deletions test/fixtures/onerror-biz/app/router.js
Original file line number Diff line number Diff line change
@@ -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);
};
37 changes: 37 additions & 0 deletions test/fixtures/onerror-biz/config/config.default.js
Original file line number Diff line number Diff line change
@@ -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 '<h2>' + err.message + '</h2>';
},
},
};
3 changes: 3 additions & 0 deletions test/fixtures/onerror-biz/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "onerror"
}
1 change: 1 addition & 0 deletions test/onerror.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,4 +473,5 @@ describe('test/onerror.test.js', () => {
});

});

});