Add content length to PUT GCP multipart complete #257
Conversation
This commit fixes the GCP mulipart complete implementation by adding the Content-Length header to the PUT XML requests that happens when the completed parts are empty. GCP is strict about setting the Content-Length header on requests with empty bodies, so previously this would result in a 411 error.
|
@alamb I wasn't able to come up with a good unit test for this. |
|
Thanks @jkosh44 -- I am going to wait to merge this PR for a day or so to let some of the other maintainers double check I got this new repo set up correctly, but I think this PR is ready to merge from my perspective |
src/gcp/client.rs
Outdated
| let result = self | ||
| .request(Method::PUT, path) | ||
| .header(&CONTENT_LENGTH, "0") | ||
| .idempotent(true) | ||
| .do_put() | ||
| .await?; |
There was a problem hiding this comment.
| let result = self | |
| .request(Method::PUT, path) | |
| .header(&CONTENT_LENGTH, "0") | |
| .idempotent(true) | |
| .do_put() | |
| .await?; | |
| let result = self.put(path, PutPayload::new(), Default::default()).await?; |
This will help to avoid any potential divergence in future, and also perhaps be a bit more clear what is going on?
There was a problem hiding this comment.
I've updated to do this, but I also had to update the implementation of self.put to set Content-Length for empty payloads.
There was a problem hiding this comment.
Put already handles this, this is why we don't see this issue with regular empty non-multipart uploads
src/gcp/client.rs
Outdated
| let builder = self.request(Method::PUT, path); | ||
| let builder = if payload.content_length() == 0 { | ||
| builder.header(&CONTENT_LENGTH, "0") | ||
| } else { | ||
| builder | ||
| }; | ||
| let builder = builder |
There was a problem hiding this comment.
This change is unnecessary, with_payload already handles this
There was a problem hiding this comment.
Ah got it, I just reverted this change.
src/gcp/client.rs
Outdated
| // GCS doesn't allow empty multipart uploads | ||
| let result = self | ||
| .request(Method::PUT, path) | ||
| .idempotent(true) | ||
| .do_put() | ||
| .put(path, PutPayload::new(), Default::default()) | ||
| .await?; | ||
| self.multipart_cleanup(path, multipart_id).await?; |
There was a problem hiding this comment.
@tustvold I was also trying to think about how to make this more obvious. Perhaps the following structure is more clear? The comment clarifies that we're falling back to a non-multipart upload. Also moving the PUT after the cleanup makes it clear that the cleanup is not affecting the PUT. What do you think?
// GCS doesn't allow empty multipart uploads, so fallback to regular upload.
self.multipart_cleanup(path, multipart_id).await?;
let result = self
.put(path, PutPayload::new(), Default::default())
.await?;There was a problem hiding this comment.
I don't feel strongly either way
There was a problem hiding this comment.
Ok, I made the change.
|
Thank you all! |
@andrew-pienso if you still have a repro of #278, it would definitely be helpful to double check that this does in fact resolve your issue. |
When I filed this bug, this example here would repro. I played around with this for a good half hour and am admittedly not able to repro this anymore in my environment. |
Which issue does this PR close?
Rationale for this change
Fixes unreported error
What changes are included in this PR?
This commit fixes the GCP mulipart complete implementation by adding the Content-Length header to the PUT XML requests that happens when the completed parts are empty. GCP is strict about setting the Content-Length header on requests with empty bodies, so previously this would result in a 411 error.
Are there any user-facing changes?
Yes, empty multipart uploads don't cause 411 in GCP.