VROOM-Project
Silent fail on impossible file write
Suppose we have an input.json file in the current directory and foo is not a valid directory. Then
vroom -i input.json -o foo/output.json
fails to write any output. The return code is 0, which is somehow consistent with exiting silently.
We should really raise an error when writing to a file is requested and turns out to be impossible.
Hey, I will like to work on it. Can you please tell the file name in which I have to make changes.
@rsh-raj thanks for considering this!
Writing the solution (or any other error message) happens in the write_to_json function:
https://github.com/VROOM-Project/vroom/blob/7be0b4ffb38015543229bd50e7f7082388c0e62e/src/utils/output_json.cpp#L347-L356
But throwing an exception there would not be of much help. Indeed, we have a number of try clauses in main.cpp for various errors and the catch part usually calls write_to_json for errors too, expecting the output file to be valid. For example, the high-level calls for parsing/solving/writing are:
https://github.com/VROOM-Project/vroom/blob/7be0b4ffb38015543229bd50e7f7082388c0e62e/src/main.cpp#L177-L196
So I think our best bet would be to check for a valid output file right after all flags are parsed. I'm not very familiar with output streams and file handling but it looks like std::ofstream objects have a fail member function.
@jcoupey I think there are decision to be made. After making these decisions, I will try to do my best.
- Should we a) check before try to write? b) catch exception after trying to write?
- If directory not exist: a) should it be created? b) raise an error? c) fall back to stdout?
- Should we check permission to create a file also?
The method I would suggest is to create a validation function to vroom::io that validate permission and existence, then check right after the arguments are parsed. If invalid to write an output then fallback to the standard output. (1-a, 2-c, 3)
@erdemuysal thanks for stepping up on this one too!
Should we a) check before try to write? b) catch exception after trying to write?
I agree a) would be better here since we'd then avoid performing routing requests and solving the problem for nothing.
If directory not exist: a) should it be created? b) raise an error? c) fall back to stdout?
Deciding to create a non-existing directory on behalf of the user running vroom sounds a bit extreme. Most command-line tools raise an error when trying to operate on missing directories so I'd go for b).
Option c) would require users to be fully aware of the default fallback, and means they'd have to check both the expected file or stdout when solving which IMHO adds complexity. Also having some unexpected json output appear on stdout may break some existing workflows when doing batch solving in scripts where the rest of the output is used. For example on our benchmark workflow or when solving with -DLOG_LS_OPERATORS to output debug information.
Should we check permission to create a file also?
As you suggest, catching a permission problem should probably be part of the check from point 1.
The method I would suggest is to create a validation function to vroom::io that validate permission and existence, then check right after the arguments are parsed. If invalid to write an output then fallback to the standard output. (1-a, 2-c, 3)
I'd keep this summary except I'd simply exit without doing anything in case writing to the requested file is impossible. You can have your vroom::io validation function throw a vroom::InputException in that case and simply catch that in main like here.
Moving this to the next milestone again. Maybe that would be a good opportunity to switch to using the std::filesystem support added in C++17.
switch to using the std::filesystem support added in C++17
Additionally, we could provide a better feedback for a missing input file. The current (cryptic) error is:
$ vroom -i foo.json
[Error] The document is empty. (offset: 0)
{"code":2,"error":"The document is empty. (offset: 0)"}