-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Way to set headers on HttpErrors #571
Comments
this is very app-specific IMO and belongs in middleware. i personally just do: this.status = 405
this.body = {
message: 'Nope!'
}
this.set('Allow', 'GET, POST') you could always add your own context helper like |
but koa checks for status property and sets status, it looks for message and sets the body... why not headers? especially since many error statuses dont make any sense without a corresponding header. It would be nice if modules like a router could rely on that because the way you do it the error cannot be catched. The feature would just be a few lines of code but would make a lot of difference. Of all the features that are bundled in koa (content negotiation etc.) this one would really benefit from being included in core. |
I also think it is good practice to put all the business logic inside functions that have no access to the koa context (or req/res/next in express), but only get params and return promise for an entitiy. If the entity was not found, I like them to throw an error that has already set status to 404, so it can bubble up automatically without any boilerplate code. But with errors that require headers that doesn't work because koa doesn't look for headers on the error object. |
HmmMm I agree it's sort of weird to have an in-between solution. Originally I remember we thought the I think that is beneficial in a few ways, being more symbolic means you'll have to change less down the road if you decide to handle errors differently. However since we support the status right there, it's sort of leaning more towards what you're suggesting. Maybe it's something we should remove entirely? I don't think either the declarative or imperative version is better than the other really, both have pros and cons. I'd probably lean towards declarative since in many cases you'll have to handle things like mongodb errors specifically and so on, if you simply |
Removing this functionality entirely would break a lot of middleware. But the problem is if this is not in core than other middleware on npm can't rely on this and then they will never throw errors ever but always send responses directly, so we cannot catch them in error handlers ever. No logging, no recovering, no additional info, no custom response messages, nothing. For example, many REST APIs have a unique way to provide error info in JSON (or XML). But if the middleware you installed from npm automatically sends error responses in text/plain, you cannot achieve this. And this is the case today for any HTTP error that requires additional info in headers. Being able to use try/catch for this is really one of the best features of koa. In javascript, exceptions are called errors. A HTTP error is an error. If you ask your business logic function to return the user with username "waldo" but it cannot find Waldo, it should throw an error. Native Errors have a message, a code, a stack. HTTP errors can also have a status and headers, and you can define them in an elegant way with http-errors. HTTP headers are basically additional I mean, including it would really just be no more than like 5 lines of code and all middlewares had a unified way that they can rely on to specify headers. It even works without changing anything in the http-errors API. These are a few examples of errors that have corresponding headers:
All of these currently have to be set directly on the response and therefor cannot be catched if you want to send the recommended headers. I can especially imagine third-party routers responding with 405 with Allow headers or JSON Web Token middleware responding with 401 that you might want to catch for logging, custom response format etc. but you can't because they have to send responses directly. |
Sure, I get all that, just thinking through the issue. I'm not 100% convinced that pushing it to middleware/route-level code is the best solution, we can't just obscure the errors, so then we'd have to pass those on as well, even then, why should I have some random middleware dictate what the response is going to be. In some cases that might be reasonable, definitely not in others, and in those cases we're no longer propagating the "real" error. I think maybe we should keep the distinction at exceptions vs errors – if middleware such as authentication wants to respond with a 401, doing so directly is not such a bad thing, you can still log the http status etc which is no different than having thrown it. I think throwing non-http errors with user properties is where the choice really comes in. For example if the middleware didn't supply an option to specify the Retry-After value, you could still manipulate it in middleware, there's no reason it has to be an exception. |
it is a bad thing if the middleware automatically responds with 401 if I always wanted to respond with an uniform JSON error response in my API (Google APIs do this, for example). This is especially true if you have the requirement to follow HATEOS, so you always need to provide links in the response body. Just an example. Then it would be nice to do this: app.use(async (ctx, next) => {
try {
next();
} catch (err) {
ctx.response.body = {
error: err.message,
code: err.code
};
if (err.status === 401) {
ctx.response.body.link = '/token';
}
throw err;
} finally {
switch (ctx.accepts('json', 'xml')) {
case 'json': ctx.response.body = JSON.stringify(ctx.response.body); break;
case 'xml': ctx.response.body = XML.stringify(ctx.response.body); break;
}
}
})
app.use(errorLogger());
app.use(jwtMiddlware({secret: 'suchsecretverywow', issuer: 'doge'}); this is all pseudocode of course. The point is, it doesn't work, because jwtMiddleware responds directly with 401 - because if it throws, it can't add additional info aka headers. Sure jwt could implement a In your example, middleware could technically modify In the end, there are no exceptions in javascript, only errors. And one of the great things about javascript is that we can attach custom stuff to objects. Do I understand you correctly that you think thrown errors should only be for stuff that seriously went wrong and have nothing to do with HTTP, so basically only 500? We should remove http-errors then ;) But I get your thoughts... somewhere we need to draw the line. Next we allow a |
True good point. Hmmm I'll have to come back to this in a bit haha, hacking on some other stuff. Definitely needs a nicer solution! There are exceptions though, that's what the propagation is, |
Just so you have something to think about in the shower :) |
I just wanted to point out that the usage of http-errors and all of its dirty prototype fiddling can be replaced by the one-liner throw Object.assign(new Error('an error'), {status: 400}) |
Im a agree with that, but you need a specific error, and for the moment just need a middleware that catch exeption no? ...
app.use((ctx, next) => {
try{
next();
}catch(e) {
if (e.status === 405) {
ctx.status = 405;
ctx.set(e.headers);
}
}
})
app.use((ctx, next) => {
try{
await doStuff()
} cacth(e){
if (e.status === 405) {
//do stuff
throw e // or not
}
}
}) the other problem is that middleware need to throw errors ... function async thrownStatus(status, ctx, fn) {
await fn()
if (ctx.status === status) {
let e = new Error()
e.status= status
throw e;
}
}
app.use((ctx, next) => {
try{
await thrownStatus(405, ctx, doStuff);
}catch(e) {
//do other stuff
}
}); Or the easy way is just do stuff after the call like no exception app.use(ctx => {
async doStuff();
if (ctx.status === 405) {
//do other stuff ...
}
}); But that's not very elegant ... That's just an idea |
After thinking :) on the default onerror function where status is set we may add a test for extra properties on the Error message like headers ans set it? ok i stop flooding :) // default to 500
if ('number' != typeof err.status || !statuses[err.status]) err.status = 500;
if (err.status != 500 && err.headers) this.set(err.headers);
// respond
const code = statuses[err.status]; |
@menems that's what this proposal is all about ;) |
i'm totally cool with |
Yea, |
@dougwilson i don't have a stance. i don't know too much about symbols yet. all i've ever used them for is |
same as @jonathanong-- haven't used Symbols much in the past, but I just read through this, and it seems like a fair use case. @dougwilson what are the advantages of using a symbol here? |
@tejasmanohar , the advantage would be that since Anyway, I'm sure there are other ways to accomplish this, it's just what came to my mind :) |
Hey just saw this! Can we get something in please! Haha. Let's get |
@dead-horse Yep, interesting. I'm +0.5 on Symbols for that purpose- curious to see implementation :) |
I dont really get why Symbols here. Seems to me like using it just because we can now. At the same time, nobody here would consider using an ES6 Map for the headers property - just keep it simple. But in what cases do you actually need to tell koa to "keep" a header when throwing an error?! Shouldnt "keep" be the default? All I can really think of is setting Accept when throwing a 405. |
why just 'don't remove' the headers already set ? because with this solution when you want keep a header set by module X from module Y you have to reference errors set in modules X in modules Y? or maybe a flag to say i want remove all header on error, and not i want keep some headers? |
Or just let the user explicitly deal with their own error, and koa just catch 500 and uncaught exception (-> 500) and reset headers but not on other status? |
I'm not sure if this is the right place to discuss this, maybe I should make a new issue, but I just had to go through a couple hours of mental gymnastics related to this and wanted to share my experience. Fighting a bug with CORS headers. I'm using koa@2-alpha.3 & kcors@2. In order for a controlled error, one made with const corsHeaders = [
'access-control-allow-origin',
'access-control-allow-credentials',
]
// Enable Cross Origin requests
// This is needed because the default behavior of Koa is to ditch headers
// when an error is thrown, have to explicility keep the CORS headers
app.use(compose([
async (ctx, next) => {
try {
await next()
} catch (e) {
e.headers = e.headers || {}
for (const header of corsHeaders) {
e.headers[header] = ctx.response.get(header)
}
throw e
}
},
cors({
// Allow all, just for development
origin: (ctx) => ctx.get('Origin'),
credentials: true,
}),
])) Couldn't decide whether I should implement my own app-wide error handler, most of which would be a duplicate of Koa's default.... debating whether I should throw exceptions down the chain or not.... Also forgot/discovered that the latest v2 alpha doesn't have the error headers feature in yet, that was a good 20 minutes gone 😅 Regardless of what I chose, this certainly was not a "pit of success" type-situation that we should be striving for. |
yes, always do that! no framework will ever handle errors exactly the way want to/
why not? |
@lourd This is a bit more concise: app.use(aync (ctx, next) => {
try {
await next();
} catch (e) {
e.headers = e.headers || {};
Object.assign(e.headers, corsHeaders.map(header => ctx.response.get(header)));
throw e;
}
}); But yeah you should probably be implementing your own error handler. Edit: the above example won't work, I got confused between objects and arrays, but it could be made to work. |
also, a PR for kcors@2 for |
Yeah the real problem here is the lack of kcors support for |
@jonathanong right, I see that now. I don't think the docs stress that enough, then. I've been getting along fine with the default error handler for a while on a couple different projects. The question about throwing exceptions up the chain relates to the question of overriding the default error handler. If I implement my own then I'll be more confident about the So got it, the recommendation is to do both. Sounds like an update to |
I've created koajs/cors#11 for this, it's for master but I'll rewrite it for v2.x if it is merged (shouldn't be hard). |
Currently, the default error handler in koa checks for the status property on errors and sets the status accordingly. However, many errors in the HTTP spec are defined with required or recommended headers to be send along with the status. For example, 405 Method Not Allowed should always be sent with an Allow header, a 503 Service Unavailable with a Retry After.
Currently, it is not possible to set this. You could write your own error handler that checks a
headers
property on the error object, but third party middleware can't rely on that. As a result, middleware modules on npm (like koa-router etc.) have to set status and headers on the context response manually instead of throwing an error, which means it is not possible to catch them and therefor destroys one of the best features of koa - error handling with try / catch.It would be nice if the koa error handler would check for the existence of a
headers
property on the object and set the headers in that hash (with normalized casing). http-errors already supports passing a properties hash in the constructor, so it could look like this:This would then allow middlewares from npm to actually throw errors that can be catched to add some additional custom text etc.
Thoughts?
The text was updated successfully, but these errors were encountered: