Minimize the number of queries done while noop updating a user-preference

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),
     }

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)

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):

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

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'),

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)

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])

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):

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