From c0feec5ad379c2c7c2cee1181605c99c3c1977f1 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 19 Feb 2025 13:46:58 +0100 Subject: [PATCH 1/6] add failing group name tests --- tests/test_group.py | 49 +++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/tests/test_group.py b/tests/test_group.py index 144054605e..b28118e54c 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -24,6 +24,7 @@ from zarr.errors import ContainsArrayError, ContainsGroupError from zarr.storage import LocalStore, MemoryStore, StorePath, ZipStore from zarr.storage._common import make_store_path +from zarr.storage._utils import normalize_path from zarr.testing.store import LatencyStore from .conftest import parse_store @@ -111,24 +112,27 @@ async def test_create_creates_parents(store: Store, zarr_format: ZarrFormat) -> assert g.attrs == {} -def test_group_name_properties(store: Store, zarr_format: ZarrFormat) -> None: +@pytest.mark.parametrize("store", ["memory"], indirect=True) +@pytest.mark.parametrize("root_name", ["", "/", "a", "/a"]) +@pytest.mark.parametrize("branch_name", ["foo", "/foo", "foo/bar", "/foo/bar"]) +def test_group_name_properties( + store: Store, zarr_format: ZarrFormat, root_name: str, branch_name: str +) -> None: """ - Test basic properties of groups + Test that the path, name, and basename attributes of a group and its subgroups are consistent """ - root = Group.from_store(store=store, zarr_format=zarr_format) - assert root.path == "" - assert root.name == "/" - assert root.basename == "" - - foo = root.create_group("foo") - assert foo.path == "foo" - assert foo.name == "/foo" - assert foo.basename == "foo" - - bar = root.create_group("foo/bar") - assert bar.path == "foo/bar" - assert bar.name == "/foo/bar" - assert bar.basename == "bar" + root = Group.from_store(store=StorePath(store=store, path=root_name), zarr_format=zarr_format) + assert root.path == normalize_path(root_name) + assert root.name == "/" + root.path + assert root.basename == root.path + + branch = root.create_group(branch_name) + if root.path == "": + assert branch.path == normalize_path(branch_name) + else: + assert branch.path == "/".join([root.path, normalize_path(branch_name)]) + assert branch.name == "/" + branch.path + assert branch.basename == branch_name.split("/")[-1] @pytest.mark.parametrize("consolidated_metadata", [True, False]) @@ -601,11 +605,13 @@ async def test_group_update_attributes_async(store: Store, zarr_format: ZarrForm @pytest.mark.parametrize("method", ["create_array", "array"]) +@pytest.mark.parametrize("name", ["a", "/a"]) def test_group_create_array( store: Store, zarr_format: ZarrFormat, overwrite: bool, method: Literal["create_array", "array"], + name: str, ) -> None: """ Test `Group.from_store` @@ -616,23 +622,26 @@ def test_group_create_array( data = np.arange(np.prod(shape)).reshape(shape).astype(dtype) if method == "create_array": - array = group.create_array(name="array", shape=shape, dtype=dtype) + array = group.create_array(name=name, shape=shape, dtype=dtype) array[:] = data elif method == "array": with pytest.warns(DeprecationWarning): - array = group.array(name="array", data=data, shape=shape, dtype=dtype) + array = group.array(name=name, data=data, shape=shape, dtype=dtype) else: raise AssertionError if not overwrite: if method == "create_array": with pytest.raises(ContainsArrayError): - a = group.create_array(name="array", shape=shape, dtype=dtype) + a = group.create_array(name=name, shape=shape, dtype=dtype) a[:] = data elif method == "array": with pytest.raises(ContainsArrayError), pytest.warns(DeprecationWarning): - a = group.array(name="array", shape=shape, dtype=dtype) + a = group.array(name=name, shape=shape, dtype=dtype) a[:] = data + + assert array.path == normalize_path(name) + assert array.name == "/" + normalize_path(name) assert array.shape == shape assert array.dtype == np.dtype(dtype) assert np.array_equal(array[:], data) From 322acf790fc2489af406d55c1608048f4c998f77 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 19 Feb 2025 13:47:28 +0100 Subject: [PATCH 2/6] normalize paths in storepath constructor --- src/zarr/storage/_common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zarr/storage/_common.py b/src/zarr/storage/_common.py index 6ab539bb0a..d81369f142 100644 --- a/src/zarr/storage/_common.py +++ b/src/zarr/storage/_common.py @@ -41,7 +41,7 @@ class StorePath: def __init__(self, store: Store, path: str = "") -> None: self.store = store - self.path = path + self.path = normalize_path(path) @property def read_only(self) -> bool: From f342bb9aff7d5b280add2c1f00259d98188f1dab Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 26 Feb 2025 18:33:20 +0100 Subject: [PATCH 3/6] add asserts to hypothesis tests --- src/zarr/testing/strategies.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 96d664f5aa..2fbb425dd8 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -19,6 +19,7 @@ from zarr.core.sync import sync from zarr.storage import MemoryStore, StoreLike from zarr.storage._common import _dereference_path +from zarr.storage._utils import normalize_path # Copied from Xarray _attr_keys = st.text(st.characters(), min_size=1) @@ -277,6 +278,8 @@ def arrays( if a.metadata.zarr_format == 3: assert a.fill_value is not None assert a.name is not None + assert a.path == normalize_path(array_path) + assert a.name == "/" + a.path assert isinstance(root[array_path], Array) assert nparray.shape == a.shape assert chunk_shape == a.chunks From de84391c28fd1ac6780cdd0fcc1053d619db27ab Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 26 Feb 2025 18:33:41 +0100 Subject: [PATCH 4/6] test group creation too --- tests/test_group.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/test_group.py b/tests/test_group.py index 820c3fe3f0..fbc4a337ed 100644 --- a/tests/test_group.py +++ b/tests/test_group.py @@ -659,7 +659,7 @@ def test_group_create_array( a[:] = data assert array.path == normalize_path(name) - assert array.name == "/" + normalize_path(name) + assert array.name == "/" + array.path assert array.shape == shape assert array.dtype == np.dtype(dtype) assert np.array_equal(array[:], data) @@ -950,20 +950,23 @@ async def test_asyncgroup_delitem(store: Store, zarr_format: ZarrFormat) -> None raise AssertionError +@pytest.mark.parametrize("name", ["a", "/a"]) async def test_asyncgroup_create_group( store: Store, + name: str, zarr_format: ZarrFormat, ) -> None: agroup = await AsyncGroup.from_store(store=store, zarr_format=zarr_format) - sub_node_path = "sub_group" attributes = {"foo": 999} - subnode = await agroup.create_group(name=sub_node_path, attributes=attributes) - - assert isinstance(subnode, AsyncGroup) - assert subnode.attrs == attributes - assert subnode.store_path.path == sub_node_path - assert subnode.store_path.store == store - assert subnode.metadata.zarr_format == zarr_format + subgroup = await agroup.create_group(name=name, attributes=attributes) + + assert isinstance(subgroup, AsyncGroup) + assert subgroup.path == normalize_path(name) + assert subgroup.name == "/" + subgroup.path + assert subgroup.attrs == attributes + assert subgroup.store_path.path == subgroup.path + assert subgroup.store_path.store == store + assert subgroup.metadata.zarr_format == zarr_format async def test_asyncgroup_create_array( From 64ef4897c5eb573c6113388344c94332564d0158 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 26 Feb 2025 18:50:25 +0100 Subject: [PATCH 5/6] code review --- src/zarr/testing/strategies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/zarr/testing/strategies.py b/src/zarr/testing/strategies.py index 2fbb425dd8..aa42329be7 100644 --- a/src/zarr/testing/strategies.py +++ b/src/zarr/testing/strategies.py @@ -284,7 +284,6 @@ def arrays( assert nparray.shape == a.shape assert chunk_shape == a.chunks assert shard_shape == a.shards - assert array_path == a.path, (path, name, array_path, a.name, a.path) assert a.basename == name, (a.basename, name) assert dict(a.attrs) == expected_attrs From c5ad6fbbaeb29d6c2a200411a2beb390ab7112e8 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Tue, 4 Mar 2025 18:14:28 +0100 Subject: [PATCH 6/6] changelog --- changes/2850.fix.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changes/2850.fix.rst diff --git a/changes/2850.fix.rst b/changes/2850.fix.rst new file mode 100644 index 0000000000..dbba120ce2 --- /dev/null +++ b/changes/2850.fix.rst @@ -0,0 +1,2 @@ +Fixed a bug where ``StorePath`` creation would not apply standard path normalization to the ``path`` parameter, +which led to the creation of arrays and groups with invalid keys. \ No newline at end of file