Skip to content

Support Additional Parameters for S3 UploadPartCopy #13105

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 36 commits into from
Sep 10, 2025
Merged

Support Additional Parameters for S3 UploadPartCopy #13105

merged 36 commits into from
Sep 10, 2025
+1,009 −10

Conversation

aidehn
Copy link
Contributor

@aidehn aidehn commented Sep 5, 2025 •
edited
Loading

Motivation

Resolves #13106

  • Improve our parity with S3's UploadPartCopy functionality. This was achieved by adding support for the following conditionals: CopySourceIfMatch, CopySourceIfModifiedSince, CopySourceIfNoneMatch and CopySourceIfUnmodifiedSince

Changes

  • Updated the S3Provider.upload_part_copy method to handle the above mentioned conditions.
  • Created a utility which handles which precondition is raised when the method fails.
  • Added parity testing for the different possible failures and paths for combinations of the different conditions.
  • Note: Not every combination is accounted for, I mainly prioritised single condition usage as well as specific cases outlined in the documentation https://docs.aws.amazon.com/AmazonS3/latest/API/API_UploadPartCopy.html

Testing

  • This was tested using parity tests for the new functionality.

Future Changes

  • Additional parity tests to test the new functionality against older functionality e.g. with CopySource.
  • Create a multi-part upload fixture to handle repeated logic within the tests (creating a bucket -> uploading an object -> creating the multi-part upload)
  • Not all the different combinations are tested for / accounted for, in the future I would like to add more test coverage for these different cases.

localstack-bot
localstack-bot reviewed Sep 5, 2025
View reviewed changes
Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@localstack-bot
Copy link
Contributor

localstack-bot commented Sep 5, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@aidehn
Copy link
Contributor Author

aidehn commented Sep 5, 2025

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Sep 5, 2025
@localstack-bot
@aidehn has signed the CLA in #13105
ca8eeb1
@aidehn aidehn force-pushed the aidehn/upload-part-copy-extension branch from 3098db5 to 19a1e96 Compare September 5, 2025 08:16
@alexrashed alexrashed added this to the Playground milestone Sep 5, 2025
@bentsku bentsku modified the milestones: Playground, 4.9 Sep 5, 2025
@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Sep 5, 2025
@bentsku bentsku added the skip-docs Pull request does not require documentation changes label Sep 5, 2025
@aidehn aidehn marked this pull request as ready for review September 5, 2025 11:39
bentsku
bentsku reviewed Sep 5, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

This is very nice, great job! 🚀 I like that you went a step further than what we already had for CopyObject.

Thanks for the great PR description too.

I only have a few comments, I gave it a try and could see that AWS would accept IfMatch and IfNoneMatch values that wouldn't be quoted, and this would fail against this implementation.

I also wonder if we should add the case you hit when testing CopySourceIfUnmodifiedSince, with the date in the future passing?

One other question, what do you mean by:

Additional parity tests to test the new functionality against older functionality e.g. with CopySource.

Once the comments are addressed, I believe we're good to merge! Nice job 🚀

@aidehn
Copy link
Contributor Author

aidehn commented Sep 6, 2025

This is very nice, great job! 🚀 I like that you went a step further than what we already had for CopyObject.

Thanks for the great PR description too.

I only have a few comments, I gave it a try and could see that AWS would accept IfMatch and IfNoneMatch values that wouldn't be quoted, and this would fail against this implementation.

I also wonder if we should add the case you hit when testing CopySourceIfUnmodifiedSince, with the date in the future passing?

One other question, what do you mean by:

Additional parity tests to test the new functionality against older functionality e.g. with CopySource.

Once the comments are addressed, I believe we're good to merge! Nice job 🚀

Thank you for the review @bentsku ! You've been a great help during the process - I'll go ahead and make the following changes:

  • Remove duplicate snapshot information in the newly added tests.
  • Handle non-double quoted values for ETag
  • Handle odd behaviour with CopySourceIfModifiedSince which allows copying if the time is in the future
  • Add positive parity tests for each parameter being supported in this PR, as well as a parity test for the above behaviour

One other question, what do you mean by:

Additional parity tests to test the new functionality against older functionality e.g. with CopySource.

Also I completely misspoke (sorry!), I meant to say test the functionality I added against currently supported fields such as CopySourceRange (not CopySource).

bentsku
bentsku approved these changes Sep 8, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome, thanks for addressing the comments I had, this looks good! I only have a very minor nitpick regarding the _ assignment that is not needed, but otherwise everything is all good!

Thanks for adding so many test cases, the functionality is very well covered!

@bentsku
Copy link
Contributor

bentsku commented Sep 8, 2025

Oops, @aidehn it seems the linting step is failing, could you fix it so the PR would be in a mergeable state? Thank you 🙏

@alexrashed alexrashed modified the milestones: 4.9, 4.8 Sep 10, 2025
@bentsku bentsku merged commit 9395686 into localstack:main Sep 10, 2025
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localstack-bot localstack-bot localstack-bot left review comments

@bentsku bentsku bentsku approved these changes

@k-a-il k-a-il Awaiting requested review from k-a-il k-a-il is a code owner

Assignees
No one assigned
Labels
aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases skip-docs Pull request does not require documentation changes
Projects
None yet
Milestone
4.8
Development

Successfully merging this pull request may close these issues.

feature request: Support Additional Parameters for S3 UploadPartCopy
4 participants
@aidehn @localstack-bot @bentsku @alexrashed