From 1811f7235f720fbe6090dfd811ce211950f79abb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 26 Apr 2017 14:18:49 -0400 Subject: [PATCH] Minimize the number of queries done while noop updating a user-preference --- .../tests/test_field_override_performance.py | 54 +++++++++---------- .../courseware/tests/test_course_info.py | 4 +- lms/djangoapps/courseware/tests/test_views.py | 10 ++-- .../django_comment_client/base/tests.py | 8 +-- .../djangoapps/bookmarks/tests/test_views.py | 2 +- .../lang_pref/tests/test_middleware.py | 41 ++++++++++++++ .../user_api/accounts/tests/test_views.py | 14 ++--- openedx/core/djangoapps/user_api/errors.py | 2 + .../djangoapps/user_api/preferences/api.py | 17 +++--- 9 files changed, 99 insertions(+), 53 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 339a10b853..08eb0387d4 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -231,18 +231,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (25, 1), - ('no_overrides', 2, True, False): (25, 1), - ('no_overrides', 3, True, False): (25, 1), - ('ccx', 1, True, False): (25, 1), - ('ccx', 2, True, False): (25, 1), - ('ccx', 3, True, False): (25, 1), - ('no_overrides', 1, False, False): (25, 1), - ('no_overrides', 2, False, False): (25, 1), - ('no_overrides', 3, False, False): (25, 1), - ('ccx', 1, False, False): (25, 1), - ('ccx', 2, False, False): (25, 1), - ('ccx', 3, False, False): (25, 1), + ('no_overrides', 1, True, False): (23, 1), + ('no_overrides', 2, True, False): (23, 1), + ('no_overrides', 3, True, False): (23, 1), + ('ccx', 1, True, False): (23, 1), + ('ccx', 2, True, False): (23, 1), + ('ccx', 3, True, False): (23, 1), + ('no_overrides', 1, False, False): (23, 1), + ('no_overrides', 2, False, False): (23, 1), + ('no_overrides', 3, False, False): (23, 1), + ('ccx', 1, False, False): (23, 1), + ('ccx', 2, False, False): (23, 1), + ('ccx', 3, False, False): (23, 1), } @@ -254,19 +254,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (25, 3), - ('no_overrides', 2, True, False): (25, 3), - ('no_overrides', 3, True, False): (25, 3), - ('ccx', 1, True, False): (25, 3), - ('ccx', 2, True, False): (25, 3), - ('ccx', 3, True, False): (25, 3), - ('ccx', 1, True, True): (26, 3), - ('ccx', 2, True, True): (26, 3), - ('ccx', 3, True, True): (26, 3), - ('no_overrides', 1, False, False): (25, 3), - ('no_overrides', 2, False, False): (25, 3), - ('no_overrides', 3, False, False): (25, 3), - ('ccx', 1, False, False): (25, 3), - ('ccx', 2, False, False): (25, 3), - ('ccx', 3, False, False): (25, 3), + ('no_overrides', 1, True, False): (23, 3), + ('no_overrides', 2, True, False): (23, 3), + ('no_overrides', 3, True, False): (23, 3), + ('ccx', 1, True, False): (23, 3), + ('ccx', 2, True, False): (23, 3), + ('ccx', 3, True, False): (23, 3), + ('ccx', 1, True, True): (24, 3), + ('ccx', 2, True, True): (24, 3), + ('ccx', 3, True, True): (24, 3), + ('no_overrides', 1, False, False): (23, 3), + ('no_overrides', 2, False, False): (23, 3), + ('no_overrides', 3, False, False): (23, 3), + ('ccx', 1, False, False): (23, 3), + ('ccx', 2, False, False): (23, 3), + ('ccx', 3, False, False): (23, 3), } diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 0013f5ad02..3e35bd0efe 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -367,7 +367,7 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest self.assertEqual(resp.status_code, 200) def test_num_queries_instructor_paced(self): - self.fetch_course_info_with_queries(self.instructor_paced_course, 24, 4) + self.fetch_course_info_with_queries(self.instructor_paced_course, 18, 4) def test_num_queries_self_paced(self): - self.fetch_course_info_with_queries(self.self_paced_course, 24, 4) + self.fetch_course_info_with_queries(self.self_paced_course, 18, 4) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 2bf010404e..64022d42dc 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -206,8 +206,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 147), - (ModuleStoreEnum.Type.split, 4, 147), + (ModuleStoreEnum.Type.mongo, 10, 141), + (ModuleStoreEnum.Type.split, 4, 141), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1408,12 +1408,12 @@ class ProgressPageTests(ModuleStoreTestCase): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(42), check_mongo_calls(1): + with self.assertNumQueries(39), check_mongo_calls(1): self._get_progress_page() @ddt.data( - (False, 42, 28), - (True, 35, 24) + (False, 39, 25), + (True, 32, 21) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 720b249753..f9bbaacb9a 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -377,8 +377,8 @@ class ViewsQueryCountTestCase( return inner @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 4, 31), - (ModuleStoreEnum.Type.split, 3, 13, 31), + (ModuleStoreEnum.Type.mongo, 3, 4, 30), + (ModuleStoreEnum.Type.split, 3, 13, 30), ) @ddt.unpack @count_queries @@ -386,8 +386,8 @@ class ViewsQueryCountTestCase( self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 3, 27), - (ModuleStoreEnum.Type.split, 3, 10, 27), + (ModuleStoreEnum.Type.mongo, 3, 3, 26), + (ModuleStoreEnum.Type.split, 3, 10, 26), ) @ddt.unpack @count_queries diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py index e8344f70ec..30ce17cc2d 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_views.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_views.py @@ -268,7 +268,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase): self.assertEqual(response.data['developer_message'], u'Parameter usage_id not provided.') # Send empty data dictionary. - with self.assertNumQueries(8): # 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/lang_pref/tests/test_middleware.py b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py index e70f84e9d9..5ee2b9db8e 100644 --- a/openedx/core/djangoapps/lang_pref/tests/test_middleware.py +++ b/openedx/core/djangoapps/lang_pref/tests/test_middleware.py @@ -170,3 +170,44 @@ class TestUserPreferenceMiddleware(TestCase): self.assertIs(result, response) self.assertEqual(response.mock_calls, []) + + def test_preference_update_noop(self): + self.request.COOKIES[settings.LANGUAGE_COOKIE] = 'es' + + # No preference yet, should write to the database + + self.assertEqual(get_user_preference(self.user, LANGUAGE_KEY), None) + + with self.assertNumQueries(5): + self.middleware.process_request(self.request) + + self.assertEqual(get_user_preference(self.user, LANGUAGE_KEY), 'es') + + response = mock.Mock(spec=HttpResponse) + + with self.assertNumQueries(1): + self.middleware.process_response(self.request, response) + + # Preference is the same as the cookie, shouldn't write to the database + + with self.assertNumQueries(3): + self.middleware.process_request(self.request) + + self.assertEqual(get_user_preference(self.user, LANGUAGE_KEY), 'es') + + response = mock.Mock(spec=HttpResponse) + + with self.assertNumQueries(1): + self.middleware.process_response(self.request, response) + + # Cookie changed, should write to the database again + + self.request.COOKIES[settings.LANGUAGE_COOKIE] = 'en' + + with self.assertNumQueries(5): + self.middleware.process_request(self.request) + + self.assertEqual(get_user_preference(self.user, LANGUAGE_KEY), 'en') + + with self.assertNumQueries(1): + self.middleware.process_response(self.request, response) 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 8b86f1823f..d3c2506fb4 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -174,7 +174,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase): 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(15) + self._verify_get_own_username(14) def test_get_username_inactive(self): """ @@ -184,7 +184,7 @@ class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase): self.client.login(username=self.user.username, password=TEST_PASSWORD) self.user.is_active = False self.user.save() - self._verify_get_own_username(15) + self._verify_get_own_username(14) def test_get_username_not_logged_in(self): """ @@ -305,7 +305,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(19): + with self.assertNumQueries(18): response = self.send_get(self.different_client) self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY) @@ -320,7 +320,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(19): + with self.assertNumQueries(18): response = self.send_get(self.different_client) self._verify_private_account_response(response, account_privacy=PRIVATE_VISIBILITY) @@ -395,12 +395,12 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): self.assertEqual(False, data["accomplishments_shared"]) self.client.login(username=self.user.username, password=TEST_PASSWORD) - verify_get_own_information(17) + verify_get_own_information(16) # 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(11) + verify_get_own_information(10) def test_get_account_empty_string(self): """ @@ -414,7 +414,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): legacy_profile.save() self.client.login(username=self.user.username, password=TEST_PASSWORD) - with self.assertNumQueries(17): + with self.assertNumQueries(16): response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "bio"): self.assertIsNone(response.data[empty_field]) diff --git a/openedx/core/djangoapps/user_api/errors.py b/openedx/core/djangoapps/user_api/errors.py index 9435529759..1930d31365 100644 --- a/openedx/core/djangoapps/user_api/errors.py +++ b/openedx/core/djangoapps/user_api/errors.py @@ -83,6 +83,7 @@ class PreferenceValidationError(PreferenceRequestError): """ def __init__(self, preference_errors): self.preference_errors = preference_errors + super(PreferenceValidationError, self).__init__(preference_errors) class PreferenceUpdateError(PreferenceRequestError): @@ -93,6 +94,7 @@ class PreferenceUpdateError(PreferenceRequestError): def __init__(self, developer_message, user_message=None): self.developer_message = developer_message self.user_message = user_message + super(PreferenceUpdateError, self).__init__(developer_message) class CountryCodeError(ValueError): diff --git a/openedx/core/djangoapps/user_api/preferences/api.py b/openedx/core/djangoapps/user_api/preferences/api.py index f8e0c7c6c4..a017d91252 100644 --- a/openedx/core/djangoapps/user_api/preferences/api.py +++ b/openedx/core/djangoapps/user_api/preferences/api.py @@ -142,7 +142,9 @@ def update_user_preferences(requesting_user, update, user=None): if preference_value is not None: try: serializer = serializers[preference_key] - serializer.save() + + if serializer.instance is None or serializer.instance.value != serializer.validated_data['value']: + serializer.save() except Exception as error: raise _create_preference_update_error(preference_key, preference_value, error) else: @@ -177,10 +179,11 @@ def set_user_preference(requesting_user, preference_key, preference_value, usern existing_user = _get_authorized_user(requesting_user, username) serializer = create_user_preference_serializer(existing_user, preference_key, preference_value) validate_user_preference_serializer(serializer, preference_key, preference_value) - try: - serializer.save() - except Exception as error: - raise _create_preference_update_error(preference_key, preference_value, error) + if serializer.instance is None or serializer.instance.value != serializer.validated_data['value']: + try: + serializer.save() + except Exception as error: + raise _create_preference_update_error(preference_key, preference_value, error) @intercept_errors(UserAPIInternalError, ignore_errors=[UserAPIRequestError]) @@ -353,13 +356,13 @@ def create_user_preference_serializer(user, preference_key, preference_value): except ObjectDoesNotExist: existing_user_preference = None new_data = { - "user": user.id, "key": preference_key, "value": preference_value, } if existing_user_preference: - serializer = RawUserPreferenceSerializer(existing_user_preference, data=new_data) + serializer = RawUserPreferenceSerializer(existing_user_preference, data=new_data, partial=True) else: + new_data['user'] = user.id serializer = RawUserPreferenceSerializer(data=new_data) return serializer