Merge pull request #89 from ImageMarkup/better-metadata-errors

Ensure metadata output file is writable before download

Author: danlamanna

isic_cli/cli/metadata.py

@@ -18,7 +18,7 @@
 from rich.progress import Progress, track
 from rich.table import Table
 
-from isic_cli.cli.types import CommaSeparatedIdentifiers, SearchString
+from isic_cli.cli.types import CommaSeparatedIdentifiers, SearchString, WritableFilePath
 from isic_cli.cli.utils import _extract_metadata, suggest_guest_login
 from isic_cli.io.http import get_images, get_num_images
 
@@ -164,7 +164,7 @@ def validate(csv_file: io.TextIOWrapper):  # noqa: C901, PLR0915, PLR0912
 @click.option(
     "-o",
     "--outfile",
-    type=click.Path(file_okay=True, dir_okay=False, path_type=Path),
+    type=WritableFilePath(dir_okay=False, file_okay=True, writable=True, path_type=Path),
     help="A filepath to write the output CSV to.",
 )
 @click.pass_obj

isic_cli/cli/types.py

@@ -77,3 +77,35 @@ def convert(self, value: str, param, ctx) -> str:
                 raise
 
         return value
+
+
+class WritableFilePath(click.Path):
+    name = "writable_file_path"
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        if not self.file_okay:
+            raise ValueError("file_okay must be True")
+        elif self.dir_okay:
+            raise ValueError("dir_okay must be False")
+
+    def convert(self, value, param, ctx):
+        value = super().convert(value, param, ctx)
+
+        # writeable checks on click.Path only apply to already existing paths, see
+        # https://github.com/pallets/click/issues/2495.
+        # check if the final path is writable before going to the effort of downloading the data.
+        if value is not None and str(value) != "-":
+            try:
+                value.parent.mkdir(parents=True, exist_ok=True)
+                with value.open("w", newline="", encoding="utf8"):
+                    pass
+            except PermissionError:
+                self.fail(f"Permission denied - cannot write to '{value}'.", param, ctx)
+            except OSError as e:
+                # this is a general catch-all for weirder issues like a read only filesystem,
+                # filenames that are too long or have invalid chars, etc.
+                self.fail(f"Cannot write to '{value}'. {e!s}", param, ctx)
+
+        return value

tests/test_cli_metadata.py

@@ -2,6 +2,7 @@
 
 from pathlib import Path
 import re
+import sys
 
 import pytest
 from pytest_lazy_fixtures import lf
@@ -87,6 +88,25 @@ def test_metadata_download_file(cli_runner):
     assert re.search(r"ISIC_0000000.*Foo.*CC-0.*melanoma.*male", output), output
 
 
+@pytest.mark.skipif(sys.platform == "win32", reason="Windows does not support this test")
+@pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem")
+def test_metadata_download_file_no_write(cli_run):
+    result = cli_run(["metadata", "download", "-o", "/metadata.csv"])
+    # it's important that the exit code is 2 and not 1, because the key constraint of this
+    # functionality is that the user gets the error message before spending their time
+    # downloading the data. exit code 2 is for usage errors with click.
+    assert result.exit_code == 2, result.exception
+    assert re.search(r"Permission denied", result.output), result.output
+
+
+@pytest.mark.usefixtures("_mock_image_metadata", "_isolated_filesystem")
+def test_metadata_download_file_bad_filename(cli_run):
+    result = cli_run(["metadata", "download", "-o", f"{'1' * 255}.csv"])
+    # see comment in test_metadata_download_file_no_write for why exit code is 2
+    assert result.exit_code == 2, result.exception
+    assert re.search(r"Cannot write to", result.output), result.output
+
+
 @pytest.mark.usefixtures("_mock_image_metadata")
 @pytest.mark.parametrize(
     "cli_runner",