Skip to content

Commit

Permalink
Reduce the number of queries used in tagging API [FC-00036] (#157)
Browse files Browse the repository at this point in the history
* fix: copy Django's internal prefetch cache when casting taxonomies to preserve any prefetch_related data that's available.
* fix: prevents TaxonomyTagsView from running get_queryset twice
  Overrides the permissions._queryset method to avoid using view.get_queryset(), since that method accesses the database.
  Also renames the permission class to better represent the view it guards.
* fix: annotate taxonomies with their tags_count instead of prefetching all the tags.
* chore: bumps version
  • Loading branch information
pomegranited authored Feb 15, 2024
1 parent dbc11b5 commit f3779de
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 9 deletions.
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
"""
Open edX Learning ("Learning Core").
"""
__version__ = "0.6.1"
__version__ = "0.6.2"
6 changes: 6 additions & 0 deletions openedx_tagging/core/tagging/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,12 @@ def copy(self, taxonomy: Taxonomy) -> Taxonomy:
self.visible_to_authors = taxonomy.visible_to_authors
self.export_id = taxonomy.export_id
self._taxonomy_class = taxonomy._taxonomy_class # pylint: disable=protected-access

# Copy Django's internal prefetch_related cache to reduce queries required on the casted taxonomy.
if hasattr(taxonomy, '_prefetched_objects_cache'):
# pylint: disable=protected-access,attribute-defined-outside-init
self._prefetched_objects_cache: dict = taxonomy._prefetched_objects_cache

return self

def get_filtered_tags(
Expand Down
14 changes: 11 additions & 3 deletions openedx_tagging/core/tagging/rest_api/v1/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import rules # type: ignore[import]
from rest_framework.permissions import DjangoObjectPermissions

from ...models import Tag
from ...models import Tag, Taxonomy


class TaxonomyObjectPermissions(DjangoObjectPermissions):
Expand Down Expand Up @@ -37,9 +37,9 @@ class ObjectTagObjectPermissions(DjangoObjectPermissions):
}


class TagObjectPermissions(DjangoObjectPermissions):
class TaxonomyTagsObjectPermissions(DjangoObjectPermissions):
"""
Maps each REST API methods to its corresponding Tag permission.
Maps each REST API methods to its corresponding Taxonomy permission.
"""
perms_map = {
"GET": ["%(app_label)s.view_%(model_name)s"],
Expand All @@ -58,3 +58,11 @@ def has_object_permission(self, request, view, obj):
"""
obj = obj.taxonomy if isinstance(obj, Tag) else obj
return rules.has_perm("oel_tagging.list_tag", request.user, obj)

def _queryset(self, view):
"""
Returns the queryset to use when checking model and object permissions.
The base class method calls view.get_queryset(), but that method performs database queries, so we override it.
"""
return Taxonomy.objects
7 changes: 7 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ def to_representation(self, instance):
return super().to_representation(instance)

def get_tags_count(self, instance):
"""
Return the "tags_count" annotation if present.
Or just count the taxonomy's tags.
"""
if hasattr(instance, 'tags_count'):
return instance.tags_count
return instance.tag_set.count()

def get_can_tag_object(self, instance) -> bool | None:
Expand Down
8 changes: 5 additions & 3 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from ...rules import ObjectTagPermissionItem
from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination
from ..utils import view_auth_classes
from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions
from .permissions import ObjectTagObjectPermissions, TaxonomyObjectPermissions, TaxonomyTagsObjectPermissions
from .serializers import (
ObjectTagListQueryParamsSerializer,
ObjectTagsByTaxonomySerializer,
Expand Down Expand Up @@ -252,7 +252,9 @@ def get_queryset(self) -> models.QuerySet:
query_params.is_valid(raise_exception=True)
enabled = query_params.data.get("enabled", None)

return get_taxonomies(enabled)
qs = get_taxonomies(enabled)
qs = qs.annotate(tags_count=models.Count("tag", distinct=True))
return qs

def perform_create(self, serializer) -> None:
"""
Expand Down Expand Up @@ -724,7 +726,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView):
"""

permission_classes = [TagObjectPermissions]
permission_classes = [TaxonomyTagsObjectPermissions]
pagination_class = TagsPagination
serializer_class = TagDataSerializer

Expand Down
4 changes: 2 additions & 2 deletions tests/openedx_tagging/core/tagging/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def test_list_taxonomy_query_count(self):
url = TAXONOMY_LIST_URL

self.client.force_authenticate(user=self.user)
with self.assertNumQueries(5):
with self.assertNumQueries(3):
response = self.client.get(url)

assert response.status_code == 200
Expand Down Expand Up @@ -1461,7 +1461,7 @@ def test_small_query_count(self):
url = f"{self.small_taxonomy_url}?search_term=eU"

self.client.force_authenticate(user=self.staff)
with self.assertNumQueries(5):
with self.assertNumQueries(4):
response = self.client.get(url)

assert response.status_code == status.HTTP_200_OK
Expand Down

0 comments on commit f3779de

Please sign in to comment.