diff --git a/changes/3285.feature.md b/changes/3285.feature.md index 3809c0ab02..520047942b 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. +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..2aaa98cf27 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,9 @@ 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] + # 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, data_type=data_type, 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 46890244ec..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,17 +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" + 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 dict(arr2.attrs) == attributes @pytest.mark.parametrize(("chunks", "shards"), [((2, 2), None), ((2, 2), (4, 4))]) 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