From c2e4ee9bdaa94bfb64d1b083829082869cdc0c01 Mon Sep 17 00:00:00 2001
From: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
Date: Fri, 6 Dec 2024 16:40:31 -0800
Subject: [PATCH] 986 input validation for directory names (#1024)

---
 news/986_input_validation.rst             | 23 +++++++++++++++++++++++
 openfecli/commands/quickrun.py            |  9 ++++-----
 openfecli/parameters/output.py            | 12 ++++++++++--
 openfecli/tests/commands/test_quickrun.py |  9 ++++++++-
 4 files changed, 45 insertions(+), 8 deletions(-)
 create mode 100644 news/986_input_validation.rst

diff --git a/news/986_input_validation.rst b/news/986_input_validation.rst
new file mode 100644
index 000000000..adeed8591
--- /dev/null
+++ b/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>
diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py
index dcd6f9d33..c7dd6f833 100644
--- a/openfecli/commands/quickrun.py
+++ b/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):
diff --git a/openfecli/parameters/output.py b/openfecli/parameters/output.py
index a82652883..c943537df 100644
--- a/openfecli/parameters/output.py
+++ b/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",
diff --git a/openfecli/tests/commands/test_quickrun.py b/openfecli/tests/commands/test_quickrun.py
index d78b82827..b82d281e5 100644
--- a/openfecli/tests/commands/test_quickrun.py
+++ b/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')