diff --git a/readthedocs/projects/filters.py b/readthedocs/projects/filters.py index 8f3ddb01781..2172c128b7e 100644 --- a/readthedocs/projects/filters.py +++ b/readthedocs/projects/filters.py @@ -138,11 +138,7 @@ def filter(self, qs, value): else: order_bys.append(field_ordered) - # Prefetch here not only prefetches the query, but it also changes how `get_latest_build` - # works. Normally from templates `project.get_latest_build` only returns - # the latest _finished_ build. But with prefetch, _all_ builds are - # considered and `get_latest_build` will pop the first off this list of - # _all_ builds. + # prefetch_latest_build does some extra optimizations to avoid additional queries. return qs.prefetch_latest_build().annotate(**annotations).order_by(*order_bys) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index b7b67332e97..0c0bece6407 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -645,9 +645,6 @@ class Project(models.Model): blank=True, ) - # Property used for storing the latest build for a project when prefetching - LATEST_BUILD_CACHE = "_latest_build" - class Meta: ordering = ("slug",) verbose_name = _("project") @@ -1100,23 +1097,10 @@ def full_find(self, filename, version): matches.append(os.path.join(root, match)) return matches - def get_latest_build(self, finished=True): - """ - Get latest build for project. - - :param finished: Return only builds that are in a finished state - """ - # Check if there is `_latest_build` attribute in the Queryset. - # Used for Database optimization. - if hasattr(self, self.LATEST_BUILD_CACHE): - if self._latest_build: - return self._latest_build[0] - return None - - kwargs = {"type": "html"} - if finished: - kwargs["state"] = "finished" - return self.builds(manager=INTERNAL).filter(**kwargs).first() + @cached_property + def latest_internal_build(self): + """Get the latest internal build for the project.""" + return self.builds(manager=INTERNAL).select_related("version").first() def active_versions(self): from readthedocs.builds.models import Version diff --git a/readthedocs/projects/querysets.py b/readthedocs/projects/querysets.py index 7a4d62c17d8..8b9a2883dd1 100644 --- a/readthedocs/projects/querysets.py +++ b/readthedocs/projects/querysets.py @@ -5,7 +5,6 @@ from django.db.models import Count from django.db.models import Exists from django.db.models import OuterRef -from django.db.models import Prefetch from django.db.models import Q from readthedocs.core.permissions import AdminPermission @@ -134,7 +133,7 @@ def max_concurrent_builds(self, project): def prefetch_latest_build(self): """ - Prefetch "latest build" for each project. + Prefetch and annotate to avoid N+1 queries. .. note:: @@ -142,17 +141,18 @@ def prefetch_latest_build(self): """ from readthedocs.builds.models import Build - # Prefetch the latest build for each project. - latest_build = Prefetch( - "builds", - Build.internal.select_related("version").order_by("-date")[:1], - to_attr=self.model.LATEST_BUILD_CACHE, - ) - query = self.prefetch_related(latest_build) + # NOTE: prefetching the latest build will perform worse than just + # accessing the latest build for each project. + # While prefetching reduces the number of queries, + # the query used to fetch the latest build can be quite expensive, + # specially in projects with lots of builds. + # Not prefetching here is fine, as this query is paginated by 15 + # items per page, so it will generate at most 15 queries. + # This annotation performs fine in all cases. # Annotate whether the project has a successful build or not, # to avoid N+1 queries when showing the build status. - return query.annotate( + return self.annotate( _has_good_build=Exists(Build.internal.filter(project=OuterRef("pk"), success=True)) ) diff --git a/readthedocs/projects/views/base.py b/readthedocs/projects/views/base.py index d7c14f21bd6..2a5e2a4e471 100644 --- a/readthedocs/projects/views/base.py +++ b/readthedocs/projects/views/base.py @@ -30,7 +30,7 @@ def get_context_data(self, **kwargs): # Show for the first few builds, return last build state if project.builds.count() <= 5: - onboard["build"] = project.get_latest_build(finished=False) + onboard["build"] = project.latest_internal_build if "github" in project.repo: onboard["provider"] = "github" elif "bitbucket" in project.repo: diff --git a/readthedocs/rtd_tests/tests/test_project.py b/readthedocs/rtd_tests/tests/test_project.py index a04b14c8b94..301d86df7ef 100644 --- a/readthedocs/rtd_tests/tests/test_project.py +++ b/readthedocs/rtd_tests/tests/test_project.py @@ -223,11 +223,11 @@ def test_has_good_build_excludes_external_versions(self): # Test that External Version is not considered for has_good_build. self.assertFalse(self.pip.has_good_build) - def test_get_latest_build_excludes_external_versions(self): + def test_latest_internal_build_excludes_external_versions(self): # Delete all versions excluding External Versions. self.pip.versions.exclude(type=EXTERNAL).delete() - # Test that External Version is not considered for get_latest_build. - self.assertEqual(self.pip.get_latest_build(), None) + # Test that External Version is not considered for latest_internal_build. + self.assertEqual(self.pip.latest_internal_build, None) def test_git_provider_github(self): self.pip.repo = "https://github.com/pypa/pip" diff --git a/readthedocs/rtd_tests/tests/test_project_views.py b/readthedocs/rtd_tests/tests/test_project_views.py index 8e33406a858..8e1d3b672c0 100644 --- a/readthedocs/rtd_tests/tests/test_project_views.py +++ b/readthedocs/rtd_tests/tests/test_project_views.py @@ -399,7 +399,10 @@ def test_dashboard_number_of_queries(self): state=BUILD_STATE_FINISHED, ) - with self.assertNumQueries(12): + # This number is bit higher, but for projects with lots of builds + # is better to have more queries than optimizing with a prefetch, + # see comment in prefetch_latest_build. + with self.assertNumQueries(27): r = self.client.get(reverse(("projects_dashboard"))) assert r.status_code == 200