Skip to content

Add support for UploadPartCopy on multipart part uploads - #20

Open
Pastalikek65 wants to merge 3 commits into
rclone:masterfrom
Pastalikek65:upload-part-copy
Open

Add support for UploadPartCopy on multipart part uploads#20
Pastalikek65 wants to merge 3 commits into
rclone:masterfrom
Pastalikek65:upload-part-copy

Conversation

@Pastalikek65

Copy link
Copy Markdown

Multipart server-side copies (rclone/rclone#7454) fail against gofakes3: UploadPartCopy sends the copy source in an X-Amz-Copy-Source header and an empty body, but putMultipartUploadPart handled it like a regular part upload - stored the empty body and answered with a bare ETag, which S3 clients can't deserialize as a CopyPartResult.

This adds the copy-source branch to putMultipartUploadPart (mirroring the single-object PUT path), copying the source object or the range from X-Amz-Copy-Source-Range into the part slot through the same streaming/in-memory path as a regular part, and answering with CopyPartResult (200, or 206 for ranged).

What I tested:

  • TestUploadPartCopy and TestUploadPartCopyRange (gofakes3 unit tests, in-memory backend): both fail on master with deserialization failed, received empty response payload and pass with the change.
  • Full go test ./... + go vet on the repo.
  • End to end via rclone: cmd/serve/s3 regression test driving UploadPartCopy with the aws-sdk (the exact request rclone's s3 backend makes for chunked copies) fails against gofakes3 v0.0.7 and passes against this branch. I'll send the rclone side (dep bump + test) once this is merged.

What I did not test: conditional copy headers (x-amz-copy-source-if-match etc.) - the single-object copy path doesn't support them either, so I left them out for parity.

UploadPartCopy sends the copy source in an X-Amz-Copy-Source header and
an empty body, but putMultipartUploadPart treated it like a regular part
upload: it stored the empty body and answered with a bare ETag, so S3
clients hit a deserialization error and multipart server-side copies
always failed.

putMultipartUploadPart now branches on X-Amz-Copy-Source and copies the
source object - or the range from X-Amz-Copy-Source-Range - into the part
slot through the same streaming/in-memory path as a regular part, and
answers with a CopyPartResult (200, or 206 when a range was copied).

Ranged copies resolve the range against the source size from HeadObject
and read the bytes via GetObject, so backends don't need to honor range
requests themselves.
The in-memory s3mem backend never exercised the part-copy path for
backends that implement MultipartBackend: the copy request bypasses
AddPart entirely and lands in multipart.UploadPart via the streaming
wrapper. Covers that dispatch with a round-trip test so a regression
in the streaming copy path is caught without spinning up rclone.
@maximilize

Copy link
Copy Markdown

Checked this against the four points from rclone/rclone#7454: the streaming path and the missing CopyPartResult / range handling are both covered, and splitting storeMultipartUploadPart out keeps the two paths honest.

Two things reproduce against f5d477d.

1. The copy branch sits below the Content-Length read, so a body-less UploadPartCopy answers 411. The new guard is at gofakes3.go:985, but putMultipartUploadPart still parses Content-Length at :961 and returns ErrMissingContentLength at :963, so the comment above the guard ("Handle it before any body/size parsing") does not hold yet. Raw request with no Content-Length header:

PUT /<bucket>/dst.txt?uploadId=<id>&partNumber=1 HTTP/1.1
Host: 127.0.0.1:NNNNN
X-Amz-Copy-Source: /<bucket>/src.txt

→ HTTP/1.1 411 Length Required

This is not reachable from a Go client: net/http always sends Content-Length: 0 on PUT, which is why your tests and the rclone path pass. It is reachable from any client that omits the header, which S3 permits since the request carries no body. Hoisting the guard above :961 fixes it — upload is only fetched at :966, so either that block moves up too, or the branch reads X-Amz-Copy-Source off r.Header directly instead of the parsed meta.

2. A ranged copy answers 206. With w.WriteHeader(http.StatusPartialContent) in front of the encoder:

no range      → 200
bytes=0-49    → 206

S3 answers 200 with a CopyPartResult body in both cases. The range is an input parameter here, not a partial response — the returned body is complete, and copyObject in this file returns 200 for the single-object equivalent.

Two review findings against f5d477d:

- UploadPartCopy was parsed as a body-bearing request: the
  Content-Length guard ran before the copy branch, so a request
  without a Content-Length header (valid, since the part is copied
  server-side and carries no body) failed with 411. The copy branch
  now runs first and reads X-Amz-Copy-Source from the request
  headers directly.
- A ranged UploadPartCopy answered 206 Partial Content. S3 answers
  200 with a CopyPartResult body in both cases; the range is an
  input parameter, not a partial response.

Adds tests for both cases.
@Pastalikek65

Copy link
Copy Markdown
Author

Thanks for the thorough review — both findings reproduce against f5d477d exactly as you described, and both are fixed in 328d50b.

1. Body-less UploadPartCopy now works

The copy branch was sitting below the Content-Length read, so a request without a Content-Length header died with 411 before ever reaching it. The branch now runs first and reads X-Amz-Copy-Source straight off r.Header instead of the parsed metadata, so there's no body/size parsing on the copy path at all. Raw request with no Content-Length header now answers 200 and stores the part.

2. Ranged copy answers 200

The 206 Partial Content write is gone. A ranged UploadPartCopy now answers 200 with a CopyPartResult body, same as the single-object copy path. The range stays an input parameter, not a response status.

Added TestUploadPartCopyWithoutContentLength (raw socket request, no Content-Length header → 200) and TestUploadPartCopyRangeStatusOK (ranged request → 200 + CopyPartResult body). Both failed before the fix, and the full test suite (go test ./...) and go vet are green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants