Prevent mode='r+' from creating new directories (#3307)

* Prevent mode='r+' from creating new directories

* Add test

* Open store for mode = r+

* Add changelog

* Fix errr raised when opening non-existent path

---------

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

Author: dstansby

changes/3307.bugfix.rst

@@ -0,0 +1 @@
+Opening an array or group with ``mode="r+"`` will no longer create new arrays or groups.

src/zarr/storage/_common.py

@@ -358,7 +358,7 @@ async def make_store_path(
 
     elif isinstance(store_like, Path):
         # Create a new LocalStore
-        store = await LocalStore.open(root=store_like, read_only=_read_only)
+        store = await LocalStore.open(root=store_like, mode=mode)
 
     elif isinstance(store_like, str):
         # Either a FSSpec URI or a local filesystem path

src/zarr/storage/_local.py

@@ -5,7 +5,7 @@
 import os
 import shutil
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Self
 
 from zarr.abc.store import (
     ByteRequest,
@@ -16,7 +16,7 @@
 )
 from zarr.core.buffer import Buffer
 from zarr.core.buffer.core import default_buffer_prototype
-from zarr.core.common import concurrent_map
+from zarr.core.common import AccessModeLiteral, concurrent_map
 
 if TYPE_CHECKING:
     from collections.abc import AsyncIterator, Iterable
@@ -102,16 +102,56 @@ def __init__(self, root: Path | str, *, read_only: bool = False) -> None:
             )
         self.root = root
 
-    def with_read_only(self, read_only: bool = False) -> LocalStore:
+    def with_read_only(self, read_only: bool = False) -> Self:
         # docstring inherited
         return type(self)(
             root=self.root,
             read_only=read_only,
         )
 
-    async def _open(self) -> None:
+    @classmethod
+    async def open(
+        cls, root: Path | str, *, read_only: bool = False, mode: AccessModeLiteral | None = None
+    ) -> Self:
+        """
+        Create and open the store.
+
+        Parameters
+        ----------
+        root : str or Path
+            Directory to use as root of store.
+        read_only : bool
+            Whether the store is read-only
+        mode :
+            Mode in which to create the store. This only affects opening the store,
+            and the final read-only state of the store is controlled through the
+            read_only parameter.
+
+        Returns
+        -------
+        Store
+            The opened store instance.
+        """
+        # If mode = 'r+', want to open in read only mode (fail if exists),
+        # but return a writeable store
+        if mode is not None:
+            read_only_creation = mode in ["r", "r+"]
+        else:
+            read_only_creation = read_only
+        store = cls(root, read_only=read_only_creation)
+        await store._open()
+
+        # Set read_only state
+        store = store.with_read_only(read_only)
+        await store._open()
+        return store
+
+    async def _open(self, *, mode: AccessModeLiteral | None = None) -> None:
         if not self.read_only:
             self.root.mkdir(parents=True, exist_ok=True)
+
+        if not self.root.exists():
+            raise FileNotFoundError(f"{self.root} does not exist")
         return await super()._open()
 
     async def clear(self) -> None:

tests/test_api.py

@@ -45,7 +45,6 @@
 from zarr.errors import (
     ArrayNotFoundError,
     MetadataValidationError,
-    NodeNotFoundError,
     ZarrDeprecationWarning,
     ZarrUserWarning,
 )
@@ -191,7 +190,7 @@ async def test_open_array(memory_store: MemoryStore, zarr_format: ZarrFormat) ->
     assert z.read_only
 
     # path not found
-    with pytest.raises(NodeNotFoundError):
+    with pytest.raises(FileNotFoundError):
         zarr.api.synchronous.open(store="doesnotexist", mode="r", zarr_format=zarr_format)
 
 
@@ -333,8 +332,12 @@ def test_open_with_mode_r(tmp_path: Path) -> None:
 
 def test_open_with_mode_r_plus(tmp_path: Path) -> None:
     # 'r+' means read/write (must exist)
+    new_store_path = tmp_path / "new_store.zarr"
+    assert not new_store_path.exists(), "Test should operate on non-existent directory"
     with pytest.raises(FileNotFoundError):
-        zarr.open(store=tmp_path, mode="r+")
+        zarr.open(store=new_store_path, mode="r+")
+    assert not new_store_path.exists(), "mode='r+' should not create directory"
+
     zarr.ones(store=tmp_path, shape=(3, 3))
     z2 = zarr.open(store=tmp_path, mode="r+")
     assert isinstance(z2, Array)