Skip to content

Conversation

@kdaily
Copy link
Member

@kdaily kdaily commented Dec 10, 2025

Issue #, if available:

Description of changes:

This PR is merging to the s3-no-overwrite feature branch.

Now that AWS S3 supports the IfNoneMatch header on CopyObject operations, we no longer need the more complicated solution that forces multipart upload copies for all copy and move operations. Additionally, we don't need to have a special case for 0-byte object. This PR rolls back some of the changes already added and goes back to allowing IfNoneMatch header to go to CopyObject operations.

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

@kdaily kdaily force-pushed the nooverwrite-copyobject-with-ifnonematch branch from 6183e9e to a61aa8e Compare December 12, 2025 17:29
This commit rolls back changes made to force multipart copies if no
overwrite is specified. This includes checking for 0-length files.
It  also removes the CopyObject argument block list, since IfNoneMatch
is now allowed and was the only argument in the list.
Removes the tests for 0 length files and moves some tests to the
TestNonMultipartCopy class, since CopyObject accepts IfNoneMatch and no
longer needs to force multipart copying.
Update the tests to account for different operations being called, since multipart upload is now not necessary.
@kdaily kdaily force-pushed the nooverwrite-copyobject-with-ifnonematch branch from a61aa8e to 5dce4bc Compare December 12, 2025 17:31
The IfNoneMatch header is set on the CopyObject request
@kdaily kdaily requested a review from a team December 12, 2025 20:51
@kdaily kdaily marked this pull request as ready for review December 12, 2025 20:51
@hssyoo hssyoo self-assigned this Dec 15, 2025
'IfNoneMatch',
]

COPY_OBJECT_ARGS_BLOCKLIST = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete the whole property?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, and then I should also roll back changes to this test:

if arg not in CopySubmissionTask.COPY_OBJECT_ARGS_BLOCKLIST

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 529124d

Comment on lines 143 to 144
# If it is less than threshold and ifNoneMatch is not in parameters
# do a regular copy else do multipart copy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment to reflect code change

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 1652763

Removes the constant and updates the test using it.
Replace with the previously existing comment since the IfNoneMatch header is no
longer used for the copy operation logic.
@kdaily kdaily requested a review from hssyoo December 15, 2025 18:22
s3transfer does not have a 'no overwrite' flag, which is specific to the CLI.
@kdaily kdaily requested a review from aemous December 16, 2025 16:48
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.

2 small nits

The test was doing an upload instead of a (bucket-to-bucket) copy.

Fixed the test expected responses to match what should be called.
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

@kdaily kdaily merged commit ef03551 into aws:s3-no-overwrite Dec 16, 2025
43 of 45 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