From e81ac19be7a684f498cdb0509c72db7c269a9ac4 Mon Sep 17 00:00:00 2001 From: kyleknap Date: Wed, 21 Oct 2015 15:58:49 -0700 Subject: [PATCH] Refactored the validation logic Hoisted the user provided local paths to the CommandArchitecture class --- awscli/customizations/s3/filegenerator.py | 53 ++----------------- awscli/customizations/s3/subcommands.py | 15 ++++++ tests/functional/s3/test_sync_command.py | 2 +- .../customizations/s3/test_plugin.py | 2 +- .../customizations/s3/test_filegenerator.py | 41 +++++--------- 5 files changed, 34 insertions(+), 79 deletions(-) diff --git a/awscli/customizations/s3/filegenerator.py b/awscli/customizations/s3/filegenerator.py index 2841eb0d0b67..bc12877c9ffb 100644 --- a/awscli/customizations/s3/filegenerator.py +++ b/awscli/customizations/s3/filegenerator.py @@ -130,24 +130,11 @@ def call(self, files): ``dir_op`` and ``use_src_name`` flags affect which files are used and ensure the proper destination paths and compare keys are formed. """ + function_table = {'s3': self.list_objects, 'local': self.list_files} source = files['src']['path'] src_type = files['src']['type'] dest_type = files['dest']['type'] - if src_type == 'local': - direction_context = src_type + dest_type - # If the file generator has no associated operation, it - # is not the driving force behind an operation. That means - # it is being used as the other generator (the one being used - # for listing of files in the destination) in a sync command. - # More specifically this if statement will only be hit if it - # is the file generator listing files for a downloading sync. - if self.operation_name == '': - direction_context = dest_type + src_type - file_list = self.list_files( - source, files['dir_op'], - transfer_direction_context=direction_context) - else: - file_list = self.list_objects(source, files['dir_op']) + file_list = function_table[src_type](source, files['dir_op']) for src_path, size, last_update in file_list: dest_path, compare_key = find_dest_path_comp_key(files, src_path) yield FileStat(src=src_path, dest=dest_path, @@ -156,7 +143,7 @@ def call(self, files): dest_type=dest_type, operation_name=self.operation_name) - def list_files(self, path, dir_op, transfer_direction_context=None): + def list_files(self, path, dir_op): """ This function yields the appropriate local file or local files under a directory depending on if the operation is on a directory. @@ -167,7 +154,7 @@ def list_files(self, path, dir_op, transfer_direction_context=None): """ join, isdir, isfile = os.path.join, os.path.isdir, os.path.isfile error, listdir = os.error, os.listdir - if not self.should_ignore_file(path, transfer_direction_context): + if not self.should_ignore_file(path): if not dir_op: size, last_update = get_file_stat(path) yield path, size, last_update @@ -233,27 +220,11 @@ def should_ignore_file_with_decoding_warnings(self, dirname, filename): path = os.path.join(dirname, filename) return self.should_ignore_file(path) - def should_ignore_file(self, path, transfer_direction_context=None): + def should_ignore_file(self, path): """ This function checks whether a file should be ignored in the file generation process. This includes symlinks that are not to be followed and files that generate warnings. - - :type path: string - :param path: The local path to check if a file should be ignored. - - :type transfer_direction_context: string - :param transfer_direction_context: If provided indicates the direction - in which the local transfer is happening. 'locals3' indicates an - upload and 's3local' indicates a download. If the direction is - indicated, extra logic is added to wheter the file path should - be skipped as the ignore logic will deter based on what direction - the transfer is happening. For example, if we are downloading to - a non-existant directory we should not be warning about the - directory not existing because we will create it anyways. - Likewise, if we are uploading a file and the local path specified - does not exist, we should error out because it is impossible - to upload the file/directory as it does not exist. """ if not self.follow_symlinks: if os.path.isdir(path) and path.endswith(os.sep): @@ -261,20 +232,6 @@ def should_ignore_file(self, path, transfer_direction_context=None): path = path[:-1] if os.path.islink(path): return True - - if not os.path.exists(path): - # Handling user provided paths for uploads. This path should - # always exist as the user specified it. - if transfer_direction_context == 'locals3': - raise RuntimeError( - 'The user-provided path %s does not exist.' % path) - # Handling user provided paths for downloads. It is not important - # if this path does not exist because we try to download to it no - # matter its existance and therefore we should ignore it but - # not warn about it. - elif transfer_direction_context == 's3local': - return True - warning_triggered = self.triggers_warning(path) if warning_triggered: return True diff --git a/awscli/customizations/s3/subcommands.py b/awscli/customizations/s3/subcommands.py index 09777fa0cdaf..ddbcd7323db2 100644 --- a/awscli/customizations/s3/subcommands.py +++ b/awscli/customizations/s3/subcommands.py @@ -749,6 +749,21 @@ def run(self): 'rb': 'remove_bucket' } result_queue = queue.Queue() + + # If the user provided local path does not exist, hard fail because + # we know that we will not be able to upload the file. + if 'locals3' == paths_type and not self.parameters['is_stream']: + if not os.path.exists(files['src']['path']): + raise RuntimeError( + 'The user-provided path %s does not exist.' % + files['src']['path']) + # If the operation is downloading to a directory that does not exist, + # create the directories so no warnings are thrown during the syncing + # process. + elif 's3local' == paths_type and files['dir_op']: + if not os.path.exists(files['dest']['path']): + os.makedirs(files['dest']['path']) + operation_name = cmd_translation[paths_type][self.cmd] file_generator = FileGenerator(self._source_client, operation_name, diff --git a/tests/functional/s3/test_sync_command.py b/tests/functional/s3/test_sync_command.py index 9523ece5c383..40d88a2569cb 100644 --- a/tests/functional/s3/test_sync_command.py +++ b/tests/functional/s3/test_sync_command.py @@ -60,7 +60,7 @@ def test_sync_from_non_existant_directory(self): self.parsed_responses = [ {"CommonPrefixes": [], "Contents": []} ] - _, stderr, _ = self.run_cmd(cmdline, expected_rc=1) + _, stderr, _ = self.run_cmd(cmdline, expected_rc=255) self.assertIn('does not exist', stderr) def test_sync_to_non_existant_directory(self): diff --git a/tests/integration/customizations/s3/test_plugin.py b/tests/integration/customizations/s3/test_plugin.py index a4dd4b9d33c8..0b3bad3f8966 100644 --- a/tests/integration/customizations/s3/test_plugin.py +++ b/tests/integration/customizations/s3/test_plugin.py @@ -780,7 +780,7 @@ def test_no_exist(self): p = aws('s3 cp %s s3://%s/' % (filename, self.bucket_name)) # If the local path provided by the user is nonexistant for an # upload, this should error out. - self.assertEqual(p.rc, 1, p.stderr) + self.assertEqual(p.rc, 255, p.stderr) self.assertIn('The user-provided path %s does not exist.' % filename, p.stderr) diff --git a/tests/unit/customizations/s3/test_filegenerator.py b/tests/unit/customizations/s3/test_filegenerator.py index 546fcd98016b..3d741dff7ebf 100644 --- a/tests/unit/customizations/s3/test_filegenerator.py +++ b/tests/unit/customizations/s3/test_filegenerator.py @@ -110,7 +110,7 @@ def test_local_file(self): 'type': 's3'}, 'dir_op': False, 'use_src_name': False} params = {'region': 'us-east-1'} - files = FileGenerator(self.client, 'upload').call(input_local_file) + files = FileGenerator(self.client, '').call(input_local_file) result_list = [] for filename in files: result_list.append(filename) @@ -118,7 +118,7 @@ def test_local_file(self): file_stat = FileStat(src=self.local_file, dest='bucket/text1.txt', compare_key='text1.txt', size=size, last_update=last_update, src_type='local', - dest_type='s3', operation_name='upload') + dest_type='s3', operation_name='') ref_list = [file_stat] self.assertEqual(len(result_list), len(ref_list)) for i in range(len(result_list)): @@ -134,7 +134,7 @@ def test_local_directory(self): 'type': 's3'}, 'dir_op': True, 'use_src_name': True} params = {'region': 'us-east-1'} - files = FileGenerator(self.client, 'upload').call(input_local_dir) + files = FileGenerator(self.client, '').call(input_local_dir) result_list = [] for filename in files: result_list.append(filename) @@ -142,7 +142,7 @@ def test_local_directory(self): file_stat = FileStat(src=self.local_file, dest='bucket/text1.txt', compare_key='text1.txt', size=size, last_update=last_update, src_type='local', - dest_type='s3', operation_name='upload') + dest_type='s3', operation_name='') path = self.local_dir + 'another_directory' + os.sep \ + 'text2.txt' size, last_update = get_file_stat(path) @@ -151,7 +151,7 @@ def test_local_directory(self): compare_key='another_directory/text2.txt', size=size, last_update=last_update, src_type='local', - dest_type='s3', operation_name='upload') + dest_type='s3', operation_name='') ref_list = [file_stat2, file_stat] self.assertEqual(len(result_list), len(ref_list)) for i in range(len(result_list)): @@ -175,7 +175,7 @@ def tearDown(self): def test_warning(self): path = os.path.join(self.files.rootdir, 'badsymlink') os.symlink('non-existent-file', path) - filegenerator = FileGenerator(self.client, 'upload', True) + filegenerator = FileGenerator(self.client, '', True) self.assertTrue(filegenerator.should_ignore_file(path)) def test_skip_symlink(self): @@ -185,7 +185,7 @@ def test_skip_symlink(self): contents='foo.txt contents') sym_path = os.path.join(self.files.rootdir, 'symlink') os.symlink(filename, sym_path) - filegenerator = FileGenerator(self.client, 'upload', False) + filegenerator = FileGenerator(self.client, '', False) self.assertTrue(filegenerator.should_ignore_file(sym_path)) def test_no_skip_symlink(self): @@ -195,7 +195,7 @@ def test_no_skip_symlink(self): contents='foo.txt contents') sym_path = os.path.join(self.files.rootdir, 'symlink') os.symlink(path, sym_path) - filegenerator = FileGenerator(self.client, 'upload', True) + filegenerator = FileGenerator(self.client, '', True) self.assertFalse(filegenerator.should_ignore_file(sym_path)) self.assertFalse(filegenerator.should_ignore_file(path)) @@ -205,7 +205,7 @@ def test_no_skip_symlink_dir(self): os.mkdir(path) sym_path = os.path.join(self.files.rootdir, 'symlink') os.symlink(path, sym_path) - filegenerator = FileGenerator(self.client, 'upload', True) + filegenerator = FileGenerator(self.client, '', True) self.assertFalse(filegenerator.should_ignore_file(sym_path)) self.assertFalse(filegenerator.should_ignore_file(path)) @@ -321,8 +321,7 @@ def test_no_follow_symlink(self): 'dest': {'path': self.bucket, 'type': 's3'}, 'dir_op': True, 'use_src_name': True} - file_stats = FileGenerator(self.client, 'upload', False).call( - input_local_dir) + file_stats = FileGenerator(self.client, '', False).call(input_local_dir) self.filenames.sort() result_list = [] for file_stat in file_stats: @@ -344,7 +343,7 @@ def test_warn_bad_symlink(self): 'type': 's3'}, 'dir_op': True, 'use_src_name': True} file_stats = FileGenerator(self.client, '', True).call(input_local_dir) - file_gen = FileGenerator(self.client, 'upload', True) + file_gen = FileGenerator(self.client, '', True) file_stats = file_gen.call(input_local_dir) all_filenames = self.filenames + self.symlink_files all_filenames.sort() @@ -367,8 +366,7 @@ def test_follow_symlink(self): 'dest': {'path': self.bucket, 'type': 's3'}, 'dir_op': True, 'use_src_name': True} - file_stats = FileGenerator(self.client, 'upload', True).call( - input_local_dir) + file_stats = FileGenerator(self.client, '', True).call(input_local_dir) all_filenames = self.filenames + self.symlink_files all_filenames.sort() result_list = [] @@ -450,21 +448,6 @@ def test_list_local_files_with_unicode_chars(self): ]] self.assertEqual(values, expected_order) - def test_list_local_directory_that_does_not_exist_from_local_to_s3(self): - path = os.path.join(self.directory, 'nonexistantdirectory') - file_generator = FileGenerator(None, None, None) - with self.assertRaises(RuntimeError): - list(file_generator.list_files( - path, dir_op=True, transfer_direction_context='locals3')) - - def test_list_local_directory_that_does_not_exist_from_s3_to_local(self): - path = os.path.join(self.directory, 'nonexistantdirectory') - file_generator = FileGenerator(None, None, None) - file_list = list( - file_generator.list_files( - path, dir_op=True, transfer_direction_context='s3local')) - self.assertEqual(file_list, []) - class TestNormalizeSort(unittest.TestCase): def test_normalize_sort(self):