Normalize path while requesting list of assets from the server #1454
Conversation
Duplication leads to bugs and also makes it harder to read the test since requires mental "filtering" to disregard unnecessary duplicated code while ensuring that not missing some critical difference. E.g. I did miss that some have sorted() and some not. Now it is more obvious IMHO. Ideally other tests with similar pattern should be redone as well. Ideally there might be a way to use parametric testing with a common fixture (did not check how/if possible yet)
What we call "path" is really just a prefix for dandi-archive. To ensure more consistent operation for a user, let's normalize those paths, even though normalization would seemingly allow for obnoxious cases like "nonexistent/../realfolder/". According to #1452 it is likely that we had operation more robust at server-side before and were accepting "./" whenever now it just returns an empty list. Not sure if worth seeking change at dandi-archive level ATM.
yarikoptic
added
patch
If worked before, it stopped working at some point. We are likely to make it more robust on dandi-cli side: - dandi/dandi-cli#1454 but I think it would be better to just not specify such way in the example. You might want to elaborate that example with actually pointing to a specific folder instead
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1454 +/- ##
==========================================
+ Coverage 88.47% 88.58% +0.10%
==========================================
Files 77 77
Lines 10518 10535 +17
==========================================
+ Hits 9306 9332 +26
+ Misses 1212 1203 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dandi/dandiapi.py
Outdated
| @@ -1154,6 +1177,8 @@ def get_assets_with_path_prefix( | |||
| Returns an iterator of all assets in this version of the Dandiset whose | |||
| `~RemoteAsset.path` attributes start with ``path`` | |||
|
|
|||
| ``path`` is normalized first to possibly remove leading `./` or relative | |||
| paths (e.g., `../`) within it. | |||
There was a problem hiding this comment.
Add a blank line after this line to force a paragraph break.
Also, this paragraph should probably also be added to the docs for get_asset_by_path() and download_directory(), since those pass their path arguments to this method.
There was a problem hiding this comment.
ok, will add use to get_asset_by_path as well
dandi/dandiapi.py
Outdated
| ``path`` is normalized first to possibly remove leading `./` or relative | ||
| paths (e.g., `../`) within it. |
There was a problem hiding this comment.
| ``path`` is normalized first to possibly remove leading `./` or relative | |
| paths (e.g., `../`) within it. | |
| ``path`` is normalized first to possibly remove leading ``./`` or relative | |
| paths (e.g., ``../``) within it. |
dandi/dandiapi.py
Outdated
| Helper to normalize path before passing it to the server. | ||
|
|
||
| We and API call it "path" but it is really a "prefix" with inherent | ||
| semantics of containing directory divider '/' and emphasizing with |
There was a problem hiding this comment.
I don't think "emphasizing" is the word you want here. Maybe "and referring to a directory when terminated with '/'."
dandi/dandiapi.py
Outdated
| # possibly specifying ./ or some other relative paths etc, let's normalize | ||
| # the path. | ||
| # Ref: https://github.com/dandi/dandi-cli/issues/1452 | ||
| path_normed = os.path.normpath(path) |
There was a problem hiding this comment.
Use posixpath instead of os.path here so that forward slashes will remain forward slashes when running on Windows.
|
ha -- I have missed that my change to |
|
windows -- flaky |
|
🚀 PR was released in |
Closes #1452
See individual commits for more description/rationale.
Related: