Skip to content
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.

Sign up for GitHub

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

Jump to bottom

A discussion on needed /paths properties and metadata #1851

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

A discussion on needed /paths properties and metadata #1851

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c87cc8c
Just a discussion pasted from #1837
yarikoptic Feb 1, 2024
24e1188
First batch of annotations on what we have already or desire
yarikoptic Feb 1, 2024
e3c4500
Forgotten path
yarikoptic Feb 1, 2024
9994f2c
codespell ;)
yarikoptic Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions doc/design/paths-endpoint-webdav.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Discussion on metadata needed for dandidav

Submitted as a PR to avoid spaghetti thread discussion on https://github.com/dandi/dandi-archive/issues/1837#issuecomment-1921864949


@jwodder listed needed properties and metadata fields:

* Asset properties used by dandidav:
* `asset_id` (PRESENT)
* `blob_id`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what for do we need blob_id if we have contentUrl?

Copy link
Member

@jwodder jwodder Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contentUrl returned in .../assets/paths/ responses is a plain S3 URL, and serving it to the user in a web browser would result in the file being downloaded and named with the blob ID. In order for the downloaded file to be named with the asset's filename instead, we need a signed S3 URL, which is obtained via the API download URL in the contentUrl field of the metadata.

EDIT: Sorry, I misinterpreted the question. For blob ID, the code just checks whether blob_id or zarr_id is non-None in order to determine whether the asset is a blob or a Zarr.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so we the actually need result_type (Folder, AssetBlob, AssetZarr) as I suggested in #1837 (comment) .

* `zarr_id`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess zarr_id is needed for traversal etc? or could contentUrl version be used? (probably we do not want to rely on parsing it).

So it feels that we might ask for asset record to include blob_id (although not sure what for yet exactly) and zarr_id, hence

Suggested change
* `zarr_id`
* `zarr_id` (DESIRED)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dandidav does parse S3 contentUrls, as that's necessary in order to discover the name of the S3 bucket without hardcoding it or requiring it to be passed on the command line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so in principle we can just parse out bucket, zarr_id and blob_id from contenturl, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'd rather dandidav not have to make too many assumptions about how our S3 URLs are structured.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. We would only need to be able to construct zarr "http s3" URLs from manifests so it would need to know that , right?
For blobs -- contentUrl should be good enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that asset metadata lists two contentUrls, an unsigned S3 URL and an API download URL of the form https://api.dandiarchive.org/api/assets/{asset_id}/download/.

  • For Zarr assets, dandidav needs the S3 URL in order to know how to query the Zarr's entries from S3.

  • However, for blob assets, dandidav currently uses the API download URL, as it provides a better UX. Specifically, if dandidav were to redirect GET requests for a blob to the blob's unsigned S3 URL, then the browser would save the response to a file named after the last component of the S3 URL, which is (always?) the blob ID. In order to get the downloaded files to be named after the filename portion of the asset's path instead, a signed S3 URL with a Content-Disposition has to be used, and that is acquired by redirecting to the API download URL, which in turn redirects to such a signed S3 URL.

* `path` (PRESENT)
* `size` (PRESENT)
* `created` (GOOD TO HAVE)
* `modified` (SUGGESTED to be included)
* Note: It might be more accurate to use the `blobDateModified` metadata field for this instead; cf. dandi/dandidav#17

* Asset metadata used by dandidav:
* `encodingFormat` (blobs only) (STRONGLY DESIRED but not REQUIRED)
* `contentUrl` (API download URL for blobs, S3 URL for Zarrs) (INCLUDED)
* `digest["dandi:dandi-etag"]` (blobs only)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what this one for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Etag (reported in PROPFIND responses)