Search: optimize queryset when searching for subprojects with organizations

readthedocs/api/v3/tests/test_projects.py

@@ -212,7 +212,10 @@ def test_projects_detail_other_user(self):
         response = self.client.get(url, data)
         self.assertEqual(response.status_code, 404)
 
-    @override_settings(ALLOW_PRIVATE_REPOS=True)
+    @override_settings(
+        ALLOW_PRIVATE_REPOS=True,
+        RTD_ALLOW_ORGANIZATIONS=True,
+    )
     def test_own_projects_detail_privacy_levels_enabled(self):
         url = reverse(
             "projects-detail",
@@ -227,10 +230,12 @@ def test_own_projects_detail_privacy_levels_enabled(self):
         self.client.credentials(HTTP_AUTHORIZATION=f"Token {self.token.key}")
         response = self.client.get(url, query_params)
         self.assertEqual(response.status_code, 200)
-        self.assertDictEqual(
-            response.json(),
-            self._get_response_dict("projects-detail"),
-        )
+
+        expected = self._get_response_dict("projects-detail")
+        # No users when organzations are enabled
+        expected.pop("users")
+
+        self.assertDictEqual(response.json(), expected)
 
         self.project.privacy_level = "private"
         self.project.external_builds_privacy_level = "private"
@@ -245,6 +250,10 @@ def test_own_projects_detail_privacy_levels_enabled(self):
         # We don't care about the modified date.
         expected.pop("modified")
         response.pop("modified")
+
+        # No users when organzations are enabled
+        expected.pop("users")
+
         self.assertDictEqual(response, expected)
 
     def test_projects_superproject_anonymous_user(self):

readthedocs/api/v3/tests/test_subprojects.py

@@ -155,7 +155,6 @@ def test_projects_subprojects_detail_other_user(self):
 
     def test_projects_subprojects_list_post(self):
         newproject = self._create_new_project()
-        self.organization.projects.add(newproject)
 
         self.assertEqual(self.project.subprojects.count(), 1)
         url = reverse(

readthedocs/audit/models.py

@@ -223,7 +223,7 @@ def save(self, **kwargs):
         if self.project:
             self.log_project_id = self.project.id
             self.log_project_slug = self.project.slug
-            organization = self.project.organizations.first()
+            organization = self.project.organization
             if organization:
                 self.organization = organization
         if self.organization:

readthedocs/audit/tests/test_models.py

@@ -1,12 +1,13 @@
 from django.contrib.auth.models import User
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django_dynamic_fixture import get
 
 from readthedocs.audit.models import AuditLog
 from readthedocs.organizations.models import Organization, OrganizationOwner
 from readthedocs.projects.models import Project
 
 
+@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
 class TestAuditModels(TestCase):
     def setUp(self):
         self.user = get(User)

readthedocs/audit/tests/test_tasks.py

@@ -1,7 +1,7 @@
 from unittest import mock
 
 from django.contrib.auth.models import User
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django.utils import timezone
 from django_dynamic_fixture import get
 
@@ -11,6 +11,7 @@
 from readthedocs.projects.models import Project
 
 
+@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
 class AuditTasksTest(TestCase):
     def setUp(self):
         self.user = get(User)

readthedocs/builds/querysets.py

@@ -246,7 +246,7 @@ def concurrent(self, project):
 
         # If the project belongs to an organization, count all the projects
         # from this organization as well
-        organization = project.organizations.first()
+        organization = project.organization
         if organization:
             query |= Q(project__in=organization.projects.all())
 

readthedocs/builds/tests/test_build_queryset.py

@@ -1,3 +1,4 @@
+from django.test import override_settings
 import django_dynamic_fixture as fixture
 import pytest
 
@@ -79,6 +80,7 @@ def test_concurrent_builds_translations(self):
         assert (True, 4, 4) == Build.objects.concurrent(translation)
         assert (True, 4, 4) == Build.objects.concurrent(project)
 
+    @override_settings(RTD_ALLOW_ORGANIZATIONS=True)
     def test_concurrent_builds_organization(self):
         organization = fixture.get(
             Organization,
@@ -111,6 +113,7 @@ def test_concurrent_builds_organization(self):
             )
         assert (True, 6, 4) == Build.objects.concurrent(project)
 
+    @override_settings(RTD_ALLOW_ORGANIZATIONS=True)
     def test_concurrent_builds_organization_limited(self):
         organization = fixture.get(
             Organization,
@@ -138,6 +141,7 @@ def test_concurrent_builds_organization_limited(self):
         # from ``project_with_builds`` as well
         assert (False, 2, 10) == Build.objects.concurrent(project_without_builds)
 
+    @override_settings(RTD_ALLOW_ORGANIZATIONS=True)
     def test_concurrent_builds_organization_and_project_limited(self):
         organization = fixture.get(
             Organization,

readthedocs/core/permissions.py

@@ -118,7 +118,7 @@ def owners(cls, obj):
 
         if isinstance(obj, Project):
             if settings.RTD_ALLOW_ORGANIZATIONS:
-                obj = obj.organizations.first()
+                obj = obj.organization
             else:
                 return obj.users.all()
 

readthedocs/core/resolver.py

@@ -198,11 +198,16 @@ def _get_project_domain(self, project, external_version_slug=None, use_canonical
         domain = self._get_project_subdomain(canonical_project)
         if external_version_slug:
             domain = self._get_external_subdomain(canonical_project, external_version_slug)
-        elif use_canonical_domain and self._use_cname(canonical_project):
+        # NOTE: call _use_cname only if the project has a canonical custom domain.
+        # Calling _use_cname is more expensive when we have organizations.
+        elif (
+            use_canonical_domain
+            and canonical_project.canonical_custom_domain
+            and self._use_cname(canonical_project)
+        ):
             domain_object = canonical_project.canonical_custom_domain
-            if domain_object:
-                use_https = domain_object.https
-                domain = domain_object.domain
+            use_https = domain_object.https
+            domain = domain_object.domain
 
         return domain, use_https
 
@@ -365,4 +370,22 @@ def _fix_filename(self, filename):
 
     def _use_cname(self, project):
         """Test if to allow direct serving for project on CNAME."""
+        # If the project belongs to an organization, use the other
+        # method, to allow caching the result for that organization.
+        if project.organization:
+            return self._organization_allows_custom_domain(project.organization)
+
         return bool(get_feature(project, feature_type=TYPE_CNAME))
+
+    @cache
+    def _organization_allows_custom_domain(self, organization):
+        """
+        Test if the organization allows custom domains.
+
+        .. note::
+
+           This is done on a separate method, so we can cache the result
+           for each organization. This is useful when resolving multiple
+           projects from the same organization.
+        """
+        return bool(get_feature(organization, feature_type=TYPE_CNAME))

readthedocs/invitations/tests/test_views.py

@@ -1,5 +1,5 @@
 from django.contrib.auth.models import User
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django.urls import reverse
 from django.utils import timezone
 from django_dynamic_fixture import get
@@ -10,6 +10,7 @@
 from readthedocs.projects.models import Project
 
 
+@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
 class TestViews(TestCase):
     def setUp(self):
         self.user = get(User)

readthedocs/organizations/tasks.py

@@ -18,7 +18,7 @@ def mark_organization_assets_not_cleaned(build_pk):
         log.debug("Build does not exist.", build_pk=build_pk)
         return
 
-    organization = build.project.organizations.first()
+    organization = build.project.organization
     if organization and organization.artifacts_cleaned:
         log.info("Marking organization as not cleaned.", origanization_slug=organization.slug)
         organization.artifacts_cleaned = False

readthedocs/projects/models.py

@@ -1490,7 +1490,7 @@ def get_subproject_candidates(self, user):
 
         Both projects need to share the same owner/admin.
         """
-        organization = self.organizations.first()
+        organization = self.organization
         queryset = (
             Project.objects.for_admin_user(user)
             .filter(organizations=organization)
@@ -1500,8 +1500,17 @@ def get_subproject_candidates(self, user):
         )
         return queryset
 
-    @property
+    @cached_property
     def organization(self):
+        # If organizations aren't supported,
+        # we don't need to query the database.
+        if not settings.RTD_ALLOW_ORGANIZATIONS:
+            return None
+
+        if hasattr(self, "_organizations"):
+            if self._organizations:
+                return self._organizations[0]
+            return None
         return self.organizations.first()
 
     @property
@@ -1780,7 +1789,7 @@ def get_payload(self, version, build, event):
             return None
 
         project = version.project
-        organization = project.organizations.first()
+        organization = project.organization
 
         organization_name = ""
         organization_slug = ""

readthedocs/projects/querysets.py

@@ -5,6 +5,7 @@
 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
@@ -85,7 +86,7 @@ def is_active(self, project):
         """
         spam_project = False
         any_owner_banned = any(u.profile.banned for u in project.users.all())
-        organization = project.organizations.first()
+        organization = project.organization
 
         if "readthedocsext.spamfighting" in settings.INSTALLED_APPS:
             from readthedocsext.spamfighting.utils import spam_score  # noqa
@@ -123,7 +124,7 @@ def max_concurrent_builds(self, project):
         from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS
 
         max_concurrent_organization = None
-        organization = project.organizations.first()
+        organization = project.organization
         if organization:
             max_concurrent_organization = organization.max_concurrent_builds
 
@@ -156,6 +157,33 @@ def prefetch_latest_build(self):
             _has_good_build=Exists(Build.internal.filter(project=OuterRef("pk"), success=True))
         )
 
+    def prefetch_organization(self, select_related: list[str] | None = None):
+        """
+        Prefetch the organizations related to the projects.
+
+        :param select_related: List of related fields to select_related
+         when fetching the organizations.
+
+        .. note::
+
+           This would normally be a select_related call,
+           but since our relationship is ManyToMany,
+           we need to use prefetch_related.
+        """
+        # If organizations aren't supported,
+        # we don't need to query the database.
+        if not settings.RTD_ALLOW_ORGANIZATIONS:
+            return self
+
+        from readthedocs.organizations.models import Organization
+
+        query = Organization.objects.all()
+        if select_related:
+            query = query.select_related(*select_related)
+        return self.prefetch_related(
+            Prefetch("organizations", queryset=query, to_attr="_organizations")
+        )
+
     # Aliases
 
     def dashboard(self, user):

readthedocs/proxito/tests/test_hosting.py

@@ -39,6 +39,8 @@
     GLOBAL_ANALYTICS_CODE=None,
     RTD_ALLOW_ORGANIZATIONS=False,
     RTD_EXTERNAL_VERSION_DOMAIN="dev.readthedocs.build",
+    RTD_PRODUCTS={},
+    ALLOW_PRIVATE_REPOS=False,
 )
 @pytest.mark.proxito
 class TestReadTheDocsConfigJson(TestCase):

readthedocs/rtd_tests/tests/test_project_querysets.py

@@ -2,7 +2,7 @@
 
 import django_dynamic_fixture as fixture
 from django.contrib.auth.models import User
-from django.test import TestCase
+from django.test import TestCase, override_settings
 from django_dynamic_fixture import get
 
 from readthedocs.organizations.models import Organization
@@ -90,6 +90,7 @@ def test_subproject_queryset_as_manager_gets_correct_class(self):
             "ManagerFromParentRelatedProjectQuerySet",
         )
 
+    @override_settings(RTD_ALLOW_ORGANIZATIONS=True)
     def test_is_active(self):
         project = get(Project, skip=False)
         self.assertTrue(Project.objects.is_active(project))
@@ -109,10 +110,14 @@ def test_is_active(self):
 
         organization = get(Organization, disabled=False)
         organization.projects.add(project)
+        # Clear cached organization
+        del project.organization
         self.assertTrue(Project.objects.is_active(project))
 
         organization.disabled = True
         organization.save()
+        # Clear cached organization
+        del project.organization
         self.assertFalse(Project.objects.is_active(project))
 
     def test_dashboard(self):
@@ -188,6 +193,7 @@ def test_for_user_and_viewer_same_user(self):
         self.assertEqual(query.count(), len(projects))
         self.assertEqual(set(query), projects)
 
+    @override_settings(RTD_ALLOW_ORGANIZATIONS=True)
     def test_only_owner(self):
         user = get(User)
         another_user = get(User)

readthedocs/search/api/v2/serializers.py

@@ -98,18 +98,19 @@ class PageSearchSerializer(serializers.Serializer):
     def __init__(self, *args, projects=None, **kwargs):
         if projects:
             context = kwargs.setdefault("context", {})
+            # NOTE: re-using the resolver for different projects usually doesn't save queries,
+            # but when projects belong to the same organization, it does.
+            resolver = Resolver()
+            context["resolver"] = resolver
             context["projects_data"] = {
-                project.slug: self._build_project_data(project, version=version)
+                project.slug: self._build_project_data(project, version=version, resolver=resolver)
                 for project, version in projects
             }
         super().__init__(*args, **kwargs)
 
-    def _build_project_data(self, project, version):
+    def _build_project_data(self, project, version, resolver: Resolver):
         """Build a `ProjectData` object given a project and its version."""
-        # NOTE: re-using the resolver doesn't help here,
-        # as this method is called just once per project,
-        # re-using the resolver is useful when resolving the same project multiple times.
-        url = Resolver().resolve_version(project, version)
+        url = resolver.resolve_version(project, version)
         project_alias = None
         if project.parent_relationship:
             project_alias = project.parent_relationship.alias
@@ -135,6 +136,7 @@ def _get_project_data(self, obj):
         if project_data:
             return project_data
 
+        resolver = self.context.get("resolver", Resolver())
         version = (
             Version.objects.filter(project__slug=obj.project, slug=obj.version)
             .select_related("project")
@@ -143,7 +145,9 @@ def _get_project_data(self, obj):
         if version:
             project = version.project
             projects_data = self.context.setdefault("projects_data", {})
-            projects_data[obj.project] = self._build_project_data(project, version=version)
+            projects_data[obj.project] = self._build_project_data(
+                project, version=version, resolver=resolver
+            )
             return projects_data[obj.project]
         return None