folder download needing trailing /

[yarikoptic]

While preparing answer for #1255 I hit a regression of being unable to download a folder given a url from web UI

❯ dandi download https://dandiarchive.org/dandiset/000029/0.230317.1553/files?location=sub-anm369962
2023-03-21 08:56:59,998 [ WARNING] A newer version (0.51.0) of dandi/dandi-cli is available. You are using 0.50.1+19.gd2576648
2023-03-21 08:57:14,154 [    INFO] Logs saved in /home/yoh/.cache/dandi-cli/log/20230321125656Z-464601.log
Error: No asset at path 'sub-anm369962'

but then tried with / explicitly added and it worked:

❯ dandi download https://dandiarchive.org/dandiset/000029/0.230317.1553/files?location=sub-anm369962/
PATH                                             SIZE    DONE    DONE% CHECKSUM STATUS          MESSAGE
sub-anm369962/sub-anm369962_behavior+ecephys.nwb 6.6 MB  6.6 MB   100%    ok    done                   
Summary:                                         6.6 MB  6.6 MB                 1 done                 
                                                         100.00%                                       
2023-03-21 09:00:47,239 [    INFO] Logs saved in /home/yoh/.cache/dandi-cli/log/20230321130019Z-465662.log

so, I think if "no asset at path" and path does not end with / we should try with /.

[jwodder]

@yarikoptic It's already established & documented that a .../files?location={path} URL where path does not end in a slash refers to a single asset. I don't see the advantage to changing it, and the suggested change would complicate things that need to know whether a URL refers to one or multiple assets, e.g., for determining the path(s) at which to download assets.

[yarikoptic]

advantage is - support of URLs copy/pasted from web UI to simplify user data workflows. Unfortunately, I think, web UI changed its behavior (I guess) and no longer includes that trailing /. I have filed dandi/dandi-archive#1546 with hopes to get it back, but I think we should also address it one way or another in dandi-cli. If not implementing "sensing" of some kind and remove reliance on / as indicator, whenever such an exception is raised -- No asset at path 'sub-anm369962' -- could it be improved as `No asset at path 'sub-anm369962' - add a trailing '/' if you intended to point to a folder'. Please implement at least that.

But as to me that would be just inferior UX -- "why software, if it knows that it might be a folder, doesn't just handle it as a folder?"

[jwodder]

"why software, if it knows that it might be a folder, doesn't just handle it as a folder?"

Answer: Because it only might be a folder; it might also be the case that the user left off, say, a trailing 5. Playing guessing games when the user gives us garbage is an even worse UX.

[jwodder]

@yarikoptic Side note: We had almost this same discussion before in #598 (comment), but the message suggesting the user add a trailing slash currently only shows up when strict=False, yet strict=True when retrieving the assets to download.

[jwodder]

@yarikoptic Taking into account the way AssetItemURL.get_assets() currently works, exactly what changes do you want made?

[yarikoptic]

936e36b made it strict in #820 (Make download fail immediately on nonexistent resources) to address #817 (we are silently failing to download a non-existing asset)...
"what changes" - I don't know yet exactly since I would have just done it already. What effect I want ideally to achieve is that whenever URL is not pointing to assets (as a prefix folder or not) - we error out, if it points to asset or prefix as a folder -- it downloads.

[jwodder]

@yarikoptic

What effect I want ideally to achieve is that whenever URL is not pointing to assets (as a prefix folder or not) - we error out, if it points to asset or prefix as a folder -- it downloads.

Please reread this comment about why that would lead to problems.

When I asked what changes you wanted made, I was referring to your previous comment in which you asked for the "No asset at path" message to conditionally include a recommendation to append a slash. If you look at the current implementation of AssetItemURL.get_assets(), then the following questions immediately leap to mind:

• When strict=True, should the trailing slash recommendation only be added if the path in question points to a folder, as is done for strict=False?
• When the error message is appended, should the exception type be NotFoundError or ValueError (as is done for strict=False)? If NotFoundError, should the current ValueError be changed to NotFoundError as well?

[yarikoptic]

@yarikoptic

What effect I want ideally to achieve is that whenever URL is not pointing to assets (as a prefix folder or not) - we error out, if it points to asset or prefix as a folder -- it downloads.

Please reread this comment about why that would lead to problems.

I reread -- seems to point to our underlying code assumptions/logic, not some fundamental problem (like if we could have prefix asset blob and prefix/ as prefix to other assets), so there is no objective reason IMHO why it could not in principle be implemented. Thanks for the analysis/questions -- I will review the code and follow up later.

[github-actions]

🚀 Issue was released in 0.57.0 🚀