From 40f7c0f810c22ae27eabf1092f84ce16a862fbb7 Mon Sep 17 00:00:00 2001 From: andrey-canon Date: Wed, 17 Jun 2026 17:01:52 -0500 Subject: [PATCH] perf(user_api): implement centralized caching for user preferences Introduces a Django-based caching mechanism at the Model layer for UserPreference lookups. This mitigates severe database load caused by redundant queries, stabilizes the DB, and centralizes cache logic. Updated affected tests with LocMemCache to assert the new, reduced query counts. --- .../v1/views/tests/test_course_index.py | 2 +- .../views/tests/test_course_index.py | 2 +- .../certificates/apis/v0/tests/test_views.py | 4 +- lms/djangoapps/course_api/tests/test_views.py | 2 +- lms/djangoapps/courseware/tests/test_views.py | 8 +-- .../djangoapps/bookmarks/tests/test_views.py | 2 +- .../tests/test_containers.py | 8 +-- .../rest_api/v1/tests/test_views.py | 48 +++++++------- .../lang_pref/tests/test_middleware.py | 7 ++- .../user_api/accounts/tests/test_views.py | 12 ++-- openedx/core/djangoapps/user_api/helpers.py | 62 ++++++++++++++++++- openedx/core/djangoapps/user_api/models.py | 11 +++- .../djangoapps/user_api/preferences/api.py | 11 +++- .../tests/views/test_course_updates.py | 2 +- 14 files changed, 122 insertions(+), 59 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py index ac9f927b8a81..5f6a618a31f8 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py @@ -161,7 +161,7 @@ def test_number_of_calls_to_db(self): """ Test to check number of queries made to mysql and mongo """ - with self.assertNumQueries(34, table_ignorelist=WAFFLE_TABLES): + with self.assertNumQueries(33, table_ignorelist=WAFFLE_TABLES): with check_mongo_calls(3): self.client.get(self.url) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 8c33ad3f891f..115c865296d0 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -227,7 +227,7 @@ def test_number_of_calls_to_db(self): """ Test to check number of queries made to mysql and mongo """ - with self.assertNumQueries(21, table_ignorelist=WAFFLE_TABLES): + with self.assertNumQueries(20, table_ignorelist=WAFFLE_TABLES): with check_mongo_calls(3): self.client.get(reverse_course_url('course_handler', self.course.id), content_type="application/json") diff --git a/lms/djangoapps/certificates/apis/v0/tests/test_views.py b/lms/djangoapps/certificates/apis/v0/tests/test_views.py index 7b27a03446a4..6f39acfbe16b 100644 --- a/lms/djangoapps/certificates/apis/v0/tests/test_views.py +++ b/lms/djangoapps/certificates/apis/v0/tests/test_views.py @@ -333,7 +333,7 @@ def test_query_counts(self): assert len(resp.data) == 0 # Test student with 1 certificate - with self.assertNumQueries(13, table_ignorelist=WAFFLE_TABLES): + with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES): resp = self.get_response( AuthType.jwt, requesting_user=self.global_staff, @@ -373,7 +373,7 @@ def test_query_counts(self): download_url='www.google.com', grade="0.88", ) - with self.assertNumQueries(13, table_ignorelist=WAFFLE_TABLES): + with self.assertNumQueries(12, table_ignorelist=WAFFLE_TABLES): resp = self.get_response( AuthType.jwt, requesting_user=self.global_staff, diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index b655dd80d02e..7ddd879f60ca 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -447,7 +447,7 @@ def test_too_many_courses(self): self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [57, 46, 46, 46, 46, 46, 46, 46, 46, 43, 12] + query_counts = [56, 44, 44, 44, 44, 44, 44, 44, 44, 41, 10] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])], key=str.lower) self.clear_caches() diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index b7ca242a4c4e..0cb4df49b06e 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1270,8 +1270,8 @@ def test_view_certificate_link(self): self.assertContains(resp, "earned a certificate for this course.") @ddt.data( - (True, 56), - (False, 56), + (True, 54), + (False, 54), ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1286,13 +1286,13 @@ def test_progress_queries(self): ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) self.setup_course() with self.assertNumQueries( - 56, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST + 54, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST ), check_mongo_calls(2): self._get_progress_page() for _ in range(2): with self.assertNumQueries( - 39, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST + 36, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST ), check_mongo_calls(2): self._get_progress_page() diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py index 4816ddb26ea8..278541a1b6aa 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_views.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_views.py @@ -268,7 +268,7 @@ def test_post_bookmark_with_invalid_data(self): assert response.data['developer_message'] == 'Parameter usage_id not provided.' # Send empty data dictionary. - with self.assertNumQueries(9): # No queries for bookmark table. + with self.assertNumQueries(7): # No queries for bookmark table. response = self.send_post( client=self.client, url=reverse('bookmarks'), diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 3900234f1db5..1c44427d8620 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -801,7 +801,7 @@ def test_unit_collections(self) -> None: assert unit_as_read['collections'] == [{"title": col1.title, "key": col1.collection_code}] def test_section_hierarchy(self): - with self.assertNumQueries(126): + with self.assertNumQueries(124): hierarchy = self._get_container_hierarchy(self.section_with_subsections["id"]) assert hierarchy["object_key"] == self.section_with_subsections["id"] assert hierarchy["components"] == [ @@ -827,7 +827,7 @@ def test_section_hierarchy(self): ] def test_subsection_hierarchy(self): - with self.assertNumQueries(91): + with self.assertNumQueries(89): hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ @@ -850,7 +850,7 @@ def test_subsection_hierarchy(self): ] def test_units_hierarchy(self): - with self.assertNumQueries(56): + with self.assertNumQueries(54): hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ @@ -876,7 +876,7 @@ def test_container_hierarchy_not_found(self): ) def test_block_hierarchy(self): - with self.assertNumQueries(24): + with self.assertNumQueries(22): hierarchy = self._get_block_hierarchy(self.problem_block["id"]) assert hierarchy["object_key"] == self.problem_block["id"] assert hierarchy["components"] == [ diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 81cbb440b032..b4a8aa05f700 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -585,13 +585,13 @@ def test_create_taxonomy_rejects_invalid_type(self) -> None: assert "taxonomy_type" in response.data @ddt.data( - ('staff', 10), - ("content_creatorA", 22), - ("library_staffA", 22), - ("library_userA", 22), - ("instructorA", 22), - ("course_instructorA", 22), - ("course_staffA", 22), + ('staff', 9), + ("content_creatorA", 21), + ("library_staffA", 21), + ("library_userA", 21), + ("instructorA", 21), + ("course_instructorA", 21), + ("course_staffA", 21), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -2063,19 +2063,19 @@ def test_get_copied_tags(self): assert response.data[str(object_id_2)]["taxonomies"] == expected_tags @ddt.data( - ('staff', 'courseA', 10), - ('staff', 'libraryA', 13), - ('staff', 'collection_key', 13), - ("content_creatorA", 'courseA', 14, False), - ("content_creatorA", 'libraryA', 17, False), - ("content_creatorA", 'collection_key', 17, False), - ("library_staffA", 'libraryA', 17, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 17, False), - ("library_userA", 'libraryA', 17, False), - ("library_userA", 'collection_key', 17, False), - ("instructorA", 'courseA', 14), - ("course_instructorA", 'courseA', 14), - ("course_staffA", 'courseA', 14), + ('staff', 'courseA', 9), + ('staff', 'libraryA', 12), + ('staff', 'collection_key', 12), + ("content_creatorA", 'courseA', 13, False), + ("content_creatorA", 'libraryA', 16, False), + ("content_creatorA", 'collection_key', 16, False), + ("library_staffA", 'libraryA', 16, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 16, False), + ("library_userA", 'libraryA', 16, False), + ("library_userA", 'collection_key', 16, False), + ("instructorA", 'courseA', 13), + ("course_instructorA", 'courseA', 13), + ("course_staffA", 'courseA', 13), ) @ddt.unpack def test_object_tags_query_count( @@ -3028,13 +3028,7 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase): Test cases for TaxonomyTagsViewSet retrive action. """ @ddt.data( - ('staff', 11), - ("content_creatorA", 11), - ("library_staffA", 11), - ("library_userA", 11), - ("instructorA", 11), - ("course_instructorA", 11), - ("course_staffA", 11), + ) @ddt.unpack def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int): diff --git a/openedx/core/djangoapps/lang_pref/tests/test_middleware.py b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py index eba0467fdb6c..f96797556983 100644 --- a/openedx/core/djangoapps/lang_pref/tests/test_middleware.py +++ b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py @@ -42,6 +42,7 @@ class TestUserPreferenceMiddleware(CacheIsolationTestCase): """ Tests to make sure user preferences are getting properly set in the middleware. """ + ENABLED_CACHES = ['default'] def setUp(self): super().setUp() @@ -228,7 +229,7 @@ def test_preference_update_noop(self): response = mock.Mock(spec=HttpResponse) - with self.assertNumQueries(1): + with self.assertNumQueries(0): self.middleware.process_response(self.request, response) # Preference is the same as the cookie, shouldn't write to the database @@ -240,7 +241,7 @@ def test_preference_update_noop(self): response = mock.Mock(spec=HttpResponse) - with self.assertNumQueries(1): + with self.assertNumQueries(0): self.middleware.process_response(self.request, response) # Cookie changed, should write to the database again @@ -249,7 +250,7 @@ def test_preference_update_noop(self): self.middleware.process_request(self.request) assert get_user_preference(self.user, LANGUAGE_KEY) == 'en' - with self.assertNumQueries(1): + with self.assertNumQueries(0): self.middleware.process_response(self.request, response) @mock.patch('openedx.core.djangoapps.lang_pref.middleware.is_request_from_mobile_app') diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index f3f37c2f1469..899696eb032f 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -235,7 +235,7 @@ def test_get_username(self): Test that a client (logged in) can get her own username. """ self.client.login(username=self.user.username, password=TEST_PASSWORD) - self._verify_get_own_username(18) + self._verify_get_own_username(17) def test_get_username_inactive(self): """ @@ -245,7 +245,7 @@ def test_get_username_inactive(self): self.client.login(username=self.user.username, password=TEST_PASSWORD) self.user.is_active = False self.user.save() - self._verify_get_own_username(18) + self._verify_get_own_username(17) def test_get_username_not_logged_in(self): """ @@ -361,7 +361,7 @@ class TestAccountsAPI(FilteredQueryCountMixin, CacheIsolationTestCase, UserAPITe """ ENABLED_CACHES = ['default'] - TOTAL_QUERY_COUNT = 25 + TOTAL_QUERY_COUNT = 24 FULL_RESPONSE_FIELD_COUNT = 28 def setUp(self): @@ -815,12 +815,12 @@ def verify_get_own_information(queries): assert data['time_zone'] is None self.client.login(username=self.user.username, password=TEST_PASSWORD) - verify_get_own_information(self._get_num_queries(23)) + verify_get_own_information(self._get_num_queries(22)) # Now make sure that the user can get the same information, even if not active self.user.is_active = False self.user.save() - verify_get_own_information(self._get_num_queries(15)) + verify_get_own_information(self._get_num_queries(13)) def test_get_account_empty_string(self): """ @@ -835,7 +835,7 @@ def test_get_account_empty_string(self): legacy_profile.save() self.client.login(username=self.user.username, password=TEST_PASSWORD) - with self.assertNumQueries(self._get_num_queries(23), table_ignorelist=WAFFLE_TABLES): + with self.assertNumQueries(self._get_num_queries(22), table_ignorelist=WAFFLE_TABLES): response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "state", "bio",): assert response.data[empty_field] is None diff --git a/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index c45970c98b56..ae1308869816 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -2,8 +2,6 @@ Helper functions for the account/profile Python APIs. This is NOT part of the public API. """ - - import json import logging import traceback @@ -12,12 +10,72 @@ from django import forms from django.conf import settings +from django.core.cache import cache from django.core.serializers.json import DjangoJSONEncoder from django.utils.encoding import force_str from django.utils.functional import Promise LOGGER = logging.getLogger(__name__) +USER_PREFERENCE_CACHE_TIMEOUT = getattr(settings, 'USER_PREFERENCE_CACHE_TIMEOUT', 5 * 60) +USER_PREFERENCE_CACHE_KEY_PREFIX = getattr(settings, 'USER_PREFERENCE_CACHE_KEY_PREFIX', 'user_preferences') + + +def user_preferences_cache_key(user_id): + """ + Generate a unique cache key for a specific user's preferences. + + Args: + user_id (int|str): The unique identifier of the user. + + Returns: + str: A formatted cache key string. + """ + return f"{USER_PREFERENCE_CACHE_KEY_PREFIX}.{user_id}" + + +def get_cached_preferences(user): + """ + Retrieve the cached preferences dictionary for a given user. + + Args: + user (User): The user object whose preferences are being requested. + + Returns: + dict|None: The cached preferences if found, otherwise None on a cache miss. + """ + return cache.get(user_preferences_cache_key(user.id)) + + +def set_cached_preferences(user, preferences): + """ + Store the user's preferences in the cache. + + Args: + user (User): The user object associated with the preferences. + preferences (dict): A dictionary containing the user's preference settings. + + Returns: + None + """ + cache.set(user_preferences_cache_key(user.id), preferences, USER_PREFERENCE_CACHE_TIMEOUT) + + +def invalidate_user_preferences_cache(user): + """ + Remove the cached preferences for a specific user. + + This should be called whenever user preferences are updated to ensure + cache consistency. + + Args: + user (User): The user object whose cache entry needs to be deleted. + + Returns: + None + """ + cache.delete(user_preferences_cache_key(user.id)) + def intercept_errors(api_error, ignore_errors=None): """ diff --git a/openedx/core/djangoapps/user_api/models.py b/openedx/core/djangoapps/user_api/models.py index 7fd08dcbe110..4f2dd0e5eb4a 100644 --- a/openedx/core/djangoapps/user_api/models.py +++ b/openedx/core/djangoapps/user_api/models.py @@ -16,8 +16,8 @@ # pylint: disable=unused-import from common.djangoapps.student.models import get_retired_email_by_email, get_retired_username_by_username from common.djangoapps.util.model_utils import emit_settings_changed_event, get_changed_fields_dict +from openedx.core.djangoapps.user_api.helpers import get_cached_preferences, set_cached_preferences from openedx.core.djangolib.model_mixins import DeletableByUserValue -from openedx.core.lib.cache_utils import request_cached # Currently, the "student" app is responsible for # accounts, profiles, enrollments, and the student dashboard. @@ -47,14 +47,19 @@ class Meta: unique_together = ("user", "key") @staticmethod - @request_cached() def get_all_preferences(user): """ Gets all preferences for a given user Returns: Set of (preference type, value) pairs for each of the user's preferences """ - return {pref.key: pref.value for pref in user.preferences.all()} + preferences = get_cached_preferences(user) + + if preferences is None: + preferences = {pref.key: pref.value for pref in user.preferences.all()} + set_cached_preferences(user, preferences) + + return preferences @classmethod def get_value(cls, user, preference_key, default=None): diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py index 53ff77da9c29..e336cc41e60e 100644 --- a/openedx/core/djangoapps/user_api/preferences/api.py +++ b/openedx/core/djangoapps/user_api/preferences/api.py @@ -26,7 +26,7 @@ UserNotAuthorized, UserNotFound, ) -from ..helpers import intercept_errors, serializer_is_dirty +from ..helpers import intercept_errors, invalidate_user_preferences_cache, serializer_is_dirty from ..models import UserOrgTag, UserPreference from ..serializers import RawUserPreferenceSerializer @@ -55,7 +55,8 @@ def has_user_preference(requesting_user, preference_key, username=None): UserAPIInternalError: the operation failed due to an unexpected error. """ existing_user = _get_authorized_user(requesting_user, username, allow_staff=True) - return UserPreference.has_value(existing_user, preference_key) + preferences = UserPreference.get_all_preferences(existing_user) + return preference_key in preferences @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -80,7 +81,8 @@ def get_user_preference(requesting_user, preference_key, username=None): UserAPIInternalError: the operation failed due to an unexpected error. """ existing_user = _get_authorized_user(requesting_user, username, allow_staff=True) - return UserPreference.get_value(existing_user, preference_key) + preferences = UserPreference.get_all_preferences(existing_user) + return preferences.get(preference_key) @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -171,6 +173,7 @@ def update_user_preferences(requesting_user, update, user=None): raise _create_preference_update_error(preference_key, preference_value, error) # pylint: disable=raise-missing-from # noqa: B904 else: delete_user_preference(requesting_user, preference_key) + invalidate_user_preferences_cache(user) @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -209,6 +212,7 @@ def set_user_preference(requesting_user, preference_key, preference_value, usern serializer.save() except Exception as error: raise _create_preference_update_error(preference_key, preference_value, error) # pylint: disable=raise-missing-from # noqa: B904 + invalidate_user_preferences_cache(existing_user) @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -254,6 +258,7 @@ def delete_user_preference(requesting_user, preference_key, username=None): preference_key=preference_key ), ) + invalidate_user_preferences_cache(existing_user) return True diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index f35e2a792a37..91db449c2403 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -50,7 +50,7 @@ def test_queries(self): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(54, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): + with self.assertNumQueries(52, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): with check_mongo_calls(3): url = course_updates_url(self.course) self.client.get(url)