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

TransferManager.copy() performs GetObjectMetadata request without passing sourceVersionId #1009

Closed
ejono opened this issue Feb 1, 2017 · 6 comments
Assignees
Labels
bug This issue is a bug.

Comments

@ejono
Copy link

ejono commented Feb 1, 2017

When TransferManager.copy() performs a GetObjectMetadata request, it always requests the metadata for the latest object version (i.e., it doesn't pass in a versionId), even though it's possible for the CopyObjectRequest to specify a sourceVersionId. When constructing the GetObjectMetadataRequest, it should also pass in copyObjectRequest.getSourceVersionId().

There are several bugs that could be caused by not doing this if a non-current version of an object is being copied:

  • If you try to copy an object version when the object's current version has been deleted (i.e., the latest version is a delete marker), the copy will fail when it shouldn't.
  • If the size of the object version being copied differs from that of the current version, the transfer progress will be wrong because it will be using the wrong total size.
  • If the size of the object version being copied is greater than the multipart copy threshold size but the size of the current version is less than the threshold, multipart copy will not be used when it should be (or vice versa).
  • When performing a multipart copy, and the CopyObjectRequest didn't include any "newObjectMetadata" (or the newObjectMetadata contentType wasn't set), the contentType of the new object will be set to the content type of the current object version rather than the version being copied.
@dagnir dagnir added the bug This issue is a bug. label Feb 2, 2017
@dagnir dagnir self-assigned this Feb 2, 2017
@dagnir
Copy link
Contributor

dagnir commented Feb 2, 2017

Thank you for reporting this. I will look into it and get a fix out. We also love pull requests if you'd like to contribute one!

@ejono
Copy link
Author

ejono commented Feb 2, 2017

Thanks for the response! I could certainly fix this myself because it's such a simple change, but I couldn't help but notice that the s3 module is one of the very few that doesn't even seem to have any tests. Are there really no tests for the S3 Java SDK? Should I just submit a PR for the fix itself (just a one-liner)?

@dagnir
Copy link
Contributor

dagnir commented Feb 2, 2017

Yes I apologize about the lack of tests. We have a large number of them internally but we don't currently publish them publicly, but we're working on it. Feel free to just submit the change to the TransferManager code and I can take care writing the tests and making sure everything is good.

@dagnir
Copy link
Contributor

dagnir commented Feb 3, 2017

I have a fix for this upstream. It should be available in our Monday release next week (2/6).

@ejono
Copy link
Author

ejono commented Feb 3, 2017

Thank you!

@dagnir
Copy link
Contributor

dagnir commented Feb 8, 2017

Hi @ejono, the fix was pushed as part of 1.11.87.

@dagnir dagnir closed this as completed Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

No branches or pull requests

2 participants