From f7fa060afeddf20ad7f44ea00516f900587dbd9f Mon Sep 17 00:00:00 2001 From: Dave Rocamora Date: Fri, 7 Sep 2018 14:08:36 -0700 Subject: [PATCH 1/5] s3 commands that list buckets will use ListObjectsV2 --- awscli/customizations/s3/filegenerator.py | 2 +- awscli/customizations/s3/subcommands.py | 4 +-- awscli/customizations/s3/utils.py | 2 +- tests/functional/s3/test_cp_command.py | 14 +++++----- tests/functional/s3/test_rb_command.py | 6 ++-- tests/functional/s3/test_rm_command.py | 2 +- tests/functional/s3/test_sync_command.py | 28 +++++++++---------- .../customizations/s3/test_subcommands.py | 8 +++--- tests/unit/customizations/s3/test_utils.py | 2 +- 9 files changed, 34 insertions(+), 34 deletions(-) diff --git a/awscli/customizations/s3/filegenerator.py b/awscli/customizations/s3/filegenerator.py index 3d802aba15ce..63a66996811e 100644 --- a/awscli/customizations/s3/filegenerator.py +++ b/awscli/customizations/s3/filegenerator.py @@ -318,7 +318,7 @@ def list_objects(self, s3_path, dir_op): yield self._list_single_object(s3_path) else: lister = BucketLister(self._client) - extra_args = self.request_parameters.get('ListObjects', {}) + extra_args = self.request_parameters.get('ListObjectsV2', {}) for key in lister.list_objects(bucket=bucket, prefix=prefix, page_size=self.page_size, extra_args=extra_args): diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index 946acf377611..9488605509ed 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -500,7 +500,7 @@ def _run_main(self, parsed_args, parsed_globals): def _list_all_objects(self, bucket, key, page_size=None, request_payer=None): - paginator = self.client.get_paginator('list_objects') + paginator = self.client.get_paginator('list_objects_v2') paging_args = { 'Bucket': bucket, 'Prefix': key, 'Delimiter': '/', 'PaginationConfig': {'PageSize': page_size} @@ -548,7 +548,7 @@ def _list_all_buckets(self): def _list_all_objects_recursive(self, bucket, key, page_size=None, request_payer=None): - paginator = self.client.get_paginator('list_objects') + paginator = self.client.get_paginator('list_objects_v2') paging_args = { 'Bucket': bucket, 'Prefix': key, 'PaginationConfig': {'PageSize': page_size} diff --git a/awscli/customizations/s3/utils.py b/awscli/customizations/s3/utils.py index b46c3d0019cc..22f915c8cab3 100644 --- a/awscli/customizations/s3/utils.py +++ b/awscli/customizations/s3/utils.py @@ -365,7 +365,7 @@ def list_objects(self, bucket, prefix=None, page_size=None, if extra_args is not None: kwargs.update(extra_args) - paginator = self._client.get_paginator('list_objects') + paginator = self._client.get_paginator('list_objects_v2') pages = paginator.paginate(**kwargs) for page in pages: contents = page.get('Contents', []) diff --git a/tests/functional/s3/test_cp_command.py b/tests/functional/s3/test_cp_command.py index 94218c59501a..751e046dea95 100644 --- a/tests/functional/s3/test_cp_command.py +++ b/tests/functional/s3/test_cp_command.py @@ -133,10 +133,10 @@ def test_operations_used_in_recursive_download(self): cmdline = '%s s3://bucket/key.txt %s --recursive' % ( self.prefix, self.files.rootdir) self.run_cmd(cmdline, expected_rc=0) - # We called ListObjects but had no objects to download, so - # we only have a single ListObjects operation being called. + # We called ListObjectsV2 but had no objects to download, so + # we only have a single ListObjectsV2 operation being called. self.assertEqual(len(self.operations_called), 1, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') def test_website_redirect_ignore_paramfile(self): full_path = self.files.create_file('foo.txt', 'mycontent') @@ -268,7 +268,7 @@ def test_recursive_glacier_download_with_force_glacier(self): % (self.prefix, self.files.rootdir) self.run_cmd(cmdline, expected_rc=0) self.assertEqual(len(self.operations_called), 2, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'GetObject') def test_recursive_glacier_download_without_force_glacier(self): @@ -287,7 +287,7 @@ def test_recursive_glacier_download_without_force_glacier(self): self.prefix, self.files.rootdir) _, stderr, _ = self.run_cmd(cmdline, expected_rc=2) self.assertEqual(len(self.operations_called), 1, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertIn('GLACIER', stderr) def test_warns_on_glacier_incompatible_operation(self): @@ -784,7 +784,7 @@ def test_recursive_download(self): self.run_cmd(cmdline, expected_rc=0) self.assert_operations_called( [ - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'mybucket', 'Prefix': '', 'EncodingType': 'url', @@ -899,7 +899,7 @@ def test_recursive_copy(self): self.run_cmd(cmdline, expected_rc=0) self.assert_operations_called( [ - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'sourcebucket', 'Prefix': '', 'EncodingType': 'url', diff --git a/tests/functional/s3/test_rb_command.py b/tests/functional/s3/test_rb_command.py index 14565ac8e360..06d82387bcf8 100644 --- a/tests/functional/s3/test_rb_command.py +++ b/tests/functional/s3/test_rb_command.py @@ -27,7 +27,7 @@ def test_rb_force_empty_bucket(self): command = self.prefix + 's3://bucket --force' self.run_cmd(command) self.assertEqual(len(self.operations_called), 2) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'DeleteBucket') def test_rb_force_non_empty_bucket(self): @@ -43,7 +43,7 @@ def test_rb_force_non_empty_bucket(self): }, {}, {}] self.run_cmd(command) self.assertEqual(len(self.operations_called), 3) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'DeleteObject') self.assertEqual(self.operations_called[2][0].name, 'DeleteBucket') @@ -59,7 +59,7 @@ def test_rb_force_with_failed_rm(self): _, stderr, _ = self.run_cmd(command, expected_rc=255) self.assertIn('remove_bucket failed:', stderr) self.assertEqual(len(self.operations_called), 1) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') def test_nonzero_exit_if_uri_scheme_not_provided(self): command = self.prefix + 'bucket' diff --git a/tests/functional/s3/test_rm_command.py b/tests/functional/s3/test_rm_command.py index bd97ad5ef0df..b5358f3f8432 100644 --- a/tests/functional/s3/test_rm_command.py +++ b/tests/functional/s3/test_rm_command.py @@ -55,7 +55,7 @@ def test_recursive_delete_with_requests(self): self.run_cmd(cmdline, expected_rc=0) self.assert_operations_called( [ - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'mybucket', 'Prefix': '', 'EncodingType': 'url', diff --git a/tests/functional/s3/test_sync_command.py b/tests/functional/s3/test_sync_command.py index 7c9f8461414e..893841d78553 100644 --- a/tests/functional/s3/test_sync_command.py +++ b/tests/functional/s3/test_sync_command.py @@ -32,9 +32,9 @@ def test_website_redirect_ignore_paramfile(self): ] self.run_cmd(cmdline, expected_rc=0) - # The only operations we should have called are ListObjects/PutObject. + # The only operations we should have called are ListObjectsV2/PutObject. self.assertEqual(len(self.operations_called), 2, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'PutObject') # Make sure that the specified web address is used as opposed to the # contents of the web address when uploading the object @@ -90,7 +90,7 @@ def test_glacier_sync_with_force_glacier(self): self.prefix, self.files.rootdir) self.run_cmd(cmdline, expected_rc=0) self.assertEqual(len(self.operations_called), 2, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'GetObject') def test_handles_glacier_incompatible_operations(self): @@ -105,7 +105,7 @@ def test_handles_glacier_incompatible_operations(self): # There should not have been a download attempted because the # operation was skipped because it is glacier incompatible. self.assertEqual(len(self.operations_called), 1) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertIn('GLACIER', stderr) def test_turn_off_glacier_warnings(self): @@ -120,7 +120,7 @@ def test_turn_off_glacier_warnings(self): # There should not have been a download attempted because the # operation was skipped because it is glacier incompatible. self.assertEqual(len(self.operations_called), 1) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual('', stderr) def test_warning_on_invalid_timestamp(self): @@ -139,7 +139,7 @@ def test_warning_on_invalid_timestamp(self): # We should still have put the object self.assertEqual(len(self.operations_called), 2, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertEqual(self.operations_called[1][0].name, 'PutObject') def test_sync_with_delete_on_downloads(self): @@ -152,9 +152,9 @@ def test_sync_with_delete_on_downloads(self): ] self.run_cmd(cmdline, expected_rc=0) - # The only operations we should have called are ListObjects. + # The only operations we should have called are ListObjectsV2. self.assertEqual(len(self.operations_called), 1, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') self.assertFalse(os.path.exists(full_path)) @@ -185,7 +185,7 @@ def side_effect(_): # We should not call PutObject because the file was deleted # before we could transfer it self.assertEqual(len(self.operations_called), 1, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') # This test covers the case where an OSError is emitted. def test_sync_skips_over_files_deleted_between_listing_and_transfer_oserror(self): @@ -208,7 +208,7 @@ def side_effect(_): # We should not call PutObject because the file was deleted # before we could transfer it self.assertEqual(len(self.operations_called), 1, self.operations_called) - self.assertEqual(self.operations_called[0][0].name, 'ListObjects') + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') def test_request_payer(self): cmdline = '%s s3://sourcebucket/ s3://mybucket --request-payer' % ( @@ -234,13 +234,13 @@ def test_request_payer(self): self.run_cmd(cmdline, expected_rc=0) self.assert_operations_called( [ - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'sourcebucket', 'Prefix': '', 'EncodingType': 'url', 'RequestPayer': 'requester', }), - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'mybucket', 'Prefix': '', 'EncodingType': 'url', @@ -280,13 +280,13 @@ def test_request_payer_with_deletes(self): self.run_cmd(cmdline, expected_rc=0) self.assert_operations_called( [ - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'sourcebucket', 'Prefix': '', 'EncodingType': 'url', 'RequestPayer': 'requester', }), - ('ListObjects', { + ('ListObjectsV2', { 'Bucket': 'mybucket', 'Prefix': '', 'EncodingType': 'url', diff --git a/tests/unit/customizations/s3/test_subcommands.py b/tests/unit/customizations/s3/test_subcommands.py index 1b80d03a66c5..7bd8529b6f87 100644 --- a/tests/unit/customizations/s3/test_subcommands.py +++ b/tests/unit/customizations/s3/test_subcommands.py @@ -103,9 +103,9 @@ def test_ls_command_for_bucket(self): .return_value.paginate # We should make no operation calls. self.assertEqual(call.call_count, 0) - # And only a single pagination call to ListObjects. + # And only a single pagination call to ListObjectsV2. self.session.create_client.return_value.get_paginator.\ - assert_called_with('list_objects') + assert_called_with('list_objects_v2') ref_call_args = {'Bucket': u'mybucket', 'Delimiter': '/', 'Prefix': u'', 'PaginationConfig': {'PageSize': u'5'}} @@ -160,9 +160,9 @@ def test_ls_with_requester_pays(self): .return_value.paginate # We should make no operation calls. self.assertEqual(call.call_count, 0) - # And only a single pagination call to ListObjects. + # And only a single pagination call to ListObjectsV2. self.session.create_client.return_value.get_paginator.\ - assert_called_with('list_objects') + assert_called_with('list_objects_v2') ref_call_args = { 'Bucket': u'mybucket', 'Delimiter': '/', 'Prefix': u'', 'PaginationConfig': {'PageSize': '5'}, diff --git a/tests/unit/customizations/s3/test_utils.py b/tests/unit/customizations/s3/test_utils.py index 2860dc75582b..09a24f878dee 100644 --- a/tests/unit/customizations/s3/test_utils.py +++ b/tests/unit/customizations/s3/test_utils.py @@ -223,7 +223,7 @@ def setUp(self): def fake_paginate(self, *args, **kwargs): for response in self.responses: - self.emitter.emit('after-call.s3.ListObjects', parsed=response) + self.emitter.emit('after-call.s3.ListObjectsV2', parsed=response) return self.responses def test_list_objects(self): From 1f14b31a45bfa285f3c7ab00ad0d00d0ff8ba8a5 Mon Sep 17 00:00:00 2001 From: Dave Rocamora Date: Mon, 10 Sep 2018 16:01:38 -0700 Subject: [PATCH 2/5] Updating customizations to include Requester Pays headers for ListObjectsV2 --- awscli/customizations/s3/subcommands.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index 9488605509ed..284c474d23c6 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -1081,7 +1081,8 @@ def run(self): def _get_file_generator_request_parameters_skeleton(self): return { 'HeadObject': {}, - 'ListObjects': {} + 'ListObjects': {}, + 'ListObjectsV2': {} } def _map_request_payer_params(self, request_parameters): @@ -1095,6 +1096,11 @@ def _map_request_payer_params(self, request_parameters): 'request_payer': self.parameters.get('request_payer') } ) + RequestParamsMapper.map_list_objects_params( + request_parameters['ListObjectsV2'], { + 'request_payer': self.parameters.get('request_payer') + } + ) def _map_sse_c_params(self, request_parameters, paths_type): # SSE-C may be neaded for HeadObject for copies/downloads/deletes From 55217f780721d99ce517c34c3e633f7e4cbd3002 Mon Sep 17 00:00:00 2001 From: Dave Rocamora Date: Wed, 12 Sep 2018 12:01:12 -0700 Subject: [PATCH 3/5] updating test to expect ListObjectsV2 --- tests/integration/customizations/s3/test_plugin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/customizations/s3/test_plugin.py b/tests/integration/customizations/s3/test_plugin.py index a93ea8ac803c..55ebbe1f7561 100644 --- a/tests/integration/customizations/s3/test_plugin.py +++ b/tests/integration/customizations/s3/test_plugin.py @@ -1149,7 +1149,7 @@ def test_ls_non_existent_bucket(self): self.assertEqual(p.rc, 255) self.assertIn( ('An error occurred (NoSuchBucket) when calling the ' - 'ListObjects operation: The specified bucket does not exist'), + 'ListObjectsV2 operation: The specified bucket does not exist'), p.stderr) # There should be no stdout if we can't find the bucket. self.assertEqual(p.stdout, '') From a369d466303acf9b007fde7e8f8186b644a4175f Mon Sep 17 00:00:00 2001 From: Dave Rocamora Date: Wed, 12 Sep 2018 14:04:43 -0700 Subject: [PATCH 4/5] renamed map_list_objects_params to map_list_objects_v2_params and updated tests --- awscli/customizations/s3/subcommands.py | 7 +------ awscli/customizations/s3/utils.py | 2 +- tests/unit/customizations/s3/test_subcommands.py | 2 +- tests/unit/customizations/s3/test_utils.py | 2 +- 4 files changed, 4 insertions(+), 9 deletions(-) diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index 284c474d23c6..691261343197 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -1091,12 +1091,7 @@ def _map_request_payer_params(self, request_parameters): 'request_payer': self.parameters.get('request_payer') } ) - RequestParamsMapper.map_list_objects_params( - request_parameters['ListObjects'], { - 'request_payer': self.parameters.get('request_payer') - } - ) - RequestParamsMapper.map_list_objects_params( + RequestParamsMapper.map_list_objects_v2_params( request_parameters['ListObjectsV2'], { 'request_payer': self.parameters.get('request_payer') } diff --git a/awscli/customizations/s3/utils.py b/awscli/customizations/s3/utils.py index 22f915c8cab3..dbbc35271f0c 100644 --- a/awscli/customizations/s3/utils.py +++ b/awscli/customizations/s3/utils.py @@ -480,7 +480,7 @@ def map_delete_object_params(cls, request_params, cli_params): cls._set_request_payer_param(request_params, cli_params) @classmethod - def map_list_objects_params(cls, request_params, cli_params): + def map_list_objects_v2_params(cls, request_params, cli_params): cls._set_request_payer_param(request_params, cli_params) @classmethod diff --git a/tests/unit/customizations/s3/test_subcommands.py b/tests/unit/customizations/s3/test_subcommands.py index 7bd8529b6f87..a4418a79dae4 100644 --- a/tests/unit/customizations/s3/test_subcommands.py +++ b/tests/unit/customizations/s3/test_subcommands.py @@ -98,7 +98,7 @@ def test_ls_command_for_bucket(self): summarize=False, request_payer=None) parsed_globals = mock.Mock() ls_command._run_main(parsed_args, parsed_globals) - call = self.session.create_client.return_value.list_objects + call = self.session.create_client.return_value.list_objects_v2 paginate = self.session.create_client.return_value.get_paginator\ .return_value.paginate # We should make no operation calls. diff --git a/tests/unit/customizations/s3/test_utils.py b/tests/unit/customizations/s3/test_utils.py index 09a24f878dee..3f3eb03b930a 100644 --- a/tests/unit/customizations/s3/test_utils.py +++ b/tests/unit/customizations/s3/test_utils.py @@ -469,7 +469,7 @@ def test_delete_object(self): def test_list_objects(self): params = {} - RequestParamsMapper.map_list_objects_params( + RequestParamsMapper.map_list_objects_v2_params( params, self.cli_params) self.assertEqual(params, {'RequestPayer': 'requester'}) From 2a7b96fdfcdeea33d4a4a709243cedd443112f3f Mon Sep 17 00:00:00 2001 From: Dave Rocamora Date: Wed, 12 Sep 2018 15:49:13 -0700 Subject: [PATCH 5/5] updating changelog --- .changes/next-release/enhancement-s3-89026.json | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changes/next-release/enhancement-s3-89026.json diff --git a/.changes/next-release/enhancement-s3-89026.json b/.changes/next-release/enhancement-s3-89026.json new file mode 100644 index 000000000000..b322d7d52b78 --- /dev/null +++ b/.changes/next-release/enhancement-s3-89026.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "s3", + "description": "``aws s3`` subcommands that list objects will use ListObjectsV2 instead of ListObjects `#3549 `__." +}