Change make_path_safe to is_path_safe and update tests to reflect this

src/openflexure_microscope_server/stitching.py

@@ -18,7 +18,7 @@ from pydantic import BaseModel
 
 import labthings_fastapi as lt
 
-from openflexure_microscope_server.utilities import make_path_safe
+from openflexure_microscope_server.utilities import is_path_safe
 
 IS_WINDOWS = os.name == "nt"
 
@@ -68,7 +68,7 @@ def validate_command(cmd: list[str]) -> None:
     """Validate that the command only contains characters that are allowed in a path.
 
     The values in the commands should be numbers, commandline flags, paths, and
-    executables. All of these should be allowed by ``make_path_safe``.
+    executables. All of these should be allowed by ``is_path_safe``.
 
     This also checks against a blacklist of forbidden commands for defense-in-depth.
 
@@ -82,8 +82,7 @@ def validate_command(cmd: list[str]) -> None:
             )
 
         # Ensure characters are safe for a path component
-        _, error_raised = make_path_safe(element)
+        if not is_path_safe(element):
-        if error_raised:
             raise StitcherValidationError(
                 f"Invalid stitching command: Element '{element}' contains unsafe characters."
             )
@@ -155,8 +154,7 @@ class BaseStitcher:
 
         :raises RuntimeError: if inputs are unsafe.
         """
-        _, error_raised = make_path_safe(self.images_dir)
+        if not is_path_safe(self.images_dir):
-        if error_raised:
             raise StitcherValidationError(
                 "Invalid directory path: Contains unsafe characters."
             )

src/openflexure_microscope_server/utilities.py

@@ -110,7 +110,7 @@ _WINDOWS_RESERVED_NAMES = {
 }
 
 
-def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]:
+def is_path_safe(unsafe_path_string: str) -> bool:
     """Check if a file path has any unsafe elements in it.
 
     The path is not coerced into a safe form because if we ask
@@ -119,8 +119,7 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]:
 
     :param unsafe_path_string: The original path string to sanitise.
 
-    :returns: The input string unchanged and a boolean indicating if
+    :returns: A boolean indicating if any unsafe features were found.
-    any unsafe features were found.
     """
     # Split by separators first to sanitise components independently
 
@@ -195,14 +194,13 @@ def make_path_safe(unsafe_path_string: str) -> tuple[str, bool]:
             )
             reserved_word = True
 
