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

Resolved requested changes

NIT change

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),
@@ -464,9 +464,7 @@ def _warn_if_file_exists_with_no_overwrite(self, fileinfo):
         fileout = self._get_fileout(fileinfo)
         if os.path.exists(fileout):
             LOGGER.debug(
-                "Warning: Skipping file %s as it already exists on %s",
-                fileinfo.src,
-                fileinfo.dest,
+                f"warning: skipping {fileinfo.src} -> {fileinfo.dest}, file exists at destination"
             )
             return True
         return False
@@ -543,9 +541,7 @@ def _warn_if_zero_byte_file_exists_with_no_overwrite(self, fileinfo):
         try:
             client.head_object(Bucket=bucket, Key=key)
             LOGGER.debug(
-                "Warning: Skipping file %s as it already exists on %s",
-                fileinfo.src,
-                fileinfo.dest,
+                f"warning: skipping {fileinfo.src} -> {fileinfo.dest}, file exists at destination"
             )
             return True
         except ClientError as e:

awscli/customizations/s3/subcommands.py

@@ -1143,7 +1143,7 @@ class SyncCommand(S3TransferCommand):
             }
         ]
         + TRANSFER_ARGS
-        + [METADATA, COPY_PROPS, METADATA_DIRECTIVE]
+        + [METADATA, COPY_PROPS, METADATA_DIRECTIVE, NO_OVERWRITE]
     )
 
 
@@ -1400,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
         )
 
@@ -1509,6 +1510,16 @@ 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
+        """
+        params = self.parameters.copy()
+        if self.cmd == 'sync':
+            params.pop('no_overwrite', None)
+        return params
+
 
 # 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
@@ -1572,6 +1583,7 @@ def add_paths(self, paths):
         elif len(paths) == 1:
             self.parameters['dest'] = paths[0]
         self._validate_streaming_paths()
+        self._validate_no_overwrite_for_download_streaming()
         self._validate_path_args()
         self._validate_sse_c_args()
         self._validate_not_s3_express_bucket_for_sync()
@@ -1602,7 +1614,6 @@ 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()
 
     def _validate_path_args(self):
         # If we're using a mv command, you can't copy the object onto itself.
@@ -1825,15 +1836,18 @@ 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.
 
         Raises:
             ParamValidationError: If no-overwrite is specified with a streaming download.
         """
