S3: delete directories in batches

readthedocs/storage/s3_storage.py

@@ -17,6 +17,7 @@
 from django.core.exceptions import SuspiciousFileOperation
 from storages.backends.s3boto3 import S3Boto3Storage
 from storages.backends.s3boto3 import S3ManifestStaticStorage
+from storages.utils import clean_name
 
 from readthedocs.storage.mixins import RTDBaseStorage
 from readthedocs.storage.rclone import RCloneS3Remote
@@ -47,42 +48,33 @@ def _rclone(self):
     def join(self, directory, filepath):
         return safe_join(directory, filepath)
 
-    @staticmethod
-    def _dirpath(path):
-        """
-        Make the path to end with `/`.
-
-        It may just be Azure, but for listdir to work correctly, this is needed.
-        """
-        path = str(path)
-        if not path.endswith("/"):
-            path += "/"
-
-        return path
-
     def delete_directory(self, path):
         """
         Delete all files under a certain path from storage.
 
         Many storage backends (S3, Azure storage) don't care about "directories".
         The directory effectively doesn't exist if there are no files in it.
-        However, in these backends, there is no "rmdir" operation so you have to recursively
-        delete all files.
+        However, in these backends, there is no "rmdir" operation,
+        but boto3 allows batch actions over a prefix, so we can delete
+        all files with a certain prefix to achieve the same result
+        with fewer API calls than deleting each file one by one.
 
         :param path: the path to the directory to remove
         """
-        if path in ("", "/"):
+        # S3Boto3Storage does this normalization before interacting with
+        # the path in all its methods, following the same pattern here.
+        path = self._normalize_name(clean_name(path))
+
+        # The path needs to end with a slash.
+        if not path.endswith("/"):
+            path += "/"
+
+        location = self.location.rstrip("/") + "/"
+        if path == location:
             raise SuspiciousFileOperation("Deleting all storage cannot be right")
 
-        log.debug("Deleting path from media storage", path=path)
-        folders, files = self.listdir(self._dirpath(path))
-        for folder_name in folders:
-            if folder_name:
-                # Recursively delete the subdirectory
-                self.delete_directory(self.join(path, folder_name))
-        for filename in files:
-            if filename:
-                self.delete(self.join(path, filename))
+        log.debug("Deleting path from storage", path=path)
+        self.bucket.objects.filter(Prefix=path).delete()
 
 
 class S3BuildMediaStorage(OverrideHostnameMixin, RTDS3Storage):

readthedocs/storage/tests/test_s3_storage.py

@@ -0,0 +1,49 @@
+from unittest import mock
+
+import pytest
+from django.core.exceptions import SuspiciousFileOperation, SuspiciousOperation
+from django.test import TestCase
+
+from readthedocs.storage.s3_storage import RTDS3Storage
+
+
+class TestRTDS3Storage(TestCase):
+
+    def setUp(self):
+        self.storage = RTDS3Storage()
+
+    def test_delete_directory(self):
+        mock_bucket = mock.MagicMock()
+        self.storage._bucket = mock_bucket
+        self.storage.delete_directory("projects/my-project/en/latest/")
+        mock_bucket.objects.filter.assert_called_once_with(
+            Prefix="projects/my-project/en/latest/"
+        )
+        mock_bucket.objects.filter.return_value.delete.assert_called_once()
+
+    def test_delete_directory_adds_trailing_slash(self):
+        mock_bucket = mock.MagicMock()
+        self.storage._bucket = mock_bucket
+        self.storage.delete_directory("projects/my-project/en/latest")
+        mock_bucket.objects.filter.assert_called_once_with(
+            Prefix="projects/my-project/en/latest/"
+        )
+        mock_bucket.objects.filter.return_value.delete.assert_called_once()
+
+    def test_delete_directory_raises_for_root_path(self):
+        with pytest.raises(SuspiciousFileOperation):
+            self.storage.delete_directory("/")
+
+    def test_delete_directory_raises_for_empty_path(self):
+        with pytest.raises(SuspiciousFileOperation):
+            self.storage.delete_directory("")
+
+    def test_delete_directory_raises_for_root_path_with_location(self):
+        self.storage.location = "projects"
+        with pytest.raises(SuspiciousOperation):
+            self.storage.delete_directory("/")
+
+    def test_delete_directory_raises_for_empty_path_with_location(self):
+        self.storage.location = "projects"
+        with pytest.raises(SuspiciousFileOperation):
+            self.storage.delete_directory("")