issue with download

[djarecka]

I'm not able to reopen an issue so opening a new one

@yarikoptic - I followed your instruction, clicking copy-paste etc, and I succeeded to get the url, but I have the same error as in #550:

(dandi_dev) yasah:dandi-cli dorota$ dandi download https://api.dandiarchive.org/api/dandisets/010006/versions/draft/assets/3442ac20-595a-4b47-83cf-78da1495bf06/download/
2021-04-12 14:09:39,851 [ WARNING] A newer version (0.13.1) of dandi/dandi-cli is available. You are using 0.10.0+461.gc57717c
2021-04-12 14:09:40,361 [    INFO] Logs saved in /Users/dorota/Library/Logs/dandi-cli/20210412180939Z-29364.log
Error: 'asset_id'

edit
It's the same error, but it's different than #550

[jwodder]

It appears that the asset metadata on the server was changed to store the asset ID in an "identifier" field instead of "asset_id". @satra When did that happen? I though you had yet to make that change. Moreover, why I am not notified when API changes like this happen?

[yarikoptic]

Eh, too many ids ;) I think this one has nothing to do with identifier -> identifier + @id change which is yet to come, and probably just some not unittested (within dandi-cli) use-case and a true bug?
Now that we have dandi-cli tests ran as part of the dandi-api CI, as long as we have a test for it - should not go by unnoticed. May be we just indeed need the fix @djarecka implemented in https://github.com/dandi/dandi-cli/pull/552/files and which I (prematurely) closed?

[jwodder]

@yarikoptic That patch is doing entirely the wrong thing. The root of the problem in this issue is that the asset structures returned by /dandisets/{versions__dandiset__pk}/versions/{versions__version}/assets/ and …/assets/{asset_id}/ are not compatible, as they use different names for the same fields.

[yarikoptic]

do you see a straightforward way to support both and add a test for both end points?

[jwodder]

@yarikoptic My preferred solution would be for the API to make its return values consistent (e.g., by making …/assets/ return a paginated list of asset metadata) so that the client only needs to know about a single set of field names.

[djarecka]

I've changed the title because it looks like it's indeed different than #550. But I believe it shows my point I was making about the tests -- the parse_dandi_url(url) does not raise any error but download does

[yarikoptic]

(e.g., by making …/assets/ return a paginated list of asset metadata)

that would be unnecessarily expensive since the idea IIRC was to not return metadata in the listing, and return metadata only for for individual asset endpoint. And since the URL in the original issue description is the one which users would copy-paste from the browser, we better support it in the dandi-cli.

But I am not sure why it has anything to do with what API returns @jwodder? we should use those urls only to parse them up to identify the dandiset and possibly assets (i.e. a singular asset for such a URL) to download. Then based on that parsed information we compose the download urls (possibly reconstructing the original url in this case). So it seems that we just do not correctly identify that it is a URL for a singular asset (and not a path with multiple assets)

[yarikoptic]

But I believe it shows my point I was making about the tests -- the parse_dandi_url(url) does not raise any error but download does

well -- we (tried to?;-)) decoupled the 1. parsing of url, 2. operation on what was parsed, so we do not need to test full combinatorial combination of parse + operation. parsing has its own tests and operations are tested on some set of URLs. In this particular case (didn't check though) I think parsing is buggy and missing test for this usecase.

[djarecka]

@yarikoptic - decoupling is great, but I would still vote for adding any tests for download (or download_generator, navigate_url) for server_type == "api". not really sure what is controversial...

[jwodder]

@yarikoptic I can change the code to get the asset ID directly from the URL when present, but the difference in return structures from navigate_url() depending on the format of the URL will still lead to issues with the ls command and may cause other problems later on up until a proper asset class is created for #447.