refactor: remove _dereference_path in favor of _join_paths

Author: d-v-b

src/zarr/storage/_common.py

@@ -24,7 +24,7 @@
 from zarr.errors import ContainsArrayAndGroupError, ContainsArrayError, ContainsGroupError
 from zarr.storage._local import LocalStore
 from zarr.storage._memory import ManagedMemoryStore, MemoryStore
-from zarr.storage._utils import _dereference_path, normalize_path, parse_store_url
+from zarr.storage._utils import _join_paths, normalize_path, parse_store_url
 
 _has_fsspec = importlib.util.find_spec("fsspec")
 if _has_fsspec:
@@ -255,10 +255,10 @@ def delete_sync(self) -> None:
 
     def __truediv__(self, other: str) -> StorePath:
         """Combine this store path with another path"""
-        return self.__class__(self.store, _dereference_path(self.path, other))
+        return self.__class__(self.store, _join_paths([self.path, other]))
 
     def __str__(self) -> str:
-        return _dereference_path(str(self.store), self.path)
+        return _join_paths([str(self.store), self.path])
 
     def __repr__(self) -> str:
         return f"StorePath({self.store.__class__.__name__}, '{self}')"

src/zarr/storage/_fsspec.py

@@ -16,7 +16,7 @@
 )
 from zarr.core.buffer import Buffer
 from zarr.errors import ZarrUserWarning
-from zarr.storage._utils import _dereference_path
+from zarr.storage._utils import _join_paths
 
 if TYPE_CHECKING:
     from collections.abc import AsyncIterator, Iterable
