From d412062bae743692c57640b1ebdcc60d849ff114 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Wed, 20 Nov 2024 11:33:04 -0800 Subject: [PATCH 01/10] adding test that reproduces bug --- openfecli/tests/commands/test_quickrun.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/openfecli/tests/commands/test_quickrun.py b/openfecli/tests/commands/test_quickrun.py index d78b82827..1cd7b3119 100644 --- a/openfecli/tests/commands/test_quickrun.py +++ b/openfecli/tests/commands/test_quickrun.py @@ -54,6 +54,19 @@ 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): + runner = CliRunner() + outfile = "not_dir/foo.json" + result = runner.invoke(quickrun, [json_file, '-o', outfile]) + assert result.exit_code == 0 + assert "Here is the result" in result.output + + assert pathlib.Path(outfile).exists() + with open(outfile, mode='r') as outf: + dct = json.load(outf, cls=JSON_HANDLER.decoder) + + assert set(dct) == {'estimate', 'uncertainty', + 'protocol_result', 'unit_results'} def test_quickrun_unit_error(): with resources.files('openfecli.tests.data') as d: From 874c619db9f7ca398052848f2b6b600e9c3342cc Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Wed, 20 Nov 2024 16:08:11 -0800 Subject: [PATCH 02/10] move default output name up --- openfecli/commands/quickrun.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index dcd6f9d33..445159eea 100644 --- a/openfecli/commands/quickrun.py +++ b/openfecli/commands/quickrun.py @@ -97,6 +97,10 @@ def quickrun(transformation, work_dir, output): # TODO: change this to `Transformation.load(transformation)` dct = json.load(transformation, cls=JSON_HANDLER.decoder) trans = gufe.Transformation.from_dict(dct) + + if output is None: + output = work_dir / (str(trans.key) + '_results.json') + write("Planning simulations for this edge...") dag = trans.create() write("Starting the simulations for this edge...") @@ -126,8 +130,6 @@ def quickrun(transformation, work_dir, output): } } - if output is None: - output = work_dir / (str(trans.key) + '_results.json') with open(output, mode='w') as outf: json.dump(out_dict, outf, cls=JSON_HANDLER.encoder) From 9abb07350c2fe6e63d6f2ab70cf4c58433e598b0 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Wed, 20 Nov 2024 16:17:04 -0800 Subject: [PATCH 03/10] adding file-writability check before simulatin execution --- openfecli/commands/quickrun.py | 9 +++++++++ openfecli/tests/commands/test_quickrun.py | 12 +++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index 445159eea..03040b2e5 100644 --- a/openfecli/commands/quickrun.py +++ b/openfecli/commands/quickrun.py @@ -101,6 +101,15 @@ def quickrun(transformation, work_dir, output): if output is None: output = work_dir / (str(trans.key) + '_results.json') + try: + with open(output, mode='w') as outf: + # json.dump(out_dict, outf, cls=JSON_HANDLER.encoder) + outf.write("in-progress") + except FileNotFoundError as e: + raise click.ClickException(f"Unable to write to {output}, please provide a valid path.") + + # TODO: if it's just a missing directory, should we try creating it? + write("Planning simulations for this edge...") dag = trans.create() write("Starting the simulations for this edge...") diff --git a/openfecli/tests/commands/test_quickrun.py b/openfecli/tests/commands/test_quickrun.py index 1cd7b3119..b7edb3502 100644 --- a/openfecli/tests/commands/test_quickrun.py +++ b/openfecli/tests/commands/test_quickrun.py @@ -58,16 +58,10 @@ def test_quickrun_output_file_in_nonexistent_directory(json_file): runner = CliRunner() outfile = "not_dir/foo.json" result = runner.invoke(quickrun, [json_file, '-o', outfile]) - assert result.exit_code == 0 - assert "Here is the result" in result.output - - assert pathlib.Path(outfile).exists() - with open(outfile, mode='r') as outf: - dct = json.load(outf, cls=JSON_HANDLER.decoder) - - assert set(dct) == {'estimate', 'uncertainty', - 'protocol_result', 'unit_results'} + assert result.exit_code == 1 # TODO: which error type is appropriate here? + assert "Unable to write" in result.output + def test_quickrun_unit_error(): with resources.files('openfecli.tests.data') as d: json_file = str(d / 'bad_transformation.json') From dcdf505cfc3d8b0f39e3e4b40cbec55dbadb0cb4 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Wed, 20 Nov 2024 16:18:35 -0800 Subject: [PATCH 04/10] whitespace :( --- openfecli/tests/commands/test_quickrun.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openfecli/tests/commands/test_quickrun.py b/openfecli/tests/commands/test_quickrun.py index b7edb3502..549d3193f 100644 --- a/openfecli/tests/commands/test_quickrun.py +++ b/openfecli/tests/commands/test_quickrun.py @@ -60,7 +60,6 @@ def test_quickrun_output_file_in_nonexistent_directory(json_file): result = runner.invoke(quickrun, [json_file, '-o', outfile]) assert result.exit_code == 1 # TODO: which error type is appropriate here? assert "Unable to write" in result.output - def test_quickrun_unit_error(): with resources.files('openfecli.tests.data') as d: From 32a5590870009880febd709eb6c6bc2a7a82659a Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 21 Nov 2024 08:53:17 -0800 Subject: [PATCH 05/10] adding TODO --- openfecli/commands/quickrun.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index 03040b2e5..7026ed2b0 100644 --- a/openfecli/commands/quickrun.py +++ b/openfecli/commands/quickrun.py @@ -102,9 +102,9 @@ def quickrun(transformation, work_dir, output): output = work_dir / (str(trans.key) + '_results.json') try: + # make sure the output file is writeable before running simulations with open(output, mode='w') as outf: - # json.dump(out_dict, outf, cls=JSON_HANDLER.encoder) - outf.write("in-progress") + outf.write("in-progress") # TODO: do we just want to pass here? except FileNotFoundError as e: raise click.ClickException(f"Unable to write to {output}, please provide a valid path.") From 77365592d3e7e349ae41209da9be0f9968667d83 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 21 Nov 2024 13:24:45 -0800 Subject: [PATCH 06/10] moving to click function --- openfecli/commands/quickrun.py | 18 ++++-------------- openfecli/parameters/output.py | 12 ++++++++++-- openfecli/tests/commands/test_quickrun.py | 6 ++++-- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index 7026ed2b0..dc827f88a 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 @@ -36,7 +36,7 @@ def _format_exception(exception) -> str: 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, + callback=validate_outfile, ) @print_duration def quickrun(transformation, work_dir, output): @@ -98,18 +98,6 @@ def quickrun(transformation, work_dir, output): dct = json.load(transformation, cls=JSON_HANDLER.decoder) trans = gufe.Transformation.from_dict(dct) - if output is None: - output = work_dir / (str(trans.key) + '_results.json') - - try: - # make sure the output file is writeable before running simulations - with open(output, mode='w') as outf: - outf.write("in-progress") # TODO: do we just want to pass here? - except FileNotFoundError as e: - raise click.ClickException(f"Unable to write to {output}, please provide a valid path.") - - # TODO: if it's just a missing directory, should we try creating it? - write("Planning simulations for this edge...") dag = trans.create() write("Starting the simulations for this edge...") @@ -139,6 +127,8 @@ def quickrun(transformation, work_dir, output): } } + if output is None: + output = work_dir / (str(trans.key) + '_results.json') with open(output, mode='w') as outf: json.dump(out_dict, outf, cls=JSON_HANDLER.encoder) 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 549d3193f..21948bd8e 100644 --- a/openfecli/tests/commands/test_quickrun.py +++ b/openfecli/tests/commands/test_quickrun.py @@ -1,6 +1,7 @@ import pytest import click from importlib import resources +import os import pathlib import json from click.testing import CliRunner @@ -55,11 +56,12 @@ def test_quickrun_output_file_exists(json_file): 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 == 1 # TODO: which error type is appropriate here? - assert "Unable to write" in result.output + assert result.exit_code == 2 + assert "Cannot write" in result.output def test_quickrun_unit_error(): with resources.files('openfecli.tests.data') as d: From a03a0122be326dcc9232966e42b3f90b90edac7a Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 21 Nov 2024 14:49:26 -0800 Subject: [PATCH 07/10] remove writable option since it does nothing --- openfecli/commands/quickrun.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index dc827f88a..be33caa17 100644 --- a/openfecli/commands/quickrun.py +++ b/openfecli/commands/quickrun.py @@ -33,8 +33,7 @@ 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), + type=click.Path(dir_okay=False, file_okay=True, path_type=pathlib.Path), help="output file (JSON format) for the final results", callback=validate_outfile, ) @@ -97,7 +96,6 @@ def quickrun(transformation, work_dir, output): # TODO: change this to `Transformation.load(transformation)` dct = json.load(transformation, cls=JSON_HANDLER.decoder) trans = gufe.Transformation.from_dict(dct) - write("Planning simulations for this edge...") dag = trans.create() write("Starting the simulations for this edge...") From e40a9ea74d35fb66677ea8f7a3f7483a7dfdc301 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Thu, 21 Nov 2024 14:51:15 -0800 Subject: [PATCH 08/10] removing unused import --- openfecli/tests/commands/test_quickrun.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openfecli/tests/commands/test_quickrun.py b/openfecli/tests/commands/test_quickrun.py index 21948bd8e..b82d281e5 100644 --- a/openfecli/tests/commands/test_quickrun.py +++ b/openfecli/tests/commands/test_quickrun.py @@ -1,7 +1,6 @@ import pytest import click from importlib import resources -import os import pathlib import json from click.testing import CliRunner From e879f13a0e008d6c949573aac36ce0b44a180002 Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Fri, 22 Nov 2024 09:39:47 -0800 Subject: [PATCH 09/10] adding news entry --- news/986_input_validation.rst | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) 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..786c4b612 --- /dev/null +++ b/news/986_input_validation.rst @@ -0,0 +1,23 @@ +**Added:** + +* + +**Changed:** + +* ``openfe quickrun`` now fails up-front when the user-supplied output file (``-o``) is invalid. + +**Deprecated:** + +* + +**Removed:** + +* + +**Fixed:** + +* + +**Security:** + +* From 985cdca58e6fdc70dee452d0aadd305938da9d6c Mon Sep 17 00:00:00 2001 From: Alyssa Travitz Date: Wed, 4 Dec 2024 16:17:14 -0800 Subject: [PATCH 10/10] adding clarity to docs --- news/986_input_validation.rst | 2 +- openfecli/commands/quickrun.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/news/986_input_validation.rst b/news/986_input_validation.rst index 786c4b612..adeed8591 100644 --- a/news/986_input_validation.rst +++ b/news/986_input_validation.rst @@ -4,7 +4,7 @@ **Changed:** -* ``openfe quickrun`` now fails up-front when the user-supplied output file (``-o``) is invalid. +* ``openfe quickrun`` now fails up-front when the user-supplied output file (``-o``) already exists or has a non-existent parent directory. **Deprecated:** diff --git a/openfecli/commands/quickrun.py b/openfecli/commands/quickrun.py index be33caa17..c7dd6f833 100644 --- a/openfecli/commands/quickrun.py +++ b/openfecli/commands/quickrun.py @@ -34,7 +34,7 @@ def _format_exception(exception) -> str: @click.option( 'output', '-o', default=None, type=click.Path(dir_okay=False, file_okay=True, path_type=pathlib.Path), - help="output file (JSON format) for the final results", + help="Filepath at which to create and write the JSON-formatted results.", callback=validate_outfile, ) @print_duration