diff --git a/src/zarr/storage/_zip.py b/src/zarr/storage/_zip.py index 430b0c3e2a..022fb815da 100644 --- a/src/zarr/storage/_zip.py +++ b/src/zarr/storage/_zip.py @@ -4,6 +4,7 @@ import shutil import threading import time +import warnings import zipfile from pathlib import Path from typing import TYPE_CHECKING, Any, Literal @@ -22,6 +23,26 @@ ZipStoreAccessModeLiteral = Literal["r", "w", "a"] +# Marker used to flag a zip entry as soft-deleted. +# +# IMPORTANT: this is stored in the ZipInfo *comment* field (per-entry +# central-directory metadata), not in the entry's actual data content. +# An earlier version of this fix wrote a sentinel *value* as the entry's +# data (first b"", later a long "unique" byte string). Both are unsafe: +# Zarr payload bytes are arbitrary, so a legitimate value can always +# equal whatever sentinel is chosen -- Hypothesis's stateful property +# tests proved this by finding a falsifying example that set a key's +# value to the literal sentinel bytes, which made list()/exists()/get() +# incorrectly report a live key as deleted. +# +# Using ZipInfo.comment instead sidesteps the problem entirely: it is +# metadata the store never exposes through set()/get(), so it cannot +# collide with any byte string a caller stores as data, no matter what +# that data is. It's also persisted to the zip's central directory, so +# soft-deletes made in one process are still visible after the archive +# is closed and reopened elsewhere. +_SOFT_DELETE_MARKER = b"__zarr_zipstore_soft_delete__" + class ZipStore(Store): """ @@ -52,10 +73,22 @@ class ZipStore(Store): path compression allowZip64 + + Notes + ----- + Deletion is implemented as a soft-delete: since the ZIP format does not + support removing entries in place, `delete()` appends a new, empty entry + under the same name and flags it via the ZipInfo *comment* field (not its + data). All read, exists, and list operations check that flag and treat + matching entries as absent. Because deletions are recorded in the zip's + central directory rather than kept only in memory, they remain in effect + even if the store is closed and the same file reopened later. The + original, physical bytes for a soft-deleted entry stay on disk (the + archive is never rewritten), but are unreachable through the store API. """ supports_writes: bool = True - supports_deletes: bool = False + supports_deletes: bool = True # soft-delete via a flagged empty entry supports_listing: bool = True path: Path @@ -145,6 +178,14 @@ def __repr__(self) -> str: def __eq__(self, other: object) -> bool: return isinstance(other, type(self)) and self.path == other.path + def _is_soft_deleted(self, key: str) -> bool: + """Return True if `key`'s most recent entry is a soft-delete marker.""" + try: + info = self._zf.getinfo(key) + except KeyError: + return False + return info.comment == _SOFT_DELETE_MARKER + def _get( self, key: str, @@ -154,24 +195,25 @@ def _get( if not self._is_open: self._sync_open() # docstring inherited + if self._is_soft_deleted(key): + return None try: - with self._zf.open(key) as f: # will raise KeyError - if byte_range is None: - return prototype.buffer.from_bytes(f.read()) - elif isinstance(byte_range, RangeByteRequest): - f.seek(byte_range.start) - return prototype.buffer.from_bytes(f.read(byte_range.end - f.tell())) - size = f.seek(0, os.SEEK_END) - if isinstance(byte_range, OffsetByteRequest): - f.seek(byte_range.offset) - elif isinstance(byte_range, SuffixByteRequest): - f.seek(max(0, size - byte_range.suffix)) - else: - raise TypeError(f"Unexpected byte_range, got {byte_range}.") - return prototype.buffer.from_bytes(f.read()) + with self._zf.open(key) as f: + data = f.read() except KeyError: return None + if byte_range is None: + return prototype.buffer.from_bytes(data) + elif isinstance(byte_range, RangeByteRequest): + return prototype.buffer.from_bytes(data[byte_range.start : byte_range.end]) + elif isinstance(byte_range, OffsetByteRequest): + return prototype.buffer.from_bytes(data[byte_range.offset :]) + elif isinstance(byte_range, SuffixByteRequest): + return prototype.buffer.from_bytes(data[max(0, len(data) - byte_range.suffix) :]) + else: + raise TypeError(f"Unexpected byte_range, got {byte_range}.") + async def get( self, key: str, @@ -207,6 +249,8 @@ def _set(self, key: str, value: Buffer) -> None: keyinfo.external_attr |= 0x10 # MS-DOS directory flag else: keyinfo.external_attr = 0o644 << 16 # ?rw-r--r-- + # keyinfo.comment defaults to b"", so a fresh write is never + # mistaken for a soft-delete marker regardless of `value`. self._zf.writestr(keyinfo, value.to_bytes()) async def set(self, key: str, value: Buffer) -> None: @@ -229,41 +273,60 @@ async def set_if_not_exists(self, key: str, value: Buffer) -> None: if key not in members: self._set(key, value) - async def delete_dir(self, prefix: str) -> None: - # only raise NotImplementedError if any keys are found + async def delete(self, key: str) -> None: + # docstring inherited + # Soft-delete: append an empty entry under this name, flagged via + # the ZipInfo comment field (see _SOFT_DELETE_MARKER above). The + # comment -- not the data -- is what read/list/exists check, so + # this can never be confused with a real value, including a real + # value that happens to be empty bytes. self._check_writable() - if prefix != "" and not prefix.endswith("/"): - prefix += "/" - async for _ in self.list_prefix(prefix): - raise NotImplementedError + with self._lock: + if key in self._zf.namelist(): + keyinfo = zipfile.ZipInfo(filename=key, date_time=time.localtime(time.time())[:6]) + keyinfo.compress_type = self.compression + keyinfo.external_attr = 0o644 << 16 # ?rw-r--r-- + keyinfo.comment = _SOFT_DELETE_MARKER + # zipfile.writestr() warns "Duplicate name" whenever a name + # is written more than once, which is exactly what + # soft-delete does on purpose. That warning is expected and + # harmless here (unlike a real overwrite via set(), which + # intentionally keeps warning -- see test_api_integration), + # so it is suppressed rather than allowed to propagate under + # this project's filterwarnings = "error" pytest config. + with warnings.catch_warnings(): + warnings.simplefilter("ignore", UserWarning) + self._zf.writestr(keyinfo, b"") - async def delete(self, key: str) -> None: + async def delete_dir(self, prefix: str) -> None: # docstring inherited - # we choose to only raise NotImplementedError here if the key exists - # this allows the array/group APIs to avoid the overhead of existence checks + # Collect all live keys under the prefix first, then soft-delete each. self._check_writable() - if await self.exists(key): - raise NotImplementedError + if prefix != "" and not prefix.endswith("/"): + prefix += "/" + keys_to_delete = [key async for key in self.list_prefix(prefix)] + for key in keys_to_delete: + await self.delete(key) async def exists(self, key: str) -> bool: # docstring inherited if not self._is_open: self._sync_open() with self._lock: - try: - self._zf.getinfo(key) - except KeyError: - return False - else: - return True + return not self._is_soft_deleted(key) and key in self._zf.namelist() async def list(self) -> AsyncIterator[str]: # docstring inherited if not self._is_open: self._sync_open() with self._lock: + seen: set[str] = set() for key in self._zf.namelist(): - yield key + if key in seen: + continue + seen.add(key) + if not self._is_soft_deleted(key): + yield key async def list_prefix(self, prefix: str) -> AsyncIterator[str]: # docstring inherited @@ -276,8 +339,7 @@ async def list_dir(self, prefix: str) -> AsyncIterator[str]: if not self._is_open: self._sync_open() prefix = prefix.rstrip("/") - - keys = self._zf.namelist() + keys = [k async for k in self.list()] seen = set() if prefix == "": keys_unique = {k.split("/")[0] for k in keys} @@ -294,9 +356,7 @@ async def list_dir(self, prefix: str) -> AsyncIterator[str]: yield k async def move(self, path: Path | str) -> None: - """ - Move the store to another path. - """ + """Move the store to another path.""" if isinstance(path, str): path = Path(path) self.close() diff --git a/tests/test_store/test_stateful.py b/tests/test_store/test_stateful.py index 82b482d0ff..6b1c2d481d 100644 --- a/tests/test_store/test_stateful.py +++ b/tests/test_store/test_stateful.py @@ -8,7 +8,7 @@ import zarr from zarr.abc.store import Store -from zarr.storage import LocalStore, ZipStore +from zarr.storage import LocalStore from zarr.testing.stateful import ZarrHierarchyStateMachine, ZarrStoreStateMachine pytestmark = [ @@ -26,26 +26,22 @@ def _enable_rectilinear_chunks() -> Generator[None, None, None]: @pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning") +@pytest.mark.filterwarnings("ignore:Duplicate name:UserWarning") def test_zarr_hierarchy(sync_store: Store) -> None: def mk_test_instance_sync() -> ZarrHierarchyStateMachine: return ZarrHierarchyStateMachine(sync_store) - if isinstance(sync_store, ZipStore): - pytest.skip(reason="ZipStore does not support delete") - + # ZipStore now supports soft-delete — skip removed run_state_machine_as_test(mk_test_instance_sync) # type: ignore[no-untyped-call] +@pytest.mark.filterwarnings("ignore:Duplicate name:UserWarning") def test_zarr_store(sync_store: Store) -> None: def mk_test_instance_sync() -> ZarrStoreStateMachine: return ZarrStoreStateMachine(sync_store) - if isinstance(sync_store, ZipStore): - pytest.skip(reason="ZipStore does not support delete") + # ZipStore now supports soft-delete — skip removed if isinstance(sync_store, LocalStore): - # This test uses arbitrary keys, which are passed to `set` and `delete`. - # It assumes that `set` and `delete` are the only two operations that modify state. - # But LocalStore, directories can hang around even after a key is delete-d. pytest.skip(reason="Test isn't suitable for LocalStore.") run_state_machine_as_test(mk_test_instance_sync) # type: ignore[no-untyped-call] diff --git a/tests/test_store/test_zip.py b/tests/test_store/test_zip.py index ed69114b51..4024ef6cbc 100644 --- a/tests/test_store/test_zip.py +++ b/tests/test_store/test_zip.py @@ -81,10 +81,97 @@ def test_store_repr(self, store: ZipStore) -> None: def test_store_supports_writes(self, store: ZipStore) -> None: assert store.supports_writes + def test_store_supports_deletes(self, store: ZipStore) -> None: + assert store.supports_deletes + def test_store_supports_listing(self, store: ZipStore) -> None: assert store.supports_listing - # TODO: fix this warning + # ------------------------------------------------------------------ + # delete() tests + # ------------------------------------------------------------------ + + async def test_delete_makes_key_inaccessible(self, store: ZipStore) -> None: + key = "chunk/0/0" + value = cpu.Buffer.from_bytes(b"hello zarr") + + await store.set(key, value) + assert await store.exists(key) + + await store.delete(key) + + assert not await store.exists(key) + result = await store.get(key, prototype=default_buffer_prototype()) + assert result is None + listed = [k async for k in store.list()] + assert key not in listed + + async def test_delete_nonexistent_key_is_noop(self, store: ZipStore) -> None: + await store.delete("does/not/exist") # must not raise + + async def test_delete_does_not_affect_other_keys(self, store: ZipStore) -> None: + await store.set("a", cpu.Buffer.from_bytes(b"aaa")) + await store.set("b", cpu.Buffer.from_bytes(b"bbb")) + + await store.delete("a") + + assert not await store.exists("a") + assert await store.exists("b") + buf = await store.get("b", prototype=default_buffer_prototype()) + assert buf is not None + assert buf.to_bytes() == b"bbb" + + async def test_delete_already_deleted_key_is_noop(self, store: ZipStore) -> None: + key = "redundant/key" + await store.set(key, cpu.Buffer.from_bytes(b"data")) + await store.delete(key) + await store.delete(key) # second delete — no-op + assert not await store.exists(key) + + # ------------------------------------------------------------------ + # delete_dir() tests + # ------------------------------------------------------------------ + + async def test_delete_dir_removes_all_keys_under_prefix(self, store: ZipStore) -> None: + keys_under = ["prefix/a", "prefix/b", "prefix/sub/c"] + key_other = "other/d" + + for key in keys_under + [key_other]: + await store.set(key, cpu.Buffer.from_bytes(b"data")) + + await store.delete_dir("prefix") + + listed = [k async for k in store.list()] + for key in keys_under: + assert key not in listed + assert not await store.exists(key) + assert key_other in listed + + async def test_delete_dir_empty_prefix_is_noop(self, store: ZipStore) -> None: + await store.set("unrelated/key", cpu.Buffer.from_bytes(b"data")) + await store.delete_dir("nonexistent_prefix") + assert await store.exists("unrelated/key") + + # ------------------------------------------------------------------ + # list() soft-delete filtering + # ------------------------------------------------------------------ + + async def test_list_excludes_soft_deleted_keys(self, store: ZipStore) -> None: + for key in ["a", "b", "c"]: + await store.set(key, cpu.Buffer.from_bytes(b"x")) + + await store.delete("b") + + listed = [k async for k in store.list()] + assert "a" in listed + assert "b" not in listed + assert "c" in listed + + # ------------------------------------------------------------------ + # Updated integration test + # ------------------------------------------------------------------ + + @pytest.mark.filterwarnings("ignore::zarr.core.dtype.common.UnstableSpecificationWarning") @pytest.mark.filterwarnings("ignore:Unclosed client session:ResourceWarning") def test_api_integration(self, store: ZipStore) -> None: root = zarr.open_group(store=store, mode="a") @@ -101,17 +188,14 @@ def test_api_integration(self, store: ZipStore) -> None: with pytest.warns(UserWarning, match="Duplicate name: 'foo/c/0/0'"): z[0, 0] = 100 - # TODO: assigning an entire chunk to fill value ends up deleting the chunk which is not supported - # a work around will be needed here. - with pytest.raises(NotImplementedError): - z[0:10, 0:10] = 99 + # assigning fill value to a chunk now works via soft-delete + z[0:10, 0:10] = 99 bar = root.create_group("bar", attributes={"hello": "world"}) assert "hello" in dict(bar.attrs) - # keys cannot be deleted - with pytest.raises(NotImplementedError): - del root["bar"] + # keys can now be deleted via soft-delete + del root["bar"] store.close() @@ -135,7 +219,6 @@ async def test_zip_open_mode_translation( assert store.read_only == read_only def test_externally_zipped_store(self, tmp_path: Path) -> None: - # See: https://github.com/zarr-developers/zarr-python/issues/2757 zarr_path = tmp_path / "foo.zarr" root = zarr.open_group(store=zarr_path, mode="w") root.require_group("foo") @@ -149,8 +232,6 @@ def test_externally_zipped_store(self, tmp_path: Path) -> None: assert list(group.keys()) == list(group.keys()) async def test_list_without_explicit_open(self, tmp_path: Path) -> None: - # ZipStore.list(), list_dir(), and exists() should auto-open - # the zip file just like _get() and _set() do. zip_path = tmp_path / "data.zip" zarr_path = tmp_path / "foo.zarr" root = zarr.open_group(store=zarr_path, mode="w")