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.

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

Add bulk upload #28991

Merged
merged 3 commits into from
Oct 16, 2021
Merged

Add bulk upload #28991

merged 3 commits into from
Oct 16, 2021

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Sep 29, 2021

Fix: #28443

The PR adds a new DAV plugin to handle requests on the /dav/bulk endpoint.

  • The endpoint path is open to discussion.
  • A new capability that the clients can check: bulkupload

Request templates

--boundary_bec011bf527a43d6
X-File-Path: /bulk_upload/boundary_bec011bf527a43d6_2_10/bb0fff48c6c9b4e9.txt
X-File-Mtime: 1632920977
X-File-Md5: f454251fe1ad474229e7e364a31c1e18
Content-Length: 10

This is a file content that ends with a new line.

--boundary_bec011bf527a43d6
X-File-Path: /bulk_upload/boundary_bec011bf527a43d6_2_10/2d5d1ae90a99cdf7.txt
X-File-Mtime: 1632920977
X-File-Md5: 40b0d4f753a86c333625344342a35d2a
Content-Length: 10

This is a file content that does not end with a new line.
--boundary_bec011bf527a43d6--
  • Where bec011bf527a43d6 is a random string
  • Headers and file content are separated by one and only one blank line
  • File content will be read according to their Content-Length header

Response template

{
    "/path/for/file/n/1": "ETag of the written file",
    "/path/for/file/n/2": "ETag of the written file",
    ....
}

Benchmarks

This PR contains some benchmarks written in bash. They are be flexible enough to test different file counts, file sizes, bandwidth and concurrency.

@artonge artonge added this to the Nextcloud 23 milestone Sep 29, 2021
@artonge artonge self-assigned this Sep 29, 2021
@artonge artonge force-pushed the feature/bulk_upload branch from e5185f4 to fd5ad5c Compare September 29, 2021 13:23
@artonge artonge force-pushed the feature/bulk_upload branch 13 times, most recently from 099f95a to 733d015 Compare October 13, 2021 08:13
@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@artonge artonge force-pushed the feature/bulk_upload branch 11 times, most recently from 9fc8fad to a28c72d Compare October 13, 2021 14:49
@skjnldsv skjnldsv added the 3. to review Waiting for reviews label Oct 13, 2021
@nickvergessen

This comment has been minimized.

@artonge artonge force-pushed the feature/bulk_upload branch 2 times, most recently from dc07da1 to 02629da Compare October 14, 2021 11:38
@artonge
Copy link
Contributor Author

artonge commented Oct 14, 2021

Should use a different URL. With this you will break the dav endpoint for a user with userid bulk.

Changed to dav/bulk inspired by chunked upload that sits at dav/upload/...

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Code looks good. Thanks for the tests too! 👍

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Tested with some small adjustment that would need a fix, works good with that otherwise and code also looks nice an clean 👍

apps/dav/lib/BulkUpload/BulkUploadPlugin.php Outdated Show resolved Hide resolved
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code looks good in general, great to see many tests.
I only had minor comments

👍

apps/dav/lib/BulkUpload/BulkUploadPlugin.php Outdated Show resolved Hide resolved
apps/dav/lib/BulkUpload/MultipartRequestParser.php Outdated Show resolved Hide resolved
apps/dav/lib/BulkUpload/MultipartRequestParser.php Outdated Show resolved Hide resolved
apps/dav/lib/BulkUpload/MultipartRequestParser.php Outdated Show resolved Hide resolved
@artonge artonge force-pushed the feature/bulk_upload branch 3 times, most recently from a34b31b to 4180a79 Compare October 15, 2021 09:51
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the feature/bulk_upload branch 6 times, most recently from 79a3c08 to 8dbaeda Compare October 15, 2021 12:17
@skjnldsv skjnldsv requested a review from juliusknorr October 15, 2021 16:55
@skjnldsv
Copy link
Member

Tests failure unrelated

@skjnldsv
Copy link
Member

Once @juliushaertl confirmed, we can get this in 🚀

Signed-off-by: Louis Chemineau <louis@chmn.me>
@juliusknorr
Copy link
Member

Pushed a fix for the failing capabilities test

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 16, 2021
@juliusknorr juliusknorr merged commit 6204c63 into master Oct 16, 2021
@juliusknorr juliusknorr deleted the feature/bulk_upload branch October 16, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: dav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk upload
5 participants