From 5bcbbd8988876a4b810226bb0c0c73cfaff83c4e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 10 Jun 2019 22:59:35 -0500 Subject: [PATCH 1/4] Use querysets from the class not from an instance When filtering using `public` and using a user, the queryset hit this https://github.com/rtfd/readthedocs.org/blob/45df7fd0da44be9eab3c0cb2888f6a9a15421fc5/readthedocs/builds/querysets.py#L22-L24 When the user is authenticated, we call to `get_objects_for_user` which gets all the versions from all the user's projects. Overriding any previous filter (`project.versions` in this case) We don't see this in production because we serve from another domain. And we don't see this in the corporate site because we override the serve_docs view. Fix #4350 Closes #4356 --- readthedocs/api/v2/views/footer_views.py | 2 +- readthedocs/core/views/serve.py | 12 +++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/readthedocs/api/v2/views/footer_views.py b/readthedocs/api/v2/views/footer_views.py index 09709ac1bb1..38cb8b56d83 100644 --- a/readthedocs/api/v2/views/footer_views.py +++ b/readthedocs/api/v2/views/footer_views.py @@ -25,7 +25,7 @@ def get_version_compare_data(project, base_version=None): :param base_version: We assert whether or not the base_version is also the highest version in the resulting "is_highest" value. """ - versions_qs = project.versions.public().filter(active=True) + versions_qs = Version.objects.public(project=project) # Take preferences over tags only if the project has at least one tag if versions_qs.filter(type=TAG).exists(): diff --git a/readthedocs/core/views/serve.py b/readthedocs/core/views/serve.py index 390063f256a..cd5ab8fdb64 100644 --- a/readthedocs/core/views/serve.py +++ b/readthedocs/core/views/serve.py @@ -202,7 +202,11 @@ def serve_docs( if not version_slug: version_slug = project.get_default_version() try: - version = project.versions.public(request.user).get(slug=version_slug) + version = ( + Version.objects + .public(user=request.user, project=project) + .get(slug=version_slug) + ) except Version.DoesNotExist: # Properly raise a 404 if the version doesn't exist (or is inactive) and # a 401 if it does @@ -409,8 +413,10 @@ def changefreqs_generator(): if project.translations.exists(): for translation in project.translations.all(): - translation_versions = translation.versions.public( - ).values_list('slug', flat=True) + translation_versions = ( + Version.objects.public(project=translation) + .values_list('slug', flat=True) + ) if version.slug in translation_versions: href = project.get_docs_url( version_slug=version.slug, From 8808a6e4d61b40c9ae62f466f9b14d2de71def93 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 11 Jun 2019 00:13:10 -0500 Subject: [PATCH 2/4] A test, of course --- .../rtd_tests/tests/test_doc_serving.py | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_doc_serving.py b/readthedocs/rtd_tests/tests/test_doc_serving.py index 134b191235c..4f6cb93990c 100644 --- a/readthedocs/rtd_tests/tests/test_doc_serving.py +++ b/readthedocs/rtd_tests/tests/test_doc_serving.py @@ -1,16 +1,16 @@ - import os import django_dynamic_fixture as fixture import mock from django.conf import settings from django.contrib.auth.models import User -from django.http import Http404 +from django.http import Http404, HttpResponse from django.test import RequestFactory, TestCase from django.test.utils import override_settings from django.urls import reverse from mock import mock_open, patch +from readthedocs.builds.constants import LATEST from readthedocs.builds.models import Version from readthedocs.core.middleware import SubdomainMiddleware from readthedocs.core.views import server_error_404_subdomain @@ -293,3 +293,39 @@ def test_sitemap_xml(self): # hreflang should use hyphen instead of underscore # in language and country value. (zh_CN should be zh-CN) self.assertContains(response, 'zh-CN') + + @override_settings( + PYTHON_MEDIA=True, + USE_SUBDOMAIN=False, + ) + @patch( + 'readthedocs.core.views.serve._serve_symlink_docs', + new=mock.MagicMock(return_value=HttpResponse(content_type='text/html')), + ) + def test_user_with_multiple_projects_serve_from_same_domain(self): + project = fixture.get( + Project, + main_language_project=None, + users=[self.eric], + ) + other_project = fixture.get( + Project, + main_language_project=None, + users=[self.eric], + ) + + # Trigger the save method of the versions + project.versions.get(slug=LATEST).save() + other_project.versions.get(slug=LATEST).save() + + # Two projects of one owner with versions with the same slug + self.assertIn(self.eric, project.users.all()) + self.assertIn(self.eric, other_project.users.all()) + self.assertTrue(project.versions.filter(slug=LATEST).exists()) + self.assertTrue(other_project.versions.filter(slug=LATEST).exists()) + + self.client.force_login(self.eric) + request = self.client.get( + f'/docs/{project.slug}/en/latest/' + ) + self.assertEqual(request.status_code, 200) From d7e228b6db90639716b90f6897c6e28ef05379b2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 11 Jun 2019 00:17:59 -0500 Subject: [PATCH 3/4] Increase expected query numbers --- readthedocs/rtd_tests/tests/test_footer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_footer.py b/readthedocs/rtd_tests/tests/test_footer.py index 34c1fec1f92..d1db902892b 100644 --- a/readthedocs/rtd_tests/tests/test_footer.py +++ b/readthedocs/rtd_tests/tests/test_footer.py @@ -233,7 +233,7 @@ class TestFooterPerformance(APITestCase): # The expected number of queries for generating the footer # This shouldn't increase unless we modify the footer API - EXPECTED_QUERIES = 9 + EXPECTED_QUERIES = 10 def setUp(self): self.pip = Project.objects.get(slug='pip') From 394c2b3210016576dcfe1f5cc06d66dfcacefb9b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 13 Jun 2019 10:25:27 -0500 Subject: [PATCH 4/4] Fix performance tests Thanks @davidfischer! --- readthedocs/api/v2/views/footer_views.py | 3 +++ readthedocs/rtd_tests/tests/test_footer.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/readthedocs/api/v2/views/footer_views.py b/readthedocs/api/v2/views/footer_views.py index 38cb8b56d83..04d4fe8210b 100644 --- a/readthedocs/api/v2/views/footer_views.py +++ b/readthedocs/api/v2/views/footer_views.py @@ -31,6 +31,9 @@ def get_version_compare_data(project, base_version=None): if versions_qs.filter(type=TAG).exists(): versions_qs = versions_qs.filter(type=TAG) + # Optimization + versions_qs = versions_qs.select_related('project') + highest_version_obj, highest_version_comparable = highest_version( versions_qs, ) diff --git a/readthedocs/rtd_tests/tests/test_footer.py b/readthedocs/rtd_tests/tests/test_footer.py index d1db902892b..34c1fec1f92 100644 --- a/readthedocs/rtd_tests/tests/test_footer.py +++ b/readthedocs/rtd_tests/tests/test_footer.py @@ -233,7 +233,7 @@ class TestFooterPerformance(APITestCase): # The expected number of queries for generating the footer # This shouldn't increase unless we modify the footer API - EXPECTED_QUERIES = 10 + EXPECTED_QUERIES = 9 def setUp(self): self.pip = Project.objects.get(slug='pip')