Skip to content

Commit 540c7c0

Browse files
Accept broken symlinks as path arguments of dandi validate (#1937)
* Accept broken symlinks as path arguments of `dandi validate` `dandi validate` already handles annexed files whose content has not been fetched (the broken symlinks of a DataLad dataset) according to `--missing-file-content`: `error`, `skip`, or `only-non-data`. Those policies were applied only when such a file was reached through its directory, though: given directly on the command line, the file never got past argument parsing, because `click.Path(exists=True)` follows the link and rejected it with "does not exist". Check the path with `lexists()` instead, so that dandi validate --missing-file-content=skip sub-01/sub-01.nwb works the same as validating `sub-01/`, while paths that truly do not exist are still rejected with the same error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HFhMPs6vF5zyeHDvPTvRcA * Harden and move the broken-symlink-accepting path type Address review on the `click.Path` subclass accepting broken symlinks: - Rename `ExistingPath` to `PathOrBrokenSymlink`, which says what sets it apart, and move it to `dandi/cli/base.py` with the other reusable `ParamType`s. - Force `exists=False` and refuse `exists=True` or `resolve_path=True`: the former would bring back click's own check, which rejects broken links, and the latter would replace an annexed link with its `.git/annex/objects/...` target. - Match `click.Path.convert` more closely: skip the check for `-` when `allow_dash` applies, and build the message with gettext and `click.utils.format_filename()` as click does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Replace PathOrBrokenSymlink with a click.Path accepting a lexists flag Per review: rather than a special-purpose type whose name raises "what about a symlink that is not broken?", provide `dandi.cli.base.Path`, a `click.Path` taking a `lexists` flag next to `exists`. With `lexists=True` a path must satisfy `os.path.lexists()`, so it may be a symlink whose target is missing, which `exists=True` (following the link) rejects. `dandi validate` needs that so its `--missing-file-content` policies (error, skip, only-non-data) also apply to an annexed file whose content was not fetched when that file is given directly on the command line, not only when reached via its directory. `lexists=True` refuses `resolve_path=True`, which would replace such a link with its `.git/annex/objects/...` target; `exists=True` keeps its click meaning. The docstring now describes only the behavior, leaving the rationale to this message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Deny Claude posting comments/reviews on GitHub PRs and issues Add project-level .claude/settings.json denying the GitHub MCP tools that post, edit, or resolve comments and reviews on pull requests and issues. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Simplify docstring of cli.base.Path Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Rename cli.base.Path to LinkAwarePath and explain its purpose Path clashed with pathlib.Path; the docstring now explains why a symlink-aware path type is needed rather than how it works. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Focus LinkAwarePath docstring on its purpose; document lexists on __init__ Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Use a plain f-string for the LinkAwarePath error message Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr * Remove project-level Claude settings Keep that configuration personal instead, as in con/fscacher#113. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qn5WBSiQgoZoL4fytF6nEr --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 817818b commit 540c7c0

4 files changed

Lines changed: 109 additions & 3 deletions

File tree

‎dandi/cli/base.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
from enum import Enum
22
from functools import wraps
33
import os
4+
from typing import Any
45

56
import click
7+
from click.utils import format_filename
68

79
from .. import get_logger
810

@@ -70,6 +72,44 @@ def get_metavar(self, param, ctx=None):
7072
return "[" + ",".join(self.values) + ",all]"
7173

7274

75+
class LinkAwarePath(click.Path):
76+
"""
77+
A ``click.Path`` for commands that should accept broken symlinks
78+
79+
The typical broken symlink is a git-annex (or DataLad) file whose content
80+
has not been fetched. A command like ``dandi validate`` should still take
81+
such a path, to report on it or skip it, rather than refuse it as missing.
82+
"""
83+
84+
def __init__(self, *, lexists: bool = False, **kwargs: Any) -> None:
85+
"""
86+
Parameters
87+
----------
88+
lexists
89+
If True, the path must exist, but a symlink to a missing target
90+
counts as existing (unlike with ``exists=True``). Cannot be
91+
combined with ``resolve_path=True``.
92+
**kwargs
93+
Passed to ``click.Path``.
94+
"""
95+
if lexists and kwargs.get("resolve_path"):
96+
raise ValueError("lexists=True cannot be combined with resolve_path=True")
97+
super().__init__(**kwargs)
98+
self.lexists = lexists
99+
100+
def convert(
101+
self, value: Any, param: click.Parameter | None, ctx: click.Context | None
102+
) -> Any:
103+
is_dash = self.file_okay and self.allow_dash and value in ("-", b"-")
104+
if self.lexists and not is_dash and not os.path.lexists(value):
105+
self.fail(
106+
f"{self.name.title()} {format_filename(value)!r} does not exist.",
107+
param,
108+
ctx,
109+
)
110+
return super().convert(value, param, ctx)
111+
112+
73113
# ???: could make them always available but hidden
74114
# via hidden=True.
75115
def devel_option(*args, **kwargs):

‎dandi/cli/cmd_validate.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@
1111

1212
import click
1313

14-
from .base import devel_debug_option, devel_option, map_to_click_exceptions
14+
from .base import (
15+
LinkAwarePath,
16+
devel_debug_option,
17+
devel_option,
18+
map_to_click_exceptions,
19+
)
1520
from .formatter import JSONFormatter, JSONLinesFormatter, TextFormatter, YAMLFormatter
1621
from ..utils import pluralize
1722
from ..validate._core import validate as validate_
@@ -176,7 +181,7 @@ def _filter_results(
176181
multiple=True,
177182
default=(),
178183
)
179-
@click.argument("paths", nargs=-1, type=click.Path(exists=True, dir_okay=True))
184+
@click.argument("paths", nargs=-1, type=LinkAwarePath(lexists=True, dir_okay=True))
180185
@click.pass_context
181186
@devel_debug_option()
182187
@map_to_click_exceptions

‎dandi/cli/tests/test_base.py‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
from enum import StrEnum
2+
import os
3+
from pathlib import Path
24

35
import click
46
from click.testing import CliRunner
57
import pytest
68

7-
from ..base import EnumChoice
9+
from ..base import EnumChoice, LinkAwarePath
810

911

1012
class _Existing(StrEnum):
@@ -69,3 +71,40 @@ def cmd(existing):
6971
r = CliRunner().invoke(cmd, [])
7072
assert r.exit_code == 0, r.output
7173
assert captured["existing"] is _Existing.SKIP
74+
75+
76+
@pytest.mark.ai_generated
77+
def test_path_lexists_rejects_resolve_path():
78+
with pytest.raises(ValueError, match="resolve_path"):
79+
LinkAwarePath(lexists=True, resolve_path=True)
80+
81+
82+
@pytest.mark.ai_generated
83+
@pytest.mark.parametrize(
84+
"kwargs,broken_ok",
85+
[
86+
({"lexists": True}, True),
87+
({"lexists": True, "exists": True}, False),
88+
({"exists": True}, False),
89+
],
90+
)
91+
def test_path_lexists(tmp_path: Path, kwargs: dict, broken_ok: bool) -> None:
92+
@click.command()
93+
@click.argument("path", type=LinkAwarePath(allow_dash=True, **kwargs))
94+
def cmd(path):
95+
click.echo(f"got:{path}")
96+
97+
(tmp_path / "file.txt").write_text("content")
98+
broken = tmp_path / "broken"
99+
try:
100+
os.symlink(tmp_path / "missing", broken)
101+
except OSError:
102+
pytest.skip("symlinks are not supported here")
103+
for ok in [str(tmp_path / "file.txt"), "-"] + ([str(broken)] if broken_ok else []):
104+
r = CliRunner().invoke(cmd, [ok])
105+
assert r.exit_code == 0, r.output
106+
assert r.output == f"got:{ok}\n"
107+
for bad in [str(tmp_path / "missing")] + ([] if broken_ok else [str(broken)]):
108+
r = CliRunner().invoke(cmd, [bad])
109+
assert r.exit_code == 2
110+
assert f"Path {bad!r} does not exist." in r.output

‎dandi/cli/tests/test_cmd_validate.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -996,3 +996,25 @@ def test_validate_missing_file_content_no_broken_symlinks(tmp_path: Path) -> Non
996996
assert r_default.exit_code == r_skip.exit_code == 0
997997
assert "FILE_CONTENT_MISSING" not in r_default.output
998998
assert "FILE_CONTENT_MISSING" not in r_skip.output
999+
1000+
1001+
@pytest.mark.ai_generated
1002+
def test_validate_broken_symlink_path_argument(tmp_path: Path) -> None:
1003+
"""A broken symlink can be given directly as a path to validate.
1004+
1005+
The --missing-file-content policies then apply to it just as they do when
1006+
the containing directory is given.
1007+
"""
1008+
ds = _make_dandiset_with_broken_symlinks(tmp_path)
1009+
link = ds / "sub-001" / "sub-001.nwb"
1010+
r = CliRunner().invoke(validate, [str(link)])
1011+
assert r.exit_code == 1
1012+
assert "does not exist" not in r.output
1013+
assert "FILE_CONTENT_MISSING" in r.output
1014+
r = CliRunner().invoke(validate, ["--missing-file-content", "skip", str(link)])
1015+
assert r.exit_code == 0
1016+
assert "FILE_CONTENT_MISSING_SKIPPED" in r.output
1017+
# Paths that truly do not exist are still rejected
1018+
r = CliRunner().invoke(validate, [str(ds / "sub-001" / "nonexistent.nwb")])
1019+
assert r.exit_code == 2
1020+
assert "does not exist" in r.output

0 commit comments

Comments
 (0)