986 input validation for directory names (#1024)

news/986_input_validation.rst

@@ -0,0 +1,23 @@
+**Added:**
+
+* <news item>
+
+**Changed:**
+
+* ``openfe quickrun`` now fails up-front when the user-supplied output file (``-o``) already exists or has a non-existent parent directory.
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* <news item>
+
+**Security:**
+
+* <news item>

openfecli/commands/quickrun.py

@@ -6,7 +6,7 @@
 import pathlib
 
 from openfecli import OFECommandPlugin
-from openfecli.parameters.output import ensure_file_does_not_exist
+from openfecli.parameters.output import validate_outfile
 from openfecli.utils import write, print_duration, configure_logger
 
 
@@ -33,10 +33,9 @@ def _format_exception(exception) -> str:
 )
 @click.option(
     'output', '-o', default=None,
-    type=click.Path(dir_okay=False, file_okay=True, writable=True,
-                    path_type=pathlib.Path),
-    help="output file (JSON format) for the final results",
-    callback=ensure_file_does_not_exist,
+    type=click.Path(dir_okay=False, file_okay=True, path_type=pathlib.Path),
+    help="Filepath at which to create and write the JSON-formatted results.",
+    callback=validate_outfile,
 )
 @print_duration
 def quickrun(transformation, work_dir, output):

openfecli/parameters/output.py

@@ -9,11 +9,19 @@ def get_file_and_extension(user_input, context):
     return file, ext
 
 
-def ensure_file_does_not_exist(ctx, param, value):
+def ensure_file_does_not_exist(value):
+    # TODO: I believe we can replace this with click.option(file_okay=False)
     if value and value.exists():
         raise click.BadParameter(f"File '{value}' already exists.")
-    return value
 
+def ensure_parent_dir_exists(value):
+    if value and not value.parent.is_dir():
+        raise click.BadParameter(f"Cannot write to {value}, parent directory does not exist.")
+
+def validate_outfile(ctx, param, value):
+    ensure_file_does_not_exist(value)
+    ensure_parent_dir_exists(value)
+    return value
 
 OUTPUT_FILE_AND_EXT = Option(
     "-o", "--output",

openfecli/tests/commands/test_quickrun.py

@@ -54,7 +54,14 @@ def test_quickrun_output_file_exists(json_file):
         assert result.exit_code == 2  # usage error
         assert "File 'foo.json' already exists." in result.output
 
-
+def test_quickrun_output_file_in_nonexistent_directory(json_file):
+    """Should catch invalid filepaths up front."""
+    runner = CliRunner()
+    outfile = "not_dir/foo.json"
+    result = runner.invoke(quickrun, [json_file, '-o', outfile])
+    assert result.exit_code == 2
+    assert "Cannot write" in result.output
+
 def test_quickrun_unit_error():
     with resources.files('openfecli.tests.data') as d:
         json_file = str(d / 'bad_transformation.json')