From cdae5abd1fcbcfbce80e16391da4d075608aad84 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:50:39 +0000 Subject: [PATCH 1/6] Fix zarr entry filtering for URLs pointing inside a Zarr asset Two bugs in the partial-zarr download support added in #1816, both affecting the use case of #1461 (an asset path that points within a zarr): - The filter derived from a URL's zarr subpath was accumulated into a single global list and handed to every `Downloader`, so in a multi-URL invocation one URL's subpath silently restricted the download of all the others: `dandi download /sub/path ` fetched only the `sub/path`-matching subset of zarrB. URL-derived filters are now resolved per-URL through a new `ParsedDandiURL.get_zarr_filter()` hook, overridden by `AssetZarrEntryURL`. Filters from `--zarr` remain global, as before. - A subpath that matched no entries (e.g. a typo) reported "done" and exited 0 without downloading anything, where before #1816 the user got a clear "No asset at path" error. Entries named by the URL are now required to exist: matching none is reported as an error. Filters from `--zarr` keep their previous semantics of being selectors that may legitimately match nothing. Since the two filter sources are now distinguished, `--sync` reports which of them it conflicts with instead of always naming `--zarr`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- dandi/dandiarchive.py | 19 +++++ dandi/download.py | 67 +++++++++++------ dandi/tests/test_dandiarchive.py | 24 +++++++ dandi/tests/test_download.py | 119 ++++++++++++++++++++++++++++++- 4 files changed, 207 insertions(+), 22 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 262d162d8..9e4dbf7cc 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -54,6 +54,7 @@ from .dandiapi import BaseRemoteAsset, DandiAPIClient, RemoteDandiset from .exceptions import FailedToConnectError, NotFoundError, UnknownURLError from .utils import get_instance, get_retry_after +from .zarr_filter import ZarrFilter lgr = get_logger() @@ -201,6 +202,20 @@ def get_asset_download_path( """ ... + def get_zarr_filter(self) -> list[ZarrFilter]: + """ + Returns the filters restricting which entries within the Zarr assets + returned by `get_assets()` should be downloaded. An empty list means + that no restriction is implied by the URL and all entries are to be + downloaded. + + Only `AssetZarrEntryURL` — a URL pointing inside a Zarr asset — returns + a non-empty list. + + :meta private: + """ + return [] + @abstractmethod def is_under_download_path(self, path: str) -> bool: """ @@ -512,6 +527,10 @@ def get_assets( with _maybe_strict(strict): yield dandiset.get_asset_by_path(self.asset_path) + def get_zarr_filter(self) -> list[ZarrFilter]: + """Restrict the download to the entries at or under `zarr_subpath`.""" + return [ZarrFilter("path", self.zarr_subpath)] + @dataclass class AssetFolderURL(MultiAssetURL): diff --git a/dandi/download.py b/dandi/download.py index a87d5380e..6eb44c67f 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -48,7 +48,6 @@ from .dandiapi import AssetType, BaseRemoteZarrAsset, RemoteDandiset from .dandiarchive import ( AssetItemURL, - AssetZarrEntryURL, DandisetURL, ParsedDandiURL, SingleAssetURL, @@ -122,23 +121,20 @@ def download( parsed_urls = [parse_dandi_url(u, glob=path_type is PathType.GLOB) for u in urls] - # Parse zarr entry filters - zarr_entry_filter: Callable[[str], bool] | None = None - all_zf: list[ZarrFilter] = [] - if zarr_filters: - for spec in zarr_filters: - all_zf.extend(parse_zarr_filter(spec)) + # Parse the explicit ``--zarr`` filters. Filters implied by a URL that + # points inside a Zarr asset are resolved per-URL by each `Downloader`, so + # that the subpath of one URL does not restrict the download of another. + explicit_zarr_filters: list[ZarrFilter] = [] + for spec in zarr_filters: + explicit_zarr_filters.extend(parse_zarr_filter(spec)) - # Merge URL-derived path filters from AssetZarrEntryURL - for purl in parsed_urls: - if isinstance(purl, AssetZarrEntryURL): - all_zf.append(ZarrFilter("path", purl.zarr_subpath)) - - if all_zf: - zarr_entry_filter = make_zarr_entry_filter(all_zf) - - if sync and zarr_entry_filter is not None: - raise ValueError("--sync and --zarr cannot be used together") + if sync: + if explicit_zarr_filters: + raise ValueError("--sync and --zarr cannot be used together") + if any(purl.get_zarr_filter() for purl in parsed_urls): + raise ValueError( + "--sync cannot be used with a URL pointing inside a Zarr asset" + ) # dandi.cli.formatters are used in cmd_ls to provide switchable pyout_style = pyouts.get_style(hide_if_missing=False) @@ -176,7 +172,7 @@ def download( preserve_tree=preserve_tree, jobs_per_zarr=jobs_per_zarr, on_error="yield" if format is DownloadFormat.PYOUT else "raise", - zarr_entry_filter=zarr_entry_filter, + zarr_filters=explicit_zarr_filters, **kw, ) for purl in parsed_urls @@ -272,7 +268,16 @@ class Downloader: preserve_tree: bool jobs_per_zarr: int | None on_error: Literal["raise", "yield"] - zarr_entry_filter: Callable[[str], bool] | None = None + #: Filters from the ``--zarr`` option; they apply to every Zarr asset, and + #: matching no entries in a given asset is not an error + zarr_filters: list[ZarrFilter] = field(default_factory=list) + #: Predicate for selecting entries within a Zarr asset, combining + #: `zarr_filters` with the filters implied by `url`; `None` means that + #: every entry is to be downloaded + zarr_entry_filter: Callable[[str], bool] | None = field(init=False) + #: Message to report when the filters implied by `url` match no entries in + #: a Zarr asset; `None` when `url` implies no filters + empty_zarr_filter_error: str | None = field(init=False) #: which will be set .gen to assets. Purpose is to make it possible to get #: summary statistics while already downloading. TODO: reimplement #: properly! @@ -292,6 +297,21 @@ def __post_init__(self, output_dir: str | Path) -> None: else: self.output_prefix = Path() self.output_path = Path(output_dir, self.output_prefix) + # Filters implied by the URL (i.e., a URL pointing inside a Zarr + # asset) name entries the user explicitly asked for, so -- unlike the + # ``--zarr`` filters -- matching nothing is an error. + url_zarr_filters = self.url.get_zarr_filter() + all_filters = url_zarr_filters + list(self.zarr_filters) + self.zarr_entry_filter = ( + make_zarr_entry_filter(all_filters) if all_filters else None + ) + if url_zarr_filters: + patterns = ", ".join(repr(f.pattern) for f in url_zarr_filters) + self.empty_zarr_filter_error = ( + f"No entries in the Zarr asset match {patterns}" + ) + else: + self.empty_zarr_filter_error = None def is_dandiset_yaml(self) -> bool: return isinstance(self.url, AssetItemURL) and self.url.path == "dandiset.yaml" @@ -398,6 +418,7 @@ def download_generator(self) -> Iterator[dict]: jobs=self.jobs_per_zarr, lock=lock, zarr_entry_filter=self.zarr_entry_filter, + empty_filter_error=self.empty_zarr_filter_error, ) def _progress_filter(gen): @@ -1041,6 +1062,7 @@ def _download_zarr( lock: Lock, jobs: int | None = None, zarr_entry_filter: Callable[[str], bool] | None = None, + empty_filter_error: str | None = None, ) -> Iterator[dict]: # Avoid heavy import by importing within function: from .support.digests import get_zarr_checksum @@ -1095,8 +1117,11 @@ def downloads_gen(): break else: if zarr_entry_filter is not None: - # Filter matched no entries; still report completion - yield {"status": "done"} + if not entries and empty_filter_error is not None: + yield {"status": "error", "message": empty_filter_error} + else: + # Filter matched no entries; still report completion + yield {"status": "done"} return if zarr_entry_filter is not None: diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 1154b025b..1cd28445a 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -25,6 +25,7 @@ from dandi.tests.skip import mark from .fixtures import DandiAPI, SampleDandiset +from ..zarr_filter import ZarrFilter @pytest.mark.parametrize( @@ -409,6 +410,29 @@ def test_split_zarr_location(location: str, expected: tuple[str, str] | None) -> assert split_zarr_location(location) == expected +@pytest.mark.ai_generated +def test_asset_zarr_entry_url_get_zarr_filter() -> None: + """A URL pointing inside a Zarr asset restricts the download to its subpath.""" + url = parse_dandi_url("dandi://dandi/000108/sub-1/file.ome.zarr/0/0/0") + assert isinstance(url, AssetZarrEntryURL) + assert url.get_zarr_filter() == [ZarrFilter("path", "0/0/0")] + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "url", + [ + "dandi://dandi/000108", + "dandi://dandi/000108/sub-1/file.ome.zarr", + "dandi://dandi/000108/sub-1/file.nwb", + "dandi://dandi/000108/sub-1/", + ], +) +def test_non_zarr_entry_urls_have_no_zarr_filter(url: str) -> None: + """Any other URL leaves Zarr assets unfiltered.""" + assert parse_dandi_url(url).get_zarr_filter() == [] + + @pytest.mark.parametrize( "url,parsed_url", [ diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 43bb9579f..87a6c7ec5 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -32,7 +32,7 @@ from .skip import mark from .test_helpers import TWO_ARRAY_ZARR_LAYOUT, assert_dirtrees_eq, zarr_format_of from ..consts import DRAFT, MTIME_TOLERANCE, SyncMode, dandiset_metadata_file -from ..dandiarchive import DandisetURL +from ..dandiarchive import DandisetURL, parse_dandi_url from ..download import ( DownloadDirectory, Downloader, @@ -48,6 +48,7 @@ from ..exceptions import NotFoundError from ..support.digests import Digester from ..utils import list_paths, yaml_load +from ..zarr_filter import ZarrFilter # both urls point to 000027 (lean test dataset), and both draft and "released" @@ -1652,3 +1653,119 @@ def test_download_zarr_sync_conflict() -> None: sync=True, zarr_filters=("metadata",), ) + + +def _make_downloader( + url: str, tmp_path: Path, zarr_filters: list[ZarrFilter] | None = None +) -> Downloader: + return Downloader( + url=parse_dandi_url(url), + output_dir=tmp_path, + existing=DownloadExisting.ERROR, + get_metadata=True, + get_assets=True, + preserve_tree=False, + jobs_per_zarr=None, + on_error="raise", + zarr_filters=zarr_filters if zarr_filters is not None else [], + ) + + +@pytest.mark.ai_generated +def test_downloader_zarr_filter_is_per_url(tmp_path: Path) -> None: + """A URL's Zarr subpath must not restrict the download of any other URL.""" + dl = _make_downloader("dandi://dandi/000108/sub-1/file.ome.zarr/0/0", tmp_path) + assert dl.zarr_entry_filter is not None + assert dl.zarr_entry_filter("0/0/.zarray") + assert not dl.zarr_entry_filter("1/1/.zarray") + # Entries the URL asked for must exist, so matching nothing is an error + assert dl.empty_zarr_filter_error is not None + + # A second URL downloaded in the same invocation is unaffected + other = _make_downloader("dandi://dandi/000108/sub-2/other.ome.zarr", tmp_path) + assert other.zarr_entry_filter is None + assert other.empty_zarr_filter_error is None + + +@pytest.mark.ai_generated +def test_downloader_explicit_zarr_filters_apply_to_all_urls(tmp_path: Path) -> None: + """``--zarr`` filters apply to every asset, and matching nothing is not an error.""" + dl = _make_downloader( + "dandi://dandi/000108/sub-2/other.ome.zarr", + tmp_path, + zarr_filters=[ZarrFilter("path", "a")], + ) + assert dl.zarr_entry_filter is not None + assert dl.zarr_entry_filter("a/data.bin") + assert not dl.zarr_entry_filter("b/data.bin") + assert dl.empty_zarr_filter_error is None + + +@pytest.mark.ai_generated +def test_download_zarr_url_sync_conflict() -> None: + """--sync cannot be combined with a URL pointing inside a Zarr asset.""" + with pytest.raises( + ValueError, match="--sync cannot be used with a URL pointing inside a Zarr" + ): + download( + "dandi://dandi/000027/sample.zarr/0/0", + "/tmp/unused", + sync=True, + ) + + +def _upload_two_zarrs(ds: SampleDandiset) -> None: + """Upload ``sample.zarr`` (subdirs a, b) and ``other.zarr`` (subdirs c, d).""" + for name, subdirs in [("sample.zarr", "ab"), ("other.zarr", "cd")]: + zf = ds.dspath / name + zf.mkdir() + for sub in subdirs: + (zf / sub).mkdir() + (zf / sub / "data.bin").write_text(f"data-{sub}") + ds.upload(validation="skip") + + +@pytest.mark.ai_generated +def test_download_zarr_url_subpath( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A URL pointing inside a Zarr asset downloads only that subtree.""" + _upload_two_zarrs(new_dandiset) + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/a", + tmp_path, + ) + zarr_dir = tmp_path / "sample.zarr" + assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" + assert not (zarr_dir / "b").exists() + + +@pytest.mark.ai_generated +def test_download_zarr_url_subpath_nonexistent( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A URL naming a nonexistent path inside a Zarr asset errors out.""" + _upload_two_zarrs(new_dandiset) + with pytest.raises(RuntimeError, match="1 error while downloading"): + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/nonexistent", + tmp_path, + ) + + +@pytest.mark.ai_generated +def test_download_zarr_url_subpath_does_not_filter_other_urls( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A Zarr subpath in one URL must not restrict a second URL's download.""" + _upload_two_zarrs(new_dandiset) + prefix = f"dandi://{new_dandiset.api.instance_id}/{new_dandiset.dandiset_id}" + download([f"{prefix}/sample.zarr/a", f"{prefix}/other.zarr"], tmp_path) + # First URL: only the requested subtree + assert (tmp_path / "sample.zarr" / "a" / "data.bin").exists() + assert not (tmp_path / "sample.zarr" / "b").exists() + # Second URL: the whole Zarr, unaffected by the first URL's subpath + assert (tmp_path / "other.zarr" / "c" / "data.bin").read_text() == "data-c" + assert (tmp_path / "other.zarr" / "d" / "data.bin").read_text() == "data-d" From 33432aff2b16939764542483c72e5c422dc590c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 22:03:33 +0000 Subject: [PATCH 2/6] Close gaps found reviewing the zarr entry filtering fixes - A `--zarr` filter that matched entries masked a URL subpath that matched none: the emptiness check looked at the combined (OR'ed) result, so `dandi download --zarr metadata /typo` downloaded the metadata files and exited 0. Track matches of the URL-derived filters separately from the combined predicate, and check them on both the "nothing downloaded" and the "download completed" paths. - A URL with a subpath whose asset turns out not to be a Zarr (a blob named `foo.zarr`) downloaded the whole blob and discarded the subpath silently; report it as an error instead. - `AssetZarrEntryURL.get_zarr_filter()` now returns no filters for an empty `zarr_subpath`. `parse_dandi_url()` cannot produce one, but the class is public and such a filter would reject every entry. `_download_zarr()` now takes the required filters themselves rather than a prepared message, which drops the duplicate naming between it and `Downloader` and lets the message be built where the error is reported. Tests: the new `_download_zarr()` unit tests exercise the required-filter logic without a local archive instance, so the fixes stay covered in the jobs that always run; the integration test now asserts the error message rather than just that some error occurred. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- dandi/dandiarchive.py | 4 + dandi/download.py | 77 ++++++++++++------ dandi/tests/test_dandiarchive.py | 2 +- dandi/tests/test_download.py | 133 ++++++++++++++++++++++++++++--- 4 files changed, 180 insertions(+), 36 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 9e4dbf7cc..9fa34a2dc 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -529,6 +529,10 @@ def get_assets( def get_zarr_filter(self) -> list[ZarrFilter]: """Restrict the download to the entries at or under `zarr_subpath`.""" + if not self.zarr_subpath: + # `parse_dandi_url()` never produces this, but the class is public + # and an empty subpath would otherwise reject every entry. + return [] return [ZarrFilter("path", self.zarr_subpath)] diff --git a/dandi/download.py b/dandi/download.py index 6eb44c67f..e5c916501 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -271,13 +271,14 @@ class Downloader: #: Filters from the ``--zarr`` option; they apply to every Zarr asset, and #: matching no entries in a given asset is not an error zarr_filters: list[ZarrFilter] = field(default_factory=list) + #: Filters implied by `url` pointing inside a Zarr asset. Unlike + #: `zarr_filters`, these name entries the user explicitly asked for, so + #: matching none of them is an error + url_zarr_filters: list[ZarrFilter] = field(init=False, default_factory=list) #: Predicate for selecting entries within a Zarr asset, combining - #: `zarr_filters` with the filters implied by `url`; `None` means that - #: every entry is to be downloaded - zarr_entry_filter: Callable[[str], bool] | None = field(init=False) - #: Message to report when the filters implied by `url` match no entries in - #: a Zarr asset; `None` when `url` implies no filters - empty_zarr_filter_error: str | None = field(init=False) + #: `zarr_filters` with `url_zarr_filters`; `None` means that every entry is + #: to be downloaded + zarr_entry_filter: Callable[[str], bool] | None = field(init=False, default=None) #: which will be set .gen to assets. Purpose is to make it possible to get #: summary statistics while already downloading. TODO: reimplement #: properly! @@ -297,21 +298,11 @@ def __post_init__(self, output_dir: str | Path) -> None: else: self.output_prefix = Path() self.output_path = Path(output_dir, self.output_prefix) - # Filters implied by the URL (i.e., a URL pointing inside a Zarr - # asset) name entries the user explicitly asked for, so -- unlike the - # ``--zarr`` filters -- matching nothing is an error. - url_zarr_filters = self.url.get_zarr_filter() - all_filters = url_zarr_filters + list(self.zarr_filters) + self.url_zarr_filters = self.url.get_zarr_filter() + all_filters = self.url_zarr_filters + list(self.zarr_filters) self.zarr_entry_filter = ( make_zarr_entry_filter(all_filters) if all_filters else None ) - if url_zarr_filters: - patterns = ", ".join(repr(f.pattern) for f in url_zarr_filters) - self.empty_zarr_filter_error = ( - f"No entries in the Zarr asset match {patterns}" - ) - else: - self.empty_zarr_filter_error = None def is_dandiset_yaml(self) -> bool: return isinstance(self.url, AssetItemURL) and self.url.path == "dandiset.yaml" @@ -365,6 +356,17 @@ def download_generator(self) -> Iterator[dict]: download_path = Path(self.output_path, path) path = str(self.output_prefix / path) + if self.url_zarr_filters and asset.asset_type is not AssetType.ZARR: + # The URL named a path inside the asset, but the asset is + # not a Zarr, so there is nothing to descend into. + yield { + "path": path, + "status": "error", + "message": f"Asset {asset.path!r} is not a Zarr asset," + " so it has no entries to download", + } + continue + try: metadata = asset.get_raw_metadata() except NotFoundError as e: @@ -418,7 +420,7 @@ def download_generator(self) -> Iterator[dict]: jobs=self.jobs_per_zarr, lock=lock, zarr_entry_filter=self.zarr_entry_filter, - empty_filter_error=self.empty_zarr_filter_error, + required_filters=self.url_zarr_filters, ) def _progress_filter(gen): @@ -1062,7 +1064,7 @@ def _download_zarr( lock: Lock, jobs: int | None = None, zarr_entry_filter: Callable[[str], bool] | None = None, - empty_filter_error: str | None = None, + required_filters: list[ZarrFilter] | None = None, ) -> Iterator[dict]: # Avoid heavy import by importing within function: from .support.digests import get_zarr_checksum @@ -1072,16 +1074,34 @@ def _download_zarr( entries: list = [] digests: dict[str, str] = {} pc = ProgressCombiner(zarr_size=asset.size) + # `zarr_entry_filter` is the OR of the required filters and the ``--zarr`` + # ones, so a non-empty `entries` does not mean the required filters + # matched; track them separately. + required_match = ( + make_zarr_entry_filter(required_filters) if required_filters else None + ) + matched_required = False + + def unmatched_required_error() -> dict: + assert required_filters is not None + patterns = ", ".join(repr(f.pattern) for f in required_filters) + return { + "status": "error", + "message": f"No entries in the Zarr asset match {patterns}", + } def digest_callback(path: str, algoname: str, d: str) -> None: if algoname == "md5": digests[path] = d def downloads_gen(): + nonlocal matched_required for entry in asset.iterfiles(): entry_path = str(entry) if zarr_entry_filter is not None and not zarr_entry_filter(entry_path): continue + if required_match is not None and required_match(entry_path): + matched_required = True entries.append(entry) etag = entry.digest assert etag.algorithm is DigestType.md5 @@ -1116,14 +1136,19 @@ def downloads_gen(): if final_out is not None: break else: - if zarr_entry_filter is not None: - if not entries and empty_filter_error is not None: - yield {"status": "error", "message": empty_filter_error} - else: - # Filter matched no entries; still report completion - yield {"status": "done"} + if required_filters and not matched_required: + yield unmatched_required_error() + elif zarr_entry_filter is not None: + # Filter matched no entries; still report completion + yield {"status": "done"} return + if required_filters and not matched_required: + # Entries downloaded for the ``--zarr`` filters do not make up for the + # ones the URL named but the asset does not have. + yield unmatched_required_error() + return + if zarr_entry_filter is not None: # Partial download: skip deleting extra local files and skip # whole-zarr checksum verification (individual file checksums diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 1cd28445a..f0b37fdda 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -23,9 +23,9 @@ ) from dandi.exceptions import FailedToConnectError, NotFoundError, UnknownURLError from dandi.tests.skip import mark +from dandi.zarr_filter import ZarrFilter from .fixtures import DandiAPI, SampleDandiset -from ..zarr_filter import ZarrFilter @pytest.mark.parametrize( diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index 87a6c7ec5..bc0888792 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Callable, Iterator +from collections.abc import Callable, Iterator, Sequence from contextlib import nullcontext from datetime import datetime, timedelta, timezone from email.utils import parsedate_to_datetime @@ -19,7 +19,7 @@ from typing import Any from unittest import mock -from dandischema.models import ID_PATTERN +from dandischema.models import ID_PATTERN, DigestType import numpy as np import pytest from pytest_mock import MockerFixture @@ -28,6 +28,8 @@ import responses import zarr +import dandi.download + from .fixtures import SampleDandiset, SampleDandisetFactory from .skip import mark from .test_helpers import TWO_ARRAY_ZARR_LAYOUT, assert_dirtrees_eq, zarr_format_of @@ -46,9 +48,10 @@ download, ) from ..exceptions import NotFoundError +from ..misctypes import Digest from ..support.digests import Digester from ..utils import list_paths, yaml_load -from ..zarr_filter import ZarrFilter +from ..zarr_filter import ZarrFilter, make_zarr_entry_filter # both urls point to 000027 (lean test dataset), and both draft and "released" @@ -1656,7 +1659,7 @@ def test_download_zarr_sync_conflict() -> None: def _make_downloader( - url: str, tmp_path: Path, zarr_filters: list[ZarrFilter] | None = None + url: str, tmp_path: Path, zarr_filters: Sequence[ZarrFilter] = () ) -> Downloader: return Downloader( url=parse_dandi_url(url), @@ -1667,7 +1670,7 @@ def _make_downloader( preserve_tree=False, jobs_per_zarr=None, on_error="raise", - zarr_filters=zarr_filters if zarr_filters is not None else [], + zarr_filters=list(zarr_filters), ) @@ -1679,12 +1682,12 @@ def test_downloader_zarr_filter_is_per_url(tmp_path: Path) -> None: assert dl.zarr_entry_filter("0/0/.zarray") assert not dl.zarr_entry_filter("1/1/.zarray") # Entries the URL asked for must exist, so matching nothing is an error - assert dl.empty_zarr_filter_error is not None + assert dl.url_zarr_filters == [ZarrFilter("path", "0/0")] # A second URL downloaded in the same invocation is unaffected other = _make_downloader("dandi://dandi/000108/sub-2/other.ome.zarr", tmp_path) assert other.zarr_entry_filter is None - assert other.empty_zarr_filter_error is None + assert other.url_zarr_filters == [] @pytest.mark.ai_generated @@ -1698,7 +1701,7 @@ def test_downloader_explicit_zarr_filters_apply_to_all_urls(tmp_path: Path) -> N assert dl.zarr_entry_filter is not None assert dl.zarr_entry_filter("a/data.bin") assert not dl.zarr_entry_filter("b/data.bin") - assert dl.empty_zarr_filter_error is None + assert dl.url_zarr_filters == [] @pytest.mark.ai_generated @@ -1743,7 +1746,7 @@ def test_download_zarr_url_subpath( @pytest.mark.ai_generated def test_download_zarr_url_subpath_nonexistent( - tmp_path: Path, new_dandiset: SampleDandiset + tmp_path: Path, new_dandiset: SampleDandiset, caplog: pytest.LogCaptureFixture ) -> None: """A URL naming a nonexistent path inside a Zarr asset errors out.""" _upload_two_zarrs(new_dandiset) @@ -1753,6 +1756,7 @@ def test_download_zarr_url_subpath_nonexistent( f"/{new_dandiset.dandiset_id}/sample.zarr/nonexistent", tmp_path, ) + assert "No entries in the Zarr asset match 'nonexistent'" in caplog.text @pytest.mark.ai_generated @@ -1769,3 +1773,114 @@ def test_download_zarr_url_subpath_does_not_filter_other_urls( # Second URL: the whole Zarr, unaffected by the first URL's subpath assert (tmp_path / "other.zarr" / "c" / "data.bin").read_text() == "data-c" assert (tmp_path / "other.zarr" / "d" / "data.bin").read_text() == "data-d" + + +class _FakeZarrEntry: + """Minimal stand-in for a `RemoteZarrEntry` for `_download_zarr` tests.""" + + def __init__(self, path: str, size: int = 10) -> None: + self.path = path + self.size = size + self.digest = Digest(algorithm=DigestType.md5, value="0" * 32) + self.modified = datetime(2024, 1, 1, tzinfo=timezone.utc) + + def __str__(self) -> str: + return self.path + + def get_download_file_iter(self) -> Callable[[], Iterator[bytes]]: + return lambda: iter([]) + + +class _FakeZarrAsset: + """Minimal stand-in for a `BaseRemoteZarrAsset` for `_download_zarr` tests.""" + + def __init__(self, paths: Sequence[str]) -> None: + self.paths = list(paths) + self.size = 10 * len(self.paths) + + def iterfiles(self, prefix: str | None = None) -> Iterator[_FakeZarrEntry]: + return iter([_FakeZarrEntry(p) for p in self.paths]) + + +def _run_download_zarr( + tmp_path: Path, + paths: Sequence[str], + filters: Sequence[ZarrFilter], + required_filters: Sequence[ZarrFilter] = (), +) -> list[dict]: + """Drive `_download_zarr` over a fake asset, stubbing out the file transfer.""" + with mock.patch.object( + dandi.download, + "_download_file", + lambda *args, **kwargs: iter( + [{"size": 10}, {"status": "downloading"}, {"status": "done"}] + ), + ): + return list( + dandi.download._download_zarr( + _FakeZarrAsset(paths), # type: ignore[arg-type] + tmp_path / "sample.zarr", + toplevel_path=tmp_path, + existing=DownloadExisting.OVERWRITE, + lock=Lock(), + jobs=1, + zarr_entry_filter=make_zarr_entry_filter(list(filters)), + required_filters=list(required_filters), + ) + ) + + +@pytest.mark.ai_generated +def test_download_zarr_required_filter_no_match_errors(tmp_path: Path) -> None: + """A URL subpath matching no entry is reported as an error.""" + required = [ZarrFilter("path", "nonexistent")] + out = _run_download_zarr( + tmp_path, [".zgroup", "a/data.bin"], required, required_filters=required + ) + errors = [r for r in out if r.get("status") == "error"] + assert len(errors) == 1 + assert errors[0]["message"] == "No entries in the Zarr asset match 'nonexistent'" + + +@pytest.mark.ai_generated +def test_download_zarr_required_filter_not_masked_by_explicit_filter( + tmp_path: Path, +) -> None: + """``--zarr`` entries downloaded alongside must not hide a missing subpath.""" + required = [ZarrFilter("path", "nonexistent")] + # ``--zarr metadata``-style filter matches .zgroup, so entries are downloaded + out = _run_download_zarr( + tmp_path, + [".zgroup", "a/data.bin"], + required + [ZarrFilter("glob", "**/.z*")], + required_filters=required, + ) + # The metadata entry really was downloaded ... + assert any("size" in r or "done" in r for r in out) + # ... and yet the missing subpath is still reported + errors = [r for r in out if r.get("status") == "error"] + assert len(errors) == 1 + assert errors[0]["message"] == "No entries in the Zarr asset match 'nonexistent'" + + +@pytest.mark.ai_generated +def test_download_zarr_explicit_filter_no_match_is_not_an_error( + tmp_path: Path, +) -> None: + """Without required filters, matching nothing stays a silent no-op.""" + out = _run_download_zarr( + tmp_path, [".zgroup", "a/data.bin"], [ZarrFilter("path", "nonexistent")] + ) + assert not [r for r in out if r.get("status") == "error"] + assert out[-1] == {"status": "done"} + + +@pytest.mark.ai_generated +def test_download_zarr_required_filter_match_is_not_an_error(tmp_path: Path) -> None: + """A URL subpath that does match downloads without error.""" + required = [ZarrFilter("path", "a")] + out = _run_download_zarr( + tmp_path, [".zgroup", "a/data.bin"], required, required_filters=required + ) + assert not [r for r in out if r.get("status") == "error"] + assert out[-1] == {"status": "done"} From 565c4f7f9361b69e162f12494fda1d2d3df69013 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 01:49:15 +0000 Subject: [PATCH 3/6] Handle trailing slashes at zarr boundaries; document --zarr A trailing slash routed the location to `AssetFolderURL` before the zarr boundary was considered, so `dandi://.../x.zarr/a/` failed with No assets found under folder 'x.zarr/a/' rather than downloading the entries under `a`. A path within a zarr can never name a folder of assets -- entries inside a zarr are not assets -- so the boundary is now checked first. `.../x.zarr/` (a trailing slash at the boundary, with no subpath) now names the zarr asset itself. `AssetFolderURL` could never resolve it: the asset's own path has no trailing slash, so nothing had that prefix unless a Dandiset held blobs nested under a `.zarr/` path, which uploading a zarr directory does not produce. A trailing slash is thus ignored at and below a zarr boundary, and folders that are not zarrs are untouched. Docs: `--zarr` was not documented at all. `download.rst` now covers the filter syntax, the `metadata` alias, OR composition, the URL-implied filter and why a URL subpath that matches nothing is an error, plus the `--sync` restriction; `urls.rst` gains a section on paths within zarr assets, including the trailing-slash rule, and the two `location` bullets now mention `AssetZarrEntryURL`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- dandi/dandiarchive.py | 72 +++++++++++++++++++++++-------- dandi/tests/test_dandiarchive.py | 73 ++++++++++++++++++++++++++++++++ dandi/tests/test_download.py | 27 ++++++++++-- docs/source/cmdline/download.rst | 43 +++++++++++++++++++ docs/source/ref/urls.rst | 40 +++++++++++++++++ 5 files changed, 234 insertions(+), 21 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index 9fa34a2dc..e5033394e 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -496,6 +496,32 @@ def split_zarr_location(location: str) -> tuple[str, str] | None: return None +def at_zarr_boundary(location: str) -> bool: + """Whether ``location``'s last component ends with a Zarr extension. + + Parameters + ---------- + location : str + A POSIX-style path, e.g. ``"sub-1/file.ome.zarr/"``. + + Returns + ------- + bool + True if the path ends at a Zarr asset (ignoring a trailing slash). + + Examples + -------- + >>> at_zarr_boundary("sub-1/file.ome.zarr/") + True + >>> at_zarr_boundary("sub-1/file.ome.zarr/0/0") # below the boundary + False + >>> at_zarr_boundary("sub-1/") + False + """ + parts = [p for p in location.split("/") if p] + return bool(parts) and any(parts[-1].endswith(ext) for ext in ZARR_EXTENSIONS) + + @dataclass class AssetZarrEntryURL(SingleAssetURL): """Parsed from a URL that points into entries within a Zarr asset. @@ -928,6 +954,28 @@ def parse( version_id=version_id, path=location, ) + elif (zarr_split := split_zarr_location(location)) is not None: + # The location crosses a zarr boundary. This is checked + # before the folder case, as a path within a zarr never names + # a folder of assets: entries within a zarr are not assets. + asset_path, zarr_subpath = zarr_split + parsed_url = AssetZarrEntryURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + asset_path=asset_path, + zarr_subpath=zarr_subpath, + ) + elif location.endswith("/") and at_zarr_boundary(location): + # `.../x.zarr/` names the zarr asset itself; a folder of + # assets by that name could never hold it, as the asset's own + # path does not end in a slash. + parsed_url = AssetItemURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + path=location.rstrip("/"), + ) elif location.endswith("/"): parsed_url = AssetFolderURL( instance=instance, @@ -936,24 +984,12 @@ def parse( path=location, ) else: - # Check if location crosses a zarr boundary - zarr_split = split_zarr_location(location) - if zarr_split is not None: - asset_path, zarr_subpath = zarr_split - parsed_url = AssetZarrEntryURL( - instance=instance, - dandiset_id=dandiset_id, - version_id=version_id, - asset_path=asset_path, - zarr_subpath=zarr_subpath, - ) - else: - parsed_url = AssetItemURL( - instance=instance, - dandiset_id=dandiset_id, - version_id=version_id, - path=location, - ) + parsed_url = AssetItemURL( + instance=instance, + dandiset_id=dandiset_id, + version_id=version_id, + path=location, + ) elif asset_id: if dandiset_id is None: parsed_url = BaseAssetIDURL(instance=instance, asset_id=asset_id) diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index f0b37fdda..348156d82 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -16,6 +16,7 @@ BaseAssetIDURL, DandisetURL, ParsedDandiURL, + at_zarr_boundary, follow_redirect, multiasset_target, parse_dandi_url, @@ -433,6 +434,78 @@ def test_non_zarr_entry_urls_have_no_zarr_filter(url: str) -> None: assert parse_dandi_url(url).get_zarr_filter() == [] +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "location,expected", + [ + ("sub-1/file.ome.zarr/", True), + ("sub-1/file.ome.zarr", True), + ("sub-1/file.ngff/", True), + ("file.zarr", True), + # Below the boundary + ("sub-1/file.ome.zarr/0/0", False), + # Not a zarr at all + ("sub-1/", False), + ("sub-1/file.nwb", False), + ("", False), + ], +) +def test_at_zarr_boundary(location: str, expected: bool) -> None: + assert at_zarr_boundary(location) == expected + + +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "url,parsed_url", + [ + # A trailing slash below a zarr boundary names entries, not a folder + ( + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", + AssetZarrEntryURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + asset_path="sub-1/file.ome.zarr", + zarr_subpath="0/0", + ), + ), + # A trailing slash at a zarr boundary names the zarr asset itself + ( + "dandi://dandi/000108/sub-1/file.ome.zarr/", + AssetItemURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/file.ome.zarr", + ), + ), + # Folders that are not zarrs are unaffected + ( + "dandi://dandi/000108/sub-1/", + AssetFolderURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/", + ), + ), + ], +) +def test_parse_zarr_url_with_trailing_slash( + url: str, parsed_url: ParsedDandiURL +) -> None: + """A trailing slash is not meaningful at or below a zarr boundary.""" + assert parse_dandi_url(url) == parsed_url + + +@pytest.mark.ai_generated +def test_parse_zarr_glob_url_unaffected_by_trailing_slash() -> None: + """``--path-type glob`` still yields a glob URL for a zarr-looking path.""" + url = parse_dandi_url("dandi://dandi/000108/sub-1/*.zarr/a/", glob=True) + assert isinstance(url, AssetGlobURL) + assert url.path == "sub-1/*.zarr/a/" + + @pytest.mark.parametrize( "url,parsed_url", [ diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index bc0888792..bbda3309a 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1729,14 +1729,19 @@ def _upload_two_zarrs(ds: SampleDandiset) -> None: @pytest.mark.ai_generated +@pytest.mark.parametrize("suffix", ["a", "a/"]) def test_download_zarr_url_subpath( - tmp_path: Path, new_dandiset: SampleDandiset + tmp_path: Path, new_dandiset: SampleDandiset, suffix: str ) -> None: - """A URL pointing inside a Zarr asset downloads only that subtree.""" + """A URL pointing inside a Zarr asset downloads only that subtree. + + A trailing slash is not meaningful below a zarr boundary, so both + spellings behave the same. + """ _upload_two_zarrs(new_dandiset) download( f"dandi://{new_dandiset.api.instance_id}" - f"/{new_dandiset.dandiset_id}/sample.zarr/a", + f"/{new_dandiset.dandiset_id}/sample.zarr/{suffix}", tmp_path, ) zarr_dir = tmp_path / "sample.zarr" @@ -1744,6 +1749,22 @@ def test_download_zarr_url_subpath( assert not (zarr_dir / "b").exists() +@pytest.mark.ai_generated +def test_download_zarr_url_trailing_slash_at_boundary( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """``.../x.zarr/`` downloads the whole zarr asset, as ``.../x.zarr`` does.""" + _upload_two_zarrs(new_dandiset) + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/", + tmp_path, + ) + zarr_dir = tmp_path / "sample.zarr" + assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" + assert (zarr_dir / "b" / "data.bin").read_text() == "data-b" + + @pytest.mark.ai_generated def test_download_zarr_url_subpath_nonexistent( tmp_path: Path, new_dandiset: SampleDandiset, caplog: pytest.LogCaptureFixture diff --git a/docs/source/cmdline/download.rst b/docs/source/cmdline/download.rst index 32407ac76..424dc3887 100644 --- a/docs/source/cmdline/download.rst +++ b/docs/source/cmdline/download.rst @@ -65,3 +65,46 @@ Options .. option:: --sync Delete local assets that do not exist on the server after downloading + + Cannot be combined with ``--zarr`` or with a URL that points inside a Zarr + asset: a partial download of a Zarr leaves out entries that are on the + server, which ``--sync`` would then delete locally. + +.. option:: --zarr + + Download only the entries within Zarr assets that match ``filter``, given + as :samp:`{type}:{pattern}` where ``type`` is one of: + + ``glob`` + Match the entry path against a glob pattern. ``*`` matches within a + single path component and ``**`` matches across components, e.g. + ``glob:**/.zarray``. + + ``path`` + Match the entry at ``pattern`` and everything under it, e.g. + ``path:0/0``. + + ``regex`` + Match the entry path against a Python regular expression, e.g. + ``regex:^0/[0-9]+/``. + + In place of :samp:`{type}:{pattern}`, the predefined filter ``metadata`` + may be given; it selects the Zarr metadata files (``.zarray``, ``.zgroup``, + ``.zattrs``, ``.zmetadata``, and ``zarr.json``). + + The option may be given more than once, in which case an entry is + downloaded if it matches **any** of the filters. + + A URL that points inside a Zarr asset (see :ref:`resource_ids`) restricts + the download in the same way, as though ``path:`` had been given for the + portion of the URL below the Zarr asset:: + + dandi download dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + + Unlike ``--zarr``, such a URL names entries that the Dandiset is expected + to have: if no entry matches, the download fails rather than quietly + downloading nothing. + + Because only part of a Zarr is fetched, extra local files are not deleted + and the Zarr checksum of the result is not verified; the checksum of each + individual downloaded entry still is. diff --git a/docs/source/ref/urls.rst b/docs/source/ref/urls.rst index d314d6d6a..88fb6bded 100644 --- a/docs/source/ref/urls.rst +++ b/docs/source/ref/urls.rst @@ -37,6 +37,11 @@ has one, and its draft version will be used otherwise. a collection of assets whose paths match the glob pattern ``path``, and `parse_dandi_url()` will convert the URL to an `AssetGlobURL`. + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + descends into a Zarr asset, the URL refers to the entries at or under that + location within the Zarr, and `parse_dandi_url()` will convert the URL to + an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. + - If the ``glob``/``--path-type glob`` option is not in effect, the URL refers to an asset folder by path, and `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. @@ -75,6 +80,11 @@ has one, and its draft version will be used otherwise. a collection of assets whose paths match the glob pattern ``path``, and `parse_dandi_url()` will convert the URL to an `AssetGlobURL`. + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + descends into a Zarr asset, the URL refers to the entries at or under that + location within the Zarr, and `parse_dandi_url()` will convert the URL to + an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` ends with a trailing slash, the URL refers to an asset folder by path, and `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. @@ -84,3 +94,33 @@ has one, and its draft version will be used otherwise. path, and `parse_dandi_url()` will convert the URL to an `AssetItemURL`. - Any other HTTPS URL that redirects to one of the above + + +.. _zarr_entry_urls: + +Paths Within Zarr Assets +------------------------ + +A Zarr asset is a directory, and the paths inside it are entries of that asset +rather than assets of their own. A URL whose path continues past a Zarr asset +therefore refers to entries within it. The boundary is recognised by the +extensions in ``dandi.consts.ZARR_EXTENSIONS`` (:file:`.zarr` and +:file:`.ngff`), so in:: + + dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + +the asset is :file:`sub-1/file.ome.zarr` and ``0/0`` names the entries at or +under :file:`0/0` within it. `parse_dandi_url()` converts this to an +`AssetZarrEntryURL`. + +:program:`dandi download` recreates the Zarr's leading directories locally and +fetches only the matching entries; see the ``--zarr`` option of +:doc:`dandi download `. :program:`dandi ls` +lists the matching entries. Because such a URL names entries the Dandiset is +expected to have, a download whose path matches no entry fails rather than +quietly downloading nothing. + +A trailing slash is not meaningful at or below a Zarr boundary, since entries +within a Zarr are not assets: :samp:`{...}/file.ome.zarr/0/0/` is equivalent to +:samp:`{...}/file.ome.zarr/0/0`, and :samp:`{...}/file.ome.zarr/` refers to the +Zarr asset as a whole, just as :samp:`{...}/file.ome.zarr` does. From 10610617302e5bbd06cce4766d0ef4fb080b11ff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 22:30:15 +0000 Subject: [PATCH 4/6] Simplify zarr path helpers; group zarr tests into parametrized tables `at_zarr_boundary()` split the path only to look at its last component, which `str.endswith` already does given a tuple of suffixes. Apply the same idiom to the `any(... endswith ...)` loop in `split_zarr_location()`. Verified equivalent over the boundary cases, including repeated and bare slashes and a leading-dot `.zarr` component. Group the zarr tests into tables rather than a function per case: - `split_zarr_location()` and `at_zarr_boundary()` share one table of locations, so a location's classification is stated in one place - the `get_zarr_filter()` cases become one table of URL -> filters - the trailing-slash parsing cases move into the existing `test_parse_api_url`/`test_parse_api_url_glob` tables - the two `Downloader` filter tests become one table - the four `_download_zarr()` required-filter tests become one table - the two `--sync` conflict tests become one table - the URL-subpath download test covers all four spellings of the path, with and without a trailing slash, at and below the boundary - `--zarr`'s glob-filter and `metadata`-alias tests differed only in the filter, so they become one table too Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- dandi/dandiarchive.py | 5 +- dandi/tests/test_dandiarchive.py | 165 ++++++++----------- dandi/tests/test_download.py | 274 +++++++++++++++---------------- 3 files changed, 194 insertions(+), 250 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index e5033394e..b92a27098 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -489,7 +489,7 @@ def split_zarr_location(location: str) -> tuple[str, str] | None: """ parts = [p for p in location.split("/") if p] for i, part in enumerate(parts): - if any(part.endswith(ext) for ext in ZARR_EXTENSIONS): + if part.endswith(tuple(ZARR_EXTENSIONS)): asset_path = "/".join(parts[: i + 1]) zarr_subpath = "/".join(parts[i + 1 :]) return (asset_path, zarr_subpath) if zarr_subpath else None @@ -518,8 +518,7 @@ def at_zarr_boundary(location: str) -> bool: >>> at_zarr_boundary("sub-1/") False """ - parts = [p for p in location.split("/") if p] - return bool(parts) and any(parts[-1].endswith(ext) for ext in ZARR_EXTENSIONS) + return location.rstrip("/").endswith(tuple(ZARR_EXTENSIONS)) @dataclass diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 348156d82..9f06baf30 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -292,6 +292,16 @@ zarr_subpath="scale0/0/0", ), ), + ( # a trailing slash below the boundary still names entries + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", + AssetZarrEntryURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + asset_path="sub-1/file.ome.zarr", + zarr_subpath="0/0", + ), + ), ( # plain .zarr URL without subpath should stay AssetItemURL "dandi://dandi/000108/sub-1/file.ome.zarr", AssetItemURL( @@ -301,6 +311,15 @@ path="sub-1/file.ome.zarr", ), ), + ( # ... as should one with a trailing slash at the boundary + "dandi://dandi/000108/sub-1/file.ome.zarr/", + AssetItemURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/file.ome.zarr", + ), + ), ( "https://api.dandiarchive.org/api/dandisets/000003/versions/draft" "/assets/0a748f90-d497-4a9c-822e-9c63811db412/download/", @@ -391,119 +410,53 @@ def test_parse_api_url(url: str, parsed_url: ParsedDandiURL) -> None: @pytest.mark.ai_generated @pytest.mark.parametrize( - "location,expected", - [ - # Crosses zarr boundary - ("sub-1/file.ome.zarr/0/0/0", ("sub-1/file.ome.zarr", "0/0/0")), - ("file.zarr/scale0/data", ("file.zarr", "scale0/data")), - ("sub-1/file.ngff/0/0", ("sub-1/file.ngff", "0/0")), - # No zarr extension - ("sub-1/file.nwb", None), - ("some/path/file.txt", None), - # Zarr without subpath — no split - ("sub-1/file.ome.zarr", None), - ("file.zarr", None), - # Deeply nested subpath - ("a/b.zarr/c/d/e/f", ("a/b.zarr", "c/d/e/f")), - ], -) -def test_split_zarr_location(location: str, expected: tuple[str, str] | None) -> None: - assert split_zarr_location(location) == expected - - -@pytest.mark.ai_generated -def test_asset_zarr_entry_url_get_zarr_filter() -> None: - """A URL pointing inside a Zarr asset restricts the download to its subpath.""" - url = parse_dandi_url("dandi://dandi/000108/sub-1/file.ome.zarr/0/0/0") - assert isinstance(url, AssetZarrEntryURL) - assert url.get_zarr_filter() == [ZarrFilter("path", "0/0/0")] - - -@pytest.mark.ai_generated -@pytest.mark.parametrize( - "url", + "location,split,boundary", [ - "dandi://dandi/000108", - "dandi://dandi/000108/sub-1/file.ome.zarr", - "dandi://dandi/000108/sub-1/file.nwb", - "dandi://dandi/000108/sub-1/", - ], -) -def test_non_zarr_entry_urls_have_no_zarr_filter(url: str) -> None: - """Any other URL leaves Zarr assets unfiltered.""" - assert parse_dandi_url(url).get_zarr_filter() == [] - - -@pytest.mark.ai_generated -@pytest.mark.parametrize( - "location,expected", - [ - ("sub-1/file.ome.zarr/", True), - ("sub-1/file.ome.zarr", True), - ("sub-1/file.ngff/", True), - ("file.zarr", True), - # Below the boundary - ("sub-1/file.ome.zarr/0/0", False), + # Crosses a zarr boundary + ("sub-1/file.ome.zarr/0/0/0", ("sub-1/file.ome.zarr", "0/0/0"), False), + ("file.zarr/scale0/data", ("file.zarr", "scale0/data"), False), + ("sub-1/file.ngff/0/0", ("sub-1/file.ngff", "0/0"), False), + ("a/b.zarr/c/d/e/f", ("a/b.zarr", "c/d/e/f"), False), + # At a zarr boundary: no subpath to split off + ("sub-1/file.ome.zarr", None, True), + ("sub-1/file.ome.zarr/", None, True), + ("sub-1/file.ngff/", None, True), + ("file.zarr", None, True), # Not a zarr at all - ("sub-1/", False), - ("sub-1/file.nwb", False), - ("", False), + ("sub-1/file.nwb", None, False), + ("some/path/file.txt", None, False), + ("sub-1/", None, False), + ("", None, False), ], ) -def test_at_zarr_boundary(location: str, expected: bool) -> None: - assert at_zarr_boundary(location) == expected +def test_zarr_location_helpers( + location: str, split: tuple[str, str] | None, boundary: bool +) -> None: + """`split_zarr_location()` finds a subpath; `at_zarr_boundary()` finds the asset.""" + assert split_zarr_location(location) == split + assert at_zarr_boundary(location) is boundary @pytest.mark.ai_generated @pytest.mark.parametrize( - "url,parsed_url", + "url,expected", [ - # A trailing slash below a zarr boundary names entries, not a folder + # Only a URL pointing *inside* a zarr restricts the download ( - "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", - AssetZarrEntryURL( - instance=known_instances["dandi"], - dandiset_id="000108", - version_id=None, - asset_path="sub-1/file.ome.zarr", - zarr_subpath="0/0", - ), - ), - # A trailing slash at a zarr boundary names the zarr asset itself - ( - "dandi://dandi/000108/sub-1/file.ome.zarr/", - AssetItemURL( - instance=known_instances["dandi"], - dandiset_id="000108", - version_id=None, - path="sub-1/file.ome.zarr", - ), - ), - # Folders that are not zarrs are unaffected - ( - "dandi://dandi/000108/sub-1/", - AssetFolderURL( - instance=known_instances["dandi"], - dandiset_id="000108", - version_id=None, - path="sub-1/", - ), - ), + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/0", + [ZarrFilter("path", "0/0/0")], + ), + ("dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", [ZarrFilter("path", "0/0")]), + # Everything else leaves zarr assets unfiltered + ("dandi://dandi/000108", []), + ("dandi://dandi/000108/sub-1/file.ome.zarr", []), + ("dandi://dandi/000108/sub-1/file.ome.zarr/", []), + ("dandi://dandi/000108/sub-1/file.nwb", []), + ("dandi://dandi/000108/sub-1/", []), ], ) -def test_parse_zarr_url_with_trailing_slash( - url: str, parsed_url: ParsedDandiURL -) -> None: - """A trailing slash is not meaningful at or below a zarr boundary.""" - assert parse_dandi_url(url) == parsed_url - - -@pytest.mark.ai_generated -def test_parse_zarr_glob_url_unaffected_by_trailing_slash() -> None: - """``--path-type glob`` still yields a glob URL for a zarr-looking path.""" - url = parse_dandi_url("dandi://dandi/000108/sub-1/*.zarr/a/", glob=True) - assert isinstance(url, AssetGlobURL) - assert url.path == "sub-1/*.zarr/a/" +def test_get_zarr_filter(url: str, expected: list[ZarrFilter]) -> None: + assert parse_dandi_url(url).get_zarr_filter() == expected @pytest.mark.parametrize( @@ -528,6 +481,16 @@ def test_parse_zarr_glob_url_unaffected_by_trailing_slash() -> None: path="f*/bar.nwb", ), ), + ( + # a zarr-looking path is still a glob, trailing slash and all: + "dandi://dandi/000108/sub-1/*.zarr/a/", + AssetGlobURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id=None, + path="sub-1/*.zarr/a/", + ), + ), ( # `path=` does not produce a glob: "https://api.dandiarchive.org/api/dandisets/000003/versions/draft" diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index bbda3309a..f7f9e06fb 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1537,49 +1537,30 @@ def test__check_attempts_and_sleep_retries(status_code: int) -> None: @pytest.mark.ai_generated -def test_download_zarr_with_glob_filter( - tmp_path: Path, zarr_dandiset: SampleDandiset +@pytest.mark.parametrize( + "zarr_filters", + [("glob:**/.z*", "glob:**/zarr.json"), ("metadata",)], + ids=["glob", "alias"], +) +def test_download_zarr_metadata_only( + tmp_path: Path, zarr_dandiset: SampleDandiset, zarr_filters: tuple[str, ...] ) -> None: - """Download only metadata files from a zarr asset via a glob filter. + """Download only the metadata files of a zarr asset. Zarr v2 stores metadata in dot-files (``.zarray``/``.zgroup``/``.zattrs``), while Zarr v3 uses ``zarr.json``. Match both so the test is agnostic to the on-disk format produced by the installed zarr-python. """ download( - zarr_dandiset.dandiset.version_api_url, - tmp_path, - zarr_filters=("glob:**/.z*", "glob:**/zarr.json"), - ) - zarr_dir = tmp_path / zarr_dandiset.dandiset_id / "sample.zarr" - assert zarr_dir.exists() - # All downloaded files should be metadata files. - all_files = list_paths(zarr_dir) - assert len(all_files) > 0 - for f in all_files: - assert f.name.startswith(".z") or f.name == "zarr.json", ( - f"Non-metadata file downloaded: {f}" - ) - - -@pytest.mark.ai_generated -def test_download_zarr_metadata_alias( - tmp_path: Path, zarr_dandiset: SampleDandiset -) -> None: - """Test the 'metadata' alias for --zarr.""" - download( - zarr_dandiset.dandiset.version_api_url, - tmp_path, - zarr_filters=("metadata",), + zarr_dandiset.dandiset.version_api_url, tmp_path, zarr_filters=zarr_filters ) zarr_dir = tmp_path / zarr_dandiset.dandiset_id / "sample.zarr" assert zarr_dir.exists() all_files = list_paths(zarr_dir) assert len(all_files) > 0 for f in all_files: - assert f.name.startswith(".z") or f.name in ( - "zarr.json", - ".zmetadata", + assert ( + f.name.startswith(".z") or f.name == "zarr.json" ), f"Non-metadata file downloaded: {f}" @@ -1647,15 +1628,27 @@ def test_download_zarr_filter_nonexistent( @pytest.mark.ai_generated -def test_download_zarr_sync_conflict() -> None: - """--sync and --zarr cannot be used together.""" - with pytest.raises(ValueError, match="--sync and --zarr cannot be used together"): - download( +@pytest.mark.parametrize( + "url,zarr_filters,error", + [ + ( "https://dandiarchive.org/dandiset/000027", - "/tmp/unused", - sync=True, - zarr_filters=("metadata",), - ) + ("metadata",), + "--sync and --zarr cannot be used together", + ), + ( + "dandi://dandi/000027/sample.zarr/0/0", + (), + "--sync cannot be used with a URL pointing inside a Zarr asset", + ), + ], +) +def test_download_zarr_sync_conflict( + url: str, zarr_filters: tuple[str, ...], error: str +) -> None: + """A partial zarr download and ``--sync`` are mutually exclusive.""" + with pytest.raises(ValueError, match=re.escape(error)): + download(url, "/tmp/unused", sync=True, zarr_filters=zarr_filters) def _make_downloader( @@ -1675,46 +1668,52 @@ def _make_downloader( @pytest.mark.ai_generated -def test_downloader_zarr_filter_is_per_url(tmp_path: Path) -> None: - """A URL's Zarr subpath must not restrict the download of any other URL.""" - dl = _make_downloader("dandi://dandi/000108/sub-1/file.ome.zarr/0/0", tmp_path) - assert dl.zarr_entry_filter is not None - assert dl.zarr_entry_filter("0/0/.zarray") - assert not dl.zarr_entry_filter("1/1/.zarray") - # Entries the URL asked for must exist, so matching nothing is an error - assert dl.url_zarr_filters == [ZarrFilter("path", "0/0")] - - # A second URL downloaded in the same invocation is unaffected - other = _make_downloader("dandi://dandi/000108/sub-2/other.ome.zarr", tmp_path) - assert other.zarr_entry_filter is None - assert other.url_zarr_filters == [] - - -@pytest.mark.ai_generated -def test_downloader_explicit_zarr_filters_apply_to_all_urls(tmp_path: Path) -> None: - """``--zarr`` filters apply to every asset, and matching nothing is not an error.""" - dl = _make_downloader( - "dandi://dandi/000108/sub-2/other.ome.zarr", - tmp_path, - zarr_filters=[ZarrFilter("path", "a")], - ) - assert dl.zarr_entry_filter is not None - assert dl.zarr_entry_filter("a/data.bin") - assert not dl.zarr_entry_filter("b/data.bin") - assert dl.url_zarr_filters == [] - +@pytest.mark.parametrize( + "url,zarr_filters,included,excluded,url_filters", + [ + # A URL inside a zarr restricts that download to its subpath ... + ( + "dandi://dandi/000108/sub-1/file.ome.zarr/0/0", + (), + ["0/0/.zarray"], + ["1/1/.zarray"], + [ZarrFilter("path", "0/0")], + ), + # ... and no other URL in the same invocation is affected + ("dandi://dandi/000108/sub-2/other.ome.zarr", (), None, None, []), + # ``--zarr`` filters apply to every asset, whatever the URL + ( + "dandi://dandi/000108/sub-2/other.ome.zarr", + (ZarrFilter("path", "a"),), + ["a/data.bin"], + ["b/data.bin"], + [], + ), + ], +) +def test_downloader_zarr_filters( + tmp_path: Path, + url: str, + zarr_filters: Sequence[ZarrFilter], + included: list[str] | None, + excluded: list[str] | None, + url_filters: list[ZarrFilter], +) -> None: + """Only the URL's own subpath filters its download; ``--zarr`` filters all. -@pytest.mark.ai_generated -def test_download_zarr_url_sync_conflict() -> None: - """--sync cannot be combined with a URL pointing inside a Zarr asset.""" - with pytest.raises( - ValueError, match="--sync cannot be used with a URL pointing inside a Zarr" - ): - download( - "dandi://dandi/000027/sample.zarr/0/0", - "/tmp/unused", - sync=True, - ) + ``included``/``excluded`` of `None` means no filtering at all, i.e. the + whole zarr is downloaded. A non-empty ``url_filters`` marks the entries as + named by the user, so matching none of them is an error. + """ + dl = _make_downloader(url, tmp_path, zarr_filters) + assert dl.url_zarr_filters == url_filters + if included is None: + assert dl.zarr_entry_filter is None + else: + assert dl.zarr_entry_filter is not None + assert all(dl.zarr_entry_filter(p) for p in included) + assert excluded is not None + assert not any(dl.zarr_entry_filter(p) for p in excluded) def _upload_two_zarrs(ds: SampleDandiset) -> None: @@ -1729,40 +1728,31 @@ def _upload_two_zarrs(ds: SampleDandiset) -> None: @pytest.mark.ai_generated -@pytest.mark.parametrize("suffix", ["a", "a/"]) +@pytest.mark.parametrize( + "suffix,whole_zarr", + [ + # A subpath fetches only that subtree ... + ("/a", False), + # ... and a trailing slash below the boundary means the same + ("/a/", False), + # At the boundary, with or without a slash, the whole zarr is fetched + ("", True), + ("/", True), + ], +) def test_download_zarr_url_subpath( - tmp_path: Path, new_dandiset: SampleDandiset, suffix: str -) -> None: - """A URL pointing inside a Zarr asset downloads only that subtree. - - A trailing slash is not meaningful below a zarr boundary, so both - spellings behave the same. - """ - _upload_two_zarrs(new_dandiset) - download( - f"dandi://{new_dandiset.api.instance_id}" - f"/{new_dandiset.dandiset_id}/sample.zarr/{suffix}", - tmp_path, - ) - zarr_dir = tmp_path / "sample.zarr" - assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" - assert not (zarr_dir / "b").exists() - - -@pytest.mark.ai_generated -def test_download_zarr_url_trailing_slash_at_boundary( - tmp_path: Path, new_dandiset: SampleDandiset + tmp_path: Path, new_dandiset: SampleDandiset, suffix: str, whole_zarr: bool ) -> None: - """``.../x.zarr/`` downloads the whole zarr asset, as ``.../x.zarr`` does.""" + """A URL inside a zarr fetches that subtree; a trailing slash is ignored.""" _upload_two_zarrs(new_dandiset) download( f"dandi://{new_dandiset.api.instance_id}" - f"/{new_dandiset.dandiset_id}/sample.zarr/", + f"/{new_dandiset.dandiset_id}/sample.zarr{suffix}", tmp_path, ) zarr_dir = tmp_path / "sample.zarr" assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" - assert (zarr_dir / "b" / "data.bin").read_text() == "data-b" + assert (zarr_dir / "b").exists() is whole_zarr @pytest.mark.ai_generated @@ -1851,57 +1841,49 @@ def _run_download_zarr( ) -@pytest.mark.ai_generated -def test_download_zarr_required_filter_no_match_errors(tmp_path: Path) -> None: - """A URL subpath matching no entry is reported as an error.""" - required = [ZarrFilter("path", "nonexistent")] - out = _run_download_zarr( - tmp_path, [".zgroup", "a/data.bin"], required, required_filters=required - ) - errors = [r for r in out if r.get("status") == "error"] - assert len(errors) == 1 - assert errors[0]["message"] == "No entries in the Zarr asset match 'nonexistent'" +NO_MATCH = "No entries in the Zarr asset match 'nonexistent'" @pytest.mark.ai_generated -def test_download_zarr_required_filter_not_masked_by_explicit_filter( +@pytest.mark.parametrize( + "filters,required,error,downloads", + [ + # A URL subpath matching nothing is an error ... + ([ZarrFilter("path", "nonexistent")], True, NO_MATCH, False), + # ... even when ``--zarr`` filters match and entries are downloaded + ( + [ZarrFilter("path", "nonexistent"), ZarrFilter("glob", "**/.z*")], + True, + NO_MATCH, + True, + ), + # Without a URL subpath, matching nothing is a silent no-op + ([ZarrFilter("path", "nonexistent")], False, None, False), + # A URL subpath that does match downloads without error + ([ZarrFilter("path", "a")], True, None, True), + ], +) +def test_download_zarr_required_filters( tmp_path: Path, + filters: list[ZarrFilter], + required: bool, + error: str | None, + downloads: bool, ) -> None: - """``--zarr`` entries downloaded alongside must not hide a missing subpath.""" - required = [ZarrFilter("path", "nonexistent")] - # ``--zarr metadata``-style filter matches .zgroup, so entries are downloaded + """Entries named by a URL must exist; ``--zarr`` filters need not match. + + ``required`` marks the first filter as URL-derived. ``downloads`` says + whether any entry was expected to be fetched, which is what distinguishes + "matched nothing at all" from "matched only the ``--zarr`` filters". + """ out = _run_download_zarr( tmp_path, [".zgroup", "a/data.bin"], - required + [ZarrFilter("glob", "**/.z*")], - required_filters=required, + filters, + required_filters=filters[:1] if required else (), ) - # The metadata entry really was downloaded ... - assert any("size" in r or "done" in r for r in out) - # ... and yet the missing subpath is still reported errors = [r for r in out if r.get("status") == "error"] - assert len(errors) == 1 - assert errors[0]["message"] == "No entries in the Zarr asset match 'nonexistent'" - - -@pytest.mark.ai_generated -def test_download_zarr_explicit_filter_no_match_is_not_an_error( - tmp_path: Path, -) -> None: - """Without required filters, matching nothing stays a silent no-op.""" - out = _run_download_zarr( - tmp_path, [".zgroup", "a/data.bin"], [ZarrFilter("path", "nonexistent")] - ) - assert not [r for r in out if r.get("status") == "error"] - assert out[-1] == {"status": "done"} - - -@pytest.mark.ai_generated -def test_download_zarr_required_filter_match_is_not_an_error(tmp_path: Path) -> None: - """A URL subpath that does match downloads without error.""" - required = [ZarrFilter("path", "a")] - out = _run_download_zarr( - tmp_path, [".zgroup", "a/data.bin"], required, required_filters=required - ) - assert not [r for r in out if r.get("status") == "error"] - assert out[-1] == {"status": "done"} + assert [r["message"] for r in errors] == ([error] if error else []) + assert any("size" in r or "done" in r for r in out) is downloads + if error is None: + assert out[-1] == {"status": "done"} From bf3a7a4dc838302ea085d88ae30deee1c17b97a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 21:02:47 +0000 Subject: [PATCH 5/6] CI: run dev-deps testing under 3.12 instead of 3.11 Ported verbatim from #1928 so this PR can go green before that one merges; it is a no-op once master carries the same change. The dev-deps job installs hdmf-zarr from git, which now requires Python >= 3.12, so the install step fails before any test runs. The same job is the only failure on master at this PR's base commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- .github/workflows/run-tests.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run-tests.yml b/.github/workflows/run-tests.yml index 452a1bda4..8f03fae7e 100644 --- a/.github/workflows/run-tests.yml +++ b/.github/workflows/run-tests.yml @@ -60,7 +60,9 @@ jobs: python: 3.14 mode: obolibrary-only - os: ubuntu-latest - python: '3.11' + # cannot use dev-deps on 3.11 now, see + # https://github.com/hdmf-dev/hdmf-zarr/issues/406 + python: '3.12' mode: dev-deps - os: ubuntu-latest python: 3.14 From 3ca07c80560669fb6a229e6de69392f54bf0d4ea Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 21:25:06 +0000 Subject: [PATCH 6/6] Refuse `dandi delete` of entries within a Zarr; fix docs inaccuracies `Deleter.register_url()` passes any non-Dandiset URL to `register_assets_url()`, which deletes whatever `get_assets()` yields. For a URL pointing inside a Zarr that is the *whole Zarr asset*, so `dandi delete dandi://.../x.zarr/0/0` would take every entry, not the named ones. That much came with #1816, but routing the trailing-slash spellings to `AssetZarrEntryURL` widened it: `.../x.zarr/a/` used to fail safely with `NotFoundError`. Refuse such URLs outright, reusing the `get_zarr_filter()` hook, and verify no request is issued. Docs corrections: - `urls.rst` claimed a location with a trailing slash always yields an `AssetFolderURL`. That is now false when the location ends at a Zarr asset, which is exactly what a "browse to this Zarr" GUI URL produces, since the parser appends the slash itself. Both bullet lists now say so, and the `?path=` form's prefix semantics are noted as not Zarr-aware. - `download.rst` said `--zarr` downloads "only" matching entries and that a URL subpath "restricts" the download. Both are false together: the two are unioned, so passing both widens the download. Documented with the filter-side spelling that does narrow it. - The `metadata` alias selects any entry beginning with `.z`, not just the four names listed; `regex:` is `re.search`, so unanchored. Tests: cover the GUI `?location=` route to a Zarr, the "asset is not a Zarr" error path, and restore the `b/data.bin` content assertion that the parametrized merge of the URL-subpath tests had weakened to an `exists()` check. Also: hoist `tuple(ZARR_EXTENSIONS)` to a module constant instead of rebuilding it per path component, drop the dead `| None` from `_download_zarr(required_filters=...)`, and give `AssetZarrEntryURL`'s fields the `#:` documentation DEVELOPMENT.md asks for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011qh3EwgNtANXokhr1uimHG --- dandi/dandiarchive.py | 18 +++++++++++++----- dandi/delete.py | 7 +++++++ dandi/download.py | 5 ++--- dandi/tests/test_dandiarchive.py | 22 ++++++++++++++++++++++ dandi/tests/test_delete.py | 24 ++++++++++++++++++++++++ dandi/tests/test_download.py | 24 +++++++++++++++++++++++- docs/source/cmdline/download.rst | 25 ++++++++++++++++++++----- docs/source/ref/urls.rst | 26 +++++++++++++++++++------- 8 files changed, 130 insertions(+), 21 deletions(-) diff --git a/dandi/dandiarchive.py b/dandi/dandiarchive.py index b92a27098..686c2040c 100644 --- a/dandi/dandiarchive.py +++ b/dandi/dandiarchive.py @@ -58,6 +58,9 @@ lgr = get_logger() +#: `ZARR_EXTENSIONS` in the form `str.endswith()` takes +_ZARR_SUFFIXES = tuple(ZARR_EXTENSIONS) + @dataclass class ParsedDandiURL(ABC): @@ -489,7 +492,7 @@ def split_zarr_location(location: str) -> tuple[str, str] | None: """ parts = [p for p in location.split("/") if p] for i, part in enumerate(parts): - if part.endswith(tuple(ZARR_EXTENSIONS)): + if part.endswith(_ZARR_SUFFIXES): asset_path = "/".join(parts[: i + 1]) zarr_subpath = "/".join(parts[i + 1 :]) return (asset_path, zarr_subpath) if zarr_subpath else None @@ -518,7 +521,7 @@ def at_zarr_boundary(location: str) -> bool: >>> at_zarr_boundary("sub-1/") False """ - return location.rstrip("/").endswith(tuple(ZARR_EXTENSIONS)) + return location.rstrip("/").endswith(_ZARR_SUFFIXES) @dataclass @@ -529,8 +532,10 @@ class AssetZarrEntryURL(SingleAssetURL): produce ``asset_path="sub-1/file.ome.zarr"`` and ``zarr_subpath="0/0/0"``. """ - asset_path: str # e.g., "sub-1/file.ome.zarr" - zarr_subpath: str # e.g., "0/0/0" + #: The path of the Zarr asset, e.g. ``"sub-1/file.ome.zarr"`` + asset_path: str + #: The path within the Zarr asset, e.g. ``"0/0/0"`` + zarr_subpath: str def get_assets( self, client: DandiAPIClient, order: str | None = None, strict: bool = False @@ -553,7 +558,10 @@ def get_assets( yield dandiset.get_asset_by_path(self.asset_path) def get_zarr_filter(self) -> list[ZarrFilter]: - """Restrict the download to the entries at or under `zarr_subpath`.""" + """Restrict the download to the entries at or under `zarr_subpath`. + + :meta private: + """ if not self.zarr_subpath: # `parse_dandi_url()` never produces this, but the class is public # and an empty subpath would otherwise reject every entry. diff --git a/dandi/delete.py b/dandi/delete.py index fefd68221..2bcc171cf 100644 --- a/dandi/delete.py +++ b/dandi/delete.py @@ -142,6 +142,13 @@ def register_url(self, url: str) -> None: assert parsed_url.dandiset_id is not None self.register_dandiset(parsed_url.instance, parsed_url.dandiset_id) else: + if parsed_url.get_zarr_filter(): + # The URL points inside a Zarr asset, but `get_assets()` yields + # the whole asset, so deleting it would take the entire Zarr. + raise NotImplementedError( + "Cannot delete individual entries within a Zarr asset;" + f" {url} points inside one" + ) if parsed_url.version_id is None: parsed_url.version_id = DRAFT self.register_assets_url(url, parsed_url) diff --git a/dandi/download.py b/dandi/download.py index e5c916501..366e26c39 100644 --- a/dandi/download.py +++ b/dandi/download.py @@ -1064,7 +1064,7 @@ def _download_zarr( lock: Lock, jobs: int | None = None, zarr_entry_filter: Callable[[str], bool] | None = None, - required_filters: list[ZarrFilter] | None = None, + required_filters: Sequence[ZarrFilter] = (), ) -> Iterator[dict]: # Avoid heavy import by importing within function: from .support.digests import get_zarr_checksum @@ -1078,12 +1078,11 @@ def _download_zarr( # ones, so a non-empty `entries` does not mean the required filters # matched; track them separately. required_match = ( - make_zarr_entry_filter(required_filters) if required_filters else None + make_zarr_entry_filter(list(required_filters)) if required_filters else None ) matched_required = False def unmatched_required_error() -> dict: - assert required_filters is not None patterns = ", ".join(repr(f.pattern) for f in required_filters) return { "status": "error", diff --git a/dandi/tests/test_dandiarchive.py b/dandi/tests/test_dandiarchive.py index 9f06baf30..4f00707ba 100644 --- a/dandi/tests/test_dandiarchive.py +++ b/dandi/tests/test_dandiarchive.py @@ -292,6 +292,28 @@ zarr_subpath="scale0/0/0", ), ), + ( # the GUI's own "browse to this zarr" URL: the parser appends the + # trailing slash, which is not meaningful at a zarr boundary + "https://dandiarchive.org/dandiset/000108/draft/files" + "?location=sub-1/file.ome.zarr", + AssetItemURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id="draft", + path="sub-1/file.ome.zarr", + ), + ), + ( # ... and one browsing into it names entries + "https://dandiarchive.org/dandiset/000108/draft/files" + "?location=sub-1/file.ome.zarr/0/0", + AssetZarrEntryURL( + instance=known_instances["dandi"], + dandiset_id="000108", + version_id="draft", + asset_path="sub-1/file.ome.zarr", + zarr_subpath="0/0", + ), + ), ( # a trailing slash below the boundary still names entries "dandi://dandi/000108/sub-1/file.ome.zarr/0/0/", AssetZarrEntryURL( diff --git a/dandi/tests/test_delete.py b/dandi/tests/test_delete.py index 9a6ff8dd1..250f86a90 100644 --- a/dandi/tests/test_delete.py +++ b/dandi/tests/test_delete.py @@ -407,6 +407,30 @@ def test_delete_version( delete_spy.assert_not_called() +@pytest.mark.ai_generated +@pytest.mark.parametrize( + "suffix", ["/acquisition/data_00000_AD0", "/acquisition/data_00000_AD0/"] +) +def test_delete_within_zarr_refused( + mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, suffix: str +) -> None: + """A URL inside a Zarr asset must not delete the whole asset. + + `AssetZarrEntryURL.get_assets()` yields the Zarr asset itself, so without + a guard the deletion would take every entry, not the named ones. + """ + delete_spy = mocker.spy(RESTFullAPIClient, "delete") + with pytest.raises(NotImplementedError) as excinfo: + delete( + [f"dandi://dandi/000108/sub-1/file.ome.zarr{suffix}"], + dandi_instance="dandi", + devel_debug=True, + force=True, + ) + assert "Cannot delete individual entries within a Zarr asset" in str(excinfo.value) + delete_spy.assert_not_called() + + def test_delete_no_dandiset( mocker: MockerFixture, monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/dandi/tests/test_download.py b/dandi/tests/test_download.py index f7f9e06fb..7df8e74cb 100644 --- a/dandi/tests/test_download.py +++ b/dandi/tests/test_download.py @@ -1752,7 +1752,29 @@ def test_download_zarr_url_subpath( ) zarr_dir = tmp_path / "sample.zarr" assert (zarr_dir / "a" / "data.bin").read_text() == "data-a" - assert (zarr_dir / "b").exists() is whole_zarr + if whole_zarr: + assert (zarr_dir / "b" / "data.bin").read_text() == "data-b" + else: + assert not (zarr_dir / "b").exists() + + +@pytest.mark.ai_generated +def test_download_url_subpath_of_non_zarr_asset( + tmp_path: Path, new_dandiset: SampleDandiset +) -> None: + """A subpath under a blob that merely *looks* like a Zarr is an error. + + The URL's subpath cannot be honoured, and downloading the whole blob + instead would silently give the user something they did not ask for. + """ + (new_dandiset.dspath / "sample.zarr").write_text("This is not a Zarr.\n") + new_dandiset.upload(allow_any_path=True) + with pytest.raises(RuntimeError, match="1 error while downloading"): + download( + f"dandi://{new_dandiset.api.instance_id}" + f"/{new_dandiset.dandiset_id}/sample.zarr/0/0", + tmp_path, + ) @pytest.mark.ai_generated diff --git a/docs/source/cmdline/download.rst b/docs/source/cmdline/download.rst index 424dc3887..51f93e4fc 100644 --- a/docs/source/cmdline/download.rst +++ b/docs/source/cmdline/download.rst @@ -85,18 +85,20 @@ Options ``path:0/0``. ``regex`` - Match the entry path against a Python regular expression, e.g. + Search the entry path for a Python regular expression. The match is + unanchored, so anchor it yourself to match from the start, e.g. ``regex:^0/[0-9]+/``. In place of :samp:`{type}:{pattern}`, the predefined filter ``metadata`` - may be given; it selects the Zarr metadata files (``.zarray``, ``.zgroup``, - ``.zattrs``, ``.zmetadata``, and ``zarr.json``). + may be given; it selects the Zarr metadata files, i.e. any entry named + ``zarr.json`` or whose name begins with ``.z`` (``.zarray``, ``.zgroup``, + ``.zattrs``, ``.zmetadata``), at any depth. The option may be given more than once, in which case an entry is downloaded if it matches **any** of the filters. - A URL that points inside a Zarr asset (see :ref:`resource_ids`) restricts - the download in the same way, as though ``path:`` had been given for the + A URL that points inside a Zarr asset (see :ref:`resource_ids`) selects + entries in the same way, as though ``path:`` had been given for the portion of the URL below the Zarr asset:: dandi download dandi://dandi/000108/sub-1/file.ome.zarr/0/0 @@ -105,6 +107,19 @@ Options to have: if no entry matches, the download fails rather than quietly downloading nothing. + .. warning:: + + A URL subpath and ``--zarr`` are **unioned**, not intersected, so + passing both *widens* the download rather than narrowing it. In:: + + dandi download --zarr metadata \ + dandi://dandi/000108/sub-1/file.ome.zarr/0/0 + + the entries under ``0/0`` are downloaded *and* so is every metadata + file anywhere in the Zarr. To restrict metadata to a subtree, give + the subtree in the filter itself (``--zarr 'glob:0/0/**/.z*'``) rather + than in the URL. + Because only part of a Zarr is fetched, extra local files are not deleted and the Zarr checksum of the result is not verified; the checksum of each individual downloaded entry still is. diff --git a/docs/source/ref/urls.rst b/docs/source/ref/urls.rst index 88fb6bded..895332257 100644 --- a/docs/source/ref/urls.rst +++ b/docs/source/ref/urls.rst @@ -42,9 +42,15 @@ has one, and its draft version will be used otherwise. location within the Zarr, and `parse_dandi_url()` will convert the URL to an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. - - If the ``glob``/``--path-type glob`` option is not in effect, the URL - refers to an asset folder by path, and `parse_dandi_url()` will convert the - URL to an `AssetFolderURL`. + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + ends *at* a Zarr asset, the URL refers to that asset, and + `parse_dandi_url()` will convert the URL to an `AssetItemURL`. Note that + this applies to a plain "browse to this Zarr" GUI URL, as the trailing + slash such a URL carries is not meaningful at a Zarr boundary. + + - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` + does not involve a Zarr asset, the URL refers to an asset folder by path, + and `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. - :samp:`https://{server}[/api]/dandisets/{dandiset-id}[/versions[/{version}]]` — Refers to a Dandiset. `parse_dandi_url()` converts this format to a @@ -63,6 +69,10 @@ has one, and its draft version will be used otherwise. prefix ``path``. `parse_dandi_url()` converts this format to an `AssetPathPrefixURL`. + Note that, unlike the forms above, ``path`` here is a plain prefix and is + not interpreted against Zarr boundaries, so a prefix reaching inside a Zarr + asset matches no assets. + - :samp:`https://{server}[/api]/dandisets/{dandiset-id}/versions/{version}/assets/?glob={path}` — Refers to all assets in the given Dandiset whose paths match the glob pattern ``path``. `parse_dandi_url()` converts this format to an @@ -86,12 +96,14 @@ has one, and its draft version will be used otherwise. an `AssetZarrEntryURL`. See :ref:`zarr_entry_urls` below. - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` - ends with a trailing slash, the URL refers to an asset folder by path, and - `parse_dandi_url()` will convert the URL to an `AssetFolderURL`. + ends with a trailing slash but does not end at a Zarr asset, the URL + refers to an asset folder by path, and `parse_dandi_url()` will convert + the URL to an `AssetFolderURL`. - If the ``glob``/``--path-type glob`` option is not in effect and ``path`` - does not end with a trailing slash, the URL refers to a single asset by - path, and `parse_dandi_url()` will convert the URL to an `AssetItemURL`. + either does not end with a trailing slash or ends at a Zarr asset, the URL + refers to a single asset by path, and `parse_dandi_url()` will convert the + URL to an `AssetItemURL`. - Any other HTTPS URL that redirects to one of the above