Refactored the validation logic

Hoisted the user provided local paths to the CommandArchitecture class

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,48 +220,18 @@ 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):
                 # Trailing slash must be removed to check if it is a symlink.
                 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

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:
+            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,

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):

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)
 

tests/unit/customizations/s3/test_filegenerator.py

@@ -110,15 +110,15 @@ 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)
         size, last_update = get_file_stat(self.local_file)
         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,15 +134,15 @@ 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)
         size, last_update = get_file_stat(self.local_file)
         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):