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

Does not throw when file does not exist #43

Closed
niieani opened this issue Dec 5, 2015 · 14 comments · Fixed by #74
Closed

Does not throw when file does not exist #43

niieani opened this issue Dec 5, 2015 · 14 comments · Fixed by #74

Comments

@niieani
Copy link
Contributor

niieani commented Dec 5, 2015

If a file does not exist there is no error. The promise resolves because and returns undefined on the yield fs.stat here:

    try {
      var stats = yield fs.stat(path);
      // nothing gets executed later on...

Not sure if that's a bug in mz or with the usage by koa-send.

@haoxins
Copy link
Member

haoxins commented Dec 6, 2015

See https://github.com/koajs/send/blob/master/index.js#L92

      var notfound = ['ENOENT', 'ENAMETOOLONG', 'ENOTDIR'];
      if (~notfound.indexOf(err.code)) return;

You can check that by this

var path = yield send(...);
if (!path) {
  // do something ...
}

@niieani
Copy link
Contributor Author

niieani commented Dec 6, 2015

I see, however it's not what I (and I presume most would have) expected. Not throwing on missing file lead me to waste an hour trying to find what the problem is. I think it's logical to have the default behavior throw an error when the file does not exist - on top of that this behavior is not documented. Especially that with throwing - it's as simple as try-catching the send for the end-user.

@haoxins
Copy link
Member

haoxins commented Dec 6, 2015

I think it's logical to have the default behavior throw an error when the file does not exist

It's reasonable for me to respond 404.
But it should be in the README. PR welcome

@niieani
Copy link
Contributor Author

niieani commented Dec 6, 2015

404 is indeed reasonable, however I'll still argue the 404 should be coupled with an error thrown - since without an error thrown the 404 will not be logged (when logging errors from koa) unless the person manually watches for a null return value and manually throws the error. All major file serving HTTP servers, including Apache and Nginx, throw an error when a file is not found and log it into an error.log file unless that feature is explicitly turned off. koa-send's "quiet by default" behavior does not match that.

@tj
Copy link
Member

tj commented Dec 6, 2015

Hmm it should still be logged, if you have a logger. If not I would say that's a logger bug, or it is not the top-most middleware. The absence of a response is a 404 implicitly, but having the error code etc sounds useful, throwing sounds reasonable to me

@niieani
Copy link
Contributor Author

niieani commented Dec 6, 2015

@tj Exactly. 404 happens regardless of koa-send, by default. Without an explicit throw it is hard to diagnose the problem and differentiate between an error coming from koa, the routing or in fact, an incorrect path / missing file.

@tj
Copy link
Member

tj commented Dec 6, 2015

Haha yeah true, I find that with 404's all the time, hard to tell if it's a result of something missing in the db etc or just some screwed up route, little ambiguous. SGTM

@haoxins
Copy link
Member

haoxins commented Dec 7, 2015

@tj So, we need to do a broken change like this?

if (~notfound.indexOf(err.code)) {
  err.status = 404;
  throw err;
}

@tj
Copy link
Member

tj commented Dec 7, 2015

Yep! I think that should be good, can't think of any weird side-effects at the moment

@haoxins
Copy link
Member

haoxins commented Jan 29, 2016

@m-brian take a look at koajs/static#62 #47

It's not the topic of this issue.

@yeliex
Copy link

yeliex commented Nov 23, 2016

may be it would be better to add a option: allowedMethods (throw error instead of setting status and header,like koa-router )

@qwelias
Copy link

qwelias commented Dec 13, 2016

Throwing an error if something went wrong is simply convenient and intuitive, why one should check a result if no file found?

It's purpose is to serve files and if requested path does not resolve it is wrong and it should throw.

@haoxins haoxins mentioned this issue Feb 22, 2017
Closed
2 tasks
@haoxins
Copy link
Member

haoxins commented Mar 26, 2017

#70

haoxins pushed a commit that referenced this issue Apr 9, 2017
haoxins pushed a commit that referenced this issue Apr 9, 2017
@haoxins
Copy link
Member

haoxins commented Apr 9, 2017

v4.0.0 published !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants