Merge pull request #6323 from stsewd/clean-project-resources

Remove files from storage and delete indexes from ES when no longer needed

Author: stsewd

readthedocs/builds/models.py

@@ -333,10 +333,9 @@ def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
             args=[self.get_artifact_paths()],
         )
 
-        # Remove build artifacts from storage if the version is not external
+        # Remove resources if the version is not external
         if self.type != EXTERNAL:
-            storage_paths = self.get_storage_paths()
-            tasks.remove_build_storage_paths.delay(storage_paths)
+            tasks.clean_project_resources(self.project, self)
 
         project_pk = self.project.pk
         super().delete(*args, **kwargs)

readthedocs/projects/models.py

@@ -480,16 +480,8 @@ def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
             args=[(self.doc_path,)],
         )
 
-        # Remove build artifacts from storage
-        storage_paths = []
-        for type_ in MEDIA_TYPES:
-            storage_paths.append(
-                '{}/{}'.format(
-                    type_,
-                    self.slug,
-                )
-            )
-        tasks.remove_build_storage_paths.delay(storage_paths)
+        # Remove extra resources
+        tasks.clean_project_resources(self)
 
         super().delete(*args, **kwargs)
 
@@ -534,6 +526,19 @@ def get_subproject_urls(self):
         return [(proj.child.slug, proj.child.get_docs_url())
                 for proj in self.subprojects.all()]
 
+    def get_storage_paths(self):
+        """
+        Get the paths of all artifacts used by the project.
+
+        :return: the path to an item in storage
+                 (can be used with ``storage.url`` to get the URL).
+        """
+        storage_paths = [
+            f'{type_}/{self.slug}'
+            for type_ in MEDIA_TYPES
+        ]
+        return storage_paths
+
     def get_storage_path(
             self,
             type_,

readthedocs/projects/tasks.py

@@ -1621,8 +1621,9 @@ def _sync_imported_files(version, build, changed_files):
     # Remove old HTMLFiles from ElasticSearch
     remove_indexed_files(
         model=HTMLFile,
-        version=version,
-        build=build,
+        project_slug=version.project.slug,
+        version_slug=version.slug,
+        build_id=build,
     )
 
     # Delete SphinxDomain objects from previous versions
@@ -1844,6 +1845,47 @@ def remove_build_storage_paths(paths):
         storage.delete_directory(storage_path)
 
 
+@app.task(queue='web')
+def remove_search_indexes(project_slug, version_slug=None):
+    """Wrapper around ``remove_indexed_files`` to make it a task."""
+    remove_indexed_files(
+        model=HTMLFile,
+        project_slug=project_slug,
+        version_slug=version_slug,
+    )
+
+
+def clean_project_resources(project, version=None):
+    """
+    Delete all extra resources used by `version` of `project`.
+
+    It removes:
+
+    - Artifacts from storage.
+    - Search indexes from ES.
+
+    :param version: Version instance. If isn't given,
+                    all resources of `project` will be deleted.
+
+    .. note::
+       This function is usually called just before deleting project.
+       Make sure to not depend on the project object inside the tasks.
+    """
+    # Remove storage paths
+    storage_paths = []
+    if version:
+        storage_paths = version.get_storage_paths()
+    else:
+        storage_paths = project.get_storage_paths()
+    remove_build_storage_paths.delay(storage_paths)
+
+    # Remove indexes
+    remove_search_indexes.delay(
+        project_slug=project.slug,
+        version_slug=version.slug if version else None,
+    )
+
+
 @app.task(queue='web')
 def sync_callback(_, version_pk, commit, build, *args, **kwargs):
     """

readthedocs/projects/views/private.py

@@ -203,6 +203,10 @@ def form_valid(self, form):
                     task=tasks.remove_dirs,
                     args=[version.get_artifact_paths()],
                 )
+                tasks.clean_project_resources(
+                    version.project,
+                    version,
+                )
                 version.built = False
                 version.save()
         return HttpResponseRedirect(self.get_success_url())

readthedocs/rtd_tests/tests/test_build_forms.py

@@ -1,6 +1,7 @@
-# -*- coding: utf-8 -*-
-
+import mock
+from django.contrib.auth.models import User
 from django.test import TestCase
+from django.urls import reverse
 from django_dynamic_fixture import get
 
 from readthedocs.builds.forms import VersionForm
@@ -12,7 +13,8 @@
 class TestVersionForm(TestCase):
 
     def setUp(self):
-        self.project = get(Project)
+        self.user = get(User)
+        self.project = get(Project, users=(self.user,))
 
     def test_default_version_is_active(self):
         version = get(
@@ -50,3 +52,35 @@ def test_default_version_is_inactive(self):
         )
         self.assertFalse(form.is_valid())
         self.assertIn('active', form.errors)
+
+    @mock.patch('readthedocs.projects.tasks.clean_project_resources')
+    def test_resources_are_deleted_when_version_is_inactive(self, clean_project_resources):
+        version = get(
+            Version,
+            project=self.project,
+            active=True,
+        )
+
+        url = reverse('project_version_detail', args=(version.project.slug, version.slug))
+
+        self.client.force_login(self.user)
+
+        r = self.client.post(
+            url,
+            data={
+                'active': True,
+                'privacy_level': PRIVATE,
+            },
+        )
+        self.assertTrue(r.status_code, 200)
+        clean_project_resources.assert_not_called()
+
+        r = self.client.post(
+            url,
+            data={
+                'active': False,
+                'privacy_level': PRIVATE,
+            },
+        )
+        self.assertTrue(r.status_code, 200)
+        clean_project_resources.assert_called_once()

readthedocs/search/utils.py

@@ -43,35 +43,41 @@ def index_new_files(model, version, build):
         log.exception('Unable to index a subset of files. Continuing.')
 
 
-def remove_indexed_files(model, version, build):
+def remove_indexed_files(model, project_slug, version_slug=None, build_id=None):
     """
-    Remove files from the version from the search index.
+    Remove files from `version_slug` of `project_slug` from the search index.
 
-    This excludes files from the current build.
+    :param model: Class of the model to be deleted.
+    :param project_slug: Project slug.
+    :param version_slug: Version slug. If isn't given,
+                    all index from `project` are deleted.
+    :param build_id: Build id. If isn't given, all index from `version` are deleted.
     """
 
     if not DEDConfig.autosync_enabled():
         log.info(
             'Autosync disabled, skipping removal from the search index for: %s:%s',
-            version.project.slug,
-            version.slug,
+            project_slug,
+            version_slug,
         )
         return
 
     try:
         document = list(registry.get_documents(models=[model]))[0]
         log.info(
             'Deleting old files from search index for: %s:%s',
-            version.project.slug,
-            version.slug,
+            project_slug,
+            version_slug,
         )
-        (
+        documents = (
             document().search()
-            .filter('term', project=version.project.slug)
-            .filter('term', version=version.slug)
-            .exclude('term', build=build)
-            .delete()
+            .filter('term', project=project_slug)
         )
+        if version_slug:
+            documents.filter('term', version=version_slug)
+        if build_id:
+            documents.exclude('term', build=build_id)
+        documents.delete()
     except Exception:
         log.exception('Unable to delete a subset of files. Continuing.')