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

Koajs converts thrown errors to status code 500 #1451

Closed
jhsware opened this issue Mar 24, 2020 · 4 comments
Closed

Koajs converts thrown errors to status code 500 #1451

jhsware opened this issue Mar 24, 2020 · 4 comments

Comments

@jhsware
Copy link

jhsware commented Mar 24, 2020

If you use the AWS S3 Node.js client to create a proxy, it throws an error with status code 304 when an object hasn't been modified. This propagates to the Koa.js error handling code. Because Node.js sets the http status code of the error on the prop err.statusCode

https://github.com/nodejs/node/blob/066bdec64352278ed324a1bb680f5422e00a3aa5/lib/_http_outgoing.js#L410

and Koa.js expects err.status, Koa.js will return a 500 instead of the actual status code set on the error object by Node.js.

if ('number' != typeof err.status || !statuses[err.status]) err.status = 500;

I suggest adding the line

err.status = err.status || err.statusCode;

or changing to

    // ENOENT support
    let statusCode = err.status || err.statusCode;

    if ('ENOENT' == err.code) statusCode = 404;

    // default to 500
    if ('number' != typeof statusCode || !statuses[statusCode]) statusCode = 500;

    // respond
    const code = statuses[statusCode];
    const msg = err.expose ? err.message : code;
    this.status = err.status = statusCode;

so the developer doesn't have to set it manually, which is non-trivial to figure out and adds unnecessary code complexity in situations where node already has set a status code.

@jonathanong jonathanong transferred this issue from koajs/koa Apr 28, 2020
@jonathanong jonathanong transferred this issue from koajs/discussions Apr 28, 2020
vijaykrishnavanshi added a commit to vijaykrishnavanshi/koa that referenced this issue May 15, 2020
@vijaykrishnavanshi
Copy link
Contributor

We can close this issue now. Fixed in #1460 PR.

@jhsware
Copy link
Author

jhsware commented May 18, 2020

@vijaykrishnavanshi please add a test so this doesn't break in the future. I would have contributed with a PR but I didn't have time to set up the testing environment and didn't feel that only contributing the code was a complete solution.

@vijaykrishnavanshi
Copy link
Contributor

Already added those in the PR 👍
If you have any other specific case that you want the test for let me know.

@jhsware
Copy link
Author

jhsware commented May 18, 2020

Thanks @vijaykrishnavanshi now I found it!
ce9133a

@jhsware jhsware closed this as completed May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants