Use sync instead of copy for blob storage

readthedocs/builds/storage.py

@@ -4,8 +4,7 @@
 from django.conf import settings
 from django.core.exceptions import SuspiciousFileOperation
 from django.core.files.storage import FileSystemStorage
-from storages.utils import safe_join, get_available_overwrite_name
-
+from storages.utils import get_available_overwrite_name, safe_join
 
 log = logging.getLogger(__name__)
 
@@ -88,6 +87,48 @@ def copy_directory(self, source, destination):
                 with filepath.open('rb') as fd:
                     self.save(sub_destination, fd)
 
+    def sync_directory(self, source, destination):
+        """
+        Sync a directory recursively to storage.
+
+        Overwrites files in remote storage with files from ``source`` (no timstamp/hash checking).
+        Removes files and folders in remote storage that are not present in ``source``.
+
+        :param source: the source path on the local disk
+        :param destination: the destination path in storage
+        """
+        if destination in ('', '/'):
+            raise SuspiciousFileOperation('Syncing all storage cannot be right')
+
+        log.debug(
+            'Syncing to media storage. source=%s destination=%s',
+            source, destination,
+        )
+        source = Path(source)
+        copied_files = set()
+        copied_dirs = set()
+        for filepath in source.iterdir():
+            sub_destination = self.join(destination, filepath.name)
+            if filepath.is_dir():
+                # Recursively sync the subdirectory
+                self.sync_directory(filepath, sub_destination)
+                copied_dirs.add(filepath.name)
+            elif filepath.is_file():
+                with filepath.open('rb') as fd:
+                    self.save(sub_destination, fd)
+                copied_files.add(filepath.name)
+
+        # Remove files that are not present in ``source``
+        dest_folders, dest_files = self.listdir(self._dirpath(destination))
+        for folder in dest_folders:
+            if folder not in copied_dirs:
+                self.delete_directory(self.join(destination, folder))
+        for filename in dest_files:
+            if filename not in copied_files:
+                filepath = self.join(destination, filename)
+                log.debug('Deleting file from media storage. file=%s', filepath)
+                self.delete(filepath)
+
     def join(self, directory, filepath):
         return safe_join(directory, filepath)
 

readthedocs/projects/tasks.py

@@ -847,7 +847,7 @@ def store_build_artifacts(
                 },
             )
             try:
-                storage.copy_directory(from_path, to_path)
+                storage.sync_directory(from_path, to_path)
             except Exception:
                 # Ideally this should just be an IOError
                 # but some storage backends unfortunately throw other errors

readthedocs/rtd_tests/tests/test_build_storage.py

@@ -18,6 +18,25 @@ def setUp(self):
     def tearDown(self):
         shutil.rmtree(self.test_media_dir, ignore_errors=True)
 
+    def assertFileTree(self, source, tree):
+        """
+        Recursively check that ``source`` from storage has the same file tree as ``tree``.
+
+        :param source: source path in storage
+        :param tree: a list of strings representing files
+                     or tuples (string, list) representing directories.
+        """
+        dirs_tree = [e for e in tree if not isinstance(e, str)]
+
+        dirs, files = self.storage.listdir(source)
+        expected_dirs = [e[0] for e in dirs_tree]
+        expected_files = [e for e in tree if isinstance(e, str)]
+        self.assertCountEqual(dirs, expected_dirs)
+        self.assertCountEqual(files, expected_files)
+
+        for folder, files in dirs_tree:
+            self.assertFileTree(self.storage.join(source, folder), files)
+
     def test_copy_directory(self):
         self.assertFalse(self.storage.exists('files/test.html'))
 
@@ -27,6 +46,39 @@ def test_copy_directory(self):
         self.assertTrue(self.storage.exists('files/api.fjson'))
         self.assertTrue(self.storage.exists('files/api/index.html'))
 
+    def test_sync_directory(self):
+        tmp_files_dir = os.path.join(tempfile.mkdtemp(), 'files')
+        shutil.copytree(files_dir, tmp_files_dir)
+        storage_dir = 'files'
+
+        tree = [
+            ('api', ['index.html']),
+            'api.fjson',
+            'conf.py',
+            'test.html',
+        ]
+        self.storage.sync_directory(tmp_files_dir, storage_dir)
+        self.assertFileTree(storage_dir, tree)
+
+        tree = [
+            ('api', ['index.html']),
+            'conf.py',
+            'test.html',
+        ]
+        os.remove(os.path.join(tmp_files_dir, 'api.fjson'))
+        self.storage.sync_directory(tmp_files_dir, storage_dir)
+        self.assertFileTree(storage_dir, tree)
+
+        tree = [
+            # Cloud storage generally doesn't consider empty directories to exist
+            ('api', []),
+            'conf.py',
+            'test.html',
+        ]
+        shutil.rmtree(os.path.join(tmp_files_dir, 'api'))
+        self.storage.sync_directory(tmp_files_dir, storage_dir)
+        self.assertFileTree(storage_dir, tree)
+
     def test_delete_directory(self):
         self.storage.copy_directory(files_dir, 'files')
         dirs, files = self.storage.listdir('files')