diff --git a/changes/4494.bugfix.md b/changes/4494.bugfix.md new file mode 100644 index 0000000000..8f94b9b5d0 --- /dev/null +++ b/changes/4494.bugfix.md @@ -0,0 +1 @@ +Tighten array metadata field validation: `dimension_names` rejects a bare string and `storage_transformers` rejects a mapping. `shape` fields that contain booleans and `attributes` fields that are not mappings with string keys still parse as before, but emit a `ZarrDeprecationWarning` ahead of rejection in a future release. diff --git a/src/zarr/core/common.py b/src/zarr/core/common.py index de009d0b50..47d60bcdb2 100644 --- a/src/zarr/core/common.py +++ b/src/zarr/core/common.py @@ -21,7 +21,7 @@ from zarr.core.config import config as zarr_config from zarr.core.json_parse import convert, parse_field -from zarr.errors import ZarrRuntimeWarning +from zarr.errors import ZarrDeprecationWarning, ZarrRuntimeWarning if TYPE_CHECKING: from collections.abc import Awaitable, Callable, Iterator @@ -211,10 +211,21 @@ def parse_named_configuration( return name_parsed, configuration_parsed +def _warn_bool_shapelike() -> None: + msg = ( + "Boolean values in shape-like fields are deprecated and will be " + "rejected in a future release." + ) + warnings.warn(msg, ZarrDeprecationWarning, stacklevel=3) + + def parse_shapelike(data: ShapeLike) -> tuple[int, ...]: """ Parse a shape-like input into an explicit shape. """ + if isinstance(data, bool): + # bools are int subclasses; accept them for now but deprecate + _warn_bool_shapelike() if isinstance(data, int | np.integer): if data < 0: raise ValueError(f"Expected a non-negative integer. Got {data} instead") @@ -228,6 +239,8 @@ def parse_shapelike(data: ShapeLike) -> tuple[int, ...]: if not all(isinstance(v, int | np.integer) for v in data_tuple): msg = f"Expected an iterable of integers. Got {data} instead." raise TypeError(msg) + if any(isinstance(v, bool) for v in data_tuple): + _warn_bool_shapelike() if not all(v > -1 for v in data_tuple): msg = f"Expected all values to be non-negative. Got {data} instead." raise ValueError(msg) diff --git a/src/zarr/core/metadata/common.py b/src/zarr/core/metadata/common.py index 6367bdb28a..cb58d89d06 100644 --- a/src/zarr/core/metadata/common.py +++ b/src/zarr/core/metadata/common.py @@ -1,13 +1,23 @@ from __future__ import annotations -from typing import TYPE_CHECKING +import warnings +from collections.abc import Mapping +from typing import TYPE_CHECKING, cast + +from zarr.errors import ZarrDeprecationWarning if TYPE_CHECKING: from zarr.core.common import JSON -def parse_attributes(data: dict[str, JSON] | None) -> dict[str, JSON]: +def parse_attributes(data: object) -> dict[str, JSON]: if data is None: return {} - - return dict(data) + if not isinstance(data, Mapping) or not all(isinstance(k, str) for k in data): + msg = ( + "Attributes metadata that is not a mapping with string keys is " + "deprecated and will be rejected in a future release. " + f"Got {data!r}." + ) + warnings.warn(msg, ZarrDeprecationWarning, stacklevel=2) + return dict(cast("Mapping[str, JSON]", data)) diff --git a/src/zarr/core/metadata/v3.py b/src/zarr/core/metadata/v3.py index 2d5d169e6b..30e493da10 100644 --- a/src/zarr/core/metadata/v3.py +++ b/src/zarr/core/metadata/v3.py @@ -137,7 +137,11 @@ def validate_codecs(codecs: tuple[Codec, ...], dtype: ZDType[TBaseDType, TBaseSc def parse_dimension_names(data: object) -> tuple[str | None, ...] | None: if data is None: return data - elif isinstance(data, Iterable) and all(isinstance(x, type(None) | str) for x in data): + elif ( + isinstance(data, Iterable) + and not isinstance(data, str | bytes) + and all(isinstance(x, type(None) | str) for x in data) + ): return tuple(data) else: msg = f"Expected either None or an iterable of str, got {type(data)}" @@ -151,7 +155,7 @@ def parse_storage_transformers(data: object) -> tuple[dict[str, JSON], ...]: """ if data is None: return () - if isinstance(data, Iterable) and not isinstance(data, (str, bytes)): + if isinstance(data, Iterable) and not isinstance(data, str | bytes | dict): # Materialise once. The previous implementation called ``len(tuple(data))`` # and then returned ``data`` itself, which exhausted (and discarded) a # one-shot iterable and could return a value typed as a tuple that was not diff --git a/tests/test_array.py b/tests/test_array.py index 611524d474..9276fdfb17 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -344,7 +344,7 @@ def test_storage_transformers(store: MemoryStore, zarr_format: ZarrFormat | str) "chunk_key_encoding": {"name": "v2", "configuration": {"separator": "/"}}, "codecs": (BytesCodec().to_dict(),), "fill_value": 0, - "storage_transformers": ({"test": "should_raise"}), + "storage_transformers": [{"test": "should_raise"}], } else: metadata_dict = { @@ -356,7 +356,7 @@ def test_storage_transformers(store: MemoryStore, zarr_format: ZarrFormat | str) "codecs": (BytesCodec().to_dict(),), "fill_value": 0, "order": "C", - "storage_transformers": ({"test": "should_raise"}), + "storage_transformers": [{"test": "should_raise"}], } if zarr_format == 3: match = "Arrays with storage transformers are not supported in zarr-python at this time." diff --git a/tests/test_codecs/test_sharding.py b/tests/test_codecs/test_sharding.py index 5712c9576d..591f51b7df 100644 --- a/tests/test_codecs/test_sharding.py +++ b/tests/test_codecs/test_sharding.py @@ -36,6 +36,7 @@ from zarr.core.dtype import Int32 from zarr.core.indexing import lexicographic_order_coords from zarr.core.metadata.v3 import ArrayV3Metadata +from zarr.errors import ZarrDeprecationWarning from zarr.storage import MemoryStore, StorePath, ZipStore from ..conftest import ArrayRequest @@ -1492,15 +1493,23 @@ def test_nested_sharding_rejects_indivisible_inner_chunk_shape( assert store._store_dict == {} -@pytest.mark.parametrize("chunk_shape", [(0, 5), (5, 0), (False, 5), [0], 0]) +@pytest.mark.parametrize("chunk_shape", [(0, 5), (5, 0), [0], 0]) def test_sharding_codec_rejects_inner_chunk_size_zero(chunk_shape: Any) -> None: - """`ShardingCodec` rejects an inner chunk size of 0 (or `False`) when it is + """`ShardingCodec` rejects an inner chunk size of 0 when it is constructed, with a `ValueError` naming the dimension, so no array metadata, nested codec or stored document can hold one.""" with pytest.raises(ValueError, match=r"Dimension \d: chunk edge length must be >= 1, got"): ShardingCodec(chunk_shape=chunk_shape) +def test_sharding_codec_rejects_inner_chunk_size_false() -> None: + """`False` in an inner chunk shape warns (booleans in shape-like fields are + deprecated) and is still rejected as a zero edge length.""" + with pytest.warns(ZarrDeprecationWarning, match="Boolean values in shape-like"): + with pytest.raises(ValueError, match=r"Dimension \d: chunk edge length must be >= 1, got"): + ShardingCodec(chunk_shape=(False, 5)) + + @pytest.mark.filterwarnings( "ignore:Combining a `sharding_indexed` codec:zarr.errors.ZarrUserWarning" ) diff --git a/tests/test_common.py b/tests/test_common.py index a83649cfe6..451c394c6c 100644 --- a/tests/test_common.py +++ b/tests/test_common.py @@ -21,6 +21,7 @@ product, ) from zarr.core.config import parse_indexing_order +from zarr.errors import ZarrDeprecationWarning if TYPE_CHECKING: from typing import Any, Literal @@ -223,6 +224,13 @@ def test_parse_shapelike_invalid_iterable_types(data: Any) -> None: parse_shapelike(data) +@pytest.mark.parametrize(("data", "expected"), [(True, (1,)), ([True], (1,)), ((True, 1), (1, 1))]) +def test_parse_shapelike_bool_deprecated(data: Any, expected: tuple[int, ...]) -> None: + """Bools still parse as ints for backward compatibility, but warn.""" + with pytest.warns(ZarrDeprecationWarning, match="Boolean values"): + assert parse_shapelike(data) == expected + + @pytest.mark.parametrize("data", [(1, 2, 3, -1), (-10,)]) def test_parse_shapelike_invalid_iterable_values(data: Any) -> None: """ diff --git a/tests/test_json_parse.py b/tests/test_json_parse.py index d714325d8d..21b8fd049b 100644 --- a/tests/test_json_parse.py +++ b/tests/test_json_parse.py @@ -85,3 +85,12 @@ def test_generator_not_exhausted(self) -> None: def test_non_iterable_rejected(self) -> None: with pytest.raises(TypeError, match="Expected an iterable"): parse_storage_transformers(5) + + def test_dict_rejected(self) -> None: + """A dict is iterable, but iterating it yields keys rather than + transformer objects, so it must be rejected rather than silently + swallowed.""" + with pytest.raises(TypeError, match="Expected an iterable"): + parse_storage_transformers({}) + with pytest.raises(TypeError, match="Expected an iterable"): + parse_storage_transformers({"name": "x"}) diff --git a/tests/test_metadata/test_repair.py b/tests/test_metadata/test_repair.py index 5247754533..21cf8e4db3 100644 --- a/tests/test_metadata/test_repair.py +++ b/tests/test_metadata/test_repair.py @@ -27,7 +27,7 @@ from zarr.core.metadata.v3 import RectilinearChunkGridMetadata, RegularChunkGridMetadata from zarr.core.sync import sync from zarr.dtype import Int16 -from zarr.errors import ZarrUserWarning +from zarr.errors import ZarrDeprecationWarning, ZarrUserWarning from zarr.storage import LocalStore, MemoryStore, StorePath from zarr.storage._common import make_store_path @@ -518,7 +518,6 @@ def _sharding_chunk_shape(chunks: Any) -> tuple[tuple[int, ...], object]: (np.int64(4), (4,)), ((np.int64(4),), (4,)), (np.array([4]), (4,)), - ((True,), (1,)), (range(4, 5), (4,)), ], ) @@ -527,14 +526,23 @@ def test_chunk_shape_read_as_array_shape( ) -> None: """`ArrayV2Metadata` and `ShardingCodec` read their chunk shape as `parse_shapelike` reads an array shape: an integer or an iterable of integers, including NumPy - integers and bools.""" + integers.""" parsed, written = CHUNK_SHAPE_SITES[site](chunks) assert parsed == expected assert all(type(size) is int for size in parsed) assert written == expected -@pytest.mark.parametrize("chunks", [(0,), (False,)]) +@pytest.mark.parametrize("site", CHUNK_SHAPE_SITES) +def test_chunk_shape_bool_deprecated(site: str) -> None: + """A bool chunk size still reads as 1, but warns pending removal.""" + with pytest.warns(ZarrDeprecationWarning, match="Boolean values"): + parsed, written = CHUNK_SHAPE_SITES[site]((True,)) + assert parsed == (1,) + assert written == (1,) + + +@pytest.mark.parametrize("chunks", [(0,)]) def test_v2_chunk_size_zero_written_as_given(chunks: tuple[Any, ...]) -> None: """`ArrayV2Metadata` accepts a chunk size of 0 and writes it back as given; reading a stored 0 is `zarr.core.metadata.repair`' business. `ShardingCodec` rejects one.""" @@ -543,6 +551,15 @@ def test_v2_chunk_size_zero_written_as_given(chunks: tuple[Any, ...]) -> None: assert metadata.to_dict()["chunks"] == (0,) +def test_v2_chunk_size_false_written_as_given() -> None: + """A bool chunk size still reads and writes as before, but warns pending + removal.""" + with pytest.warns(ZarrDeprecationWarning, match="Boolean values"): + metadata = _v2_metadata((False,)) + assert metadata.chunks == (0,) + assert metadata.to_dict()["chunks"] == (0,) + + @pytest.mark.parametrize("site", CHUNK_SHAPE_SITES) def test_chunk_shape_read_as_array_shape_rejects_negative(site: str) -> None: with pytest.raises(ValueError, match="Expected all values to be non-negative"): diff --git a/tests/test_metadata/test_v3.py b/tests/test_metadata/test_v3.py index 94240c36b9..fcfed1f4c3 100644 --- a/tests/test_metadata/test_v3.py +++ b/tests/test_metadata/test_v3.py @@ -31,6 +31,7 @@ MetadataValidationError, NodeTypeValidationError, UnknownCodecError, + ZarrDeprecationWarning, ) if TYPE_CHECKING: @@ -89,13 +90,32 @@ def test_parse_dimension_names_valid(data: Any) -> None: assert result == tuple(data) -@pytest.mark.parametrize("data", [[1, 2, "a"], [None, 3]]) +@pytest.mark.parametrize("data", [[1, 2, "a"], [None, 3], "xy"]) def test_parse_dimension_names_invalid(data: Any) -> None: """Iterables containing non-string elements are rejected.""" with pytest.raises(TypeError, match="Expected either None or"): parse_dimension_names(data) +@pytest.mark.parametrize( + ("data", "expected"), [([], {}), ([["a", 1]], {"a": 1}), ({1: "x"}, {1: "x"})] +) +def test_parse_attributes_lenient_deprecated(data: Any, expected: dict[str, Any]) -> None: + """Non-mapping attributes still parse as before, but warn.""" + from zarr.core.metadata.common import parse_attributes + + with pytest.warns(ZarrDeprecationWarning, match="mapping with string keys"): + assert parse_attributes(data) == expected + + +@pytest.mark.parametrize("data", [{}, {"a": 1}]) +def test_parse_attributes_valid(data: Any) -> None: + """Dicts with string keys are accepted without a warning.""" + from zarr.core.metadata.common import parse_attributes + + assert parse_attributes(data) == data + + def test_parse_codecs_unknown_raises(monkeypatch: pytest.MonkeyPatch) -> None: """An unregistered codec name raises UnknownCodecError.""" from collections import defaultdict