perf: reduce number of queries for content tagging endpoints (openedx…

…#34200)

openedx/core/djangoapps/content_tagging/api.py

@@ -87,7 +87,7 @@ def set_taxonomy_orgs(
 
 def get_taxonomies_for_org(
     enabled=True,
-    org_owner: Organization | None = None,
+    org_short_name: str | None = None,
 ) -> QuerySet:
     """
     Generates a list of the enabled Taxonomies available for the given org, sorted by name.
@@ -100,7 +100,6 @@ def get_taxonomies_for_org(
     If you want the disabled Taxonomies, pass enabled=False.
     If you want all Taxonomies (both enabled and disabled), pass enabled=None.
     """
-    org_short_name = org_owner.short_name if org_owner else None
     return oel_tagging.get_taxonomies(enabled=enabled).filter(
         Exists(
             TaxonomyOrg.get_relationships(

openedx/core/djangoapps/content_tagging/models/base.py

@@ -64,16 +64,21 @@ def get_relationships(
 
     @classmethod
     def get_organizations(
-        cls, taxonomy: Taxonomy, rel_type: RelType
-    ) -> list[Organization]:
+        cls, taxonomy: Taxonomy, rel_type=RelType.OWNER,
+    ) -> tuple[bool, list[Organization]]:
         """
-        Returns the list of Organizations which have the given relationship to the taxonomy.
+        Returns a tuple containing:
+        * bool: flag indicating whether "all organizations" have the given relationship to the taxonomy
+        * orgs: list of Organizations which have the given relationship to the taxonomy
         """
-        rels = cls.objects.filter(
-            taxonomy=taxonomy,
-            rel_type=rel_type,
-        )
-        # A relationship with org=None means all Organizations
-        if rels.filter(org=None).exists():
-            return list(Organization.objects.all())
-        return [rel.org for rel in rels]
+        is_all_org = False
+        orgs = []
+        # Iterate over the taxonomyorgs instead of filtering to take advantage of prefetched data.
+        for taxonomy_org in taxonomy.taxonomyorg_set.all():
+            if taxonomy_org.rel_type == rel_type:
+                if taxonomy_org.org is None:
+                    is_all_org = True
+                else:
+                    orgs.append(taxonomy_org.org)
+
+        return (is_all_org, orgs)

openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py

@@ -6,7 +6,6 @@
 from rest_framework.filters import BaseFilterBackend
 
 import openedx_tagging.core.tagging.rules as oel_tagging
-from organizations.models import Organization
 
 from ...rules import get_admin_orgs, get_user_orgs
 from ...models import TaxonomyOrg
@@ -25,9 +24,8 @@ def filter_queryset(self, request, queryset, _):
         if oel_tagging.is_taxonomy_admin(request.user):
             return queryset
 
-        orgs = list(Organization.objects.all())
-        user_admin_orgs = get_admin_orgs(request.user, orgs)
-        user_orgs = get_user_orgs(request.user, orgs)  # Orgs that the user is a content creator or instructor
+        user_admin_orgs = get_admin_orgs(request.user)
+        user_orgs = get_user_orgs(request.user)  # Orgs that the user is a content creator or instructor
 
         if len(user_orgs) == 0 and len(user_admin_orgs) == 0:
             return queryset.none()
@@ -67,11 +65,10 @@ class ObjectTagTaxonomyOrgFilterBackend(BaseFilterBackend):
 
     def filter_queryset(self, request, queryset, _):
         if oel_tagging.is_taxonomy_admin(request.user):
-            return queryset
+            return queryset.prefetch_related('taxonomy__taxonomyorg_set')
 
-        orgs = list(Organization.objects.all())
-        user_admin_orgs = get_admin_orgs(request.user, orgs)
-        user_orgs = get_user_orgs(request.user, orgs)
+        user_admin_orgs = get_admin_orgs(request.user)
+        user_orgs = get_user_orgs(request.user)
         user_or_admin_orgs = list(set(user_orgs) | set(user_admin_orgs))
 
         return queryset.filter(taxonomy__enabled=True).filter(
@@ -90,4 +87,4 @@ def filter_queryset(self, request, queryset, _):
                     )
                 )
             )
-        )
+        ).prefetch_related('taxonomy__taxonomyorg_set')

openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py

@@ -4,7 +4,6 @@
 
 from __future__ import annotations
 
-from django.core.exceptions import ObjectDoesNotExist
 from rest_framework import serializers, fields
 
 from openedx_tagging.core.tagging.rest_api.v1.serializers import (
@@ -14,40 +13,30 @@
 
 from organizations.models import Organization
 
-
-class OptionalSlugRelatedField(serializers.SlugRelatedField):
-    """
-    Modifies the DRF serializer SlugRelatedField.
-
-    Non-existent slug values are represented internally as an empty queryset, instead of throwing a validation error.
-    """
-
-    def to_internal_value(self, data):
-        """
-        Returns the object related to the given slug value, or an empty queryset if not found.
-        """
-
-        queryset = self.get_queryset()
-        try:
-            return queryset.get(**{self.slug_field: data})
-        except ObjectDoesNotExist:
-            return queryset.none()
-        except (TypeError, ValueError):
-            self.fail('invalid')
+from ...models import TaxonomyOrg
 
 
 class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer):
     """
     Serializer for the query params for the GET view
     """
 
-    org: fields.Field = OptionalSlugRelatedField(
-        slug_field="short_name",
-        queryset=Organization.objects.all(),
+    org: fields.Field = serializers.CharField(
         required=False,
     )
     unassigned: fields.Field = serializers.BooleanField(required=False)
 
+    def validate(self, attrs: dict) -> dict:
+        """
+        Validate the serializer data
+        """
+        if "org" in attrs and "unassigned" in attrs:
+            raise serializers.ValidationError(
+                "'org' and 'unassigned' params cannot be both defined"
+            )
+
+        return attrs
+
 
 class TaxonomyUpdateOrgBodySerializer(serializers.Serializer):
     """
@@ -86,14 +75,20 @@ class TaxonomyOrgSerializer(TaxonomySerializer):
     def get_orgs(self, obj) -> list[str]:
         """
         Return the list of orgs for the taxonomy.
-         """
-        return [taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all() if taxonomy_org.org]
+        """
+        return [
+            taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all()
+            if taxonomy_org.org and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER
+        ]
 
     def get_all_orgs(self, obj) -> bool:
         """
         Return True if the taxonomy is associated with all orgs.
         """
-        return obj.taxonomyorg_set.filter(org__isnull=True).exists()
+        for taxonomy_org in obj.taxonomyorg_set.all():
+            if taxonomy_org.org_id is None and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER:
+                return True
+        return False
 
     class Meta:
         model = TaxonomySerializer.Meta.model

openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py

@@ -126,6 +126,11 @@ def _setUp_users(self):
             email="[email protected]",
             is_staff=True,
         )
+        self.superuser = User.objects.create(
+            username="superuser",
+            email="[email protected]",
+            is_superuser=True,
+        )
 
         self.staffA = User.objects.create(
             username="staffA",
@@ -498,27 +503,37 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None:
             if user_attr == "staffA":
                 assert response.data["orgs"] == [self.orgA.short_name]
 
-    def test_list_taxonomy_query_count(self):
+    @ddt.data(
+        ('staff', 11),
+        ("content_creatorA", 16),
+        ("library_staffA", 16),
+        ("library_userA", 16),
+        ("instructorA", 16),
+        ("course_instructorA", 16),
+        ("course_staffA", 16),
+    )
+    @ddt.unpack
+    def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int):
         """
         Test how many queries are used when retrieving taxonomies and permissions
         """
-        url = TAXONOMY_ORG_LIST_URL + f'?org=${self.orgA.short_name}&enabled=true'
-
-        self.client.force_authenticate(user=self.staff)
-        with self.assertNumQueries(16):  # TODO Why so many queries?
+        url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true'
+        user = getattr(self, user_attr)
+        self.client.force_authenticate(user=user)
+        with self.assertNumQueries(expected_queries):
             response = self.client.get(url)
 
         assert response.status_code == 200
-        assert response.data["can_add_taxonomy"]
-        assert len(response.data["results"]) == 2
+        assert response.data["can_add_taxonomy"] == user.is_staff
+        assert len(response.data["results"]) == 4
         for taxonomy in response.data["results"]:
             if taxonomy["system_defined"]:
                 assert not taxonomy["can_change_taxonomy"]
                 assert not taxonomy["can_delete_taxonomy"]
                 assert taxonomy["can_tag_object"]
             else:
-                assert taxonomy["can_change_taxonomy"]
-                assert taxonomy["can_delete_taxonomy"]
+                assert taxonomy["can_change_taxonomy"] == user.is_staff
+                assert taxonomy["can_delete_taxonomy"] == user.is_staff
                 assert taxonomy["can_tag_object"]
 
 
@@ -753,7 +768,7 @@ def test_detail_taxonomy_other_dont_see_no_org(self, user_attr: str) -> None:
             user_attr=user_attr,
             taxonomy_attr="ot1",
             expected_status=status.HTTP_404_NOT_FOUND,
-            reason="Only staff should see taxonomies with no org",
+            reason="Only taxonomy admins should see taxonomies with no org",
         )
 
     @ddt.data(
@@ -1232,11 +1247,12 @@ def test_update_org_no_perm(self, user_attr: str) -> None:
         self.client.force_authenticate(user=user)
 
         response = self.client.put(url, {"orgs": []}, format="json")
-        assert response.status_code == status.HTTP_403_FORBIDDEN
+        assert response.status_code in [status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND]
 
         # Check that the orgs didn't change
         url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.tA1.pk)
         response = self.client.get(url)
