diff --git a/changes/4485.bugfix.md b/changes/4485.bugfix.md new file mode 100644 index 0000000000..0079f4775b --- /dev/null +++ b/changes/4485.bugfix.md @@ -0,0 +1,10 @@ +Validate consolidated metadata listings at parse time. In v3 listings, +keys containing a reserved node name (`.`, `..`, `zarr.json`, or a +`__`-prefixed segment) now emit a `ZarrDeprecationWarning` instead of +being accepted silently; stores written before these names were rejected +still open. Keys with empty path segments still raise +`MetadataValidationError`, as do listings whose parent group entry is +missing or that place a node below an array; those cases previously +surfaced as `KeyError` or left the group unopenable. The reserved-name +check does not apply to v2 listings, whose specification does not +reserve node names. diff --git a/src/zarr/core/group.py b/src/zarr/core/group.py index 7792fa58de..43dc20d901 100644 --- a/src/zarr/core/group.py +++ b/src/zarr/core/group.py @@ -64,6 +64,7 @@ GroupNotFoundError, MetadataValidationError, NodeTypeValidationError, + ZarrDeprecationWarning, ZarrUserWarning, ) from zarr.storage import StoreLike, StorePath @@ -140,6 +141,53 @@ def _parse_async_node( raise TypeError(f"Unknown node type, got {type(node)}") +def _validate_consolidated_key(key: str, zarr_format: ZarrFormat) -> None: + """A consolidated metadata key is a node path: every segment must be non-empty. + + The v3 specification also reserves the names ``.``, ``..``, ``zarr.json``, and + every ``__``-prefixed name. Stores written before those names were rejected on + creation still open today, so for now such keys only warn. The v2 specification + does not reserve any node names, so no name checks apply to v2 listings.""" + segments = key.split("/") + if any(not seg for seg in segments): + msg = f"Invalid node path in consolidated metadata: {key!r}" + raise MetadataValidationError(msg) + if zarr_format == 3 and any( + seg in (".", "..", ZARR_JSON) or seg.startswith("__") for seg in segments + ): + msg = ( + f"Consolidated metadata lists {key!r}, which contains a name reserved " + "by the v3 specification. Rename the node, as reading such listings " + "will become an error." + ) + warnings.warn(msg, category=ZarrDeprecationWarning, stacklevel=1) + + +def _validate_consolidated_hierarchy( + metadata: dict[str, ArrayV2Metadata | ArrayV3Metadata | GroupMetadata], +) -> None: + """Every non-root listing key must have all of its ancestors present as + group entries. A missing ancestor makes the group unreadable, and a + listing below an array is invalid: array nodes may not have children.""" + for key in metadata: + parts = key.split("/") + for i in range(1, len(parts)): + prefix = "/".join(parts[:i]) + parent = metadata.get(prefix) + if parent is None: + msg = ( + f"Consolidated metadata lists {key!r} without a listing " + f"for its parent {prefix!r}" + ) + raise MetadataValidationError(msg) + if not isinstance(parent, GroupMetadata): + msg = ( + f"Consolidated metadata lists {key!r} below {prefix!r}, " + "which is an array; array nodes may not have children" + ) + raise MetadataValidationError(msg) + + @dataclass(frozen=True) class ConsolidatedMetadata: """ @@ -199,6 +247,7 @@ def from_dict(cls, data: dict[str, JSON], *, path: str | None = None) -> Consoli # zarr_format is present in v2 and v3. zarr_format = parse_zarr_format(v["zarr_format"]) + _validate_consolidated_key(k, zarr_format) if zarr_format == 3: node_type = parse_node_type(v.get("node_type", None)) @@ -216,6 +265,7 @@ def from_dict(cls, data: dict[str, JSON], *, path: str | None = None) -> Consoli else: assert_never(zarr_format) + _validate_consolidated_hierarchy(metadata) cls._flat_to_nested(metadata) return cls(metadata=metadata) diff --git a/tests/test_metadata/test_consolidated.py b/tests/test_metadata/test_consolidated.py index cd0fd92d74..367465f7d6 100644 --- a/tests/test_metadata/test_consolidated.py +++ b/tests/test_metadata/test_consolidated.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import warnings from typing import TYPE_CHECKING, Any import numpy as np @@ -22,7 +23,7 @@ from zarr.core.group import ConsolidatedMetadata, GroupMetadata from zarr.core.metadata import ArrayV3Metadata from zarr.core.metadata.v2 import ArrayV2Metadata -from zarr.errors import ZarrUserWarning +from zarr.errors import MetadataValidationError, ZarrDeprecationWarning, ZarrUserWarning from zarr.storage import StorePath if TYPE_CHECKING: @@ -901,3 +902,101 @@ async def test_consolidated_metadata_case_differing_siblings(memory_store: Store "obs/study/categories", "obs/study/codes", ] + + +# Regression tests for https://github.com/zarr-developers/zarr-python/issues/4453 +# item 2: consolidated metadata accepted invalid listing keys, a listing with no +# parent entry, and a listing below an array. + +_ARRAY_V3 = { + "zarr_format": 3, + "node_type": "array", + "shape": [1], + "data_type": "int8", + "chunk_grid": {"name": "regular", "configuration": {"chunk_shape": [1]}}, + "chunk_key_encoding": {"name": "default"}, + "fill_value": 0, + "codecs": [{"name": "bytes", "configuration": {"endian": "little"}}], +} +_GROUP_V3 = {"zarr_format": 3, "node_type": "group", "attributes": {}} +_GROUP_V2 = {"zarr_format": 2} + + +def _consolidated_doc(metadata: dict[str, Any]) -> dict[str, Any]: + return {"kind": "inline", "must_understand": False, "metadata": metadata} + + +@pytest.mark.parametrize( + "key", + ["", "a//b", "/a", "a/"], +) +def test_consolidated_metadata_rejects_invalid_keys(key: str) -> None: + """Listing keys are node paths; every segment must be non-empty.""" + with pytest.raises(MetadataValidationError, match="Invalid node path"): + ConsolidatedMetadata.from_dict(_consolidated_doc({key: _GROUP_V3})) + + +@pytest.mark.parametrize( + "key", + [".", "..", "zarr.json", "__a", "a/zarr.json", "a/__x"], +) +def test_consolidated_metadata_warns_on_reserved_v3_keys(key: str) -> None: + """v3 reserves '.', '..', 'zarr.json', and '__'-prefixed names, but stores + written before the names were rejected still open, so they only warn.""" + with pytest.warns(ZarrDeprecationWarning, match="reserved"): + consolidated = ConsolidatedMetadata.from_dict( + _consolidated_doc({key: _GROUP_V3, "a": _GROUP_V3}) + ) + if "/" in key: + parent, leaf = key.split("/") + parent_meta = consolidated.metadata[parent] + assert isinstance(parent_meta, GroupMetadata) + nested = parent_meta.consolidated_metadata + assert nested is not None + assert leaf in nested.metadata + else: + assert key in consolidated.metadata + + +def test_consolidated_metadata_reserved_name_below_missing_parent() -> None: + """A reserved name warns before the missing-parent error is raised.""" + with pytest.warns(ZarrDeprecationWarning, match="reserved"): + with pytest.raises(MetadataValidationError, match="without a listing"): + ConsolidatedMetadata.from_dict(_consolidated_doc({"a": _GROUP_V3, "a/../b": _GROUP_V3})) + + +@pytest.mark.parametrize( + "key", + [".", "..", "zarr.json", "__a"], +) +def test_consolidated_metadata_ignores_reserved_v2_keys(key: str) -> None: + """The v2 specification does not reserve node names.""" + with warnings.catch_warnings(): + warnings.simplefilter("error") + consolidated = ConsolidatedMetadata.from_dict(_consolidated_doc({key: _GROUP_V2})) + assert key in consolidated.metadata + + +def test_consolidated_metadata_rejects_child_without_parent() -> None: + """A listing for 'a/b' with no 'a' listing made the root group unopenable.""" + with pytest.raises(MetadataValidationError, match="without a listing"): + ConsolidatedMetadata.from_dict(_consolidated_doc({"a/b": _GROUP_V3})) + + +def test_consolidated_metadata_rejects_node_below_array() -> None: + """Array nodes may not have children; 'arr/child' below an 'arr' array is + invalid.""" + with pytest.raises(MetadataValidationError, match="array"): + ConsolidatedMetadata.from_dict( + _consolidated_doc({"arr": _ARRAY_V3, "arr/child": _GROUP_V3}) + ) + + +def test_consolidated_metadata_accepts_nested_listings() -> None: + consolidated = ConsolidatedMetadata.from_dict( + _consolidated_doc({"a": _GROUP_V3, "a/b": _GROUP_V3, "a/b/c": _ARRAY_V3}) + ) + a = consolidated.metadata["a"] + assert isinstance(a, GroupMetadata) + assert a.consolidated_metadata is not None + assert sorted(a.consolidated_metadata.metadata) == ["b"]