From dd2c7329f866a5ee59f6eb452fafd4f9c0108df3 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 9 Sep 2026 13:00:16 +0200 Subject: [PATCH 1/4] fix: remove attribute nesting depth limit Assisted-by: Codex:gpt-6 --- changes/3285.feature.md | 8 +++---- src/zarr/core/json_parse.py | 43 +++++------------------------------- src/zarr/core/metadata/v3.py | 4 ++-- tests/test_array.py | 18 +++++++++++++++ tests/test_json_parse.py | 41 +++------------------------------- 5 files changed, 32 insertions(+), 82 deletions(-) diff --git a/changes/3285.feature.md b/changes/3285.feature.md index 3809c0ab02..a06a3ece5a 100644 --- a/changes/3285.feature.md +++ b/changes/3285.feature.md @@ -1,9 +1,9 @@ JSON metadata validation now delegates to ``msgspec.convert`` for the type coercions it supports (``Literal`` membership, ``int`` / ``bool`` strictness, -list-to-tuple), replacing the per-field hand-written ``parse_*`` logic. A small -fallback validates the recursive JSON values msgspec cannot, now with an -explicit nesting-depth limit, and a latent generator-exhaustion bug in -``parse_storage_transformers`` is fixed. See #3285. +list-to-tuple), replacing the per-field hand-written ``parse_*`` logic. +User-defined attributes retain their existing JSON handling without an additional +nesting-depth limit. A latent generator-exhaustion bug in +``parse_storage_transformers`` is also fixed. See #3285. As a result some metadata inputs are now parsed more strictly. The previous per-field checks compared values with ``==``, which accepts any numerically diff --git a/src/zarr/core/json_parse.py b/src/zarr/core/json_parse.py index 09c5ca074e..08e95f8a5c 100644 --- a/src/zarr/core/json_parse.py +++ b/src/zarr/core/json_parse.py @@ -5,7 +5,7 @@ needs (``Literal`` membership, ``int``/``bool`` strictness, list-to-tuple, ``TypedDict`` with ``NotRequired``). ``convert`` is a thin wrapper that translates [`msgspec.ValidationError`][msgspec.ValidationError] into the -``TypeError`` the rest of the codebase already raises. +``ValueError`` the rest of the codebase already raises. msgspec cannot handle two things in Zarr's metadata types: @@ -13,24 +13,17 @@ schema-build time, and * PEP 728 ``extra_items=`` extension fields, which it silently drops. -``validate_json_value`` is the small hand-written fallback for the first of -those. See https://github.com/zarr-developers/zarr-python/issues/3285. +User-defined attributes are left to the JSON reader and writer rather than +recursively validated here. See https://github.com/zarr-developers/zarr-python/issues/3285. """ from __future__ import annotations -from collections.abc import Mapping -from typing import TYPE_CHECKING, Any, Final, cast, get_origin +from typing import Any, get_origin import msgspec -if TYPE_CHECKING: - from zarr.core.common import JSON - -__all__ = ["MAX_JSON_DEPTH", "convert", "parse_field", "validate_json_value"] - -MAX_JSON_DEPTH: Final = 64 -"""Maximum nesting depth accepted by ``validate_json_value``.""" +__all__ = ["convert", "parse_field"] def _type_name(type_: Any) -> str: @@ -75,29 +68,3 @@ def parse_field( raise error( f"Failed to parse input for {field!r}: expected {_type_name(type_)}, got {data!r}." ) from exc - - -def validate_json_value(value: object, *, max_depth: int = MAX_JSON_DEPTH, _depth: int = 0) -> JSON: - """Check that ``value`` is a JSON value and return it unchanged. - - msgspec cannot build a schema for Zarr's recursive ``JSON`` / ``JSONValue`` - aliases, so this covers the fields typed that way (``attributes``, - ``fill_value``, extension-field values). Unlike the previous per-field - parsers it also enforces ``max_depth``: a pathologically nested document - could otherwise exhaust the interpreter stack. - """ - if _depth > max_depth: - raise ValueError(f"JSON value nesting exceeds the maximum depth of {max_depth}.") - if value is None or isinstance(value, (bool, int, float, str)): - return cast("JSON", value) - if isinstance(value, (list, tuple)): - for item in value: - validate_json_value(item, max_depth=max_depth, _depth=_depth + 1) - return cast("JSON", value) - if isinstance(value, Mapping): - for key, item in value.items(): - if not isinstance(key, str): - raise TypeError(f"JSON object keys must be str, got {type(key).__name__}.") - validate_json_value(item, max_depth=max_depth, _depth=_depth + 1) - return cast("JSON", value) - raise TypeError(f"Value {value!r} is not a valid JSON value.") diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index 6228e17b75..3ba52251cf 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -34,7 +34,7 @@ from zarr.core.config import config from zarr.core.dtype import VariableLengthUTF8, ZDType, get_data_type_from_json from zarr.core.dtype.common import check_dtype_spec_v3 -from zarr.core.json_parse import parse_field, validate_json_value +from zarr.core.json_parse import parse_field from zarr.core.metadata.common import parse_attributes from zarr.errors import MetadataValidationError, NodeTypeValidationError from zarr.registry import get_codec_class @@ -673,7 +673,7 @@ def from_dict(cls, data: dict[str, JSON]) -> Self: chunk_grid=_data_typed["chunk_grid"], # type: ignore[arg-type] chunk_key_encoding=_data_typed["chunk_key_encoding"], # type: ignore[arg-type] codecs=_data_typed["codecs"], - attributes=validate_json_value(_data_typed.get("attributes", {})), # type: ignore[arg-type] + attributes=_data_typed.get("attributes", {}), # type: ignore[arg-type] dimension_names=_data_typed.get("dimension_names", None), fill_value=fill_value_parsed, data_type=data_type, diff --git a/tests/test_array.py b/tests/test_array.py index 46890244ec..419c738c9b 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -486,6 +486,24 @@ async def test_nbytes_stored_async() -> None: assert result == 902 # the size with all chunks filled. +@pytest.mark.parametrize("zarr_format", [2, 3]) +@pytest.mark.parametrize("depth", [0, 65, 100]) +@pytest.mark.parametrize("container", ["object", "array"]) +def test_reopen_nested_attributes(zarr_format: ZarrFormat, depth: int, container: str) -> None: + """Attributes accepted when creating an array must survive reopening it.""" + value: JSON = "leaf" + for _ in range(depth): + value = {"nested": value} if container == "object" else [value] + attributes = {"payload": value} + store = MemoryStore() + sync_api.create_array( + store, shape=(1,), dtype="int32", attributes=attributes, zarr_format=zarr_format + ) + + reopened = sync_api.open_array(store, mode="r", zarr_format=zarr_format) + assert dict(reopened.attrs) == attributes + + @pytest.mark.parametrize("zarr_format", [2, 3]) def test_update_attrs(zarr_format: ZarrFormat) -> None: # regression test for https://github.com/zarr-developers/zarr-python/issues/2328 diff --git a/tests/test_json_parse.py b/tests/test_json_parse.py index da723119aa..d714325d8d 100644 --- a/tests/test_json_parse.py +++ b/tests/test_json_parse.py @@ -1,10 +1,8 @@ """Tests for :mod:`zarr.core.json_parse`. ``convert`` delegates JSON type coercion to :func:`msgspec.convert` (translating -``msgspec.ValidationError`` into ``TypeError``); ``validate_json_value`` is the -hand-written fallback for the recursive ``JSON`` alias msgspec cannot build, -including a nesting-depth limit. The final group is a regression test for the -``parse_storage_transformers`` fix that motivated the depth limit work. +``msgspec.ValidationError`` into ``ValueError``). The final group covers the +``parse_storage_transformers`` generator-exhaustion regression. """ from __future__ import annotations @@ -13,7 +11,7 @@ import pytest -from zarr.core.json_parse import MAX_JSON_DEPTH, convert, parse_field, validate_json_value +from zarr.core.json_parse import convert, parse_field from zarr.core.metadata.v3 import parse_storage_transformers @@ -64,39 +62,6 @@ class MyError(ValueError): assert isinstance(exc_info.value.__cause__, ValueError) -class TestValidateJsonValue: - @pytest.mark.parametrize("value", [None, True, 1, 1.5, "s"]) - def test_primitives(self, value: object) -> None: - assert validate_json_value(value) is value - - def test_nested(self) -> None: - value = {"a": [1, 2.0, "x", True, None], "b": {"c": [{}]}} - assert validate_json_value(value) is value - - def test_rejects_non_str_keys(self) -> None: - with pytest.raises(TypeError, match="keys must be str"): - validate_json_value({1: "x"}) - - def test_rejects_non_json_leaf(self) -> None: - with pytest.raises(TypeError, match="not a valid JSON value"): - validate_json_value(object()) - with pytest.raises(TypeError, match="not a valid JSON value"): - validate_json_value({"a": object()}) - - def test_depth_limit(self) -> None: - def nest(depth: int) -> object: - v: object = "leaf" - for _ in range(depth): - v = {"k": v} - return v - - # At the limit it passes; one level deeper it is rejected. This bound is - # new behavior the previous per-field parsers never had. - assert validate_json_value(nest(MAX_JSON_DEPTH)) is not None - with pytest.raises(ValueError, match="maximum depth"): - validate_json_value(nest(MAX_JSON_DEPTH + 1)) - - class TestStorageTransformersRegression: """`parse_storage_transformers` used to call `len(tuple(data))` and then return `data` itself, exhausting a one-shot iterable and returning a value From f6a8719f929cf350915ec071d099ce0976575de6 Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 9 Sep 2026 15:20:07 +0200 Subject: [PATCH 2/4] test: fold the deep-attribute regression into existing round-trip tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on the PR: the standalone test_reopen_nested_attributes was redundant with the round-trip tests that already exist. Drop it and add a 100-level attributes value in two of them instead: the ArrayV3Metadata.from_dict/to_dict table in test_v3.py (the layer where the limit lived) and test_update_attrs in test_array.py, which creates an array, writes attributes and reopens it for both Zarr formats — the user-visible path that regressed. Both new cases fail on main with "nesting exceeds the maximum depth of 64" and pass here. Also add the comment the review asked for at the attributes= line in from_dict, saying why attributes are not validated there. Assisted-by: ClaudeCode:claude-fable-5-1 --- src/zarr/core/metadata/v3.py | 5 +++++ tests/test_array.py | 26 ++++++++------------------ tests/test_metadata/test_v3.py | 13 +++++++++++++ 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index 3ba52251cf..3e4c96be3e 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -673,6 +673,11 @@ def from_dict(cls, data: dict[str, JSON]) -> Self: chunk_grid=_data_typed["chunk_grid"], # type: ignore[arg-type] chunk_key_encoding=_data_typed["chunk_key_encoding"], # type: ignore[arg-type] codecs=_data_typed["codecs"], + # Attributes are user data with no schema of their own: anything the + # JSON decoder produced is a valid value, so there is nothing to + # validate beyond `parse_attributes` checking that it is a mapping + # (done in `__init__`). Recursing into them here only to enforce a + # nesting limit rejected documents this library itself had written. attributes=_data_typed.get("attributes", {}), # type: ignore[arg-type] dimension_names=_data_typed.get("dimension_names", None), fill_value=fill_value_parsed, diff --git a/tests/test_array.py b/tests/test_array.py index 419c738c9b..25e2906139 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -486,24 +486,6 @@ async def test_nbytes_stored_async() -> None: assert result == 902 # the size with all chunks filled. -@pytest.mark.parametrize("zarr_format", [2, 3]) -@pytest.mark.parametrize("depth", [0, 65, 100]) -@pytest.mark.parametrize("container", ["object", "array"]) -def test_reopen_nested_attributes(zarr_format: ZarrFormat, depth: int, container: str) -> None: - """Attributes accepted when creating an array must survive reopening it.""" - value: JSON = "leaf" - for _ in range(depth): - value = {"nested": value} if container == "object" else [value] - attributes = {"payload": value} - store = MemoryStore() - sync_api.create_array( - store, shape=(1,), dtype="int32", attributes=attributes, zarr_format=zarr_format - ) - - reopened = sync_api.open_array(store, mode="r", zarr_format=zarr_format) - assert dict(reopened.attrs) == attributes - - @pytest.mark.parametrize("zarr_format", [2, 3]) def test_update_attrs(zarr_format: ZarrFormat) -> None: # regression test for https://github.com/zarr-developers/zarr-python/issues/2328 @@ -513,9 +495,17 @@ def test_update_attrs(zarr_format: ZarrFormat) -> None: ) arr.attrs["foo"] = "bar" assert arr.attrs["foo"] == "bar" + # Deeply nested values must survive the reopen too: metadata parsing once + # capped attribute nesting at 64 levels, rejecting arrays this library had + # itself written (regression test for the limit introduced in #4063). + deep: JSON = "leaf" + for _ in range(100): + deep = {"nested": [deep]} + arr.attrs["deep"] = deep arr2 = zarr.open_array(store=store, zarr_format=zarr_format) assert arr2.attrs["foo"] == "bar" + assert arr2.attrs["deep"] == deep @pytest.mark.parametrize(("chunks", "shards"), [((2, 2), None), ((2, 2), (4, 4))]) diff --git a/tests/test_metadata/test_v3.py b/tests/test_metadata/test_v3.py index 9f78ed7b70..b73051060e 100644 --- a/tests/test_metadata/test_v3.py +++ b/tests/test_metadata/test_v3.py @@ -174,6 +174,11 @@ def test_array_metadata_keys_matches_typeddict() -> None: # Codecs after evolution for single-byte (uint8) and multi-byte (float64) types. _UINT8_CODECS = ({"name": "bytes"},) _FLOAT64_CODECS = ({"name": "bytes", "configuration": {"endian": "little"}},) +# 100 alternating object/array levels: deeper than the 64-level cap that +# from_dict once enforced on attributes. +_DEEP_ATTRIBUTES: dict[str, Any] = {"payload": "leaf"} +for _ in range(100): + _DEEP_ATTRIBUTES = {"payload": [_DEEP_ATTRIBUTES]} @pytest.mark.parametrize( @@ -249,6 +254,14 @@ def test_array_metadata_keys_matches_typeddict() -> None: ), id="extra_fields", ), + Expect( + # Attributes are not depth-limited: 100 levels once tripped a 64-level + # cap in from_dict that create_array never applied, so arrays this + # library wrote could not be reopened. + input={"attributes": _DEEP_ATTRIBUTES}, + output=minimal_metadata_dict_v3(attributes=_DEEP_ATTRIBUTES, codecs=_UINT8_CODECS), + id="deeply_nested_attributes", + ), ], ids=lambda case: case.id, ) From b6008450d3a27a04e5f74e6eddda033948938c2d Mon Sep 17 00:00:00 2001 From: Davis Vann Bennett Date: Wed, 9 Sep 2026 18:38:46 +0200 Subject: [PATCH 3/4] test: generate nested attributes for round-trip coverage Assisted-by: Codex:gpt-6 --- src/zarr/core/metadata/v3.py | 7 ++----- tests/conftest.py | 28 ++++++++++++++++++++++++++++ tests/test_array.py | 25 ++++++++++++------------- tests/test_metadata/test_v3.py | 13 ------------- 4 files changed, 42 insertions(+), 31 deletions(-) diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index 3e4c96be3e..2aaa98cf27 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -673,11 +673,8 @@ def from_dict(cls, data: dict[str, JSON]) -> Self: chunk_grid=_data_typed["chunk_grid"], # type: ignore[arg-type] chunk_key_encoding=_data_typed["chunk_key_encoding"], # type: ignore[arg-type] codecs=_data_typed["codecs"], - # Attributes are user data with no schema of their own: anything the - # JSON decoder produced is a valid value, so there is nothing to - # validate beyond `parse_attributes` checking that it is a mapping - # (done in `__init__`). Recursing into them here only to enforce a - # nesting limit rejected documents this library itself had written. + # Attribute values are arbitrary JSON, so they have no field-specific + # schema to validate. `__init__` checks the outer dict via `parse_attributes`. attributes=_data_typed.get("attributes", {}), # type: ignore[arg-type] dimension_names=_data_typed.get("dimension_names", None), fill_value=fill_value_parsed, diff --git a/tests/conftest.py b/tests/conftest.py index b54e5b4912..24a2da8541 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,6 +13,7 @@ import numpy.typing as npt import pytest from hypothesis import HealthCheck, Verbosity, settings +from hypothesis import strategies as st import zarr.registry from zarr import AsyncGroup, config @@ -296,6 +297,33 @@ def pytest_collection_modifyitems(config: Any, items: Any) -> None: settings.load_profile(os.getenv("HYPOTHESIS_PROFILE", "default")) +@st.composite +def json_attributes(draw: st.DrawFn, *, depth: int) -> dict[str, JSON]: + """Generate JSON attribute dictionaries with additional nested object/array layers.""" + keys = st.text(max_size=8) + scalars = ( + st.none() + | st.booleans() + | st.integers() + | st.floats(allow_nan=False, allow_infinity=False) + | st.text(max_size=8) + ) + values = st.recursive( + scalars, + lambda children: ( + st.lists(children, max_size=3) | st.dictionaries(keys, children, max_size=3) + ), + max_leaves=8, + ) + attributes: dict[str, JSON] = draw(st.dictionaries(keys, values, max_size=3)) + if depth: + value: JSON = attributes + for is_object in draw(st.lists(st.booleans(), min_size=depth, max_size=depth)): + value = {draw(keys): value} if is_object else [value] + attributes = {draw(keys): value} + return attributes + + # TODO: uncomment these overrides when we can get mypy to accept them """ @overload diff --git a/tests/test_array.py b/tests/test_array.py index 25e2906139..365f371f9b 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -14,11 +14,13 @@ import numpy as np import numpy.typing as npt import pytest +from hypothesis import given, settings +from hypothesis import strategies as st from packaging.version import Version import zarr.api.asynchronous import zarr.api.synchronous as sync_api -from tests.conftest import skip_object_dtype +from tests.conftest import json_attributes, skip_object_dtype from zarr import Array, Group from zarr.abc.store import Store from zarr.codecs import ( @@ -487,25 +489,22 @@ async def test_nbytes_stored_async() -> None: @pytest.mark.parametrize("zarr_format", [2, 3]) -def test_update_attrs(zarr_format: ZarrFormat) -> None: +@pytest.mark.parametrize("depth", [0, 1, 8, 32, 65, 100]) +@settings(max_examples=20, deadline=None) +@given(data=st.data()) +def test_update_attrs(zarr_format: ZarrFormat, depth: int, data: st.DataObject) -> None: # regression test for https://github.com/zarr-developers/zarr-python/issues/2328 store = MemoryStore() arr = zarr.create_array( store=store, shape=(5,), chunks=(5,), dtype="f8", zarr_format=zarr_format ) - arr.attrs["foo"] = "bar" - assert arr.attrs["foo"] == "bar" - # Deeply nested values must survive the reopen too: metadata parsing once - # capped attribute nesting at 64 levels, rejecting arrays this library had - # itself written (regression test for the limit introduced in #4063). - deep: JSON = "leaf" - for _ in range(100): - deep = {"nested": [deep]} - arr.attrs["deep"] = deep + attributes = data.draw(json_attributes(depth=depth)) + for key, value in attributes.items(): + arr.attrs[key] = value + assert dict(arr.attrs) == attributes arr2 = zarr.open_array(store=store, zarr_format=zarr_format) - assert arr2.attrs["foo"] == "bar" - assert arr2.attrs["deep"] == deep + assert dict(arr2.attrs) == attributes @pytest.mark.parametrize(("chunks", "shards"), [((2, 2), None), ((2, 2), (4, 4))]) diff --git a/tests/test_metadata/test_v3.py b/tests/test_metadata/test_v3.py index b73051060e..9f78ed7b70 100644 --- a/tests/test_metadata/test_v3.py +++ b/tests/test_metadata/test_v3.py @@ -174,11 +174,6 @@ def test_array_metadata_keys_matches_typeddict() -> None: # Codecs after evolution for single-byte (uint8) and multi-byte (float64) types. _UINT8_CODECS = ({"name": "bytes"},) _FLOAT64_CODECS = ({"name": "bytes", "configuration": {"endian": "little"}},) -# 100 alternating object/array levels: deeper than the 64-level cap that -# from_dict once enforced on attributes. -_DEEP_ATTRIBUTES: dict[str, Any] = {"payload": "leaf"} -for _ in range(100): - _DEEP_ATTRIBUTES = {"payload": [_DEEP_ATTRIBUTES]} @pytest.mark.parametrize( @@ -254,14 +249,6 @@ def test_array_metadata_keys_matches_typeddict() -> None: ), id="extra_fields", ), - Expect( - # Attributes are not depth-limited: 100 levels once tripped a 64-level - # cap in from_dict that create_array never applied, so arrays this - # library wrote could not be reopened. - input={"attributes": _DEEP_ATTRIBUTES}, - output=minimal_metadata_dict_v3(attributes=_DEEP_ATTRIBUTES, codecs=_UINT8_CODECS), - id="deeply_nested_attributes", - ), ], ids=lambda case: case.id, ) From 9ed67096b9ddaca1919c1e4f8ae279d1b270343b Mon Sep 17 00:00:00 2001 From: Davis Bennett Date: Wed, 9 Sep 2026 18:52:33 +0200 Subject: [PATCH 4/4] Apply suggestion from @d-v-b --- changes/3285.feature.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changes/3285.feature.md b/changes/3285.feature.md index a06a3ece5a..520047942b 100644 --- a/changes/3285.feature.md +++ b/changes/3285.feature.md @@ -1,8 +1,8 @@ JSON metadata validation now delegates to ``msgspec.convert`` for the type coercions it supports (``Literal`` membership, ``int`` / ``bool`` strictness, list-to-tuple), replacing the per-field hand-written ``parse_*`` logic. -User-defined attributes retain their existing JSON handling without an additional -nesting-depth limit. A latent generator-exhaustion bug in +User-defined attributes retain their existing JSON handling. +A latent generator-exhaustion bug in ``parse_storage_transformers`` is also fixed. See #3285. As a result some metadata inputs are now parsed more strictly. The previous