fix: remove attribute nesting depth limit - #4330
Draft
d-v-b wants to merge 2 commits into
Draft
Conversation
Assisted-by: Codex:gpt-6
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4330 +/- ##
==========================================
- Coverage 94.30% 94.29% -0.01%
==========================================
Files 92 92
Lines 12915 12893 -22
==========================================
- Hits 12180 12158 -22
Misses 735 735
🚀 New features to boost your workflow:
|
d-v-b
commented
Sep 9, 2026
| 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] |
Contributor
Author
There was a problem hiding this comment.
add a comment explaining why we don't validate here
d-v-b
commented
Sep 9, 2026
| @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: |
Contributor
Author
There was a problem hiding this comment.
surely this test is redundant. we need to simply add a deep attributes example to existing round-trip tests.
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
d-v-b
added a commit
to d-v-b/zarr-python
that referenced
this pull request
Sep 9, 2026
parse_zarr_format needs Literal at runtime for its parse_field type argument, so zarr-developers#4063 added a function-local import next to the existing TYPE_CHECKING one. Import it once at module level like the other modules that commit touched. The docstring and depth-limit corrections this branch originally carried are superseded by zarr-developers#4330, which removes the attribute depth limit and the validate_json_value helper entirely. Assisted-by: ClaudeCode:claude-fable-5-1
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 ai text below 🤖
Summary
A Zarr v3 array can be created with attributes nested more than 64 levels deep, but reopening the same array fails with
ValueError: JSON value nesting exceeds the maximum depth of 64. This restriction was introduced in #4063 and regresses the behavior in 3.3.0.Remove the recursive attribute validator and its depth limit, restoring the existing attribute handling through the JSON reader/writer and the metadata constructor. Keep msgspec validation for structured metadata fields. Update the unreleased changelog entry and add create/reopen regression coverage for nested objects and arrays at depths 0, 65, and 100, for both Zarr formats.
For reviewers
Please check that the restored attribute handling is consistent with array creation and group metadata. This draft is awaiting the human author's edits and review before it is marked ready.
Validation on current upstream main plus this fix: 2,188 tests passed and 48 skipped across the JSON parsing, metadata, group, and array suites in the Python 3.12 minimal Hatch environment. Ruff lint and formatting checks and
git diff --checkpassed.Author attestation
TODO