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

Bugfix/bulk upload empty files #39221

Merged
merged 1 commit into from Jul 20, 2023
Merged

Bugfix/bulk upload empty files #39221

merged 1 commit into from Jul 20, 2023

Conversation

mgallien
Copy link
Contributor

@mgallien mgallien commented Jul 7, 2023

Summary

Solve long running issue with bulk upload of empty files.
Require also fixes from desktop client see nextcloud/desktop#5871

TODO

  • ...

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Screenshots before/after for front-end changes
  • Documentation (manuals or wiki) has been updated or is not required
  • Backports requested where applicable (ex: critical bugfixes)
    should probably be backported to all maintained releases with bulk upload

@mgallien mgallien requested review from artonge, a team, ArtificialOwl, icewind1991 and come-nc and removed request for a team July 7, 2023 09:27
@mgallien mgallien force-pushed the bugfix/bulkUploadEmptyFiles branch from 7018d98 to fdbcb91 Compare July 7, 2023 09:29
@kesselb
Copy link
Contributor

kesselb commented Jul 8, 2023

Thanks for your patch 👍

The change for the desktop client is important.

I could reproduce the issue with my development setup (nginx proxy -> php container),
but the requests for /remote.php/dav/bulk did not trigger my breakpoint.

proxy_1      | nginx.1     | 2023/07/08 20:33:56 [info] 46#46: *34 client prematurely closed connection, client: 172.19.0.1, server: server.internal, request: "POST /remote.php/dav/bulk HTTP/1.1", host: "server.internal"
proxy_1      | nginx.1     | server.internal 172.19.0.1 - admin [08/Jul/2023:20:33:56 +0000] "POST /remote.php/dav/bulk HTTP/1.1" 400 0 "-" "Mozilla/5.0 (Linux) mirall/3.9.50 (build 16487) (Nextcloud, ubuntu-6.2.0-24-generic ClientArchitecture: x86_64 OsArchitecture: x86_64)" "-"

It seems to me that nginx did not forward the post request without a body to the php container.
Using the build from your pr made it work.

I did some testing with the patched desktop client and the server pr.

  1. Open a terminal
  2. Change directory to your Nextcloud sync folder
  3. Run echo "123" > test61.txt; touch test62.txt; echo "456" > test63.txt

Result:

cat test61.txt 
123
cat test62.txt 
--boundary_.oOo._EfhWH90em6640g8imIdDSkdgh4fStM3o
Content-Length: 4
Content-Type: application/octet-stream
X-File-MD5: d2d362cdc6579390f1c0617d74a7913d
X-File-Mtime: 1688851265
X-File-Path: /test63.txt

456

--boundary_.oOo._EfhWH90em6640g8imIdDSkdgh4fStM3o--

Notification: test63.txt could not be synced due to an error.
And a put request for test63.txt writing the proper content.

I guess stream_get_line with $length = 0 is the problem here,
because the default socket chunk size is used then.

Index: apps/dav/lib/BulkUpload/MultipartRequestParser.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/apps/dav/lib/BulkUpload/MultipartRequestParser.php b/apps/dav/lib/BulkUpload/MultipartRequestParser.php
--- a/apps/dav/lib/BulkUpload/MultipartRequestParser.php	(revision 9bf69115593a9abaedd8cc8d886725253100f3c0)
+++ b/apps/dav/lib/BulkUpload/MultipartRequestParser.php	(date 1688852480282)
@@ -211,7 +211,11 @@
 			throw new BadRequest("Computed md5 hash is incorrect.");
 		}
 
-		$content = stream_get_line($this->stream, $length);
+		if ($length === 0) {
+			$content = '';
+		} else {
+			$content = stream_get_line($this->stream, $length);
+		}
 
 		if ($content === false) {
 			throw new Exception("Fail to read part's content.");

Made it work for me.

Please keep my setup with the reverse proxy in mind. We should probably test the patch without a reverse proxy.
And I don't know if the logic is backwards compatible.

@solracsf solracsf added the 3. to review Waiting for reviews label Jul 9, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Jul 9, 2023
@mgallien mgallien force-pushed the bugfix/bulkUploadEmptyFiles branch from fdbcb91 to 88474c9 Compare July 10, 2023 15:20
@mgallien
Copy link
Contributor Author

@kesselb
I am testing with the docker based setup from Julius that uses a proxy
Tested with your shell snippet and pushed your suggested changes. Everything is working fine now.

@mgallien mgallien requested a review from artonge July 10, 2023 15:22
@mgallien mgallien force-pushed the bugfix/bulkUploadEmptyFiles branch from 88474c9 to 97bfd96 Compare July 19, 2023 12:53
@mgallien mgallien requested a review from come-nc July 19, 2023 12:53
@mgallien mgallien force-pushed the bugfix/bulkUploadEmptyFiles branch from 97bfd96 to eb196b0 Compare July 20, 2023 09:19
@mgallien
Copy link
Contributor Author

/backport to stable24

@mgallien
Copy link
Contributor Author

/backport to stable25

@mgallien
Copy link
Contributor Author

/backport to stable26

@mgallien
Copy link
Contributor Author

/backport to stable27

@mgallien mgallien enabled auto-merge July 20, 2023 09:49
will allow uploading empty files via bulk upload

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>
@mgallien mgallien force-pushed the bugfix/bulkUploadEmptyFiles branch from eb196b0 to 906a661 Compare July 20, 2023 11:41
@mgallien mgallien merged commit ec4ae84 into master Jul 20, 2023
37 checks passed
@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

@mgallien
Copy link
Contributor Author

/backport to stable27

@mgallien
Copy link
Contributor Author

/backport to stable23

@kesselb
Copy link
Contributor

kesselb commented Oct 25, 2023

Please don't forget to review the pr for 24 and check if we also need it: #39508

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants