-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix naming of path
and path_prefix
parameter names
#1562
Comments
FWIW I just ran into this issue with having multiple assets listed for a path whenever I expected one given the name ( ❯ curl --silent -X 'GET' 'https://api.dandiarchive.org/api/dandisets/000029/versions/draft/assets/?path=test1234&metadata=false' -H 'accept: application/json' | jq .
{
"count": 2,
"next": null,
"previous": null,
"results": [
{
"asset_id": "bb612483-0a97-4309-9763-7fb518265c70",
"blob": "0317cf5a-4047-4e19-aae1-4f7b7434d2d7",
"zarr": null,
"path": "test12345",
"size": 9,
"created": "2023-03-16T15:50:07.184996Z",
"modified": "2023-03-17T15:41:18.378517Z"
},
{
"asset_id": "28155d24-9342-48bc-a3a7-b69191e86e13",
"blob": "0317cf5a-4047-4e19-aae1-4f7b7434d2d7",
"zarr": null,
"path": "test1234",
"size": 9,
"created": "2024-01-31T17:27:07.621838Z",
"modified": "2024-01-31T17:27:07.641100Z"
}
]
} |
@waxlamp what do you think we should do about this? Might be worth bundling with a whatever solution we come up with for and may be also related fixes such as for a good old |
I don't think it needs to be a breaking change. We can just add support for the "correct" query parameters and mark the old ones as deprecated while still supporting them, can't we? |
Correct. What names do you suggest to come to replace these current ones? |
@yarikoptic Why wouldn't we just name the new parameters "path_prefix" and "path" as suggested in the original comment? |
I missed the fact that we are talking about two different endpoints -- for some reason I thought they both belong to the same end-point. Indeed -- good idea then to just fix it by renaming and having a (potentially very long) deprecation cycle. WDYT @waxlamp ? |
You're right. This is a good approach. I'll put something together. |
The meanings of these two parameters are reversed in the following places:
dandisets_versions_assets_list
, wherepath
should be calledpath_prefix
:dandi-archive/dandiapi/api/views/asset.py
Line 388 in 83d3a10
dandisets_versions_assets_paths
, wherepath_prefix
should be calledpath
:dandi-archive/dandiapi/api/views/asset.py
Line 425 in 83d3a10
It would also be good to improve the Swagger documentation for these endpoints to better explain what they do and what the parameters mean. In particular, the usage of the
glob
parameter fordandisets_versions_assets_list
.This is a breaking change. We will need to update the web application and the CLI to match the new (correct) parameter names.
See #444 for original discussion and context.
The text was updated successfully, but these errors were encountered: