Address code review findings for partial zarr support

- Pre-compile regex patterns at ZarrFilter construction time, catching
  invalid patterns early instead of on every matches() call (B1)
- Remove redundant get_asset_download_path override in AssetZarrEntryURL
  that was identical to the inherited SingleAssetURL method (H1)
- Use Literal["full", "patch"] for zarr_mode parameter instead of bare
  str to prevent silent misbehavior on invalid values (H2)
- Collapse consecutive ** glob segments to avoid exponential
  backtracking in _glob_match_parts (H3)
- Simplify split_zarr_location to use str.split instead of
  PurePosixPath (M1)
- Add explanatory comment for type: ignore in parse_zarr_filter (M2)
- Yield {"status": "done"} when zarr filter matches zero entries (M4)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: yarikoptic

dandi/dandiarchive.py

@@ -471,9 +471,7 @@ def split_zarr_location(location: str) -> tuple[str, str] | None:
     >>> split_zarr_location("sub-1/file.ome.zarr")  # no subpath
     >>> split_zarr_location("sub-1/file.nwb")        # no zarr extension
     """
-    from pathlib import PurePosixPath
-
-    parts = PurePosixPath(location).parts
+    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):
             asset_path = "/".join(parts[: i + 1])
@@ -513,15 +511,6 @@ def get_assets(
         with _maybe_strict(strict):
             yield dandiset.get_asset_by_path(self.asset_path)
 
-    def get_asset_download_path(
-        self, asset: BaseRemoteAsset, preserve_tree: bool
-    ) -> str:
-        path = asset.path.lstrip("/")
-        if preserve_tree:
-            return path
-        else:
-            return posixpath.basename(path)
-
 
 @dataclass
 class AssetFolderURL(MultiAssetURL):

dandi/download.py

@@ -1069,6 +1069,9 @@ def downloads_gen():
             if final_out is not None:
                 break
         else:
+            if zarr_entry_filter is not None:
+                # Filter matched no entries; still report completion
+                yield {"status": "done"}
             return
 
     if zarr_entry_filter is not None:

dandi/files/zarr.py

@@ -12,7 +12,7 @@
 import os.path
 from pathlib import Path
 from time import sleep
-from typing import Any, Optional
+from typing import Any, Literal, Optional
 import urllib.parse
 
 from dandischema.models import BareAsset, DigestType
@@ -557,7 +557,7 @@ def iter_upload(
         metadata: dict[str, Any],
         jobs: int | None = None,
         replacing: RemoteAsset | None = None,
-        zarr_mode: str = "full",
+        zarr_mode: Literal["full", "patch"] = "full",
     ) -> Iterator[dict]:
         """
         Upload the Zarr directory as an asset with the given metadata to the

dandi/tests/test_zarr_filter.py

@@ -165,6 +165,18 @@ def test_parse_pattern_with_colon(self) -> None:
         filters = parse_zarr_filter("regex:a:b")
         assert filters[0].pattern == "a:b"
 
+    @pytest.mark.ai_generated
+    def test_parse_invalid_regex(self) -> None:
+        """Invalid regex patterns should raise ValueError at construction time."""
+        with pytest.raises(ValueError, match="Invalid regex pattern"):
+            parse_zarr_filter("regex:[invalid")
+
+    @pytest.mark.ai_generated
+    def test_invalid_regex_direct(self) -> None:
+        """Direct ZarrFilter construction with bad regex should also fail."""
+        with pytest.raises(ValueError, match="Invalid regex pattern"):
+            ZarrFilter("regex", "(unclosed")
+
 
 class TestMakeZarrEntryFilter:
     @pytest.mark.ai_generated

dandi/upload.py

@@ -21,7 +21,7 @@
 import re
 import time
 from time import sleep
-from typing import Any, TypedDict, cast
+from typing import Any, Literal, TypedDict, cast
 from unittest.mock import patch
 
 import click
@@ -112,7 +112,7 @@ def upload(
     jobs: int | None = None,
     jobs_per_file: int | None = None,
     sync: bool = False,
-    zarr_mode: str = "full",
+    zarr_mode: Literal["full", "patch"] = "full",
 ) -> None:
     if paths:
         paths = [Path(p).absolute() for p in paths]

dandi/zarr_filter.py

@@ -7,7 +7,7 @@
 
 from __future__ import annotations
 
-from dataclasses import dataclass
+from dataclasses import dataclass, field
 from fnmatch import fnmatchcase
 import re
 from typing import Callable, Literal
@@ -19,6 +19,16 @@ class ZarrFilter:
 
     filter_type: Literal["glob", "path", "regex"]
     pattern: str
+    _compiled_regex: re.Pattern[str] | None = field(
+        init=False, default=None, repr=False, compare=False
+    )
+
+    def __post_init__(self) -> None:
+        if self.filter_type == "regex":
+            try:
+                self._compiled_regex = re.compile(self.pattern)
+            except re.error as e:
+                raise ValueError(f"Invalid regex pattern {self.pattern!r}: {e}") from e
 
     def matches(self, entry_path: str) -> bool:
         """Test if a zarr-internal path matches this filter.
@@ -41,7 +51,8 @@ def matches(self, entry_path: str) -> bool:
                 self.pattern.rstrip("/") + "/"
             )
         elif self.filter_type == "regex":
-            return re.search(self.pattern, entry_path) is not None
+            assert self._compiled_regex is not None
+            return self._compiled_regex.search(entry_path) is not None
         else:
             raise ValueError(f"Unknown filter type: {self.filter_type!r}")
 
@@ -55,7 +66,13 @@ def _glob_match(pattern: str, path: str) -> bool:
     """
     patparts = [p for p in pattern.split("/") if p]
     pathparts = [p for p in path.split("/") if p]
-    return _glob_match_parts(patparts, pathparts)
+    # Collapse consecutive ** into a single ** to avoid exponential backtracking
+    collapsed: list[str] = []
+    for p in patparts:
+        if p == "**" and collapsed and collapsed[-1] == "**":
+            continue
+        collapsed.append(p)
+    return _glob_match_parts(collapsed, pathparts)
 
 
 def _glob_match_parts(patparts: list[str], pathparts: list[str]) -> bool:
@@ -107,7 +124,8 @@ def parse_zarr_filter(spec: str) -> list[ZarrFilter]:
     Raises
     ------
     ValueError
-        If the spec is not a valid alias or ``TYPE:PATTERN`` string.
+        If the spec is not a valid alias or ``TYPE:PATTERN`` string,
+        or if a regex pattern is invalid.
     """
     if spec in ZARR_FILTER_ALIASES:
         return list(ZARR_FILTER_ALIASES[spec])
@@ -121,6 +139,7 @@ def parse_zarr_filter(spec: str) -> list[ZarrFilter]:
         raise ValueError(
             f"Unknown zarr filter type: {type_!r}. Expected 'glob', 'path', or 'regex'"
         )
+    # type_ is validated above; narrow str -> Literal for the dataclass
     return [ZarrFilter(type_, pattern)]  # type: ignore[arg-type]