From 8e3e220d065f75302eb789287674fe2de9e4839a Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Tue, 21 Jan 2025 10:03:48 -0500 Subject: [PATCH 1/4] Do not set default checksum if request_checksum_calculation config is set to when_required --- s3transfer/manager.py | 6 +- tests/functional/test_upload.py | 159 ++++++++++++++++++++++++-------- 2 files changed, 128 insertions(+), 37 deletions(-) diff --git a/s3transfer/manager.py b/s3transfer/manager.py index 23b6f305..8e1cdf06 100644 --- a/s3transfer/manager.py +++ b/s3transfer/manager.py @@ -519,7 +519,11 @@ def _validate_all_known_args(self, actual, allowed): ) def _add_operation_defaults(self, extra_args): - set_default_checksum_algorithm(extra_args) + if ( + self.client.meta.config.request_checksum_calculation + == "when_supported" + ): + set_default_checksum_algorithm(extra_args) def _submit_transfer( self, call_args, submission_task_cls, extra_main_kwargs=None diff --git a/tests/functional/test_upload.py b/tests/functional/test_upload.py index 4298680a..42aabd0c 100644 --- a/tests/functional/test_upload.py +++ b/tests/functional/test_upload.py @@ -19,6 +19,7 @@ from botocore.awsrequest import AWSRequest from botocore.client import Config from botocore.exceptions import ClientError +from botocore.httpchecksum import DEFAULT_CHECKSUM_ALGORITHM from botocore.stub import ANY from s3transfer.manager import TransferConfig, TransferManager @@ -143,7 +144,7 @@ class TestNonMultipartUpload(BaseUploadTest): __test__ = True def add_put_object_response_with_default_expected_params( - self, extra_expected_params=None, bucket=None + self, extra_expected_params=None, bucket=None, include_checksum=True ): if bucket is None: bucket = self.bucket @@ -152,8 +153,9 @@ def add_put_object_response_with_default_expected_params( 'Body': ANY, 'Bucket': bucket, 'Key': self.key, - 'ChecksumAlgorithm': 'CRC32', } + if include_checksum: + expected_params["ChecksumAlgorithm"] = DEFAULT_CHECKSUM_ALGORITHM if extra_expected_params: expected_params.update(extra_expected_params) upload_response = self.create_stubbed_responses()[0] @@ -187,6 +189,46 @@ def test_upload_with_checksum(self): self.assert_expected_client_calls_were_correct() self.assert_put_object_body_was_correct() + def test_upload_with_default_checksum_when_supported(self): + # Reset client to configure `request_checksum_calculation` to "when_supported". + self.reset_stubber_with_new_client( + {'config': Config(request_checksum_calculation="when_supported")} + ) + self.client.meta.events.register( + 'before-parameter-build.s3.*', self.collect_body + ) + self._manager = TransferManager(self.client, self.config) + + self.add_put_object_response_with_default_expected_params( + include_checksum=True + ) + future = self.manager.upload( + self.filename, self.bucket, self.key, self.extra_args + ) + future.result() + self.assert_expected_client_calls_were_correct() + self.assert_put_object_body_was_correct() + + def test_upload_with_default_checksum_when_required(self): + # Reset client to configure `request_checksum_calculation` to "when_required". + self.reset_stubber_with_new_client( + {'config': Config(request_checksum_calculation="when_required")} + ) + self.client.meta.events.register( + 'before-parameter-build.s3.*', self.collect_body + ) + self._manager = TransferManager(self.client, self.config) + + self.add_put_object_response_with_default_expected_params( + include_checksum=False + ) + future = self.manager.upload( + self.filename, self.bucket, self.key, self.extra_args + ) + future.result() + self.assert_expected_client_calls_were_correct() + self.assert_put_object_body_was_correct() + def test_upload_with_s3express_default_checksum(self): s3express_bucket = "mytestbucket--usw2-az6--x-s3" self.assertFalse("ChecksumAlgorithm" in self.extra_args) @@ -370,9 +412,7 @@ def assert_upload_part_bodies_were_correct(self): self.assertEqual(self.sent_bodies, expected_contents) def add_create_multipart_response_with_default_expected_params( - self, - extra_expected_params=None, - bucket=None, + self, extra_expected_params=None, bucket=None, include_checksum=True ): if bucket is None: bucket = self.bucket @@ -380,8 +420,9 @@ def add_create_multipart_response_with_default_expected_params( expected_params = { 'Bucket': bucket, 'Key': self.key, - 'ChecksumAlgorithm': 'CRC32', } + if include_checksum: + expected_params["ChecksumAlgorithm"] = DEFAULT_CHECKSUM_ALGORITHM if extra_expected_params: expected_params.update(extra_expected_params) response = self.create_stubbed_responses()[0] @@ -389,9 +430,7 @@ def add_create_multipart_response_with_default_expected_params( self.stubber.add_response(**response) def add_upload_part_responses_with_default_expected_params( - self, - extra_expected_params=None, - bucket=None, + self, extra_expected_params=None, bucket=None, include_checksum=True ): if bucket is None: bucket = self.bucket @@ -406,50 +445,48 @@ def add_upload_part_responses_with_default_expected_params( 'UploadId': self.multipart_id, 'Body': ANY, 'PartNumber': i + 1, - 'ChecksumAlgorithm': 'CRC32', } + if include_checksum: + expected_params["ChecksumAlgorithm"] = ( + DEFAULT_CHECKSUM_ALGORITHM + ) if extra_expected_params: expected_params.update(extra_expected_params) - name = expected_params['ChecksumAlgorithm'] - checksum_member = f'Checksum{name.upper()}' - response = upload_part_response['service_response'] - response[checksum_member] = f'sum{i+1}==' + # If ChecksumAlgorithm is in expected parameters, add checksum to the response + checksum_algorithm = expected_params.get('ChecksumAlgorithm') + if checksum_algorithm: + checksum_member = f'Checksum{checksum_algorithm.upper()}' + response = upload_part_response['service_response'] + response[checksum_member] = f'sum{i+1}==' upload_part_response['expected_params'] = expected_params self.stubber.add_response(**upload_part_response) def add_complete_multipart_response_with_default_expected_params( - self, - extra_expected_params=None, - bucket=None, + self, extra_expected_params=None, bucket=None, include_checksum=True ): if bucket is None: bucket = self.bucket + num_parts = 3 + parts = [] + for part_num in range(1, num_parts + 1): + part = { + 'ETag': f'etag-{part_num}', + 'PartNumber': part_num, + } + if include_checksum: + part[f"Checksum{DEFAULT_CHECKSUM_ALGORITHM}"] = ( + f"sum{part_num}==" + ) + parts.append(part) + expected_params = { 'Bucket': bucket, 'Key': self.key, 'UploadId': self.multipart_id, - 'MultipartUpload': { - 'Parts': [ - { - 'ETag': 'etag-1', - 'PartNumber': 1, - 'ChecksumCRC32': 'sum1==', - }, - { - 'ETag': 'etag-2', - 'PartNumber': 2, - 'ChecksumCRC32': 'sum2==', - }, - { - 'ETag': 'etag-3', - 'PartNumber': 3, - 'ChecksumCRC32': 'sum3==', - }, - ] - }, + 'MultipartUpload': {'Parts': parts}, } if extra_expected_params: expected_params.update(extra_expected_params) @@ -761,3 +798,53 @@ def test_multipart_upload_with_full_object_checksum_args(self): ) future.result() self.assert_expected_client_calls_were_correct() + + def test_multipart_upload_with_default_checksum_when_supported(self): + # Reset client to configure `request_checksum_calculation` to "when_supported". + self.reset_stubber_with_new_client( + {'config': Config(request_checksum_calculation="when_supported")} + ) + self.client.meta.events.register( + 'before-parameter-build.s3.*', self.collect_body + ) + self._manager = TransferManager(self.client, self.config) + + self.add_create_multipart_response_with_default_expected_params( + include_checksum=True + ) + self.add_upload_part_responses_with_default_expected_params( + include_checksum=True + ) + self.add_complete_multipart_response_with_default_expected_params() + + future = self.manager.upload( + self.filename, self.bucket, self.key, self.extra_args + ) + future.result() + self.assert_expected_client_calls_were_correct() + + def test_multipart_upload_with_default_checksum_when_required(self): + # Reset client to configure `request_checksum_calculation` to "when_required". + self.reset_stubber_with_new_client( + {'config': Config(request_checksum_calculation="when_required")} + ) + self.client.meta.events.register( + 'before-parameter-build.s3.*', self.collect_body + ) + self._manager = TransferManager(self.client, self.config) + + self.add_create_multipart_response_with_default_expected_params( + include_checksum=False + ) + self.add_upload_part_responses_with_default_expected_params( + include_checksum=False + ) + self.add_complete_multipart_response_with_default_expected_params( + include_checksum=False + ) + + future = self.manager.upload( + self.filename, self.bucket, self.key, self.extra_args + ) + future.result() + self.assert_expected_client_calls_were_correct() From ac063df6010825c25aa5092765db38537e58706b Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Tue, 21 Jan 2025 10:15:17 -0500 Subject: [PATCH 2/4] Add changelog entry --- .changes/next-release/bugfix-upload-80266.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/bugfix-upload-80266.json diff --git a/.changes/next-release/bugfix-upload-80266.json b/.changes/next-release/bugfix-upload-80266.json new file mode 100644 index 00000000..dd8c3852 --- /dev/null +++ b/.changes/next-release/bugfix-upload-80266.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "upload", + "description": "Do not set a default checksum if the ``request_checksum_calculation`` config is set to ``when_required``. Fixes `boto/s3transfer#327 `__." +} From daef2d9c1e078064fdfaae2f77c99840308b2a21 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Tue, 21 Jan 2025 10:22:12 -0500 Subject: [PATCH 3/4] fix changelog entry issue link --- .changes/next-release/bugfix-upload-80266.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/next-release/bugfix-upload-80266.json b/.changes/next-release/bugfix-upload-80266.json index dd8c3852..f34fa813 100644 --- a/.changes/next-release/bugfix-upload-80266.json +++ b/.changes/next-release/bugfix-upload-80266.json @@ -1,5 +1,5 @@ { "type": "bugfix", "category": "upload", - "description": "Do not set a default checksum if the ``request_checksum_calculation`` config is set to ``when_required``. Fixes `boto/s3transfer#327 `__." + "description": "Do not set a default checksum if the ``request_checksum_calculation`` config is set to ``when_required``. Fixes `boto/s3transfer#327 `__." } From ce2ab39afa6250e0156a2863913a09f629cfa31a Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Tue, 21 Jan 2025 10:29:34 -0500 Subject: [PATCH 4/4] update changelog wording --- .changes/next-release/bugfix-upload-80266.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changes/next-release/bugfix-upload-80266.json b/.changes/next-release/bugfix-upload-80266.json index f34fa813..f795db04 100644 --- a/.changes/next-release/bugfix-upload-80266.json +++ b/.changes/next-release/bugfix-upload-80266.json @@ -1,5 +1,5 @@ { "type": "bugfix", "category": "upload", - "description": "Do not set a default checksum if the ``request_checksum_calculation`` config is set to ``when_required``. Fixes `boto/s3transfer#327 `__." + "description": "Only set a default checksum if the ``request_checksum_calculation`` config is set to ``when_supported``. Fixes `boto/s3transfer#327 `__." }