From 4215922cad229ac30cf5062e162f0d981e807abb Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Fri, 13 Dec 2013 16:34:10 -0800 Subject: [PATCH 1/2] Account for s3 paths not having a trailing slash Fixes #552 --- awscli/customizations/s3/s3.py | 12 ++++++++++ .../customizations/s3/test_plugin.py | 17 +++++++++++++ .../unit/customizations/s3/test_cp_command.py | 24 +++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/awscli/customizations/s3/s3.py b/awscli/customizations/s3/s3.py index e30024ce2077..c89c3350c55b 100644 --- a/awscli/customizations/s3/s3.py +++ b/awscli/customizations/s3/s3.py @@ -639,6 +639,7 @@ def add_paths(self, paths): the destination always have some value. """ self.check_path_type(paths) + self._normalize_s3_trailing_slash(paths) src_path = paths[0] self.parameters['src'] = src_path if len(paths) == 2: @@ -646,6 +647,17 @@ def add_paths(self, paths): elif len(paths) == 1: self.parameters['dest'] = paths[0] + def _normalize_s3_trailing_slash(self, paths): + for i, path in enumerate(paths): + if path.startswith('s3://'): + bucket, key = find_bucket_key(path[5:]) + if not key and not path.endswith('/'): + # If only a bucket was specified, we need + # to normalize the path and ensure it ends + # with a '/', s3://bucket -> s3://bucket/ + path += '/' + paths[i] = path + def _verify_bucket_exists(self, bucket_name): session = self.session service = session.get_service('s3') diff --git a/tests/integration/customizations/s3/test_plugin.py b/tests/integration/customizations/s3/test_plugin.py index d0317b689424..f694f97faf75 100644 --- a/tests/integration/customizations/s3/test_plugin.py +++ b/tests/integration/customizations/s3/test_plugin.py @@ -253,6 +253,23 @@ def test_cp_to_and_from_s3(self): with open(full_path, 'r') as f: self.assertEqual(f.read(), 'this is foo.txt') + def test_cp_without_trailing_slash(self): + # There's a unit test for this, but we still want to verify this + # with an integration test. + bucket_name = self.create_bucket() + + # copy file into bucket. + foo_txt = self.files.create_file('foo.txt', 'this is foo.txt') + # Note that the destination has no trailing slash. + p = aws('s3 cp %s s3://%s' % (foo_txt, bucket_name)) + self.assert_no_errors(p) + + # Make sure object is in bucket. + self.assertTrue(self.key_exists(bucket_name, key_name='foo.txt')) + self.assertEqual( + self.get_key_contents(bucket_name, key_name='foo.txt'), + 'this is foo.txt') + def test_cp_s3_s3_multipart(self): from_bucket = self.create_bucket() to_bucket = self.create_bucket() diff --git a/tests/unit/customizations/s3/test_cp_command.py b/tests/unit/customizations/s3/test_cp_command.py index b8eab7e366be..5d6643da5ff2 100644 --- a/tests/unit/customizations/s3/test_cp_command.py +++ b/tests/unit/customizations/s3/test_cp_command.py @@ -39,6 +39,30 @@ def test_operations_used_in_upload(self): self.assertEqual(len(self.operations_called), 1, self.operations_called) self.assertEqual(self.operations_called[0][0].name, 'PutObject') + def test_key_name_added_when_only_bucket_provided(self): + full_path = self.files.create_file('foo.txt', 'mycontent') + cmdline = '%s %s s3://bucket/' % (self.prefix, full_path) + self.parsed_responses = [{'ETag': '"c8afdb36c52cf4727836669019e69222"'}] + self.run_cmd(cmdline, expected_rc=0) + # The only operation we should have called is PutObject. + self.assertEqual(len(self.operations_called), 1, self.operations_called) + self.assertEqual(self.operations_called[0][0].name, 'PutObject') + self.assertEqual(self.operations_called[0][1]['key'], 'foo.txt') + self.assertEqual(self.operations_called[0][1]['bucket'], 'bucket') + + def test_trailing_slash_appended(self): + full_path = self.files.create_file('foo.txt', 'mycontent') + # Here we're saying s3://bucket instead of s3://bucket/ + # This should still work the same as if we added the trailing slash. + cmdline = '%s %s s3://bucket/' % (self.prefix, full_path) + self.parsed_responses = [{'ETag': '"c8afdb36c52cf4727836669019e69222"'}] + self.run_cmd(cmdline, expected_rc=0) + # The only operation we should have called is PutObject. + self.assertEqual(len(self.operations_called), 1, self.operations_called) + self.assertEqual(self.operations_called[0][0].name, 'PutObject') + self.assertEqual(self.operations_called[0][1]['key'], 'foo.txt') + self.assertEqual(self.operations_called[0][1]['bucket'], 'bucket') + def test_operations_used_in_download_file(self): self.parsed_responses = [ {"ContentLength": "100", "LastModified": "00:00:00Z"}, From 0091306135d46322a184e191d27394866848cb9b Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Mon, 16 Dec 2013 10:56:23 -0800 Subject: [PATCH 2/2] Update test to *not* have trailing slash --- tests/unit/customizations/s3/test_cp_command.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/customizations/s3/test_cp_command.py b/tests/unit/customizations/s3/test_cp_command.py index 5d6643da5ff2..5bdc52db9cea 100644 --- a/tests/unit/customizations/s3/test_cp_command.py +++ b/tests/unit/customizations/s3/test_cp_command.py @@ -54,7 +54,7 @@ def test_trailing_slash_appended(self): full_path = self.files.create_file('foo.txt', 'mycontent') # Here we're saying s3://bucket instead of s3://bucket/ # This should still work the same as if we added the trailing slash. - cmdline = '%s %s s3://bucket/' % (self.prefix, full_path) + cmdline = '%s %s s3://bucket' % (self.prefix, full_path) self.parsed_responses = [{'ETag': '"c8afdb36c52cf4727836669019e69222"'}] self.run_cmd(cmdline, expected_rc=0) # The only operation we should have called is PutObject.