-
Notifications
You must be signed in to change notification settings - Fork 11
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
Changes from 7 commits
163ee41
6464f53
ad7838e
e36e074
5f308ee
77f8282
dc86603
29e644e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
bradenmacdonald marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
There was a problem hiding this comment.
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.