Add no overwrite for sync operation

Added no overwrite for download operation

handled error during download streaming streaming

Improved upload test cases

Updated test cases

Added test cases

Author: Rakshil Modi

awscli/customizations/s3/s3handler.py

@@ -452,8 +452,8 @@ def _warn_if_file_exists_with_no_overwrite(self, fileinfo):
         when the --no-overwrite flag is specified. It checks if the destination file already
         exists on the local filesystem and skips the download if found.
 
-        :type fileinfo: awscli.customizations.s3.fileinfo.FileInfo
-        :param fileinfo: The FileInfo object containing source and destination details
+        :type fileinfo: FileInfo
+        :param fileinfo: The FileInfo object containing transfer details
 
         :rtype: bool
         :returns: True if the file should be skipped (exists and no-overwrite is set),

awscli/customizations/s3/subcommands.py

@@ -684,6 +684,7 @@
     REQUEST_PAYER,
     CHECKSUM_MODE,
     CHECKSUM_ALGORITHM,
+    NO_OVERWRITE,
 ]
 
 
@@ -1072,7 +1073,6 @@ class CpCommand(S3TransferCommand):
             METADATA_DIRECTIVE,
             EXPECTED_SIZE,
             RECURSIVE,
-            NO_OVERWRITE,
         ]
     )
 
@@ -1097,7 +1097,6 @@ class MvCommand(S3TransferCommand):
             METADATA_DIRECTIVE,
             RECURSIVE,
             VALIDATE_SAME_S3_PATHS,
-            NO_OVERWRITE,
         ]
     )
 
@@ -1401,7 +1400,8 @@ def run(self):
             self._client, self._source_client, self.parameters
         )
 
-        s3_transfer_handler = S3TransferHandlerFactory(self.parameters)(
+        params = self._get_s3_handler_params()
+        s3_transfer_handler = S3TransferHandlerFactory(params)(
             self._transfer_manager, result_queue
         )
 
@@ -1510,6 +1510,19 @@ def _map_sse_c_params(self, request_parameters, paths_type):
                 },
             )
 
+    def _get_s3_handler_params(self):
+        """
+        Removing no-overwrite params from sync since file to
+        be synced are already separated out using sync strategy
+        """
+        if self.cmd == 'sync':
+            return {
+                param: val
+                for param, val in self.parameters.items()
+                if param != 'no_overwrite'
+            }
+        return self.parameters
+
 
 # TODO: This class is fairly quirky in the sense that it is both a builder
 #  and a data object. In the future we should make the following refactorings
@@ -1603,7 +1616,7 @@ def _validate_streaming_paths(self):
             self.parameters['is_stream'] = True
             self.parameters['dir_op'] = False
             self.parameters['only_show_errors'] = True
-            self._validate_streaming_no_overwrite_for_download_parameter()
+            self._validate_no_overwrite_for_download_streaming()
 
     def _validate_path_args(self):
         # If we're using a mv command, you can't copy the object onto itself.
@@ -1826,7 +1839,7 @@ def _validate_sse_c_copy_source_for_paths(self):
                     'copy operations.'
                 )
 
