Way to set headers on HttpErrors

[felixfbecker]

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:

throw new HttpError(405, {headers: {'Allow': 'GET, POST'}})
throw new HttpError(503, {headers: {'Retry-After': 60}})

This would then allow middlewares from npm to actually throw errors that can be catched to add some additional custom text etc.
Thoughts?

[jonathanong]

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 this.returnAPIResponse(405, ...) to your app

[felixfbecker]

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.

[felixfbecker]

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.

[tj]

HmmMm I agree it's sort of weird to have an in-between solution. Originally I remember we thought the properties param would be nice since then you could react to that data in error-handling middleware, and use that to determine the response.

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 if (err) this.throw(500, "mongo failed blah blah") you completely lose that information when it comes to logging etc.

[felixfbecker]

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 message properties.

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:

• 405 Method Not Allowed MUST send Allow
• 401 Unauthorized MUST send WWW-Authenticate
• 406 Not Acceptable "SHOULD include an entity containing a list of available entity characteristics and location(s) from which the user or user agent can choose the one most appropriate. The entity format is specified by the media type given in the Content-Type header field."
• 407 Proxy Authentication Required MUST send Proxy-Authenticate
• 413 Request Entitiy Too Large SHOULD send Retry-After if the condition is temporary
• 416 Requested Range Not Satisfiable SHOULD send Content-Range if the request was byte-range
• 503 Service Unavailable MAY include Retry-After if the length of the delay is known

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.

[tj]

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.

[felixfbecker]

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 errorResponseTransformer option or something like that, but at that point we would be basically passing middleware functions to factories, wtf xD

In your example, middleware could technically modify Retry-After, but the only function that knows what value makes sense is the middleware that threw the 503.
503 might be a bad example, but equally only the authorization middleware knows that WWW-Authenticate should be Basic or Digest, only the router knows that that Allow needs to be GET, PUT.

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 body property on the error... And koa3 middleware functions get only passed the request and either call next, return new KoaResponse({status, body, headers}) or throw new KoaErrorResponse({status, body, headers})...
It's just where should this line be drawn? 1) Keep errors out of HTTP responses? 2) Allow status and message (current behaviour)? 3) allow headers too? Imo either 1 or 3.

[tj]

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, new Error is an error, throw new Error is an exception :D.

[felixfbecker]

Just so you have something to think about in the shower :)
And don't you dare to build that koa3 vision without giving me credit haha

[felixfbecker]

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})

[menems]

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 ...
Or add a wrapper for the status you want but custom headers or custom body will not been added ...

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

[menems]

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 :)
on context.js something like that

 // 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];

[felixfbecker]

@menems that's what this proposal is all about ;)