Merge pull request #5530 from rtfd/davidfischer/optimize-footer-api

Optimize database performance of the footer API

Author: ericholscher

readthedocs/core/resolver.py

@@ -1,12 +1,15 @@
 """URL resolver for documentation."""
 
+import logging
 from urllib.parse import urlunparse
 
 from django.conf import settings
 
 from readthedocs.core.utils.extend import SettingsOverrideObject
 from readthedocs.projects.constants import PRIVATE, PUBLIC
 
+log = logging.getLogger(__name__)
+
 
 class ResolverBase:
 
@@ -99,7 +102,7 @@ def resolve_path(
             private=None,
     ):
         """Resolve a URL with a subset of fields defined."""
-        cname = cname or project.domains.filter(canonical=True).first()
+        cname = cname or project.get_canonical_custom_domain()
         version_slug = version_slug or project.get_default_version()
         language = language or project.language
 
@@ -115,7 +118,7 @@ def resolve_path(
         # translations, only loop twice to avoid sticking in the loop
         for _ in range(0, 2):
             main_language_project = current_project.main_language_project
-            relation = current_project.superprojects.first()
+            relation = current_project.get_parent_relationship()
 
             if main_language_project:
                 current_project = main_language_project
@@ -147,7 +150,7 @@ def resolve_path(
     def resolve_domain(self, project, private=None):
         # pylint: disable=unused-argument
         canonical_project = self._get_canonical_project(project)
-        domain = self._get_project_custom_domain(canonical_project)
+        domain = canonical_project.get_canonical_custom_domain()
         if domain:
             return domain.domain
 
@@ -167,7 +170,7 @@ def resolve(
             private = self._get_private(project, version_slug)
 
         canonical_project = self._get_canonical_project(project)
-        custom_domain = self._get_project_custom_domain(canonical_project)
+        custom_domain = canonical_project.get_canonical_custom_domain()
         use_custom_domain = self._use_custom_domain(custom_domain)
 
         if use_custom_domain:
@@ -213,9 +216,9 @@ def _get_canonical_project(self, project, projects=None):
             projects = [project]
         else:
             projects.append(project)
-        next_project = None
 
-        relation = project.superprojects.first()
+        next_project = None
+        relation = project.get_parent_relationship()
         if project.main_language_project:
             next_project = project.main_language_project
         elif relation:
@@ -232,9 +235,6 @@ def _get_project_subdomain(self, project):
             subdomain_slug = project.slug.replace('_', '-')
             return '{}.{}'.format(subdomain_slug, public_domain)
 
-    def _get_project_custom_domain(self, project):
-        return project.domains.filter(canonical=True).first()
-
     def _get_private(self, project, version_slug):
         from readthedocs.builds.models import Version
         try:

readthedocs/projects/models.py

@@ -10,6 +10,7 @@
 from django.contrib.auth.models import User
 from django.core.files.storage import get_storage_class
 from django.db import models
+from django.db.models import Prefetch
 from django.urls import NoReverseMatch, reverse
 from django.utils.translation import ugettext_lazy as _
 from django_extensions.db.models import TimeStampedModel
@@ -882,7 +883,21 @@ def ordered_active_versions(self, user=None):
         }
         if user:
             kwargs['user'] = user
-        versions = Version.objects.public(**kwargs)
+        versions = Version.objects.public(**kwargs).select_related(
+            'project',
+            'project__main_language_project',
+        ).prefetch_related(
+            Prefetch(
+                'project__superprojects',
+                ProjectRelationship.objects.all().select_related('parent'),
+                to_attr='_superprojects',
+            ),
+            Prefetch(
+                'project__domains',
+                Domain.objects.filter(canonical=True),
+                to_attr='_canonical_domains',
+            ),
+        )
         return sort_version_aware(versions)
 
     def all_active_versions(self):
@@ -980,6 +995,26 @@ def add_subproject(self, child, alias=None):
     def remove_subproject(self, child):
         ProjectRelationship.objects.filter(parent=self, child=child).delete()
 
+    def get_parent_relationship(self):
+        """Get the parent project relationship or None if this is a top level project"""
+        if hasattr(self, '_superprojects'):
+            # Cached parent project relationship
+            if self._superprojects:
+                return self._superprojects[0]
+            return None
+
+        return self.superprojects.select_related('parent').first()
+
+    def get_canonical_custom_domain(self):
+        """Get the canonical custom domain or None"""
+        if hasattr(self, '_canonical_domains'):
+            # Cached custom domains
+            if self._canonical_domains:
+                return self._canonical_domains[0]
+            return None
+
+        return self.domains.filter(canonical=True).first()
+
     @property
     def features(self):
         return Feature.objects.for_project(self)

readthedocs/rtd_tests/tests/test_footer.py

@@ -225,3 +225,53 @@ def test_highest_version_without_tags(self):
         }
         returned_data = get_version_compare_data(self.pip, base_version)
         self.assertDictEqual(valid_data, returned_data)
+
+
+class TestFooterPerformance(APITestCase):
+    fixtures = ['test_data']
+    url = '/api/v2/footer_html/?project=pip&version=latest&page=index&docroot=/'
+    factory = APIRequestFactory()
+
+    # The expected number of queries for generating the footer
+    # This shouldn't increase unless we modify the footer API
+    EXPECTED_QUERIES = 9
+
+    def setUp(self):
+        self.pip = Project.objects.get(slug='pip')
+        self.pip.versions.create_latest()
+
+    def render(self):
+        request = self.factory.get(self.url)
+        response = footer_html(request)
+        response.render()
+        return response
+
+    def test_version_queries(self):
+        # The number of Versions shouldn't impact the number of queries
+        with self.assertNumQueries(self.EXPECTED_QUERIES):
+            response = self.render()
+            self.assertContains(response, '0.8.1')
+
+        for patch in range(3):
+            identifier = '0.99.{}'.format(patch)
+            self.pip.versions.create(
+                verbose_name=identifier,
+                identifier=identifier,
+                type=TAG,
+                active=True,
+            )
+
+        with self.assertNumQueries(self.EXPECTED_QUERIES):
+            response = self.render()
+            self.assertContains(response, '0.99.0')
+
+    def test_domain_queries(self):
+        # Setting up a custom domain shouldn't impact the number of queries
+        self.pip.domains.create(
+            domain='http://docs.foobar.com',
+            canonical=True,
+        )
+
+        with self.assertNumQueries(self.EXPECTED_QUERIES):
+            response = self.render()
+            self.assertContains(response, 'docs.foobar.com')