From 17219e05e159a5fe34da0cab305bea7b6738c456 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 2 Sep 2025 18:24:10 -0500 Subject: [PATCH 1/4] Projects: improve deletion --- readthedocs/builds/models.py | 1 + readthedocs/projects/models.py | 10 ++++++++++ readthedocs/projects/tasks/utils.py | 7 +++++-- readthedocs/projects/tests/test_models.py | 19 ++++++++++++++++++- 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index de8528be2fe..a9239296792 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -609,6 +609,7 @@ class Build(models.Model): related_name="builds", on_delete=models.CASCADE, ) + # TODO: optmize deletion. version = models.ForeignKey( Version, verbose_name=_("Version"), diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 02bcbe6ad9c..f3ee03e8533 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -168,6 +168,7 @@ class AddonsConfig(TimeStampedModel): # https://github.com/readthedocs/addons/pull/415 options_load_when_embedded = models.BooleanField(default=False) + # TODO: see how wen can optimize deletion when the project is deleted. options_base_version = models.ForeignKey( "builds.Version", verbose_name=_("Base version to compare against (eg. DocDiff, File Tree Diff)"), @@ -691,6 +692,15 @@ def save(self, *args, **kwargs): def delete(self, *args, **kwargs): from readthedocs.projects.tasks.utils import clean_project_resources + qs = self.page_views.all() + qs._raw_delete(qs.db) + + qs = self.imported_files.all() + qs._raw_delete(qs.db) + + qs = self.search_queries.all() + qs._raw_delete(qs.db) + # Remove extra resources clean_project_resources(self) diff --git a/readthedocs/projects/tasks/utils.py b/readthedocs/projects/tasks/utils.py index 52a23882e36..94155293624 100644 --- a/readthedocs/projects/tasks/utils.py +++ b/readthedocs/projects/tasks/utils.py @@ -110,9 +110,12 @@ def clean_project_resources(project, version=None, version_slug=None): # Remove imported files if version: - version.imported_files.all().delete() + qs = version.imported_files.all() + qs._raw_delete(qs.db) else: - project.imported_files.all().delete() + qs = project.imported_files.all() + qs._raw_delete(qs.db) + # project.imported_files.all().delete() @app.task() diff --git a/readthedocs/projects/tests/test_models.py b/readthedocs/projects/tests/test_models.py index 6f6512a961f..4bc948aedda 100644 --- a/readthedocs/projects/tests/test_models.py +++ b/readthedocs/projects/tests/test_models.py @@ -3,7 +3,10 @@ from django.test import TestCase from django_dynamic_fixture import get -from readthedocs.projects.models import Feature, Project +from readthedocs.analytics.models import PageView +from readthedocs.builds.models import Build, Version +from readthedocs.projects.models import Feature, ImportedFile, Project +from readthedocs.search.models import SearchQuery class TestURLPatternsUtils(TestCase): @@ -137,3 +140,17 @@ def test_proxied_api_prefix(self): self.assertEqual(self.project.proxied_api_url, "prefix/_/") self.assertEqual(self.project.proxied_api_host, "/prefix/_") self.assertEqual(self.project.proxied_api_prefix, "/prefix/") + + def test_number_of_queries_on_project_deletion(self): + for i in range(5): + version = get(Version, project=self.project, slug=f"subproject-{i}", active=True, built=True) + for _ in range(50): + get(PageView, project=self.project, version=version) + get(ImportedFile, project=self.project, version=version) + get(SearchQuery, project=self.project, version=version) + # TODO: we should also see if we can delete buildss/buildcommand results efficiently. + get(Build, project=self.project, version=version) + + # This used to be 57 queries! + with self.assertNumQueries(51): + self.project.delete() From d6e3cf8270e9ffb90838a808642537a6f5ab77ec Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 2 Sep 2025 18:25:31 -0500 Subject: [PATCH 2/4] This is already done in clean_project_resources --- readthedocs/projects/models.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index f3ee03e8533..1abbe057fc5 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -695,9 +695,6 @@ def delete(self, *args, **kwargs): qs = self.page_views.all() qs._raw_delete(qs.db) - qs = self.imported_files.all() - qs._raw_delete(qs.db) - qs = self.search_queries.all() qs._raw_delete(qs.db) From c4171309fe8f5ee909316a360645fa297c6737fe Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 3 Sep 2025 12:09:23 -0500 Subject: [PATCH 3/4] Updates --- readthedocs/builds/models.py | 1 - readthedocs/projects/models.py | 6 ++++-- readthedocs/projects/tasks/utils.py | 13 +++++++++++-- readthedocs/projects/tests/test_models.py | 3 +-- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index a9239296792..de8528be2fe 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -609,7 +609,6 @@ class Build(models.Model): related_name="builds", on_delete=models.CASCADE, ) - # TODO: optmize deletion. version = models.ForeignKey( Version, verbose_name=_("Version"), diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 1abbe057fc5..8261238608e 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -168,7 +168,6 @@ class AddonsConfig(TimeStampedModel): # https://github.com/readthedocs/addons/pull/415 options_load_when_embedded = models.BooleanField(default=False) - # TODO: see how wen can optimize deletion when the project is deleted. options_base_version = models.ForeignKey( "builds.Version", verbose_name=_("Base version to compare against (eg. DocDiff, File Tree Diff)"), @@ -692,9 +691,12 @@ def save(self, *args, **kwargs): def delete(self, *args, **kwargs): from readthedocs.projects.tasks.utils import clean_project_resources + # NOTE: We use _raw_delete to avoid Django fetching all objects + # before the deletion. Be careful when using _raw_delete, signals + # won't be sent, and can cause integrity problems if the model + # has relations with other models. qs = self.page_views.all() qs._raw_delete(qs.db) - qs = self.search_queries.all() qs._raw_delete(qs.db) diff --git a/readthedocs/projects/tasks/utils.py b/readthedocs/projects/tasks/utils.py index 94155293624..92eb5345535 100644 --- a/readthedocs/projects/tasks/utils.py +++ b/readthedocs/projects/tasks/utils.py @@ -87,6 +87,13 @@ def clean_project_resources(project, version=None, version_slug=None): sometimes to clean old resources. .. note:: + + This function shouldn't delete objects that can't be recreated + by re-activating the version (e.g. page views, search queries), + as it's used when a version is deactivated. + + .. note:: + This function is usually called just before deleting project. Make sure to not depend on the project object inside the tasks. """ @@ -108,14 +115,16 @@ def clean_project_resources(project, version=None, version_slug=None): version_slug=version_slug, ) - # Remove imported files + # NOTE: We use _raw_delete to avoid Django fetching all objects + # before the deletion. Be careful when using _raw_delete, signals + # won't be sent, and can cause integrity problems if the model + # has relations with other models. if version: qs = version.imported_files.all() qs._raw_delete(qs.db) else: qs = project.imported_files.all() qs._raw_delete(qs.db) - # project.imported_files.all().delete() @app.task() diff --git a/readthedocs/projects/tests/test_models.py b/readthedocs/projects/tests/test_models.py index 4bc948aedda..569b081ea66 100644 --- a/readthedocs/projects/tests/test_models.py +++ b/readthedocs/projects/tests/test_models.py @@ -151,6 +151,5 @@ def test_number_of_queries_on_project_deletion(self): # TODO: we should also see if we can delete buildss/buildcommand results efficiently. get(Build, project=self.project, version=version) - # This used to be 57 queries! - with self.assertNumQueries(51): + with self.assertNumQueries(48): self.project.delete() From c9a5a84c69a747beff41f8632998012bf3d547f5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 3 Sep 2025 12:25:31 -0500 Subject: [PATCH 4/4] Remove todo --- readthedocs/projects/tests/test_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/readthedocs/projects/tests/test_models.py b/readthedocs/projects/tests/test_models.py index 569b081ea66..2d59b289340 100644 --- a/readthedocs/projects/tests/test_models.py +++ b/readthedocs/projects/tests/test_models.py @@ -148,7 +148,6 @@ def test_number_of_queries_on_project_deletion(self): get(PageView, project=self.project, version=version) get(ImportedFile, project=self.project, version=version) get(SearchQuery, project=self.project, version=version) - # TODO: we should also see if we can delete buildss/buildcommand results efficiently. get(Build, project=self.project, version=version) with self.assertNumQueries(48):