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

koa shouldn't special-case ENOENT errors #1420

Open
julienw opened this issue Nov 21, 2019 · 9 comments
Open

koa shouldn't special-case ENOENT errors #1420

julienw opened this issue Nov 21, 2019 · 9 comments

Comments

@julienw
Copy link
Contributor

julienw commented Nov 21, 2019

I noticed that koa special-cases ENOENT errors:

koa/lib/context.js

Lines 146 to 147 in ed84ee5

// ENOENT support
if ('ENOENT' == err.code) err.status = 404;

I think this is a mistake: ENOENT errors could happen for a lot of reasons (this happens with google's storage library when the key file isn't found, for example) that shouldn't get a 404 status.
And BTW it's not even documented.

@Usamaliaquat123
Copy link

ENOENT errors could happen for a lot of reasons (this happens with google's storage library when the key file isn't found, for example) that shouldn't get a 404 status.

So, what exactly are you getting :)

@fl0w
Copy link
Contributor

fl0w commented Nov 27, 2019

I think the semantics are pretty spot on. ENOENT is thrown when something (usually a file or a directory) isn't found, i.e. 404.

[...] this happens with google's storage library when the key file isn't found, for example [...]

I don't get it, isn't 404 a perfect match for this? Requested resource not found.

@julienw
Copy link
Contributor Author

julienw commented Nov 27, 2019

404 means "page not found", it doesn't mean "we got some error about a file not found". Koa doesn't know the context. Especially in this case it's a configuration error (some configuration file isn't found).

Probably the google library should throw a better error, but still.

@dead-horse
Copy link
Member

I'm ok to remove the 'ENOENT' case, but it would be a breaking change.

@fl0w
Copy link
Contributor

fl0w commented Nov 27, 2019

@julienw I guess it's a matter of subjective interpretation, but I guess my way of thinking is perhaps too "HTTP-minded". Either way, I think special casing should either be configurable or removed. But as dead-horse pointed out, it's a breaking change so at the very least, lets reference #1114 for future evaluation.

@julienw
Copy link
Contributor Author

julienw commented Nov 28, 2019

Yeah this is a breaking change, I totally agree with that.
Note that this is alleviated by the fact that this behavior is documented nowhere. I had to look at the source code :-) (but still it's a breaking change).

I believe this was done initially to accommodate use cases like:

ctx.body = fs.createReadStream(filePath);

So I think we need a replacement for this. Maybe a middleware that would be part of koa (or another package) that would be then easy to add for somebody who wants this functionality?
Would it be worth having a "compat-2.0" middleware that would cancel out all differences from the 3.0 changes, possibly configurable with flags to enable/disable individual bits?

@jonathanong
Copy link
Member

👍 this should be done downstream

@jkomyno
Copy link

jkomyno commented Apr 16, 2021

Has any decision been taken on this?

@IlyaSemenov
Copy link

IlyaSemenov commented Feb 18, 2022

I was hit by that as well. My perfectly legitimate API handler started to return 404 for no apparent reason.

What happened was, deep inside, a third party library (soap in my case) was doing fs.readFile for some of its internal stuff and was failing for unrelated reason. This should be generating a proper 500 Internal Server Error (which is then logged accordingly by upstream layers), not 404 how Koa presents it.

I don't see the problem with introducing a breaking change... May be it's time to fully use semver and don't be afraid of major version bumps? Many of us have done that and survived. 😆

@jonathanong jonathanong added this to the v3.0.0 milestone Aug 29, 2024
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

8 participants