From 163ee41e63d51e6aabb7c6d4f5db5fd439389102 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 07:08:31 +1030 Subject: [PATCH 1/6] fix: prefetch taxonomy.tag_set to reduce queries --- openedx_tagging/core/tagging/rest_api/v1/views.py | 2 +- tests/openedx_tagging/core/tagging/test_views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 5eed0e5a..ba07f1d2 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -252,7 +252,7 @@ def get_queryset(self) -> models.QuerySet: query_params.is_valid(raise_exception=True) enabled = query_params.data.get("enabled", None) - return get_taxonomies(enabled) + return get_taxonomies(enabled).prefetch_related('tag_set') def perform_create(self, serializer) -> None: """ diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index d29f56fd..aa36d4d2 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -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(4): response = self.client.get(url) assert response.status_code == 200 From 6464f53858cc395b96aae6138e1869670c4071c7 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 07:45:31 +1030 Subject: [PATCH 2/6] fix: copy Django's internal prefetch cache when casting taxonomies to preserve any prefetch_related data that's available. --- openedx_tagging/core/tagging/models/base.py | 6 ++++++ tests/openedx_tagging/core/tagging/test_views.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/models/base.py b/openedx_tagging/core/tagging/models/base.py index c9b4439e..dee079ad 100644 --- a/openedx_tagging/core/tagging/models/base.py +++ b/openedx_tagging/core/tagging/models/base.py @@ -360,6 +360,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( diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index aa36d4d2..ae569335 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -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(4): + with self.assertNumQueries(3): response = self.client.get(url) assert response.status_code == 200 From e36e074bd76b08006c633a38f3d3cfaea3e6c7ed Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 19:19:29 +1030 Subject: [PATCH 3/6] fix: prevents TaxonomyTagsView from running get_queryset twice Overrides the permissions.has_permissions method to avoid using view.get_queryset(), since that method accesses the database. --- .../core/tagging/rest_api/v1/permissions.py | 22 ++++++++++++++++++- .../core/tagging/test_views.py | 2 +- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/permissions.py b/openedx_tagging/core/tagging/rest_api/v1/permissions.py index b63a6b7e..946d7034 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/permissions.py +++ b/openedx_tagging/core/tagging/rest_api/v1/permissions.py @@ -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): @@ -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. + """ + # 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) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index e1936928..a3664e27 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -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 From 5f308ee699ad1cc0dbc1098fce10cb1c50661a98 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 19:53:42 +1030 Subject: [PATCH 4/6] fix: annotate taxonomies with their tags_count instead of prefetching all the tags. --- openedx_tagging/core/tagging/rest_api/v1/serializers.py | 7 +++++++ openedx_tagging/core/tagging/rest_api/v1/views.py | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 2e6c3c35..64299853 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -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: diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index ba07f1d2..98671849 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -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).prefetch_related('tag_set') + qs = get_taxonomies(enabled) + qs = qs.annotate(tags_count=models.Count("tag", distinct=True)) + return qs def perform_create(self, serializer) -> None: """ From dc86603c5ee35d034b88f548c589aa4ebb026634 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 11:32:03 +1030 Subject: [PATCH 5/6] chore: bumps version --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index f725114f..a01c57cd 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.6.1" +__version__ = "0.6.2" From 29e644e08adb4dd9e43e0e0e3de4fcbb4ce49cee Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 12:35:23 +1030 Subject: [PATCH 6/6] fix: overrides _queryset() instead of has_permission and renames the permission class to better represent the view it guards. --- .../core/tagging/rest_api/v1/permissions.py | 24 +++++-------------- .../core/tagging/rest_api/v1/views.py | 4 ++-- 2 files changed, 8 insertions(+), 20 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/permissions.py b/openedx_tagging/core/tagging/rest_api/v1/permissions.py index 946d7034..5c2f6d72 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/permissions.py +++ b/openedx_tagging/core/tagging/rest_api/v1/permissions.py @@ -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"], @@ -59,22 +59,10 @@ 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): + def _queryset(self, view): """ - Returns True if the request user is allowed the given view on the Taxonomy model. + Returns the queryset to use when checking model and object permissions. - We override this method to avoid calling our view's get_queryset(), which performs database queries. + The base class method calls view.get_queryset(), but that method performs database queries, so we override it. """ - # 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) + return Taxonomy.objects diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 98671849..114c2ea5 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -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, @@ -726,7 +726,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): """ - permission_classes = [TagObjectPermissions] + permission_classes = [TaxonomyTagsObjectPermissions] pagination_class = TagsPagination serializer_class = TagDataSerializer