Skip to content

Optimize database performance of the footer API #5530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions readthedocs/core/resolver.py
Original file line number Diff line number Diff line change
@@ -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:

Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
37 changes: 36 additions & 1 deletion readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)
Expand Down
50 changes: 50 additions & 0 deletions readthedocs/rtd_tests/tests/test_footer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')