Skip to content

fix: validate consolidated metadata listing keys and hierarchy - #4485

Open
barlowa124 wants to merge 2 commits into
zarr-developers:mainfrom
barlowa124:consolidated-metadata-4453
Open

barlowa124 wants to merge 2 commits into
zarr-developers:mainfrom
barlowa124:consolidated-metadata-4453

Conversation

@barlowa124

@barlowa124 barlowa124 commented Oct 7, 2026 •

Copy link
Copy Markdown

Summary

Fixes the second item in #4453: ConsolidatedMetadata.from_dict (and
GroupMetadata.from_dict, which delegates to it) accepted consolidated
listings the spec rules out, and two of them broke reads.

  • Listing keys are node paths. Keys with an empty segment ("", a//b) or
    a reserved segment (., .., zarr.json, __*) are now rejected with
    MetadataValidationError before any member is parsed.
  • Every non-root key must have all ancestors present as group entries.
    {"a/b": GROUP} used to raise KeyError: 'a', which surfaced as
    GroupNotFoundError on open_group. The root group became unopenable
    because of one entry. It now fails with a clear MetadataValidationError.
    A listing below an array (which members() hit as KeyError) is
    rejected the same way.

The remaining #4453 items (float fill equality, fill-value edges,
document-level member validation) are untouched.

For reviewers

The stricter empty-segment rule is deliberate: consolidated keys are node
paths made of node names, and node names may not be empty. This differs
from #4484's name check, which skips empty segments so "/foo" and
a//b keep their existing create-path normalization. If #4484 lands
first this can reuse its helper for the reserved-name portion.

Scope check: zarr_format: 2 entries inside a v3 listing are still
accepted; the issue lists it among the accepted-invalid cases but the
right handling for v2/v3 mixing is less clear-cut than the path rules, so
it is left out of this PR.

Author attestation

  • I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

TODO

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.md
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Oct 7, 2026
barlowa124 added a commit to barlowa124/zarr-python that referenced this pull request Oct 7, 2026
@barlowa124
barlowa124 force-pushed the consolidated-metadata-4453 branch from 668c547 to 7b01078 Compare October 7, 2026 21:08
@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Oct 7, 2026
@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.70%. Comparing base (069fd20) to head (7b01078).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4485   +/-   ##
=======================================
  Coverage   94.69%   94.70%           
=======================================
  Files          94       94           
  Lines       13606    13626   +20     
=======================================
+ Hits        12884    12904   +20     
  Misses        722      722           
Files with missing lines Coverage Δ
src/zarr/core/group.py 95.76% <100.00%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant