Reduce number of queries required for content tagging endpoints #34200
Conversation
Was requesting org=$orgA instead of org=orgA, and so wasn't testing the queries we needed to test.
|
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. |
openedx-webhooks
added
the
open-source-contribution
pomegranited
force-pushed
the
jill/tagging-less-queries
branch
from
9826a07
to
8e7823e
Compare
pomegranited
force-pushed
the
jill/tagging-less-queries
branch
from
e567070
to
6e094ef
Compare
which reduces the query count. TO DO: once PR is merged and tagged, update the requirement here.
to avoid re-querying. Changes TaxonomyOrg.get_organizations to return the "all orgs" flag too, which makes it useful in the rules.
for the taxonomy tags views.
instead of prefetching all the tags.
(needs fixing)
because SELECT COUNT at the database level is more efficient than returning a bunch of data we don't need.
instead of querying for CourseAccessRoles Also * removes the can_change_objecttag override method -- we only need to override the can_change_objecttag_taxonomy / _object+id methods to achieve our rules changes. * fixes up some misleading comments, and underscores some internal rules method names.
Avoids fetching and re-fetching Organizations by caching the full Organization list for the duration of the request.
Avoids re-fetching the libraries that a user has been granted access to by caching the data we need from these libraries for the duration of the request.
pomegranited
force-pushed
the
jill/tagging-less-queries
branch
2 times, most recently
from
a7047c9
to
85eb4f1
Compare
There was a problem hiding this comment.
👍 This is some really great work @pomegranited ! I followed the commits while reviewing as you suggested. I just had a few minor nits and a question.
- I tested this: Checked that the updated tests with less query counts pass
- 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.
openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py
Outdated
Show resolved
Hide resolved
openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py
Outdated
Show resolved
Hide resolved
* fix: prevent cross-org ObjectTags from being created A "cross-org" ObjectTag is when the object_id references an org that is not in the taxonomy's allowed list of orgs. Similarly, we forbid creating object tags for a taxonomy with no allowed orgs listed.
|
|
||
| class TaggingRulesCache: | ||
| """ | ||
| Caches data required for computing rules for the duration of the request. |
There was a problem hiding this comment.
This may not have been necessary if we'd have used bridgekeeper instead of rules for defining the permissions. My bad for that original mistake. It's a nice solution for now though :)
|
@pomegranited I went to merge this, but there's a conflict. Please fix it and I'll merge on Tuesday. |
Hi @bradenmacdonald! I fixed the merge conflict in case you still check it today or Monday. |
|
@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. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Tweak the queries used in the content tagging views to reduce the number of database queries made, and improve response time.
Content tagging is disabled by default, so no one should be affected by this change.
Supporting information
Closes openedx/modular-learning#185
Depends on openedx/openedx-learning#157
Relates to open-craft#633
Internal-ref: FAL-3646
Testing instructions
These changes should be validated by the unit tests.
Please also review the sql queries shown below to ensure that no duplicate queries are being performed, and there's no other obvious ways to reduce these query counts.
List Taxonomies
11 queries for Taxonomy admins (reduced from 16)
16 queries for content authors (unchanged)
Get object tags
Note: Queries shown include changes from open-craft#633.
8 queries for taxonomy admins (increased from 7, added cross-org check)
11 queries for content authors (reduced from 19)
Deadline
None