Skip to content

Implemented No-overwrite for uploads using cp command #9569

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

Merged
merged 2 commits into from
Jul 14, 2025

Conversation

rakshil14-2
Copy link

@rakshil14-2 rakshil14-2 commented Jun 24, 2025

Issue : Github Issue #2874

Description of changes:

Customers requested to prevent overwriting of objects. This is done by providing no-overwrite header using ifNoneMatch feature of Amazon S3 which only allows to upload objects that are not present on the bucket. The no-overwrite header provides support to multi-part upload as well as works well with other cp command headers. Customer can be implemented using aws s3 cp <source> <destination> --no-overwrite which allows successful uploads when object with the same name is not present on the destination. However, if the user tries to upload the object which is already present at the target bucket using command line, a warning about skipping the file will be generated and that object is not allowed to upload to S3 bucket.

Testing:
Functional testing is performed to validate the object upload process. This included both scenarios uploading an object when object with same key is already present in the bucket as well as those when the object with same key is not present on bucket. These tests were conducted for both single-part uploads as well as multipart uploads.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from a66905b to 69ea8ad Compare June 24, 2025 22:32
@rakshil14-2 rakshil14-2 marked this pull request as ready for review June 25, 2025 01:55
@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from 41b7423 to 8ad2ec2 Compare June 25, 2025 01:57
@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from e8f7134 to 7985693 Compare July 3, 2025 03:22
Copy link
Member

@kdaily kdaily left a comment

Choose a reason for hiding this comment

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

Not quite finished reviewing. Leaving a few high level/basic changes. Will continue a review of the test cases next.

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from 35e8e12 to 68fb87d Compare July 9, 2025 15:06
@aemous aemous self-requested a review July 9, 2025 16:03
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Looks good overall; requested some changes

@@ -195,6 +195,7 @@ class TransferManager:
+ [
'ChecksumType',
'MpuObjectSize',
'IfNoneMatch',
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're modifying the vendored s3transfer, we should likely be porting the same changes to boto/s3transfer.

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch 2 times, most recently from 427869f to 274113a Compare July 9, 2025 20:55
@@ -859,6 +859,49 @@ def test_can_specify_amount_for_nonseekable_stream(self):
self.assertEqual(nonseekable_fileobj.read(3), 'foo')


class TestRequestParamsMapperNoOverwrite(unittest.TestCase):
def test_set_no_overwrite_param_when_flag_present(self):
# Test when no_overwrite flag is present
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since the test name describes it well, I don't think the inline comments are needed. I would only add them if there's something not captured in the test name or needs to be called out. Same with below tests.

Copy link
Author

Choose a reason for hiding this comment

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

In last two test methods, I had to explicitly mention setting the parameter hence comments are still present and for rest it is removed

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch 2 times, most recently from bf4d8a1 to 0dcab51 Compare July 9, 2025 22:51
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Looking good overall. Left a few minor comments.

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from c096558 to e1c4d4a Compare July 10, 2025 16:12
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Very minor changes and we should be good to merge 👍

Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kdaily kdaily left a comment

Choose a reason for hiding this comment

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

Looks good overall too! Lets clean up the test case names, and remove unneeded docstrings/comments for them if the test name is already descriptive.

If possible, we should move the changes to s3transfer to its own commit.

@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch 3 times, most recently from 50a1015 to c6b558f Compare July 11, 2025 20:58
Rakshil Modi added 2 commits July 11, 2025 15:31
Updated s3Transfer manager

generating original version

Updated args list

updated transfer manager for upload
Removed unwanted comments for test
@rakshil14-2 rakshil14-2 force-pushed the no_overwrite_for_cp_command branch from c6b558f to 6345224 Compare July 11, 2025 22:42
@kdaily kdaily merged commit 54e84b6 into aws:s3-no-overwrite Jul 14, 2025
58 of 60 checks passed
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.

3 participants