From 4e2ffaabf95b3ccb4d397c2e09762deb72b9932c Mon Sep 17 00:00:00 2001 From: David Stansby Date: Sun, 18 May 2025 16:10:20 +0100 Subject: [PATCH 1/4] Error on invalid store mode --- src/zarr/storage/_common.py | 11 +++++++---- tests/test_store/test_core.py | 8 ++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/zarr/storage/_common.py b/src/zarr/storage/_common.py index d81369f142..b2fefe96d7 100644 --- a/src/zarr/storage/_common.py +++ b/src/zarr/storage/_common.py @@ -2,7 +2,7 @@ import json from pathlib import Path -from typing import TYPE_CHECKING, Any, Literal +from typing import TYPE_CHECKING, Any, Literal, Self from zarr.abc.store import ByteRequest, Store from zarr.core.buffer import Buffer, default_buffer_prototype @@ -48,9 +48,7 @@ def read_only(self) -> bool: return self.store.read_only @classmethod - async def open( - cls, store: Store, path: str, mode: AccessModeLiteral | None = None - ) -> StorePath: + async def open(cls, store: Store, path: str, mode: AccessModeLiteral | None = None) -> Self: """ Open StorePath based on the provided mode. @@ -67,6 +65,9 @@ async def open( ------ FileExistsError If the mode is 'w-' and the store path already exists. + ValueError + If the mode is not "r" and the store is read-only, or + if the mode is "r" and the store is not read-only. """ await store._ensure_open() @@ -78,6 +79,8 @@ async def open( if store.read_only and mode != "r": raise ValueError(f"Store is read-only but mode is '{mode}'") + if not store.read_only and mode == "r": + raise ValueError(f"Store is not read-only but mode is '{mode}'") match mode: case "w-": diff --git a/tests/test_store/test_core.py b/tests/test_store/test_core.py index 4b1858afb5..6a94cc0ac2 100644 --- a/tests/test_store/test_core.py +++ b/tests/test_store/test_core.py @@ -4,6 +4,7 @@ import pytest from _pytest.compat import LEGACY_PATH +import zarr from zarr import Group from zarr.core.common import AccessModeLiteral, ZarrFormat from zarr.storage import FsspecStore, LocalStore, MemoryStore, StoreLike, StorePath @@ -251,3 +252,10 @@ def test_relativize_path_invalid() -> None: msg = f"The first component of {path} does not start with {prefix}." with pytest.raises(ValueError, match=msg): _relativize_path(path="a/b/c", prefix="b") + + +def test_invalid_open_mode() -> None: + store = MemoryStore() + zarr.create((100,), store=store, zarr_format=2, path="a") + with pytest.raises(ValueError, match="Store is not read-only but mode is 'r'"): + zarr.open_array(store=store, path="a", zarr_format=2, mode="r") From f29b0dade04c54631fbb0d1fbac8317ff9bf503d Mon Sep 17 00:00:00 2001 From: David Stansby Date: Sun, 18 May 2025 16:15:11 +0100 Subject: [PATCH 2/4] Fix tests --- tests/test_api.py | 2 +- tests/test_array.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index ae112756c5..6633b7bb06 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -171,7 +171,7 @@ def test_v2_and_v3_exist_at_same_path(store: Store) -> None: zarr.create_array(store, shape=(10,), dtype="uint8", zarr_format=2) msg = f"Both zarr.json (Zarr format 3) and .zarray (Zarr format 2) metadata objects exist at {store}. Zarr v3 will be used." with pytest.warns(UserWarning, match=re.escape(msg)): - zarr.open(store=store, mode="r") + zarr.open(store=store) @pytest.mark.parametrize("store", ["memory"], indirect=True) diff --git a/tests/test_array.py b/tests/test_array.py index eb19f0e7f3..32f7887007 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -1473,7 +1473,7 @@ async def test_name(store: Store, zarr_format: ZarrFormat, path: str | None) -> for parent_path in parents: # this will raise if these groups were not created _ = await zarr.api.asynchronous.open_group( - store=store, path=parent_path, mode="r", zarr_format=zarr_format + store=store, path=parent_path, zarr_format=zarr_format ) @@ -1661,7 +1661,7 @@ def test_roundtrip_numcodecs() -> None: BYTES_CODEC = {"name": "bytes", "configuration": {"endian": "little"}} # Read in the array again and check compressor config - root = zarr.open_group(store, mode="r") + root = zarr.open_group(store) metadata = root["test"].metadata.to_dict() expected = (*filters, BYTES_CODEC, *compressors) assert metadata["codecs"] == expected From 2f93d7913956140227d4d51f8a9f0c31f5aac6f0 Mon Sep 17 00:00:00 2001 From: David Stansby Date: Sun, 18 May 2025 17:19:54 +0100 Subject: [PATCH 3/4] Add release note --- changes/3068.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/3068.bugfix.rst diff --git a/changes/3068.bugfix.rst b/changes/3068.bugfix.rst new file mode 100644 index 0000000000..9ada322c13 --- /dev/null +++ b/changes/3068.bugfix.rst @@ -0,0 +1 @@ +Trying to open an array with ``mode='r'`` when the store is not read-only now raises an error. From 2f9dc01187db97c471f705c431e752200e32f8bf Mon Sep 17 00:00:00 2001 From: David Stansby Date: Wed, 21 May 2025 12:30:48 +0100 Subject: [PATCH 4/4] Fix test --- tests/test_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api.py b/tests/test_api.py index 6633b7bb06..640478e9c1 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1285,7 +1285,7 @@ def test_no_overwrite_open(tmp_path: Path, open_func: Callable, mode: str) -> No existing_fpath = add_empty_file(tmp_path) assert existing_fpath.exists() - with contextlib.suppress(FileExistsError, FileNotFoundError): + with contextlib.suppress(FileExistsError, FileNotFoundError, ValueError): open_func(store=store, mode=mode) if mode == "w": assert not existing_fpath.exists()