From 363dccb257dde4b76fbd2465ce6609be8c967621 Mon Sep 17 00:00:00 2001 From: David Fischer Date: Fri, 22 Mar 2019 15:55:10 -0700 Subject: [PATCH 1/2] Optimize database performance of the footer API --- readthedocs/core/resolver.py | 18 +++++------ readthedocs/projects/models.py | 37 +++++++++++++++++++++- readthedocs/rtd_tests/tests/test_footer.py | 34 ++++++++++++++++++++ 3 files changed, 79 insertions(+), 10 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 6892bc3f70c..6c20d1e79de 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -1,5 +1,6 @@ """URL resolver for documentation.""" +import logging from urllib.parse import urlunparse from django.conf import settings @@ -7,6 +8,8 @@ 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: diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index fab466a58f5..d877fcb879e 100644 --- a/readthedocs/projects/models.py +++ b/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) diff --git a/readthedocs/rtd_tests/tests/test_footer.py b/readthedocs/rtd_tests/tests/test_footer.py index 1530b7bd1bc..e8438c85394 100644 --- a/readthedocs/rtd_tests/tests/test_footer.py +++ b/readthedocs/rtd_tests/tests/test_footer.py @@ -225,3 +225,37 @@ 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 = 2 + + def setUp(self): + self.pip = Project.objects.get(slug='pip') + + 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): + self.render() + + 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): + self.render() From 7d8813f6d91004a31e836d43395ca404d9211a12 Mon Sep 17 00:00:00 2001 From: David Fischer Date: Fri, 22 Mar 2019 16:21:14 -0700 Subject: [PATCH 2/2] Ensure tests are actually executed --- readthedocs/rtd_tests/tests/test_footer.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_footer.py b/readthedocs/rtd_tests/tests/test_footer.py index e8438c85394..da78ec3b0c0 100644 --- a/readthedocs/rtd_tests/tests/test_footer.py +++ b/readthedocs/rtd_tests/tests/test_footer.py @@ -234,10 +234,11 @@ class TestFooterPerformance(APITestCase): # The expected number of queries for generating the footer # This shouldn't increase unless we modify the footer API - EXPECTED_QUERIES = 2 + 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) @@ -248,7 +249,21 @@ def render(self): def test_version_queries(self): # The number of Versions shouldn't impact the number of queries with self.assertNumQueries(self.EXPECTED_QUERIES): - self.render() + 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 @@ -258,4 +273,5 @@ def test_domain_queries(self): ) with self.assertNumQueries(self.EXPECTED_QUERIES): - self.render() + response = self.render() + self.assertContains(response, 'docs.foobar.com')