Dashboard: don't prefetch latest build (#12400)

get_latest_build was doing something different than the prefetched
value, looks like we never actually used this function with
`finished=True`, so I went ahead and renamed it and made it a property
so we can cache it.

Author: stsewd

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)
 
 

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

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,25 +133,26 @@ 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::
 
             This should come after any filtering.
         """
         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))
         )
 

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:

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"

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