diff --git a/.changes/next-release/bugfix-s3sync-8932.json b/.changes/next-release/bugfix-s3sync-8932.json new file mode 100644 index 000000000000..2340b68baa42 --- /dev/null +++ b/.changes/next-release/bugfix-s3sync-8932.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "``s3 sync``", + "description": "Fix ``--exclude`` and ``--include`` filters given as absolute paths so they also apply to the destination side of ``sync``. Previously the destination copy of an absolute pattern could never match, so ``sync`` uploaded already synced files on every run and ``sync --delete`` removed objects an absolute ``--exclude`` was meant to protect (`issue 8932 `__)." +} diff --git a/awscli/customizations/s3/filters.py b/awscli/customizations/s3/filters.py index f41820ac09de..98784a523ecf 100644 --- a/awscli/customizations/s3/filters.py +++ b/awscli/customizations/s3/filters.py @@ -90,9 +90,44 @@ def __init__(self, patterns, rootdir, dst_rootdir): """ self._original_patterns = patterns + patterns = [ + (pattern[0], + self._relative_pattern(pattern[1], (rootdir, dst_rootdir))) + for pattern in patterns + ] self.patterns = self._full_path_patterns(patterns, rootdir) self.dst_patterns = self._full_path_patterns(patterns, dst_rootdir) + def _relative_pattern(self, pattern, rootdirs): + # ``_full_path_patterns`` rebases each pattern onto the source + # and destination roots with ``os.path.join``. When the pattern + # is an absolute path, ``os.path.join`` returns the pattern + # unchanged and silently discards the root, so the destination + # side copy of the pattern keeps referring to the local path and + # can never match a destination path such as ``bucket/key``. + # When an absolute pattern lies under a local root, reduce it to + # the equivalent relative pattern so it rebases correctly onto + # both roots. + local_pattern = pattern.replace('/', os.sep) + if not os.path.isabs(local_pattern): + return pattern + for rootdir in rootdirs: + if rootdir is None or not os.path.isabs(rootdir): + # Only local roots are absolute paths; an S3 root is of + # the form ``bucket/prefix``. + continue + root_prefix = rootdir.rstrip(os.sep) + os.sep + # The prefix check is textual rather than path aware so that + # wildcard characters in the pattern are preserved as typed. + # ``normcase`` handles case insensitive filesystems and + # drive letter casing on Windows. + if os.path.normcase(local_pattern).startswith( + os.path.normcase(root_prefix)): + return local_pattern[len(root_prefix):] + # The pattern lies under neither root; leave it untouched so it + # keeps its current behavior on the side where it can match. + return pattern + def _full_path_patterns(self, original_patterns, rootdir): # We need to transform the patterns into patterns that have # the root dir prefixed, so things like ``--exclude "*"`` diff --git a/tests/functional/s3/test_sync_command.py b/tests/functional/s3/test_sync_command.py index e2b9ab40e30e..24aaac870042 100644 --- a/tests/functional/s3/test_sync_command.py +++ b/tests/functional/s3/test_sync_command.py @@ -173,6 +173,29 @@ def test_sync_with_delete_on_downloads(self): self.assertFalse(os.path.exists(full_path)) + def test_sync_with_absolute_include_converges(self): + # An ``--include`` spelled as an absolute path has to apply to + # the remote listing as well. If it does not, the remote copy + # of an already synced file is filtered out of the listing and + # sync uploads the file again on every run. + full_path = self.files.create_file('foo.txt', 'mycontent') + cmdline = '%s %s s3://bucket/ --exclude * --include %s' % ( + self.prefix, self.files.rootdir, full_path) + self.parsed_responses = [ + {"CommonPrefixes": [], "Contents": [ + {"Key": "foo.txt", "Size": len('mycontent'), + "LastModified": "2099-01-09T20:45:49.000Z", + "ETag": '"c8afdb36c52cf4727836669019e69222"'}]}, + {'ETag': '"c8afdb36c52cf4727836669019e69222"'} + ] + self.run_cmd(cmdline, expected_rc=0) + + # The remote object is identical to the local file, so a second + # sync must only list the bucket and upload nothing. + self.assertEqual(len(self.operations_called), 1, + self.operations_called) + self.assertEqual(self.operations_called[0][0].name, 'ListObjectsV2') + # When a file has been deleted after listing, # awscli.customizations.s3.utils.get_file_stat may raise either some kind # of OSError, or a ValueError, depending on the environment. In both cases, diff --git a/tests/unit/customizations/s3/test_filters.py b/tests/unit/customizations/s3/test_filters.py index a4975e68996d..d004ccc07faf 100644 --- a/tests/unit/customizations/s3/test_filters.py +++ b/tests/unit/customizations/s3/test_filters.py @@ -10,8 +10,9 @@ # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import ntpath import os -from awscli.testutils import unittest +from awscli.testutils import mock, unittest import platform from awscli.customizations.s3.filegenerator import FileStat @@ -223,5 +224,133 @@ def test_create_filter_s3_to_s3(self): for filtered_file in filtered: self.assertFalse('.txt' in filtered_file.src) + # An absolute pattern that lies under a local root must behave + # exactly like the equivalent relative pattern: it has to be rebased + # onto both the source root and the destination root. Historically + # ``os.path.join`` silently discarded the root for absolute patterns, + # so the destination side copy of the pattern kept referring to the + # local path and could never match a destination path like + # ``bucket/key``. + + def test_absolute_include_local_to_s3(self): + p = platform_path + parameters = { + 'filters': [['--exclude', '*'], + ['--include', p('/abs/src/photos/*')]], + 'dir_op': True, + 'src': p('/abs/src'), + 'dest': 's3://bucket/', + } + abs_filter = self.create_filter(parameters=parameters) + # The include pattern is rebased onto both roots, exactly as the + # relative pattern ``photos/*`` would be. + self.assertEqual( + abs_filter.patterns[1], + ('include', p('/abs/src/photos/*'))) + self.assertEqual( + abs_filter.dst_patterns[1], + ('include', 'bucket/' + os.path.join('photos', '*'))) + # Local side: the file generator listing re-includes the file. + local_file = self.file_stat(p('/abs/src/photos/cat.jpg')) + self.assertEqual(list(abs_filter.call([local_file])), [local_file]) + # Destination side: the remote listing must also re-include the + # object, otherwise the Comparator never sees it and sync + # uploads it again on every run. + remote_file = self.file_stat('bucket/photos/cat.jpg', src_type='s3') + self.assertEqual(list(abs_filter.call([remote_file])), [remote_file]) + + def test_absolute_exclude_protects_destination_with_delete(self): + p = platform_path + parameters = { + 'filters': [['--exclude', p('/abs/src/keep/*')]], + 'dir_op': True, + 'src': p('/abs/src'), + 'dest': 's3://bucket/', + } + abs_filter = self.create_filter(parameters=parameters) + local_file = self.file_stat(p('/abs/src/keep/important.txt')) + self.assertEqual(list(abs_filter.call([local_file])), []) + # The excluded object must also be dropped from the destination + # listing; if it stays there ``sync --delete`` deletes the very + # object the exclude was written to protect. + remote_file = self.file_stat( + 'bucket/keep/important.txt', src_type='s3') + self.assertEqual(list(abs_filter.call([remote_file])), []) + + def test_absolute_pattern_under_destination_root(self): + # Downloads have the local directory as the destination root, so + # an absolute pattern under the destination root has to be + # rebased onto the s3 source root as well. + p = platform_path + parameters = { + 'filters': [['--exclude', '*'], + ['--include', p('/abs/dest/docs/*')]], + 'dir_op': True, + 'src': 's3://bucket/', + 'dest': p('/abs/dest'), + } + abs_filter = self.create_filter(parameters=parameters) + self.assertEqual( + abs_filter.patterns[1], + ('include', 'bucket/' + os.path.join('docs', '*'))) + self.assertEqual( + abs_filter.dst_patterns[1], + ('include', p('/abs/dest/docs/*'))) + remote_file = self.file_stat('bucket/docs/a.txt', src_type='s3') + self.assertEqual(list(abs_filter.call([remote_file])), [remote_file]) + local_file = self.file_stat(p('/abs/dest/docs/a.txt')) + self.assertEqual(list(abs_filter.call([local_file])), [local_file]) + + def test_absolute_pattern_outside_any_root(self): + p = platform_path + parameters = { + 'filters': [['--exclude', p('/elsewhere/keep/*')]], + 'dir_op': True, + 'src': p('/abs/src'), + 'dest': 's3://bucket/', + } + abs_filter = self.create_filter(parameters=parameters) + # The pattern lies under neither root, so it is left untouched on + # both sides, preserving the existing behavior. + self.assertEqual( + abs_filter.patterns[0], ('exclude', p('/elsewhere/keep/*'))) + self.assertEqual( + abs_filter.dst_patterns[0], ('exclude', p('/elsewhere/keep/*'))) + local_file = self.file_stat(p('/abs/src/file.txt')) + self.assertEqual(list(abs_filter.call([local_file])), [local_file]) + + def test_absolute_pattern_with_trailing_separator_root(self): + p = platform_path + # The local root can arrive with a trailing separator; the + # prefix strip has to treat '/abs/src/' and '/abs/src' the same. + abs_filter = self.create_filter( + [['include', p('/abs/src/photos/*')]], + root=p('/abs/src/'), dst_root='bucket') + self.assertEqual( + abs_filter.patterns[0], + ('include', p('/abs/src/photos/*'))) + self.assertEqual( + abs_filter.dst_patterns[0], + ('include', os.path.join('bucket', 'photos', '*'))) + + def test_absolute_pattern_windows_drive_and_case(self): + # Emulate Windows path semantics so the drive letter handling is + # exercised on every platform: the drive letter casing may differ + # between the pattern and the root, and the pattern may be + # spelled with forward slashes. + fake_os = mock.Mock(sep=ntpath.sep, path=ntpath) + with mock.patch( + 'awscli.customizations.s3.filters.os', fake_os): + win_filter = Filter( + [['include', 'c:/Users/om/photos/*']], + 'C:\\Users\\om', 'bucket') + self.assertEqual( + win_filter.patterns[0], + ('include', 'C:\\Users\\om\\photos\\*')) + self.assertEqual( + win_filter.dst_patterns[0], + ('include', 'bucket\\photos\\*')) + + if __name__ == "__main__": unittest.main()