@@ -282,7 +282,7 @@ async def get(
         # docstring inherited
         if not self._is_open:
             await self._open()
-        path = _dereference_path(self.path, key)
+        path = _join_paths([self.path, key])
 
         try:
             if byte_range is None:
@@ -329,7 +329,7 @@ async def set(
             raise TypeError(
                 f"FsspecStore.set(): `value` must be a Buffer instance. Got an instance of {type(value)} instead."
             )
-        path = _dereference_path(self.path, key)
+        path = _join_paths([self.path, key])
         # write data
         if byte_range:
             raise NotImplementedError
@@ -338,7 +338,7 @@ async def set(
     async def delete(self, key: str) -> None:
         # docstring inherited
         self._check_writable()
-        path = _dereference_path(self.path, key)
+        path = _join_paths([self.path, key])
         try:
             await self.fs._rm(path)
         except FileNotFoundError:
@@ -354,14 +354,14 @@ async def delete_dir(self, prefix: str) -> None:
             )
         self._check_writable()
 
-        path_to_delete = _dereference_path(self.path, prefix)
+        path_to_delete = _join_paths([self.path, prefix])
 
         with suppress(*self.allowed_exceptions):
             await self.fs._rm(path_to_delete, recursive=True)
 
     async def exists(self, key: str) -> bool:
         # docstring inherited
-        path = _dereference_path(self.path, key)
+        path = _join_paths([self.path, key])
         exists: bool = await self.fs._exists(path)
         return exists
 
@@ -378,7 +378,7 @@ async def get_partial_values(
             starts: list[int | None] = []
             stops: list[int | None] = []
             for key, byte_range in key_ranges:
-                paths.append(_dereference_path(self.path, key))
+                paths.append(_join_paths([self.path, key]))
                 if byte_range is None:
                     starts.append(None)
                     stops.append(None)
@@ -429,7 +429,7 @@ async def list_prefix(self, prefix: str) -> AsyncIterator[str]:
             yield onefile.removeprefix(f"{self.path}/")
 
     async def getsize(self, key: str) -> int:
-        path = _dereference_path(self.path, key)
+        path = _join_paths([self.path, key])
         info = await self.fs._info(path)
 
         size = info.get("size")

src/zarr/storage/_memory.py

@@ -11,7 +11,7 @@
 from zarr.core.buffer.core import default_buffer_prototype
 from zarr.core.common import concurrent_map
 from zarr.storage._utils import (
-    _dereference_path,
+    _join_paths,
     _normalize_byte_range_index,
     normalize_path,
     parse_store_url,
@@ -700,7 +700,7 @@ def __init__(self, name: str | None = None, *, path: str = "", read_only: bool =
         self.path = normalize_path(path)
 
     def __str__(self) -> str:
-        return _dereference_path(f"memory://{self._name}", self.path)
+        return _join_paths([f"memory://{self._name}", self.path])
 
     def __repr__(self) -> str:
         return f"ManagedMemoryStore('{self}')"
@@ -792,7 +792,7 @@ async def get(
     ) -> Buffer | None:
         # docstring inherited
         return await super().get(
-            _dereference_path(self.path, key), prototype=prototype, byte_range=byte_range
+            _join_paths([self.path, key]), prototype=prototype, byte_range=byte_range
         )
 
     async def get_partial_values(
@@ -801,26 +801,24 @@ async def get_partial_values(
         key_ranges: Iterable[tuple[str, ByteRequest | None]],
     ) -> list[Buffer | None]:
         # docstring inherited
-        key_ranges = [
-            (_dereference_path(self.path, key), byte_range) for key, byte_range in key_ranges
-        ]
+        key_ranges = [(_join_paths([self.path, key]), byte_range) for key, byte_range in key_ranges]
         return await super().get_partial_values(prototype, key_ranges)
 
     async def exists(self, key: str) -> bool:
         # docstring inherited
-        return await super().exists(_dereference_path(self.path, key))
+        return await super().exists(_join_paths([self.path, key]))
 
     async def set(self, key: str, value: Buffer, byte_range: tuple[int, int] | None = None) -> None:
         # docstring inherited
-        return await super().set(_dereference_path(self.path, key), value, byte_range=byte_range)
+        return await super().set(_join_paths([self.path, key]), value, byte_range=byte_range)
 
     async def set_if_not_exists(self, key: str, value: Buffer) -> None:
         # docstring inherited
-        return await super().set_if_not_exists(_dereference_path(self.path, key), value)
+        return await super().set_if_not_exists(_join_paths([self.path, key]), value)
 
     async def delete(self, key: str) -> None:
         # docstring inherited
-        return await super().delete(_dereference_path(self.path, key))
+        return await super().delete(_join_paths([self.path, key]))
 
     async def list(self) -> AsyncIterator[str]:
         # docstring inherited
@@ -831,16 +829,16 @@ async def list(self) -> AsyncIterator[str]:
 
     async def list_prefix(self, prefix: str) -> AsyncIterator[str]:
         # docstring inherited
-        # Don't use _dereference_path here because it strips trailing slashes,
-        # which would break prefix matching (e.g., "fo/" vs "foo/")
+        # Manual concatenation instead of _join_paths because we need "path/"
+        # as the prefix when prefix is empty (to list all keys under self.path)
         full_prefix = f"{self.path}/{prefix}" if self.path else prefix
         path_prefix = self.path + "/" if self.path else ""
         async for key in super().list_prefix(full_prefix):
             yield key.removeprefix(path_prefix)
 
     async def list_dir(self, prefix: str) -> AsyncIterator[str]:
         # docstring inherited
-        full_prefix = _dereference_path(self.path, prefix)
+        full_prefix = _join_paths([self.path, prefix])
         async for key in super().list_dir(full_prefix):
             yield key
 

src/zarr/storage/_utils.py

@@ -160,33 +160,6 @@ def _normalize_byte_range_index(data: Buffer, byte_range: ByteRequest | None) ->
     return (start, stop)
 
 
-def _dereference_path(root: str, path: str) -> str:
-    """
-    Combine a root path with a relative path.
-
-    Parameters
-    ----------
-    root : str
-        The root path.
-    path : str
-        The path relative to root.
-
-    Returns
-    -------
-    str
-        The combined path with trailing slashes removed.
-    """
-    if not isinstance(root, str):
-        msg = f"{root=} is not a string ({type(root)=})"  # type: ignore[unreachable]
-        raise TypeError(msg)
-    if not isinstance(path, str):
-        msg = f"{path=} is not a string ({type(path)=})"  # type: ignore[unreachable]
-        raise TypeError(msg)
-    root = root.rstrip("/")
-    path = f"{root}/{path}" if root else path
-    return path.rstrip("/")
-
-
 def _join_paths(paths: Iterable[str]) -> str:
     """
     Filter out instances of '' and join the remaining strings with '/'.

src/zarr/testing/strategies.py

@@ -22,7 +22,7 @@
 from zarr.core.metadata.v3 import RectilinearChunkGridMetadata, RegularChunkGridMetadata
 from zarr.core.sync import sync
 from zarr.storage import MemoryStore, StoreLike
-from zarr.storage._utils import _dereference_path, normalize_path
+from zarr.storage._utils import _join_paths, normalize_path
 from zarr.types import AnyArray
 
 TrueOrFalse = Literal[True, False]
@@ -288,7 +288,7 @@ def arrays(
 
     expected_attrs = {} if attributes is None else attributes
 
-    array_path = _dereference_path(path, name)
+    array_path = _join_paths([path, name])
     root = zarr.open_group(store, mode=open_mode, zarr_format=zarr_format)
 
     # Convert chunk grid metadata to a form create_array accepts:
@@ -621,7 +621,7 @@ def complex_rectilinear_arrays(
     store = draw(stores, label="store")
     path = draw(paths, label="array parent")
     name = draw(array_names, label="array name")
-    array_path = _dereference_path(path, name)
+    array_path = _join_paths([path, name])
 
     root = zarr.open_group(store, mode="w", zarr_format=3)
     with zarr.config.set({"array.rectilinear_chunks": True}):

tests/test_store/test_utils.py

@@ -5,8 +5,7 @@
 
 import pytest
 
-from tests.conftest import Expect, ExpectFail
-from zarr.storage._utils import ParsedStoreUrl, _dereference_path, parse_store_url
+from zarr.storage._utils import ParsedStoreUrl, parse_store_url
 
 
 class TestParseStoreUrl:
@@ -96,47 +95,3 @@ def test_drive_letter_not_special_on_non_windows(self, url: str) -> None:
             result = parse_store_url(url)
         # urlparse interprets the drive letter as a scheme
         assert result.scheme == "c"
-
-
-@pytest.mark.parametrize(
-    "case",
-    [
-        Expect(input=("root", "path"), output="root/path", id="basic"),
-        Expect(input=("root", ""), output="root", id="empty_path"),
-        Expect(input=("", "path"), output="path", id="empty_root"),
-        Expect(input=("", ""), output="", id="both_empty"),
-        Expect(input=("root/", "path"), output="root/path", id="root_trailing_slash"),
-        Expect(input=("root//", "path"), output="root/path", id="root_multiple_trailing_slashes"),
-        Expect(input=("root", "path/"), output="root/path", id="path_trailing_slash"),
-        Expect(input=("a/b", "c/d"), output="a/b/c/d", id="nested"),
-        Expect(input=("memory://store", "key"), output="memory://store/key", id="url_root"),
-    ],
-    ids=lambda case: case.id,
-)
-def test_dereference_path(case: Expect[tuple[str, str], str]) -> None:
-    """
-    Test the normal behavior of _dereference_path
-    """
-    root, path = case.input
-    assert _dereference_path(root, path) == case.output
-
-
-@pytest.mark.parametrize(
-    "case",
-    [
-        ExpectFail(
-            input=(1, "path"), exception=TypeError, msg="root=.*not a string", id="root_not_str"
-        ),
-        ExpectFail(
-            input=("root", 1), exception=TypeError, msg="path=.*not a string", id="path_not_str"
-        ),
-    ],
-    ids=lambda case: case.id,
-)
-def test_dereference_path_errors(case: ExpectFail[tuple[object, object]]) -> None:
-    """
-    Test that _dereference_path raises TypeError for non-string inputs.
-    """
-    root, path = case.input
-    with pytest.raises(case.exception, match=case.msg):
-        _dereference_path(root, path)  # type: ignore[arg-type]