diff --git a/changes/4533.bugfix.md b/changes/4533.bugfix.md new file mode 100644 index 0000000000..c8587e300c --- /dev/null +++ b/changes/4533.bugfix.md @@ -0,0 +1,17 @@ +Mimic fsspec behavior when parsing file:// URLs + +File URLs like + +`file://a` + +were being parsed in `parse_store_url` with a path like + +`ParsedStoreUrl(..., path="")` + +instead of + +`ParsedStoreUrl(..., path="./a")` + +which is the behavior in `fsspec`. This also makes resolves the parsed +paths to their absolute representation immediately, to prevent eventual +changes in CWD from affecting the resolution of the path later. diff --git a/src/zarr/storage/_utils.py b/src/zarr/storage/_utils.py index ca10b0679e..06c1e1f30d 100644 --- a/src/zarr/storage/_utils.py +++ b/src/zarr/storage/_utils.py @@ -90,16 +90,24 @@ def parse_store_url(url: str) -> ParsedStoreUrl: parsed = urlparse(url) - # netloc is the "host" part (store name for memory://, bucket for s3://, etc.) - name = parsed.netloc or None - - # For URLs with a scheme and netloc (like memory://store/path or s3://bucket/key), - # strip the leading slash from the path component. - # For local paths (no scheme), preserve the path as-is. - if parsed.scheme and parsed.netloc: - path = parsed.path.lstrip("/") + # Since we're allowing windows paths to carry URL separator characters, + # we'll do the same for posix paths as well + if parsed.scheme == "": + abs_path = Path(url).absolute() + return ParsedStoreUrl(scheme="", name=None, path=str(abs_path), raw=url) + + if parsed.scheme == "file": + if parsed.query: + raise ValueError(f"Query is not supported in file:// URLs: {parsed.query}") + if parsed.fragment: + raise ValueError(f"Fragments are not supported in file:// URLs: {parsed.fragment}") + name = None + # mimic fsspec and interpret file://a/b as ./a/b + path = str(Path(parsed.netloc + parsed.path).absolute()) else: - path = parsed.path + # netloc is the "host" part (store name for memory://, bucket for s3://, etc.) + name = parsed.netloc or None + path = parsed.path.lstrip("/") return ParsedStoreUrl(scheme=parsed.scheme, name=name, path=path, raw=url) diff --git a/tests/test_store/test_utils.py b/tests/test_store/test_utils.py index 291526fab8..f3700e3a47 100644 --- a/tests/test_store/test_utils.py +++ b/tests/test_store/test_utils.py @@ -1,6 +1,7 @@ from __future__ import annotations import sys +from typing import TYPE_CHECKING from unittest.mock import patch import pytest @@ -9,6 +10,9 @@ from zarr.core.buffer.core import default_buffer_prototype from zarr.storage._utils import ParsedStoreUrl, _normalize_byte_range_index, parse_store_url +if TYPE_CHECKING: + from pathlib import Path + class TestParseStoreUrl: """Tests for parse_store_url.""" @@ -39,18 +43,60 @@ def test_s3_url(self) -> None: scheme="s3", name="bucket", path="key", raw="s3://bucket/key" ) - def test_file_url(self) -> None: - result = parse_store_url("file:///tmp/test") - assert result.scheme == "file" + @pytest.mark.parametrize( + ("url", "expected_path"), + [ + ("file:///tmp/test", "/tmp/test"), + ("file:///a/b", "/a/b"), + ("file:///a/b/", "/a/b"), + ("file:///", "/"), + ], + ) + def test_absolute_file_url(self, url: str, expected_path: str) -> None: + """file:///a/b (empty netloc) is an absolute path.""" + result = parse_store_url(url) + assert result == ParsedStoreUrl(scheme="file", name=None, path=expected_path, raw=url) + + @pytest.mark.parametrize( + ("url", "relative_path"), + [ + ("file://a/b", "a/b"), + ("file://a/b/", "a/b"), + ("file://a", "a"), + ("file://./a/b", "a/b"), + ("file://", ""), + ], + ) + def test_relative_file_url( + self, url: str, relative_path: str, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """file://a/b is a path relative to the cwd, not a host named "a".""" + monkeypatch.chdir(tmp_path) + result = parse_store_url(url) + assert result == ParsedStoreUrl( + scheme="file", name=None, path=str(tmp_path / relative_path), raw=url + ) + + @pytest.mark.parametrize( + "url", + [ + "file:///a/b?query=1", + "file:///a/b#fragment", + ], + ) + def test_file_url_rejects_params_query_fragment(self, url: str) -> None: + with pytest.raises(ValueError, match="not supported in file:// URLs"): + parse_store_url(url) def test_local_absolute_path(self) -> None: result = parse_store_url("/local/path") assert result == ParsedStoreUrl(scheme="", name=None, path="/local/path", raw="/local/path") - def test_local_relative_path(self) -> None: + def test_local_relative_path(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.chdir(tmp_path) result = parse_store_url("relative/path") assert result == ParsedStoreUrl( - scheme="", name=None, path="relative/path", raw="relative/path" + scheme="", name=None, path=str(tmp_path / "relative/path"), raw="relative/path" ) @pytest.mark.parametrize(