Create read only copy if needed when opening a store path (#3156)

* Create read only copy if needed when opening a store path

* Add ValueError to Raises section

* Update expected warning

* Update src/zarr/storage/_common.py

Co-authored-by: Davis Bennett <[email protected]>

* Use ANY_ACCESS_MODE

* Update src/zarr/storage/_common.py

Co-authored-by: David Stansby <[email protected]>

* Update src/zarr/storage/_common.py

Co-authored-by: David Stansby <[email protected]>

* Update changes

* Try using get_args on definition

* Revert "Try using get_args on definition"

This reverts commit 7ad760f.

* Add test

* Remove warning

* Apply suggestion for try; except shortening

Co-authored-by: Tom Nicholas <[email protected]>

* Improve code coverage

---------

Co-authored-by: Davis Bennett <[email protected]>
Co-authored-by: David Stansby <[email protected]>
Co-authored-by: Tom Nicholas <[email protected]>
(cherry picked from commit 5731c6c)

changes/3156.bugfix.rst

@@ -0,0 +1 @@
+Trying to open a StorePath/Array with ``mode='r'`` when the store is not read-only creates a read-only copy of the store.

src/zarr/core/common.py

@@ -10,6 +10,7 @@
 from typing import (
     TYPE_CHECKING,
     Any,
+    Final,
     Literal,
     TypeVar,
     cast,
@@ -40,6 +41,7 @@
 JSON = str | int | float | Mapping[str, "JSON"] | Sequence["JSON"] | None
 MemoryOrder = Literal["C", "F"]
 AccessModeLiteral = Literal["r", "r+", "a", "w", "w-"]
+ANY_ACCESS_MODE: Final = "r", "r+", "a", "w", "w-"
 DimensionNames = Iterable[str | None] | None
 
 

src/zarr/storage/_common.py

@@ -7,7 +7,14 @@
 
 from zarr.abc.store import ByteRequest, Store
 from zarr.core.buffer import Buffer, default_buffer_prototype
-from zarr.core.common import ZARR_JSON, ZARRAY_JSON, ZGROUP_JSON, AccessModeLiteral, ZarrFormat
+from zarr.core.common import (
+    ANY_ACCESS_MODE,
+    ZARR_JSON,
+    ZARRAY_JSON,
+    ZGROUP_JSON,
+    AccessModeLiteral,
+    ZarrFormat,
+)
 from zarr.errors import ContainsArrayAndGroupError, ContainsArrayError, ContainsGroupError
 from zarr.storage._local import LocalStore
 from zarr.storage._memory import MemoryStore
@@ -54,59 +61,82 @@ def __init__(self, store: Store, path: str = "") -> None:
     def read_only(self) -> bool:
         return self.store.read_only
 
+    @classmethod
+    async def _create_open_instance(cls, store: Store, path: str) -> Self:
+        """Helper to create and return a StorePath instance."""
+        await store._ensure_open()
+        return cls(store, path)
+
     @classmethod
     async def open(cls, store: Store, path: str, mode: AccessModeLiteral | None = None) -> Self:
         """
         Open StorePath based on the provided mode.
 
-        * If the mode is 'w-' and the StorePath contains keys, raise a FileExistsError.
-        * If the mode is 'w', delete all keys nested within the StorePath
-        * If the mode is 'a', 'r', or 'r+', do nothing
+        * If the mode is None, return an opened version of the store with no changes.
+        * If the mode is 'r+', 'w-', 'w', or 'a' and the store is read-only, raise a ValueError.
+        * If the mode is 'r' and the store is not read-only, return a copy of the store with read_only set to True.
+        * If the mode is 'w-' and the store is not read-only and the StorePath contains keys, raise a FileExistsError.
+        * If the mode is 'w'  and the store is not read-only, delete all keys nested within the StorePath.
 
         Parameters
         ----------
         mode : AccessModeLiteral
             The mode to use when initializing the store path.
 
+            The accepted values are:
+
+            - ``'r'``: read only (must exist)
+            - ``'r+'``: read/write (must exist)
+            - ``'a'``: read/write (create if doesn't exist)
+            - ``'w'``: read/write (overwrite if exists)
+            - ``'w-'``: read/write (create if doesn't exist).
+
         Raises
         ------
         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()
-        self = cls(store, path)
-
         # fastpath if mode is None
         if mode is None:
-            return self
+            return await cls._create_open_instance(store, path)
 
-        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}'")
+        if mode not in ANY_ACCESS_MODE:
+            raise ValueError(f"Invalid mode: {mode}, expected one of {ANY_ACCESS_MODE}")
 
+        if store.read_only:
+            # Don't allow write operations on a read-only store
+            if mode != "r":
+                raise ValueError(
+                    f"Store is read-only but mode is {mode!r}. Create a writable store or use 'r' mode."
+                )
+            self = await cls._create_open_instance(store, path)
+        elif mode == "r":
+            # Create read-only copy for read mode on writable store
+            try:
+                read_only_store = store.with_read_only(True)
+            except NotImplementedError as e:
+                raise ValueError(
+                    "Store is not read-only but mode is 'r'. Unable to create a read-only copy of the store. "
+                    "Please use a read-only store or a storage class that implements .with_read_only()."
+                ) from e
+            self = await cls._create_open_instance(read_only_store, path)
+        else:
+            # writable store and writable mode
+            self = await cls._create_open_instance(store, path)
+
+        # Handle mode-specific operations
         match mode:
             case "w-":
                 if not await self.is_empty():
-                    msg = (
-                        f"{self} is not empty, but `mode` is set to 'w-'."
-                        "Either remove the existing objects in storage,"
-                        "or set `mode` to a value that handles pre-existing objects"
-                        "in storage, like `a` or `w`."
+                    raise FileExistsError(
+                        f"Cannot create '{path}' with mode 'w-' because it already contains data. "
+                        f"Use mode 'w' to overwrite or 'a' to append."
                     )
-                    raise FileExistsError(msg)
             case "w":
                 await self.delete_dir()
-            case "a" | "r" | "r+":
-                # No init action
-                pass
-            case _:
-                raise ValueError(f"Invalid mode: {mode}")
-
         return self
 
     async def get(

tests/test_api.py

@@ -1318,7 +1318,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, ValueError):
+    with contextlib.suppress(FileExistsError, FileNotFoundError, UserWarning):
         open_func(store=store, mode=mode)
     if mode == "w":
         assert not existing_fpath.exists()

tests/test_common.py

@@ -1,23 +1,36 @@
 from __future__ import annotations
 
-from typing import TYPE_CHECKING
-
-if TYPE_CHECKING:
-    from collections.abc import Iterable
-    from typing import Any, Literal
+from typing import TYPE_CHECKING, get_args
 
 import numpy as np
 import pytest
 
-from zarr.core.common import parse_name, parse_shapelike, product
+from zarr.core.common import (
+    ANY_ACCESS_MODE,
+    AccessModeLiteral,
+    parse_name,
+    parse_shapelike,
+    product,
+)
 from zarr.core.config import parse_indexing_order
 
+if TYPE_CHECKING:
+    from collections.abc import Iterable
+    from typing import Any, Literal
+
 
 @pytest.mark.parametrize("data", [(0, 0, 0, 0), (1, 3, 4, 5, 6), (2, 4)])
 def test_product(data: tuple[int, ...]) -> None:
     assert product(data) == np.prod(data)
 
 
+def test_access_modes() -> None:
+    """
+    Test that the access modes type and variable for run-time checking are equivalent.
+    """
+    assert set(ANY_ACCESS_MODE) == set(get_args(AccessModeLiteral))
+
+
 # todo: test
 def test_concurrent_map() -> None: ...
 

tests/test_store/test_core.py

@@ -7,7 +7,7 @@
 import zarr
 from zarr import Group
 from zarr.core.common import AccessModeLiteral, ZarrFormat
-from zarr.storage import FsspecStore, LocalStore, MemoryStore, StoreLike, StorePath
+from zarr.storage import FsspecStore, LocalStore, MemoryStore, StoreLike, StorePath, ZipStore
 from zarr.storage._common import contains_array, contains_group, make_store_path
 from zarr.storage._utils import (
     _join_paths,
@@ -263,8 +263,19 @@ def test_relativize_path_invalid() -> None:
         _relativize_path(path="a/b/c", prefix="b")
 
 
-def test_invalid_open_mode() -> None:
+def test_different_open_mode(tmp_path: LEGACY_PATH) -> None:
+    # Test with a store that implements .with_read_only()
     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'"):
+    arr = zarr.open_array(store=store, path="a", zarr_format=2, mode="r")
+    assert arr.store.read_only
+
+    # Test with a store that doesn't implement .with_read_only()
+    zarr_path = tmp_path / "foo.zarr"
+    store = ZipStore(zarr_path, mode="w")
+    zarr.create((100,), store=store, zarr_format=2, path="a")
+    with pytest.raises(
+        ValueError,
+        match="Store is not read-only but mode is 'r'. Unable to create a read-only copy of the store. Please use a read-only store or a storage class that implements .with_read_only().",
+    ):
         zarr.open_array(store=store, path="a", zarr_format=2, mode="r")