Reduce the number of queries used in tagging API [FC-00036]#157
Reduce the number of queries used in tagging API [FC-00036]#157pomegranited merged 8 commits intoopenedx:mainfrom
Conversation
to preserve any prefetch_related data that's available.
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Overrides the permissions.has_permissions method to avoid using view.get_queryset(), since that method accesses the database.
instead of prefetching all the tags.
| """ | ||
| 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.
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.
It's being called by Django Rest Framework: APIView.initial calls DjangoModelPermissions.has_permission()
yusuf-musleh
left a comment
yusuf-musleh
left a comment
There was a problem hiding this comment.
👍 Great work here @pomegranited ! Just had a question.
- I tested this: Checked that the updated tests were passing
- I read through the code
-
I checked for accessibility issues - Includes documentation
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-securerepository.
| if hasattr(instance, 'tags_count'): | ||
| return instance.tags_count |
There was a problem hiding this comment.
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.
The query counts dropped here when this branch was used: openedx/openedx-platform@016f2a5
| if hasattr(taxonomy, '_prefetched_objects_cache'): | ||
| # pylint: disable=protected-access,attribute-defined-outside-init | ||
| self._prefetched_objects_cache: dict = taxonomy._prefetched_objects_cache | ||
|
|
There was a problem hiding this comment.
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.
| """ | ||
| 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.
It's being called by Django Rest Framework: APIView.initial calls DjangoModelPermissions.has_permission()
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Looks great, thanks!
and renames the permission class to better represent the view it guards.
|
@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
5 participants
Description
Updates the base tagging library to reduce database queries when using the views or models.
Supporting information
Closes openedx/modular-learning#185
Internal-ref: FAL-3646
Testing instructions
The changes made by this PR are validated by the reduced query counts in the tests.
TO DO BEFORE MERGE: