Merge pull request #1521 from lochhh/fix-storage-win

Use POSIX separators in file-protocol storage URLs and paths

Author: dimitri-yatsenko

src/datajoint/storage.py

@@ -45,6 +45,27 @@ def is_url(path: str) -> bool:
     return path.lower().startswith(URL_PROTOCOLS)
 
 
+def _path_to_file_url(resolved_path: Path | PurePosixPath) -> str:
+    """
+    Convert an already-resolved absolute path to a ``file://`` URL.
+
+    Uses ``as_posix()`` so the same logic handles both POSIX paths (which
+    already start with ``/``) and Windows paths (``C:/...``, no leading
+    slash) without OS-specific branching.
+
+    Parameters
+    ----------
+    resolved_path : Path or PurePosixPath
+        Absolute, already-resolved path.
+
+    Returns
+    -------
+    str
+        ``file://`` URL.
+    """
+    return f"file:///{resolved_path.as_posix().lstrip('/')}"
+
+
 def normalize_to_url(path: str) -> str:
     """
     Normalize a path to URL form.
@@ -72,15 +93,7 @@ def normalize_to_url(path: str) -> str:
     """
     if is_url(path):
         return path
-    # Convert local path to file:// URL
-    # Ensure absolute path and proper format
-    abs_path = str(Path(path).resolve())
-    # Handle Windows paths (C:\...) vs Unix paths (/...)
-    if abs_path.startswith("/"):
-        return f"file://{abs_path}"
-    else:
-        # Windows: file:///C:/path
-        return f"file:///{abs_path.replace(chr(92), '/')}"
+    return _path_to_file_url(Path(path).resolve())
 
 
 def parse_url(url: str) -> tuple[str, str]:
@@ -418,7 +431,7 @@ def _full_path(self, path: str | PurePosixPath) -> str:
         elif self.protocol == "file":
             location = self.spec.get("location", "")
             if location:
-                return str(Path(location) / path)
+                return (Path(location) / path).as_posix()
             return path
         else:
             return self._require_adapter().full_path(self.spec, path)
@@ -453,13 +466,7 @@ def get_url(self, path: str | PurePosixPath) -> str:
         full_path = self._full_path(path)
 
         if self.protocol == "file":
-            # Ensure absolute path for file:// URL
-            abs_path = str(Path(full_path).resolve())
-            if abs_path.startswith("/"):
-                return f"file://{abs_path}"
-            else:
-                # Windows path
-                return f"file:///{abs_path.replace(chr(92), '/')}"
+            return _path_to_file_url(Path(full_path).resolve())
         elif self.protocol == "s3":
             return f"s3://{full_path}"
         elif self.protocol == "gcs":

tests/unit/test_gc.py

@@ -0,0 +1,35 @@
+"""Tests for garbage collection module."""
+
+from pathlib import PurePosixPath, PureWindowsPath
+from unittest.mock import MagicMock
+
+import pytest
+
+from datajoint import storage
+from datajoint.gc import GarbageCollector
+from datajoint.storage import StorageBackend
+
+
+@pytest.mark.parametrize(
+    ("path_cls", "location"),
+    [
+        pytest.param(PureWindowsPath, "data\\blobs", id="windows"),
+        pytest.param(PurePosixPath, "data/blobs", id="posix"),
+    ],
+)
+def test_delete_schema_path_prunes_parent_dir(monkeypatch, path_cls, location):
+    """Pruning's `rsplit("/", 1)` needs forward slashes; `windows` param catches
+    a regression to `str(Path(...))`, `posix` pins the already-correct case."""
+    monkeypatch.setattr(storage, "Path", path_cls)
+    backend = StorageBackend.__new__(StorageBackend)
+    backend.spec = {"protocol": "file", "location": location}
+    backend.protocol = "file"
+    backend._fs = MagicMock()
+    backend.fs.exists.return_value = True
+    # Ensure rmdir called only once: non-empty dir stops the walk one level up
+    backend.fs.ls.side_effect = lambda p: [] if p == "data/blobs/schema/ab/cd" else ["sibling"]
+    collector = GarbageCollector.__new__(GarbageCollector)
+    collector.backend = backend
+    assert collector.delete_schema_path("schema/ab/cd/hash123") is True
+    backend.fs.rm.assert_called_once_with("data/blobs/schema/ab/cd/hash123")
+    backend.fs.rmdir.assert_called_once_with("data/blobs/schema/ab/cd")

tests/unit/test_storage_adapter.py

@@ -1,14 +1,18 @@
 """Tests for the StorageAdapter plugin system."""
 
+import re
+from pathlib import PureWindowsPath
+
 import pytest
 
 import datajoint as dj
+from datajoint import storage
 from datajoint.errors import DataJointError
 from datajoint.storage import StorageBackend
 from datajoint.storage_adapter import (
+    _COMMON_STORE_KEYS,
     StorageAdapter,
     _adapter_registry,
-    _COMMON_STORE_KEYS,
     get_storage_adapter,
 )
 
@@ -193,6 +197,27 @@ def test_unsupported_protocol_get_url_raises(self):
         with pytest.raises(DataJointError, match="Unsupported storage protocol"):
             backend.get_url("schema/file.dat")
 
+    def test_file_protocol_full_path_uses_forward_slashes(self, monkeypatch):
+        """`_full_path` must return forward slashes to match fsspec's walk()
+        output (gc.py relies on this for string-prefix stripping)."""
+        # monkeypatch to PureWindowsPath so that the test is platform-independent
+        monkeypatch.setattr(storage, "Path", PureWindowsPath)
+        backend = StorageBackend.__new__(StorageBackend)
+        backend.spec = {"protocol": "file", "location": "data\\blobs"}
+        backend.protocol = "file"
+        result = backend._full_path("schema/ab/cd/hash123")
+        assert result == "data/blobs/schema/ab/cd/hash123"
+
+    def test_file_protocol_get_url_no_backslash(self, tmp_path):
+        """`get_url` must produce a valid file:// URL (forward slashes only)
+        on whatever OS the test runs on, including Windows."""
+        backend = StorageBackend.__new__(StorageBackend)
+        backend.spec = {"protocol": "file", "location": str(tmp_path)}
+        backend.protocol = "file"
+        result = backend.get_url("schema/ab/cd/hash123")
+        # exactly 3 slashes, no backslash and disregard tmp_path
+        assert re.fullmatch(r"file:///[^/\\][^\\]*/schema/ab/cd/hash123", result)
+
 
 class TestGetStoreSpecPluginDelegation:
     """Tests for plugin protocol handling in Config.get_store_spec()."""

tests/unit/test_storage_urls.py

@@ -1,9 +1,12 @@
 """Unit tests for storage URL functions."""
 
+from pathlib import PurePosixPath, PureWindowsPath
+
 import pytest
 
 from datajoint.storage import (
     URL_PROTOCOLS,
+    _path_to_file_url,
     is_url,
     normalize_to_url,
     parse_url,
@@ -79,6 +82,18 @@ def test_relative_path_becomes_absolute(self):
         assert "/" in url[7:]  # After "file://"
 
 
+class TestPathToFileUrl:
+    """Test _path_to_file_url function (platform-independent via Pure*Path)."""
+
+    def test_posix_path(self):
+        url = _path_to_file_url(PurePosixPath("/data/file.dat"))
+        assert url == "file:///data/file.dat"
+
+    def test_windows_path_no_backslash_in_result(self):
+        url = _path_to_file_url(PureWindowsPath("C:\\data\\file.dat"))
+        assert url == "file:///C:/data/file.dat"
+
+
 class TestParseUrl:
     """Test parse_url function."""