Repository navigation
fix: tighten array metadata field validation - #4494
barlowa124 wants to merge 5 commits into
Conversation
Four metadata fields accepted out-of-contract JSON: - shape accepted booleans ([true] read as (1,)); document shape fields now reject them via parse_shapelike(reject_bool=True). Stored chunks/chunk_shape fields keep the lenient default since older versions wrote true there and repair tests codify reading them. - attributes accepted a list; it now requires a dict with string keys, matching the check GroupMetadata already applies. - dimension_names accepted a bare string, iterating it into characters; strings and bytes are excluded before the element check. - storage_transformers accepted a dict, iterating it into keys; mappings are excluded. Refs zarr-developers#4453 Generated with Devin Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
github-actions
Bot
added
needs release notes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4494 +/- ##
==========================================
+ Coverage 94.69% 94.72% +0.02%
==========================================
Files 94 94
Lines 13606 13676 +70
==========================================
+ Hits 12884 12954 +70
Misses 722 722
🚀 New features to boost your workflow:
|
| if data is None: | ||
| return {} | ||
|
|
||
| if not isinstance(data, dict) or not all(isinstance(k, str) for k in data): |
There was a problem hiding this comment.
this needs to check if the input is Mapping, not dict
| if data is None: | ||
| return {} | ||
|
|
||
| if not isinstance(data, dict) or not all(isinstance(k, str) for k in data): |
There was a problem hiding this comment.
also, this conflicts a bit with the direction in #4400. I think warning first, then issuing a release where we are stricter and raise, is the safer direction
d-v-b
left a comment
d-v-b
left a comment
There was a problem hiding this comment.
- The dimension_names string exclusion and the storage_transformers mapping exclusion are correct.
- In test_array.py, ({"test": ...}) was a dict, not a tuple, so changing it to [...] is a correct fix.
only accepting dicts for attributes is a regression we should avoid, and the parse_shapelike change needs to go via deprecation
|
|
||
|
|
||
| def parse_shapelike(data: ShapeLike) -> tuple[int, ...]: | ||
| def parse_shapelike(data: ShapeLike, *, reject_bool: bool = False) -> tuple[int, ...]: |
There was a problem hiding this comment.
IMO we shouldn't give a parsing routine a kwarg that increases or decreases leniency. i think its better to deprecate bools, then reject them once the deprecation window has elapsed
- parse_attributes checks Mapping rather than dict and emits ZarrDeprecationWarning for out-of-contract input instead of raising; dict(data) still runs so accepted values behave exactly as before - parse_shapelike loses the reject_bool kwarg; bools now warn (ZarrDeprecationWarning) wherever they parse, ahead of rejection once the deprecation window elapses - dimension_names string/bytes exclusion and storage_transformers mapping exclusion unchanged - tests updated: bools and non-mapping attributes are covered by pytest.warns cases; repair fixtures reading stored true chunks keep their assertions
|
5a4f169 reworks this to deprecation-first. 4906534 updates the sharding test that hit the new warning. |
This branch has not been deployed
2 participants
Summary
Refs #4453, fifth checklist item. Four array metadata fields accepted
JSON outside the spec contract.
shape: [true]parsed as(1,)becauseboolsubclassesint.Document
shapefields now reject booleans throughparse_shapelike(..., reject_bool=True)while storedchunks/chunk_shapekeep the lenient default: old versions wrotetruethere and repair tests codify reading them.attributes: []parsed as
{}.parse_attributesnow requires a dict with stringkeys, matching
GroupMetadata's existing check.dimension_names: "xy"parsed as("x", "y")since a bare string isiterable. Strings and bytes are excluded before the element check.
storage_transformers: {"name": ...}parsed as a tuple of keys.mappings are excluded from the iterable check.
Non-
Mappingattributes now emit aZarrDeprecationWarningratherthan raising, since older documents wrote list-valued attributes.
That moved from rejection to deprecation during review. Bool
dimension_nameselements are deprecated the same way.For reviewers
parse_shapelikegains a keyword-onlyreject_boolflag whoselenient default preserves the repair path for stored chunk sizes.
attributes: nullstill parses to{}, matching the group's handlingof missing attributes.
Author attestation
TODO
Done:
Outstanding:
docs/user-guide/*.mdchanges/