-    warning_raised = (
+    return not (
         unsafe_relative_path
         or trailing_dots
         or trailing_whitespace
         or unsafe_characters
         or reserved_word
     )
-    return unsafe_path_string, warning_raised
 
 
 def make_name_safe(unsafe_name_string: str) -> str:

tests/unit_tests/test_utilities.py

@@ -7,12 +7,14 @@ import pytest
 
 from openflexure_microscope_server.utilities import (
     _WINDOWS_RESERVED_NAMES,
+    is_path_safe,
     make_name_safe,
-    make_path_safe,
 )
 
 PATHS_WITH_DOTS = {"a./b/c.", "path./to/file.", "./path./to/file."}
 
+PATHS_WITH_SPACES = {"path /to /file ", "path/to /file", "path/a long path name /file"}
+
 RESERVED_NAME_PATHS = {"path/CON/file", "path/NUL.txt/file", "C:/AUX/test"}
 
 if sys.platform.startswith("win"):
@@ -76,32 +78,26 @@ def test_make_name_safe_reserved_names_false_positives():
     assert make_name_safe("CHILLI CON CARNE") == "chilli_con_carne"
 
 
-def test_make_path_safe_basic(caplog):
+def test_is_path_safe_basic(caplog):
-    """Test basic functionality of make_path_safe."""
+    """Test basic functionality of is_path_safe."""
     # Note: behavior depends on platform for separators
     with caplog.at_level(logging.WARNING):
         if sys.platform.startswith("win"):
-            assert make_path_safe("C:\\path\\to/file")[0] == "C:\\path\\to/file"
+            assert is_path_safe("C:\\path\\to/file")
-            assert (
+            assert is_path_safe("C:\\a very long path\\to/file")
-                make_path_safe("C:\\a very long path\\to/file")[0]
-                == "C:\\a very long path\\to/file"
-            )
         else:
-            assert make_path_safe("path/to/file")[0] == "path/to/file"
+            assert is_path_safe("path/to/file")
-            assert (
+            assert is_path_safe("a very long path/to/file")
-                make_path_safe("a very long path/to/file")[0]
-                == "a very long path/to/file"
-            )
 
         # No errors should be raised. These paths are safe.
         assert len(caplog.records) == 0
 
 
 @pytest.mark.parametrize("path", UNSAFE_CHARACTER_PATHS)
-def test_make_path_safe_unsafe_characters_in_components(caplog, path):
+def test_is_path_safe_unsafe_characters_in_components(caplog, path):
     """Test unsafe characters within path components."""
     with caplog.at_level(logging.WARNING):
-        assert make_path_safe(path)[0] == path
+        assert not is_path_safe(path)
         assert (
             f"{path} contains characters that may be unsafe on this platform."
             in caplog.messages
@@ -109,68 +105,52 @@ def test_make_path_safe_unsafe_characters_in_components(caplog, path):
 
 
 @pytest.mark.parametrize("path", RESERVED_NAME_PATHS)
-def test_make_path_safe_reserved_in_components(caplog, path):
+def test_is_path_safe_reserved_in_components(caplog, path):
     """Test reserved names within path components."""
     with caplog.at_level(logging.WARNING):
-        assert make_path_safe(path)[0] == path
+        assert not is_path_safe(path)
         assert (
             f"{path} contains a reserved system name and may cause issues."
             in caplog.messages
         )
 
 
-def test_make_path_safe_reserved_name_components(caplog):
+def test_is_path_safe_reserved_name_components(caplog):
     """Test reserved names are okay as part of a larger dir name."""
     with caplog.at_level(logging.WARNING):
-        assert make_path_safe("chilli con carne/recipe")[0] == "chilli con carne/recipe"
+        assert is_path_safe("chilli con carne/recipe")
         assert len(caplog.records) == 0
 
 
-def test_make_path_safe_trailing_space_in_components(caplog):
+@pytest.mark.parametrize("path", PATHS_WITH_SPACES)
+def test_is_path_safe_trailing_space_in_components(caplog, path):
     """Test trailing spaces in path components."""
     with caplog.at_level(logging.WARNING):
-        # Test Windows - cannot have trailingspaces
+        # Test Windows - cannot have trailing spaces
         if sys.platform.startswith("win"):
-            assert make_path_safe("path /to /file ")[0] == "path /to /file "
+            assert not is_path_safe(path)
-            assert make_path_safe("path/to /file")[0] == "path/to /file"
+            assert len(caplog.records) == 1
-            # Check directories can have spaces in them but not at the end
+            assert f"{path} contains unsafe trailing spaces." in caplog.messages
-            assert (
-                make_path_safe("path/a long path name /file")[0]
-                == "path/a long path name /file"
-            )
-            assert len(caplog.records) == 3
-            assert "path /to /file  contains unsafe trailing spaces." in caplog.messages
-            assert "path/to /file contains unsafe trailing spaces." in caplog.messages
-            assert (
-                "path/a long path name /file contains unsafe trailing spaces."
-                in caplog.messages
-            )
         else:
             # Trailing spaces allows in POSIX systems
-            assert make_path_safe("path /to /file ")[0] == "path /to /file "
+            assert is_path_safe(path)
-            assert make_path_safe("path/to /file")[0] == "path/to /file"
-            # Check directories can have spaces in them but not at the end
-            assert (
-                make_path_safe("path/a long path name /file")[0]
-                == "path/a long path name /file"
-            )
             # No warnings should be raised
             assert len(caplog.records) == 0
 
 
-def test_make_path_safe_relative(caplog):
+def test_is_path_safe_relative(caplog):
     """Test that relative path components are preserved."""
     # Relative imports ./foo, ../foo or ../../foo are allowed.
     with caplog.at_level(logging.WARNING):
-        assert make_path_safe("./openflexure/data/")[0] == "./openflexure/data/"
+        assert is_path_safe("./openflexure/data/")
-        assert make_path_safe("../openflexure/data/")[0] == "../openflexure/data/"
+        assert is_path_safe("../openflexure/data/")
-        assert make_path_safe("../../openflexure/data/")[0] == "../../openflexure/data/"
+        assert is_path_safe("../../openflexure/data/")
-        assert make_path_safe(".")[0] == "."
+        assert is_path_safe(".")
-        assert make_path_safe("..")[0] == ".."
+        assert is_path_safe("..")
 
         assert len(caplog.records) == 0
 
-        assert make_path_safe("a_bad/../relative_path")[0] == "a_bad/../relative_path"
+        assert not is_path_safe("a_bad/../relative_path")
         assert len(caplog.records) == 1
         assert (
             "File path a_bad/../relative_path may be unsafe due to unexpected relative navigation."
@@ -179,10 +159,10 @@ def test_make_path_safe_relative(caplog):
 
 
 @pytest.mark.parametrize("filepath", PATHS_WITH_DOTS)
-def test_make_path_safe_dots_warning(caplog, filepath):
+def test_is_path_safe_dots_warning(caplog, filepath):
     """Test that a user is warned about trailing dots in their file paths. The paths should remain unchanged."""
     with caplog.at_level(logging.WARNING):
-        assert make_path_safe(filepath)[0] == filepath
+        assert not is_path_safe(filepath)
         assert (
             f"File path {filepath} may be unsafe due to trailing dots."
             in caplog.messages