Skip to content

Reject nonexistent requested paths during asset discovery - #1932

Open
AtomicGlance wants to merge 6 commits into
dandi:masterfrom
AtomicGlance:fix/1094-missing-path
Open

AtomicGlance wants to merge 6 commits into
dandi:masterfrom
AtomicGlance:fix/1094-missing-path

Conversation

@AtomicGlance

@AtomicGlance AtomicGlance commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1094.
find_dandi_files() currently allows nonexistent paths to be silently ignored or represented as assets. This change checks every requested root before yielding any files and raises FileNotFoundError with the offending filename.
The check uses lstat() so broken symlinks remain discoverable. This preserves DANDI’s missing-content handling for unfetched DataLad files. Existing empty directories still return no assets, and no additional recursive walk is introduced.
Tests cover missing paths with recognized, unrecognized, and hidden names; both allow_all settings; multiple requested roots; empty directories; and broken symlinks. A CLI regression test also confirms that dandi validate retains its existing missing-path error.
Validation on Windows with Python 3.13:

  • File tests excluding upload integration tests: 28 passed, 1 skipped.
  • Focused discovery and CLI tests: 12 passed, 1 skipped.
  • Black, isort, flake8, and mypy dandi passed.

@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.18919% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.83%. Comparing base (ad8f64d) to head (5fcb0d6).

Files with missing lines Patch % Lines
dandi/tests/test_files.py 85.18% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1932      +/-   ##
==========================================
+ Coverage   90.77%   90.83%   +0.05%     
==========================================
  Files          92       92              
  Lines       14130    14167      +37     
==========================================
+ Hits        12826    12868      +42     
+ Misses       1304     1299       -5     
Flag Coverage Δ
unittests 90.83% <89.18%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

This PR is ready for review, but GitHub won't let me remove its draft status, through either the website or CLI. Could you please mark it ready and add the patch label? All other checks are passing. Thank you!

@AtomicGlance
AtomicGlance marked this pull request as ready for review October 1, 2026 15:48
@AtomicGlance

Copy link
Copy Markdown
Contributor Author

This PR is ready for review, but GitHub won't let me remove its draft status, through either the website or CLI. Could you please mark it ready and add the patch label? All other checks are passing. Thank you!

@yarikoptic Hi, can you have a look at this?

Comment thread dandi/files/__init__.py Outdated
@AtomicGlance

Copy link
Copy Markdown
Contributor Author

Hi @yarikoptic, thanks again for the review! Could you please rerun the failed and cancelled jobs in the latest Tests workflow, and the cancelled label check, when you have a chance? Both failed Windows jobs report that the hosted runner lost communication with GitHub; neither provides a test failure traceback. I tried requesting a rerun, but GitHub returned a 403 because it requires repository admin rights.

The explicit lexists() fix is pushed and the review conversation is addressed. Thanks for your help!

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

Hi @yarikoptic, a gentle follow-up when you have a moment: could you please rerun the failed and cancelled jobs in the Tests workflow and the cancelled label check? The latest results are still from October 5. Both failed Windows jobs reported a runner disconnection; your requested lexists() change is in place. Once the rerun finishes, I'd appreciate another look at the PR.

My related Archive fix, dandi/dandi-archive#2914, is also waiting for review. Only the deployment-preview check is reported there, so I can't claim the backend suite is green. If someone else is better placed to review it or enable the backend tests, could you point me their way?

Thanks again for your time and guidance!

@yarikoptic

Copy link
Copy Markdown
Member

clicking "Update branch". Next time if you need rerun -- just make sure it is up to date with current master and/or rewrite/force push last commit.

@yarikoptic yarikoptic added UX patch Increment the patch version when merged labels Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

patch Increment the patch version when merged UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

make find_dandi_files raise FileNotFoundError exception if path is invalid

2 participants