Skip to content
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

Reduce the number of queries used in tagging API [FC-00036] #157

Merged
merged 8 commits into from
Feb 15, 2024
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I used code from this blog to make this change. That blog post also copies the caches for one-to-many fields that were cached using select_related, but since Taxonomy doesn't have any one-to-many fields, I omitted that part.

return self

def get_filtered_tags(
Expand Down
22 changes: 21 additions & 1 deletion 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 @@ -58,3 +58,23 @@ 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 has_permission(self, request, view):
"""
Returns True if the request user is allowed the given view on the Taxonomy model.

We override this method to avoid calling our view's get_queryset(), which performs database queries.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, do you know why this is happening?

I didn't find a place where we are actually evaluating the query. Were the queries from the Taxonomy model or some other tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's being called by Django Rest Framework: APIView.initial calls DjangoModelPermissions.has_permission()

"""
# Workaround to ensure DjangoModelPermissions are not applied
# to the root view when using DefaultRouter.
if getattr(view, '_ignore_model_permissions', False):
return True

if not request.user or (
not request.user.is_authenticated and self.authenticated_users_only):
return False

queryset = Taxonomy.objects
perms = self.get_required_permissions(request.method, queryset.model)

return request.user.has_perms(perms)
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
Comment on lines +108 to +109
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this help reduce the query count? The reason I ask is because on the commit that this was added in, the query counts in the tests did not change, so I was just wondering what effect this may have had.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query counts dropped here when this branch was used: openedx/edx-platform@016f2a5

return instance.tag_set.count()

def get_can_tag_object(self, instance) -> bool | None:
Expand Down
4 changes: 3 additions & 1 deletion openedx_tagging/core/tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
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
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
Loading