CargoSense
Briefly.Entry open/6 not handling all posix errors
Hey Ben,
Thanks so much for the awesome library, and of course all your other open source work!
Just saw this in 3 different places today (two dev machines and one production-like env) for unclear reasons, as we've been using Briefly without any issues for over a year now... this match clause on line 88 https://github.com/CargoSense/briefly/blob/cdd6ddfe80a47809c48fb3a3942c4f1d4e5749c4/lib/briefly/entry.ex#L88 only has [:eexist, :eaccess] in the list, but File.mkdir_p can return an error with any posix atom in this list http://www.erlang.org/doc/man/file.html#type-posix
When it returns one other than :eexist or :eaccess (in our case :enoent though why that's suddenly happening, I don't know...) it crashes the GenServer which makes handling the error difficult. I started trying to reproduce in a test, so far no luck, I could try and explicitly mock File.mkdir_p with mox or something if that would be useful to have a test, but I thought worth checking in for a) your preferred approach, and b) whether there's any special reason only those two posix atoms are handled, before deciding what the correct approach for enoent or other possible posix returns is.
Figured out whats causing the error, backslashes in the filename! 10/17/2019 was part of the file name, but only some of the time/when the user specified a customized date range... not sure if right way to handle that is to return a better error tuple or to escape the string if it has slashes or what.... but probably don't want to bring down the genserver...
Hey @bglusman can you show an example function call that reproducibly causes the error?
@benwilson512 sure thing! and happy to write a test and/or fix for it too, wasn't sure yet how to reproduce when I first opened but now it's easy, just Briefly.create(prefix: "whatever 10/17/2019 - 10/22/2019 ")
Also like I said, unsure how we'd like to handle, I'm changing my code to just String.replace(prefix, "/", "-") for now but unsure if auto escaping is a) an option and b) desirable
Also, while discussing file names, probably will/should be a different issue, but I wonder about option for making the unique microsend-etc part of the filename into a unique directory to hold an explicitly named file? We're sending the file as is, but product just noticed the timestampy-suffixes on the filenames we've been sending for over a year and wondered about dropping them, and there's no good option I see... can make a new issue or just take a shot at in a PR from a fork if you're open to it...
Just in case it's a useful start pushed up a commit in my fork with a failing test (though weirdly it fails a different test at the moment). can't really make it pass till we decide handling
Hey @bglusman. I agree that Briefly should handle more posix errors, but I don't want to take the extra step of transforming the provided prefix input. We should make sure that prefix is well documented to be a directory, and should therefore conform to a valid directory name on whatever platform you're on.
For your second thing, can you make a dedicated issue? I think it's worth some dedicated discussion.
I am getting {:case_clause, {:error, :eacces}} in my application. I am using puppeteer-pdf that is using briefly 0.3.0 version. It crashes the GenServer because it does not match with eaccess in 0.3.0 version.
{:error, reason} when reason in [:eexist, :eaccess] ->
in entry.ex line 95.
Closing for now. PRs welcome to handle more posix errors, but please check the Plug.Upload implementation because we would like to stay as close to that as possible, thanks!