+        assert response.status_code == status.HTTP_200_OK
         assert response.data["orgs"] == [self.orgA.short_name]
 
     def test_update_org_check_permissions_orgA(self) -> None:
@@ -1642,14 +1658,15 @@ def test_tag_library_invalid(self, user_attr, taxonomy_attr):
         assert response.status_code == status.HTTP_400_BAD_REQUEST
 
     @ddt.data(
-        ("staff", status.HTTP_200_OK),
+        ("superuser", status.HTTP_200_OK),
+        ("staff", status.HTTP_403_FORBIDDEN),
         ("staffA", status.HTTP_403_FORBIDDEN),
         ("staffB", status.HTTP_403_FORBIDDEN),
     )
     @ddt.unpack
     def test_tag_cross_org(self, user_attr, expected_status):
         """
-        Tests that only global admins can add a taxonomy from orgA to an object from orgB
+        Tests that only superusers may add a taxonomy from orgA to an object from orgB
         """
         user = getattr(self, user_attr)
         self.client.force_authenticate(user=user)
@@ -1661,14 +1678,15 @@ def test_tag_cross_org(self, user_attr, expected_status):
         assert response.status_code == expected_status
 
     @ddt.data(
-        ("staff", status.HTTP_200_OK),
+        ("superuser", status.HTTP_200_OK),
+        ("staff", status.HTTP_403_FORBIDDEN),
         ("staffA", status.HTTP_403_FORBIDDEN),
         ("staffB", status.HTTP_403_FORBIDDEN),
     )
     @ddt.unpack
     def test_tag_no_org(self, user_attr, expected_status):
         """
-        Tests that only global admins can add a no-org taxonomy to an object
+        Tests that only superusers may add a no-org taxonomy to an object
         """
         user = getattr(self, user_attr)
         self.client.force_authenticate(user=user)