-        params = self.parameters
-        if params['paths_type'] == 's3local' and params.get('no_overwrite'):
+        if (
+            self.parameters['is_stream']
+            and self.parameters.get('no_overwrite')
+            and self.parameters['dest'] == '-'
+        ):
             raise ParamValidationError(
                 "--no-overwrite parameter is not supported for "
                 "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
@@ -550,7 +538,7 @@ def test_no_overwrite_flag_on_copy_when_large_object_does_not_exist_on_target(
         # Verify the IfNoneMatch condition was set in the CompleteMultipartUpload request
         self.assertEqual(self.operations_called[5][1]['IfNoneMatch'], '*')
 
-    def test_no_overwrite_flag_on_download_when_small_object_already_exists_at_target(
+    def test_no_overwrite_flag_on_download_when_single_object_already_exists_at_target(
         self,
     ):
         full_path = self.files.create_file('foo.txt', 'existing content')
@@ -566,7 +554,7 @@ def test_no_overwrite_flag_on_download_when_small_object_already_exists_at_targe
         with open(full_path) as f:
             self.assertEqual(f.read(), 'existing content')
 
-    def test_no_overwrite_flag_on_download_when_small_object_does_not_exist_at_target(
+    def test_no_overwrite_flag_on_download_when_single_object_does_not_exist_at_target(
         self,
     ):
         full_path = self.files.full_path('foo.txt')
@@ -584,40 +572,6 @@ def test_no_overwrite_flag_on_download_when_small_object_does_not_exist_at_targe
         with open(full_path) as f:
             self.assertEqual(f.read(), 'foo')
 
-    def test_no_overwrite_flag_on_download_when_large_object_exists_at_target(
-        self,
-    ):
-        full_path = self.files.create_file('foo.txt', 'existing content')
-        cmdline = (
-            f'{self.prefix} s3://bucket/foo.txt {full_path} --no-overwrite'
-        )
-        self.parsed_responses = [
-            self.head_object_response(ContentLength=10 * (1024**2)),
-        ]
-        self.run_cmd(cmdline, expected_rc=0)
-        self.assertEqual(len(self.operations_called), 1)
-        self.assertEqual(self.operations_called[0][0].name, 'HeadObject')
-        with open(full_path) as f:
-            self.assertEqual(f.read(), 'existing content')
-
-    def test_no_overwrite_flag_on_download_when_large_object_does_not_exist_at_target(
-        self,
-    ):
-        full_path = self.files.full_path('foo.txt')
-        cmdline = (
-            f'{self.prefix} s3://bucket/foo.txt {full_path} --no-overwrite'
-        )
-        self.parsed_responses = [
-            self.head_object_response(ContentLength=10 * (1024**2)),
-            self.get_object_response(),
-            self.get_object_response(),
-        ]
-        self.run_cmd(cmdline, expected_rc=0)
-        self.assertEqual(len(self.operations_called), 3)
-        self.assertEqual(self.operations_called[0][0].name, 'HeadObject')
-        self.assertEqual(self.operations_called[1][0].name, 'GetObject')
-        self.assertEqual(self.operations_called[2][0].name, 'GetObject')
-
     def test_dryrun_download(self):
         self.parsed_responses = [self.head_object_response()]
         target = self.files.full_path('file.txt')
@@ -1496,6 +1450,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)
@@ -575,7 +563,7 @@ def test_mv_no_overwrite_flag_when_large_object_does_not_exist_on_target(
         )
         self.assertEqual(self.operations_called[6][0].name, 'DeleteObject')
 
-    def test_no_overwrite_flag_on_mv_download_when_small_object_exists_at_target(
+    def test_no_overwrite_flag_on_mv_download_when_single_object_exists_at_target(
         self,
     ):
         full_path = self.files.create_file('foo.txt', 'existing content')
@@ -591,7 +579,7 @@ def test_no_overwrite_flag_on_mv_download_when_small_object_exists_at_target(
         with open(full_path) as f:
             self.assertEqual(f.read(), 'existing content')
 
-    def test_no_overwrite_flag_on_mv_download_when_small_object_does_not_exist_at_target(
+    def test_no_overwrite_flag_on_mv_download_when_single_object_does_not_exist_at_target(
         self,
     ):
         full_path = self.files.full_path('foo.txt')
@@ -611,42 +599,6 @@ def test_no_overwrite_flag_on_mv_download_when_small_object_does_not_exist_at_ta
         with open(full_path) as f:
             self.assertEqual(f.read(), 'foo')
 
-    def test_no_overwrite_flag_on_mv_download_when_large_object_exists_at_target(
-        self,
-    ):
-        full_path = self.files.create_file('foo.txt', 'existing content')
-        cmdline = (
-            f'{self.prefix} s3://bucket/foo.txt {full_path} --no-overwrite'
-        )
-        self.parsed_responses = [
-            self.head_object_response(ContentLength=10 * (1024**2)),
-        ]
-        self.run_cmd(cmdline, expected_rc=0)
-        self.assertEqual(len(self.operations_called), 1)
-        self.assertEqual(self.operations_called[0][0].name, 'HeadObject')
-        with open(full_path) as f:
-            self.assertEqual(f.read(), 'existing content')
-
-    def test_no_overwrite_flag_on_mv_download_when_large_object_does_not_exist_at_target(
-        self,
-    ):
-        full_path = self.files.full_path('foo.txt')
-        cmdline = (
-            f'{self.prefix} s3://bucket/foo.txt {full_path} --no-overwrite'
-        )
-        self.parsed_responses = [
-            self.head_object_response(ContentLength=10 * (1024**2)),
-            self.get_object_response(),
-            self.get_object_response(),
-            self.delete_object_response(),
-        ]
-        self.run_cmd(cmdline, expected_rc=0)
-        self.assertEqual(len(self.operations_called), 4)
-        self.assertEqual(self.operations_called[0][0].name, 'HeadObject')
-        self.assertEqual(self.operations_called[1][0].name, 'GetObject')
-        self.assertEqual(self.operations_called[2][0].name, 'GetObject')
-        self.assertEqual(self.operations_called[3][0].name, 'DeleteObject')
-
 
 class TestMvWithCRTClient(BaseCRTTransferClientTest):
     def test_upload_move_using_crt_client(self):

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']),
+            {'ETag': '"c8afdb36c52cf4727836669019e69222"'},
+        ]
+        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):