fix(api): validate a new node before deleting the existing one

changes/4410.bugfix.md

@@ -0,0 +1 @@
+Opening or creating an array or group with `mode="w"` or `overwrite=True` no longer deletes the existing node when the call fails. Previously the existing node was deleted before the new one was validated, so an invalid argument (for example `zarr.open(store, mode="w", config=...)`, an invalid `fill_value`, attributes that are not JSON, or an array `zarr.save_group` cannot store) destroyed the existing data and created nothing. Deletion now belongs to node creation: `StorePath.open(mode="w")` no longer erases the keys under the path, and the node written there replaces them once its metadata is valid. On a store that cannot delete keys, such as `ZipStore`, `mode="w"` creates the node when the path is empty and raises `ContainsArrayError` or `ContainsGroupError` when a node already exists, instead of `NotImplementedError`.

src/zarr/api/asynchronous.py

@@ -366,7 +366,8 @@ async def open(
         Persistence mode: 'r' means read only (must exist); 'r+' means
         read/write (must exist); 'a' means read/write (create if doesn't
         exist); 'w' means create (overwrite if exists); 'w-' means create
-        (fail if exists).
+        (fail if exists). On a store that cannot delete keys, 'w' raises an
+        error instead of replacing an existing node.
         If the store is read-only, the default is 'r'; otherwise, it is 'a'.
     zarr_format : {2, 3, None}, optional
         The zarr format to use when saving.
@@ -404,20 +405,24 @@ async def open(
     store_path = await make_store_path(store, mode=mode, path=path, storage_options=storage_options)
 
     # TODO: the mode check below seems wrong!
-    if "shape" not in kwargs and mode in {"a", "r", "r+", "w"}:
-        try:
-            metadata_dict = await get_array_metadata(store_path, zarr_format=zarr_format)
-            # TODO: remove this cast when we fix typing for array metadata dicts
-            _metadata_dict = cast("ArrayMetadataDict", metadata_dict)
-            # for v2, the above would already have raised an exception if not an array
-            zarr_format = _metadata_dict["zarr_format"]
-            is_v3_array = zarr_format == 3 and _metadata_dict.get("node_type") == "array"
-            if is_v3_array or zarr_format == 2:
-                return AsyncArray(
-                    store_path=store_path, metadata=_metadata_dict, config=kwargs.get("config")
-                )
-        except (FileNotFoundError, NodeTypeValidationError):
-            pass
+    if "shape" not in kwargs and mode in (*_READ_MODES, "w"):
+        # mode "w" replaces any existing node, so there is nothing to open
+        if mode in _READ_MODES:
+            try:
+                metadata_dict = await get_array_metadata(store_path, zarr_format=zarr_format)
+                # TODO: remove this cast when we fix typing for array metadata dicts
+                _metadata_dict = cast("ArrayMetadataDict", metadata_dict)
+                # for v2, the above would already have raised an exception if not an array
+                zarr_format = _metadata_dict["zarr_format"]
+                is_v3_array = zarr_format == 3 and _metadata_dict.get("node_type") == "array"
+                if is_v3_array or zarr_format == 2:
+                    return AsyncArray(
+                        store_path=store_path,
+                        metadata=_metadata_dict,
+                        config=kwargs.get("config"),
+                    )
+            except (FileNotFoundError, NodeTypeValidationError):
+                pass
         return await open_group(store=store_path, zarr_format=zarr_format, mode=mode, **kwargs)
 
     try:
@@ -575,8 +580,6 @@ async def save_group(
         NumPy arrays with data to save.
     """
 
-    store_path = await make_store_path(store, path=path, mode="w", storage_options=storage_options)
-
     if zarr_format is None:
         zarr_format = _default_zarr_format()
 
@@ -591,6 +594,16 @@ async def save_group(
 
     if len(args) == 0 and len(kwargs) == 0:
         raise ValueError("at least one array must be provided")
+
+    # Resolve every data type before anything is deleted, so an array that Zarr cannot
+    # store raises while the existing node is still intact.
+    for arr in (*args, *kwargs.values()):
+        get_data_type_from_native_dtype(arr.dtype)
+
+    store_path = await make_store_path(store, path=path, mode="w", storage_options=storage_options)
+    # The group replaces whatever is stored under the path, now that the arguments are known
+    # to be valid.
+    await AsyncGroup.from_store(store_path, zarr_format=zarr_format, overwrite=True)
     aws = []
     # `store_path` already consumed `storage_options`, so passing them on again would
     # make `make_store_path` reject them as unused.
@@ -823,7 +836,8 @@ async def open_group(
         Persistence mode: 'r' means read only (must exist); 'r+' means
         read/write (must exist); 'a' means read/write (create if doesn't
         exist); 'w' means create (overwrite if exists); 'w-' means create
-        (fail if exists).
+        (fail if exists). On a store that cannot delete keys, 'w' raises an
+        error instead of replacing an existing node.
     cache_attrs : bool, optional
         If True (default), user attributes will be cached for attribute read
         operations. If False, user attributes are reloaded from the store prior
@@ -1070,7 +1084,8 @@ async def create(
           `True`.
         - `'r'` always fails.
 
-        `mode` has no effect, and is not validated, if `store` is a `StorePath`.
+        If `store` is a `StorePath`, `mode` is not validated against it: `'w'` still
+        sets `overwrite`, and the other modes have no effect.
     data : array-like, optional
         Values written into the new array after it is created. Unlike the `data`
         parameter of `create_array`, it does not set the shape or data type of the
@@ -1102,6 +1117,7 @@ async def create(
 
     if mode is None:
         mode = "a"
+    overwrite = overwrite or _infer_overwrite(mode)
     store_path = await make_store_path(store, path=path, mode=mode, storage_options=storage_options)
 
     config_parsed = parse_array_config(config)
@@ -1321,20 +1337,20 @@ async def open_array(
     if "write_empty_chunks" in kwargs:
         _warn_write_empty_chunks_kwarg()
 
-    try:
-        return await AsyncArray.open(store_path, zarr_format=zarr_format)
-    except FileNotFoundError as err:
-        if not store_path.read_only and mode in _CREATE_MODES:
-            overwrite = _infer_overwrite(mode)
-            _zarr_format = zarr_format or _default_zarr_format()
-            return await create(
-                store=store_path,
-                zarr_format=_zarr_format,
-                overwrite=overwrite,
-                **kwargs,
-            )
-        msg = f"No array found in store {store_path.store} at path {store_path.path}"
-        raise ArrayNotFoundError(msg) from err
+    # mode "w" replaces any existing array, so there is nothing to open
+    if mode != "w":
+        try:
+            return await AsyncArray.open(store_path, zarr_format=zarr_format)
+        except FileNotFoundError as err:
+            if store_path.read_only or mode not in _CREATE_MODES:
+                msg = f"No array found in store {store_path.store} at path {store_path.path}"
+                raise ArrayNotFoundError(msg) from err
+    return await create(
+        store=store_path,
+        zarr_format=zarr_format or _default_zarr_format(),
+        overwrite=_infer_overwrite(mode),
+        **kwargs,
+    )
 
 
 async def open_like(

src/zarr/api/synchronous.py

@@ -200,7 +200,8 @@ def open(
         Persistence mode: 'r' means read only (must exist); 'r+' means
         read/write (must exist); 'a' means read/write (create if doesn't
         exist); 'w' means create (overwrite if exists); 'w-' means create
-        (fail if exists).
+        (fail if exists). On a store that cannot delete keys, 'w' raises an
+        error instead of replacing an existing node.
         If the store is read-only, the default is 'r'; otherwise, it is 'a'.
     zarr_format : {2, 3, None}, optional
         The zarr format to use when saving.
@@ -518,7 +519,8 @@ def open_group(
         Persistence mode: 'r' means read only (must exist); 'r+' means
         read/write (must exist); 'a' means read/write (create if doesn't
         exist); 'w' means create (overwrite if exists); 'w-' means create
-        (fail if exists).
+        (fail if exists). On a store that cannot delete keys, 'w' raises an
+        error instead of replacing an existing node.
     cache_attrs : bool, optional
         If True (default), user attributes will be cached for attribute read
         operations. If False, user attributes are reloaded from the store prior
@@ -800,7 +802,8 @@ def create(
           `True`.
         - `'r'` always fails.
 
-        `mode` has no effect, and is not validated, if `store` is a `StorePath`.
+        If `store` is a `StorePath`, `mode` is not validated against it: `'w'` still
+        sets `overwrite`, and the other modes have no effect.
     data : array-like, optional
         Values written into the new array after it is created. Unlike the `data`
         parameter of `create_array`, it does not set the shape or data type of the

src/zarr/core/array.py

@@ -118,7 +118,7 @@
     ArrayV2MetadataDict,
     ArrayV3Metadata,
 )
-from zarr.core.metadata.io import save_metadata
+from zarr.core.metadata.io import save_metadata, save_new_metadata
 from zarr.core.metadata.v2 import (
     CompressorLikev2,
     get_object_codec_id,
@@ -146,7 +146,7 @@
     _parse_bytes_bytes_codec,
     get_pipeline_class,
 )
-from zarr.storage._common import StorePath, ensure_no_existing_node, make_store_path
+from zarr.storage._common import StorePath, make_store_path
 from zarr.storage._utils import _relativize_path
 
 if TYPE_CHECKING:
@@ -334,22 +334,6 @@ def _array_metadata_dict_v3(zarr_json_bytes: Buffer) -> dict[str, JSON]:
     return metadata_dict
 
 
-async def _prepare_overwrite(
-    store_path: StorePath, *, zarr_format: ZarrFormat, overwrite: bool
-) -> None:
-    """
-    Prepare a store path for writing a new node.
-
-    If `overwrite` is true and the store supports deletes, any existing node at
-    `store_path` is deleted. Otherwise, the absence of an existing node is enforced
-    (raising if one is present).
-    """
-    if overwrite and store_path.store.supports_deletes:
-        await store_path.delete_dir()
-    else:
-        await ensure_no_existing_node(store_path, zarr_format=zarr_format)
-
-
 @dataclass(frozen=True)
 class AsyncArray[T_ArrayMetadata: (ArrayV2Metadata, ArrayV3Metadata)]:
     """
@@ -629,8 +613,6 @@ async def _create_v3(
         attributes: dict[str, JSON] | None = None,
         overwrite: bool = False,
     ) -> AsyncArrayV3:
-        await _prepare_overwrite(store_path, zarr_format=3, overwrite=overwrite)
-
         if isinstance(chunk_key_encoding, tuple):
             chunk_key_encoding = (
                 V2ChunkKeyEncoding(separator=chunk_key_encoding[1])
@@ -650,7 +632,7 @@ async def _create_v3(
         )
 
         array = cls(metadata=metadata, store_path=store_path, config=config)
-        await array._save_metadata(metadata, ensure_parents=True)
+        await save_new_metadata(store_path, metadata, overwrite=overwrite, ensure_parents=True)
         return array
 
     @staticmethod
@@ -704,8 +686,6 @@ async def _create_v2(
         attributes: dict[str, JSON] | None = None,
         overwrite: bool = False,
     ) -> AsyncArrayV2:
-        await _prepare_overwrite(store_path, zarr_format=2, overwrite=overwrite)
-
         compressor_parsed: CompressorLikev2
         if compressor == "auto":
             compressor_parsed = default_compressor_v2(dtype)
@@ -733,7 +713,7 @@ async def _create_v2(
         )
 
         array = cls(metadata=metadata, store_path=store_path, config=config)
-        await array._save_metadata(metadata, ensure_parents=True)
+        await save_new_metadata(store_path, metadata, overwrite=overwrite, ensure_parents=True)
         return array
 
     @classmethod
@@ -4530,8 +4510,6 @@ async def init_array(
         chunk_key_encoding, zarr_format=zarr_format
     )
 
-    await _prepare_overwrite(store_path, zarr_format=zarr_format, overwrite=overwrite)
-
     # Validate rectilinear chunks constraints
     if _is_rectilinear_chunks(chunks):
         if zarr_format == 2:
@@ -4645,7 +4623,7 @@ async def init_array(
         )
 
     arr = AsyncArray(metadata=meta, store_path=store_path, config=config)
-    await arr._save_metadata(meta, ensure_parents=True)
+    await save_new_metadata(store_path, meta, overwrite=overwrite, ensure_parents=True)
     return arr
 
 

src/zarr/core/group.py

@@ -49,7 +49,7 @@
 from zarr.core.dtype import parse_data_type
 from zarr.core.json_parse import parse_field
 from zarr.core.metadata import ArrayV2Metadata, ArrayV3Metadata
-from zarr.core.metadata.io import save_metadata
+from zarr.core.metadata.io import save_metadata, save_new_metadata
 from zarr.core.metadata.v3 import AllowedExtraField, parse_extra_fields
 from zarr.core.sync import SyncMixin, sync
 from zarr.errors import (
@@ -62,7 +62,7 @@
     ZarrUserWarning,
 )
 from zarr.storage import StoreLike, StorePath
-from zarr.storage._common import ensure_no_existing_node, make_store_path
+from zarr.storage._common import make_store_path
 from zarr.storage._utils import _join_paths, _normalize_path_keys, normalize_path
 
 if TYPE_CHECKING:
@@ -506,20 +506,14 @@ async def from_store(
         zarr_format: ZarrFormat = 3,
     ) -> AsyncGroup:
         store_path = await make_store_path(store)
-
-        if overwrite:
-            if store_path.store.supports_deletes:
-                await store_path.delete_dir()
-            else:
-                await ensure_no_existing_node(store_path, zarr_format=zarr_format)
-        else:
-            await ensure_no_existing_node(store_path, zarr_format=zarr_format)
         attributes = attributes or {}
         group = cls(
             metadata=GroupMetadata(attributes=attributes, zarr_format=zarr_format),
             store_path=store_path,
         )
-        await group._save_metadata(ensure_parents=True)
+        await save_new_metadata(
+            store_path, group.metadata, overwrite=overwrite, ensure_parents=True
+        )
         return group
 
     @classmethod

src/zarr/core/metadata/io.py

@@ -9,6 +9,7 @@
 from zarr.storage._common import StorePath, ensure_no_existing_node
 
 if TYPE_CHECKING:
+    from zarr.core.buffer import Buffer
     from zarr.core.common import ZarrFormat
     from zarr.core.group import GroupMetadata
     from zarr.core.metadata import ArrayMetadata
@@ -52,11 +53,54 @@ async def save_metadata(
     ValueError
     """
     to_save = metadata.to_buffer_dict(default_buffer_prototype())
+    await _write_metadata(store_path, to_save, metadata.zarr_format, ensure_parents=ensure_parents)
+
+
+async def save_new_metadata(
+    store_path: StorePath,
+    metadata: ArrayMetadata | GroupMetadata,
+    *,
+    overwrite: bool,
+    ensure_parents: bool = False,
+) -> None:
+    """Save the metadata of a new array or group, replacing any existing node if requested.
+
+    The metadata is encoded before the store is modified, so metadata that cannot be
+    encoded raises without deleting an existing node.
+
+    Parameters
+    ----------
+    store_path : StorePath
+        Location of the new node.
+    metadata : ArrayMetadata | GroupMetadata
+        Metadata of the new node.
+    overwrite : bool
+        If true and the store supports deletes, delete everything under `store_path`.
+        Otherwise, raise if a node already exists at `store_path`; a store that cannot
+        delete keys cannot overwrite a node, so it raises the same error either way.
+    ensure_parents : bool, optional
+        Create any missing parent groups, and check no existing parents are arrays.
+    """
+    to_save = metadata.to_buffer_dict(default_buffer_prototype())
+    if overwrite and store_path.store.supports_deletes:
+        await store_path.delete_dir()
+    else:
+        await ensure_no_existing_node(store_path, zarr_format=metadata.zarr_format)
+    await _write_metadata(store_path, to_save, metadata.zarr_format, ensure_parents=ensure_parents)
+
+
+async def _write_metadata(
+    store_path: StorePath,
+    to_save: dict[str, Buffer],
+    zarr_format: ZarrFormat,
+    *,
+    ensure_parents: bool,
+) -> None:
     set_awaitables = [set_or_delete(store_path / key, value) for key, value in to_save.items()]
 
     if ensure_parents:
         # To enable zarr.create(store, path="a/b/c"), we need to create all the intermediate groups.
-        parents = _build_parents(store_path, metadata.zarr_format)
+        parents = _build_parents(store_path, zarr_format)
         ensure_array_awaitables = []
 
         for parent_path, parent_metadata in parents.items():