From a2f03ad56c08c055162a6748899d0afc5c89cb53 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 | 54 ++++++++-------- .../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, 128 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 e12c3f6e7770..0ce51d4d57e6 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 d08241b2cd79..d6be6bd8c7e3 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 15d24f5120c4..9d9bbd7b6033 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -446,7 +446,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 3c888011c16d..ec27b40c12f9 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1280,8 +1280,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): @@ -1296,13 +1296,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 0422845cc346..aaa9f72f1d36 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 @@ -520,13 +520,13 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: assert response.data["orgs"] == [self.orgA.short_name] @ddt.data( - ('staff', 11), - ("content_creatorA", 23), - ("library_staffA", 23), - ("library_userA", 23), - ("instructorA", 23), - ("course_instructorA", 23), - ("course_staffA", 23), + ('staff', 10), + ("content_creatorA", 22), + ("library_staffA", 22), + ("library_userA", 22), + ("instructorA", 22), + ("course_instructorA", 22), + ("course_staffA", 22), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -1980,19 +1980,19 @@ def test_get_copied_tags(self): assert response.data[str(object_id_2)]["taxonomies"] == expected_tags @ddt.data( - ('staff', 'courseA', 10), - ('staff', 'libraryA', 17), - ('staff', 'collection_key', 17), - ("content_creatorA", 'courseA', 20, False), - ("content_creatorA", 'libraryA', 23, False), - ("content_creatorA", 'collection_key', 23, False), - ("library_staffA", 'libraryA', 23, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 23, False), - ("library_userA", 'libraryA', 23, False), - ("library_userA", 'collection_key', 23, False), - ("instructorA", 'courseA', 20), - ("course_instructorA", 'courseA', 20), - ("course_staffA", 'courseA', 20), + ('staff', 'courseA', 9), + ('staff', 'libraryA', 16), + ('staff', 'collection_key', 16), + ("content_creatorA", 'courseA', 19, False), + ("content_creatorA", 'libraryA', 22, False), + ("content_creatorA", 'collection_key', 22, False), + ("library_staffA", 'libraryA', 22, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 22, False), + ("library_userA", 'libraryA', 22, False), + ("library_userA", 'collection_key', 22, False), + ("instructorA", 'courseA', 19), + ("course_instructorA", 'courseA', 19), + ("course_staffA", 'courseA', 19), ) @ddt.unpack def test_object_tags_query_count( @@ -2878,13 +2878,13 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase): Test cases for TaxonomyTagsViewSet retrive action. """ @ddt.data( - ('staff', 11), - ("content_creatorA", 13), - ("library_staffA", 13), - ("library_userA", 13), - ("instructorA", 13), - ("course_instructorA", 13), - ("course_staffA", 13), + ('staff', 10), + ("content_creatorA", 12), + ("library_staffA", 12), + ("library_userA", 12), + ("instructorA", 12), + ("course_instructorA", 12), + ("course_staffA", 12), ) @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)