-    def _validate_streaming_no_overwrite_for_download_parameter(self):
+    def _validate_no_overwrite_for_download_streaming(self):
         """
         Validates that no-overwrite parameter is not used with streaming downloads.
 

tests/functional/s3/test_cp_command.py

@@ -315,13 +315,7 @@ def test_no_overwrite_flag_when_object_exists_on_target(self):
         # Set up the response to simulate a PreconditionFailed error
         self.http_response.status_code = 412
         self.parsed_responses = [
-            {
-                'Error': {
-                    'Code': 'PreconditionFailed',
-                    'Message': 'At least one of the pre-conditions you specified did not hold',
-                    'Condition': 'If-None-Match',
-                }
-            }
+            self.precondition_failed_error_response(),
         ]
         self.run_cmd(cmdline, expected_rc=0)
         # Verify PutObject was attempted with IfNoneMatch
@@ -367,13 +361,7 @@ def test_no_overwrite_flag_multipart_upload_when_object_exists_on_target(
             {'UploadId': 'foo'},  # CreateMultipartUpload response
             {'ETag': '"foo-1"'},  # UploadPart response
             {'ETag': '"foo-2"'},  # UploadPart response
-            {
-                'Error': {
-                    'Code': 'PreconditionFailed',
-                    'Message': 'At least one of the pre-conditions you specified did not hold',
-                    'Condition': 'If-None-Match',
-                }
-            },  # PreconditionFailed error for CompleteMultipart Upload
+            self.precondition_failed_error_response(),  # PreconditionFailed error for CompleteMultipart Upload
             {},  # AbortMultipartUpload response
         ]
         # Checking for success as file is skipped
@@ -1496,6 +1484,15 @@ def test_streaming_download_error(self):
         )
         self.assertIn(error_message, stderr)
 
+    def test_no_overwrite_cannot_be_used_with_streaming_download(self):
+        command = "s3 cp s3://bucket/streaming.txt - --no-overwrite"
+        _, stderr, _ = self.run_cmd(command, expected_rc=252)
+        error_message = (
+            "--no-overwrite parameter is not supported for "
+            "streaming downloads"
+        )
+        self.assertIn(error_message, stderr)
+
 
 class TestCpCommandWithRequesterPayer(BaseCPCommandTest):
     def setUp(self):

tests/functional/s3/test_mv_command.py

@@ -336,13 +336,7 @@ def test_mv_no_overwrite_flag_when_object_exists_on_target(self):
         # Set up the response to simulate a PreconditionFailed error
         self.http_response.status_code = 412
         self.parsed_responses = [
-            {
-                'Error': {
-                    'Code': 'PreconditionFailed',
-                    'Message': 'At least one of the pre-conditions you specified did not hold',
-                    'Condition': 'If-None-Match',
-                }
-            }
+            self.precondition_failed_error_response(),
         ]
         self.run_cmd(cmdline, expected_rc=0)
         # Verify PutObject was attempted with IfNoneMatch
@@ -392,13 +386,7 @@ def test_mv_no_overwrite_flag_multipart_upload_when_object_exists_on_target(
             {'UploadId': 'foo'},  # CreateMultipartUpload response
             {'ETag': '"foo-1"'},  # UploadPart response
             {'ETag': '"foo-2"'},  # UploadPart response
-            {
-                'Error': {
-                    'Code': 'PreconditionFailed',
-                    'Message': 'At least one of the pre-conditions you specified did not hold',
-                    'Condition': 'If-None-Match',
-                }
-            },  # CompleteMultipartUpload response
+            self.precondition_failed_error_response(),  # CompleteMultipartUpload response
             {},  # Abort Multipart
         ]
         self.run_cmd(cmdline, expected_rc=0)

tests/functional/s3/test_sync_command.py

@@ -540,6 +540,91 @@ def test_download_with_checksum_mode_crc64nvme(self):
             ('ChecksumMode', 'ENABLED'), self.operations_called[1][1].items()
         )
 
+    def test_sync_upload_with_no_overwrite_when_file_does_not_exist_at_destination(
+        self,
+    ):
+        self.files.create_file("new_file.txt", "mycontent")
+        self.parsed_responses = [
+            self.list_objects_response(['file.txt']),
+            self.get_object_response(),
+        ]
+        cmdline = (
+            f'{self.prefix} {self.files.rootdir} s3://bucket --no-overwrite'
+        )
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 2)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+        self.assertEqual(self.operations_called[1][0].name, 'PutObject')
+        self.assertEqual(self.operations_called[1][1]['Key'], 'new_file.txt')
+
+    def test_sync_upload_with_no_overwrite_when_file_exists_at_destination(
+        self,
+    ):
+        self.files.create_file("new_file.txt", "mycontent")
+        self.parsed_responses = [
+            self.list_objects_response(['new_file.txt']),
+        ]
+        cmdline = (
+            f'{self.prefix} {self.files.rootdir} s3://bucket --no-overwrite'
+        )
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 1)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+
+    def test_sync_download_with_no_overwrite_file_not_exists_at_destination(
+        self,
+    ):
+        self.parsed_responses = [
+            self.list_objects_response(['new_file.txt']),
+            self.get_object_response(),
+        ]
+        cmdline = (
+            f'{self.prefix} s3://bucket/ {self.files.rootdir} --no-overwrite'
+        )
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 2)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+        self.assertEqual(self.operations_called[1][0].name, 'GetObject')
+        local_file_path = os.path.join(self.files.rootdir, 'new_file.txt')
+        self.assertTrue(os.path.exists(local_file_path))
+
+    def test_sync_download_with_no_overwrite_file_exists_at_destination(self):
+        self.files.create_file('file.txt', 'My content')
+        self.parsed_responses = [
+            self.list_objects_response(['file.txt']),
+        ]
+        cmdline = (
+            f'{self.prefix} s3://bucket/ {self.files.rootdir} --no-overwrite'
+        )
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 1)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+
+    def test_sync_copy_with_no_overwrite_file_not_exists_at_destination(self):
+        self.parsed_responses = [
+            self.list_objects_response(['new_file.txt']),
+            self.list_objects_response(['file1.txt']),
+            self.copy_object_response(),
+        ]
+        cmdline = f'{self.prefix} s3://bucket/ s3://bucket2/ --no-overwrite'
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 3)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+        self.assertEqual(self.operations_called[1][0].name, 'ListObjectsV2')
+        self.assertEqual(self.operations_called[2][0].name, 'CopyObject')
+        self.assertEqual(self.operations_called[2][1]['Key'], 'new_file.txt')
+
+    def test_sync_copy_with_no_overwrite_file_exists_at_destination(self):
+        self.parsed_responses = [
+            self.list_objects_response(['new_file.txt']),
+            self.list_objects_response(['new_file.txt', 'file1.txt']),
+        ]
+        cmdline = f'{self.prefix} s3://bucket/ s3://bucket2/ --no-overwrite'
+        self.run_cmd(cmdline, expected_rc=0)
+        self.assertEqual(len(self.operations_called), 2)
+        self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2')
+        self.assertEqual(self.operations_called[1][0].name, 'ListObjectsV2')
+
 
 class TestSyncSourceRegion(BaseS3CLIRunnerTest):
     def test_respects_source_region(self):

tests/unit/customizations/s3/test_s3handler.py

@@ -12,6 +12,7 @@
 # language governing permissions and limitations under the License.
 import os
 
+from botocore.exceptions import ClientError
 from s3transfer.manager import TransferManager
 
 from awscli.compat import queue
@@ -679,38 +680,39 @@ def test_submit_move_adds_delete_source_subscriber(self):
         for i, actual_subscriber in enumerate(actual_subscribers):
             self.assertIsInstance(actual_subscriber, ref_subscribers[i])
 
-    def test_submit_with_no_overwrite_flag_when_file_exists_at_destination(
-        self,
-    ):
-        # Setting up the CLI params with no_overwrite flag as True
+    def test_skip_download_when_no_overwrite_and_file_exists(self):
+        """Testing warn_if_file_exists_with_no_overwrite handler when file exists at destination"""
         self.cli_params['no_overwrite'] = True
         fileinfo = self.create_file_info(self.key)
-        # Mocking os.path.exists to simulate that file already exists
         with mock.patch('os.path.exists', return_value=True):
-            # Submitting download request
             future = self.transfer_request_submitter.submit(fileinfo)
-            # Asserting that the future is None, as the file already exists
-            self.assertIsNone(future)
-            # Asserting that no download happened
-            self.assert_no_downloads_happened()
 
-    def test_submit_with_no_overwrite_flag_when_file_does_not_exist_at_destination(
-        self,
-    ):
-        # Setting up the CLI params with no_overwrite flag as True
+        # Result Queue should be empty because it was specified to ignore no-overwrite warnings.
+        self.assertTrue(self.result_queue.empty())
+        # The transfer should be skipped, so future should be None
+        self.assertIsNone(future)
+        self.assert_no_downloads_happened()
+
+    def test_proceed_download_when_no_overwrite_and_file_not_exists(self):
+        """Testing warn_if_file_exists_with_no_overwrite handler when file does not exist at destination"""
         self.cli_params['no_overwrite'] = True
         fileinfo = self.create_file_info(self.key)
-        # Mocking os.path.exists to return False, to simulate that file does not exist
         with mock.patch('os.path.exists', return_value=False):
-            # Submitting download request
             future = self.transfer_request_submitter.submit(fileinfo)
-            # Asserting that the future is the same object returned by transfer_manager.download
-            # This confirms that download was actually initiated
-            self.assertIs(self.transfer_manager.download.return_value, future)
-            # Asserting that download happened
-            self.assertEqual(
-                len(self.transfer_manager.download.call_args_list), 1
-            )
+        # The transfer should proceed, so future should be the transfer manager's return value
+        self.assertIs(self.transfer_manager.download.return_value, future)
+        # And download should have happened
+        self.assertEqual(len(self.transfer_manager.download.call_args_list), 1)
+
+    def test_warn_if_file_exists_without_no_overwrite_flag(self):
+        self.cli_params['no_overwrite'] = False
+        fileinfo = self.create_file_info(self.key)
+        with mock.patch('os.path.exists', return_value=True):
+            future = self.transfer_request_submitter.submit(fileinfo)
+        # The transfer should proceed, so future should be the transfer manager's return value
+        self.assertIs(self.transfer_manager.download.return_value, future)
+        # And download should have happened
+        self.assertEqual(len(self.transfer_manager.download.call_args_list), 1)
 
 
 class TestCopyRequestSubmitter(BaseTransferRequestSubmitterTest):
@@ -957,6 +959,77 @@ def test_submit_move_adds_delete_source_subscriber(self):
         for i, actual_subscriber in enumerate(actual_subscribers):
             self.assertIsInstance(actual_subscriber, ref_subscribers[i])
 
+    def test_skip_copy_with_no_overwrite_and_zero_byte_file_exists(self):
+        """Testing warn_if_zero_byte_file_exists_with_no_overwrite handler when zero byte exists"""
+        self.cli_params['no_overwrite'] = True
+        fileinfo = FileInfo(
+            src=self.source_bucket + "/" + self.source_key,
+            dest=self.bucket + "/" + self.key,
+            operation_name='copy',
+            size=0,
+            source_client=mock.Mock(),
+        )
+        fileinfo.source_client.head_object.return_value = {}
+        future = self.transfer_request_submitter.submit(fileinfo)
+        # The transfer should be skipped, so future should be None
+        self.assertIsNone(future)
+        # Result Queue should be empty because it was specified to ignore no-overwrite warnings.
+        self.assertTrue(self.result_queue.empty())
+        self.assertEqual(len(self.transfer_manager.copy.call_args_list), 0)
+
+    def test_proceed_copy_with_no_overwrite_and_zero_byte_file_does_not_exist(
+        self,
+    ):
+        """Testing warn_if_zero_byte_file_exists_with_no_overwrite handler when zero byte does not exist"""
+        self.cli_params['no_overwrite'] = True
+        fileinfo = FileInfo(
+            src=self.source_bucket + "/" + self.source_key,
+            dest=self.bucket + "/" + self.key,
+            operation_name='copy',
+            size=0,
+            source_client=mock.Mock(),
+        )
+        fileinfo.source_client.head_object.side_effect = ClientError(
+            {'Error': {'Code': '404', 'Message': 'Not Found'}},
+            'HeadObject',
+        )
+        future = self.transfer_request_submitter.submit(fileinfo)
+        # The transfer should proceed, so future should be the transfer manager's return value
+        self.assertIs(self.transfer_manager.copy.return_value, future)
+        self.assertEqual(len(self.transfer_manager.copy.call_args_list), 1)
+
+    def test_proceed_copy_with_no_overwrite_for_non_zero_byte_file(self):
+        self.cli_params['no_overwrite'] = True
+        fileinfo = FileInfo(
+            src=self.source_bucket + "/" + self.source_key,
+            dest=self.bucket + "/" + self.key,
+            operation_name='copy',
+            size=100,
+            source_client=mock.Mock(),
+        )
+        future = self.transfer_request_submitter.submit(fileinfo)
+        # The transfer should proceed, so future should be the transfer manager's return value
+        self.assertIs(self.transfer_manager.copy.return_value, future)
+        self.assertEqual(len(self.transfer_manager.copy.call_args_list), 1)
+        # Head should not be called when no_overwrite is false
+        fileinfo.source_client.head_object.assert_not_called()
+
+    def test_file_exists_without_no_overwrite(self):
+        self.cli_params['no_overwrite'] = False
+        fileinfo = FileInfo(
+            src=self.source_bucket + "/" + self.source_key,
+            dest=self.bucket + "/" + self.key,
+            operation_name='copy',
+            size=100,
+            source_client=mock.Mock(),
+        )
+        future = self.transfer_request_submitter.submit(fileinfo)
+        # The transfer should proceed, so future should be the transfer manager's return value
+        self.assertIs(self.transfer_manager.copy.return_value, future)
+        self.assertEqual(len(self.transfer_manager.copy.call_args_list), 1)
+        # Head should not be called when no_overwrite is false
+        fileinfo.source_client.head_object.assert_not_called()
+
 
 class TestUploadStreamRequestSubmitter(BaseTransferRequestSubmitterTest):
     def setUp(self):