@@ -1760,26 +1778,43 @@ def test_get_tags(self):
         assert status.is_success(response3.status_code)
         assert response3.data[str(self.courseA)]["taxonomies"] == expected_tags
 
-    def test_object_tags_query_count(self):
+    @ddt.data(
+        ('staff', 'courseA', 8),
+        ('staff', 'libraryA', 8),
+        ("content_creatorA", 'courseA', 11, False),
+        ("content_creatorA", 'libraryA', 11, False),
+        ("library_staffA", 'libraryA', 11, False),  # Library users can only view objecttags, not change them?
+        ("library_userA", 'libraryA', 11, False),
+        ("instructorA", 'courseA', 11),
+        ("course_instructorA", 'courseA', 11),
+        ("course_staffA", 'courseA', 11),
+    )
+    @ddt.unpack
+    def test_object_tags_query_count(
+            self,
+            user_attr: str,
+            object_attr: str,
+            expected_queries: int,
+            expected_perm: bool = True):
         """
         Test how many queries are used when retrieving object tags and permissions
         """
-        object_key = self.courseA
+        object_key = getattr(self, object_attr)
         object_id = str(object_key)
         tagging_api.tag_object(object_id=object_id, taxonomy=self.t1, tags=["anvil", "android"])
         expected_tags = [
-            {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": True},
-            {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": True},
+            {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": expected_perm},
+            {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": expected_perm},
         ]
-
         url = OBJECT_TAGS_URL.format(object_id=object_id)
-        self.client.force_authenticate(user=self.staff)
-        with self.assertNumQueries(7):  # TODO Why so many queries?
+        user = getattr(self, user_attr)
+        self.client.force_authenticate(user=user)
+        with self.assertNumQueries(expected_queries):
             response = self.client.get(url)
 
         assert response.status_code == 200
         assert len(response.data[object_id]["taxonomies"]) == 1
-        assert response.data[object_id]["taxonomies"][0]["can_tag_object"]
+        assert response.data[object_id]["taxonomies"][0]["can_tag_object"] == expected_perm
         assert response.data[object_id]["taxonomies"][0]["tags"] == expected_tags
 
 
@@ -2364,19 +2399,30 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase):
     """
     Test cases for TaxonomyTagsViewSet retrive action.
     """
-    def test_taxonomy_tags_query_count(self):
+    @ddt.data(
+        ('staff', 11),
+        ("content_creatorA", 13),
+        ("library_staffA", 13),
+        ("library_userA", 13),
+        ("instructorA", 13),
+        ("course_instructorA", 13),
+        ("course_staffA", 13),
+    )
+    @ddt.unpack
+    def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int):
         """
         Test how many queries are used when retrieving small taxonomies+tags and permissions
         """
         url = f"{TAXONOMY_TAGS_URL}?search_term=an&parent_tag=ALPHABET".format(pk=self.t1.id)
 
-        self.client.force_authenticate(user=self.staff)
-        with self.assertNumQueries(13):  # TODO Why so many queries?
+        user = getattr(self, user_attr)
+        self.client.force_authenticate(user=user)
+        with self.assertNumQueries(expected_queries):
             response = self.client.get(url)
 
         assert response.status_code == status.HTTP_200_OK
-        assert response.data["can_add_tag"]
+        assert response.data["can_add_tag"] == user.is_staff
         assert len(response.data["results"]) == 2
         for taxonomy in response.data["results"]:
-            assert taxonomy["can_change_tag"]
-            assert taxonomy["can_delete_tag"]
+            assert taxonomy["can_change_tag"] == user.is_staff
+            assert taxonomy["can_delete_tag"] == user.is_staff