diff --git a/src/datajoint/storage.py b/src/datajoint/storage.py index 39a21ce46..4f91b3d33 100644 --- a/src/datajoint/storage.py +++ b/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": diff --git a/tests/unit/test_gc.py b/tests/unit/test_gc.py new file mode 100644 index 000000000..d81762ccc --- /dev/null +++ b/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") diff --git a/tests/unit/test_storage_adapter.py b/tests/unit/test_storage_adapter.py index a8ef4a99a..b04c4903c 100644 --- a/tests/unit/test_storage_adapter.py +++ b/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().""" diff --git a/tests/unit/test_storage_urls.py b/tests/unit/test_storage_urls.py index 649d695b2..0eef81309 100644 --- a/tests/unit/test_storage_urls.py +++ b/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."""