Revert "Decrease the number of inserts and updates needed by DjangoKeyValueStore"

This reverts commit 88b91874.

lms/djangoapps/courseware/model_data.py

@@ -284,7 +284,7 @@ class FieldDataCache(object):
 
     def find_or_create(self, key):
         '''
-        Find a model data object in this cache, or create a new one if it doesn't
+        Find a model data object in this cache, or create it if it doesn't
         exist
         '''
         field_object = self.find(key)
@@ -293,26 +293,28 @@ class FieldDataCache(object):
             return field_object
 
         if key.scope == Scope.user_state:
-            field_object = StudentModule(
+            field_object, __ = StudentModule.objects.get_or_create(
                 course_id=self.course_id,
                 student_id=key.user_id,
                 module_state_key=key.block_scope_id,
-                state=json.dumps({}),
-                module_type=key.block_scope_id.block_type,
+                defaults={
+                    'state': json.dumps({}),
+                    'module_type': key.block_scope_id.block_type,
+                },
             )
         elif key.scope == Scope.user_state_summary:
-            field_object = XModuleUserStateSummaryField(
+            field_object, __ = XModuleUserStateSummaryField.objects.get_or_create(
                 field_name=key.field_name,
                 usage_id=key.block_scope_id
             )
         elif key.scope == Scope.preferences:
-            field_object = XModuleStudentPrefsField(
+            field_object, __ = XModuleStudentPrefsField.objects.get_or_create(
                 field_name=key.field_name,
                 module_type=BlockTypeKeyV1(key.block_family, key.block_scope_id),
                 student_id=key.user_id,
             )
         elif key.scope == Scope.user_info:
-            field_object = XModuleStudentInfoField(
+            field_object, __ = XModuleStudentInfoField.objects.get_or_create(
                 field_name=key.field_name,
                 student_id=key.user_id,
             )
@@ -378,39 +380,39 @@ class DjangoKeyValueStore(KeyValueStore):
 
         """
         saved_fields = []
-        # field_objects maps id(field_object) to a the object and a list of associated fields.
-        # We use id() because FieldDataCache might return django models with no primary key
-        # set, but will return the same django model each time the same key is passed in.
-        dirty_field_objects = defaultdict(lambda: (None, []))
-        for key in kv_dict:
-            # Check key for validity
-            if key.scope not in self._allowed_scopes:
-                raise InvalidScopeError(key)
-
-            field_object = self._field_data_cache.find_or_create(key)
-            # Update the list dirtied field_objects
-            _, dirty_names = dirty_field_objects.setdefault(id(field_object), (field_object, []))
-            dirty_names.append(key.field_name)
+        # field_objects maps a field_object to a list of associated fields
+        field_objects = dict()
+        for field in kv_dict:
+            # Check field for validity
+            if field.scope not in self._allowed_scopes:
+                raise InvalidScopeError(field)
+
+            # If the field is valid and isn't already in the dictionary, add it.
+            field_object = self._field_data_cache.find_or_create(field)
+            if field_object not in field_objects.keys():
+                field_objects[field_object] = []
+            # Update the list of associated fields
+            field_objects[field_object].append(field)
 
             # Special case when scope is for the user state, because this scope saves fields in a single row
-            if key.scope == Scope.user_state:
+            if field.scope == Scope.user_state:
                 state = json.loads(field_object.state)
-                state[key.field_name] = kv_dict[key]
+                state[field.field_name] = kv_dict[field]
                 field_object.state = json.dumps(state)
             else:
                 # The remaining scopes save fields on different rows, so
                 # we don't have to worry about conflicts
-                field_object.value = json.dumps(kv_dict[key])
+                field_object.value = json.dumps(kv_dict[field])
 
-        for field_object, names in dirty_field_objects.values():
+        for field_object in field_objects:
             try:
                 # Save the field object that we made above
-                field_object.save(force_update=field_object.pk is not None)
+                field_object.save()
                 # If save is successful on this scope, add the saved fields to
                 # the list of successful saves
-                saved_fields.extend(names)
+                saved_fields.extend([field.field_name for field in field_objects[field_object]])
             except DatabaseError:
-                log.exception('Error saving fields %r', names)
+                log.exception('Error saving fields %r', field_objects[field_object])
                 raise KeyValueMultiSaveError(saved_fields)
 
     def delete(self, key):
@@ -425,7 +427,7 @@ class DjangoKeyValueStore(KeyValueStore):
             state = json.loads(field_object.state)
             del state[key.field_name]
             field_object.state = json.dumps(state)
-            field_object.save(force_update=field_object.pk is not None)
+            field_object.save()
         else:
             field_object.delete()
 

lms/djangoapps/courseware/tests/test_model_data.py

@@ -109,11 +109,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase):
 
         # There should be only one query to load a single descriptor with a single user_state field
         with self.assertNumQueries(1):
-            self.field_data_cache = FieldDataCache(
-                [mock_descriptor([mock_field(Scope.user_state, 'a_field')])],
-                course_id,
-                self.user
-            )
+            self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user)
 
         self.kvs = DjangoKeyValueStore(self.field_data_cache)
 
@@ -133,7 +129,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase):
         "Test that setting an existing user_state field changes the value"
         # We are updating a problem, so we write to courseware_studentmodulehistory
         # as well as courseware_studentmodule
-        with self.assertNumQueries(2):
+        with self.assertNumQueries(3):
             self.kvs.set(user_state_key('a_field'), 'new_value')
         self.assertEquals(1, StudentModule.objects.all().count())
         self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
@@ -142,7 +138,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase):
         "Test that setting a new user_state field changes the value"
         # We are updating a problem, so we write to courseware_studentmodulehistory
         # as well as courseware_studentmodule
-        with self.assertNumQueries(2):
+        with self.assertNumQueries(3):
             self.kvs.set(user_state_key('not_a_field'), 'new_value')
         self.assertEquals(1, StudentModule.objects.all().count())
         self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state))
@@ -151,7 +147,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase):
         "Test that deleting an existing field removes it from the StudentModule"
         # We are updating a problem, so we write to courseware_studentmodulehistory
         # as well as courseware_studentmodule
-        with self.assertNumQueries(2):
+        with self.assertNumQueries(3):
             self.kvs.delete(user_state_key('a_field'))
         self.assertEquals(1, StudentModule.objects.all().count())
         self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field'))
@@ -188,7 +184,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase):
         # Scope.user_state is stored in a single row in the database, so we only
         # need to send a single update to that table.
         # We also are updating a problem, so we write to courseware student module history
-        with self.assertNumQueries(2):
+        with self.assertNumQueries(3):
             self.kvs.set_many(kv_dict)
 
         for key in kv_dict:
@@ -232,7 +228,7 @@ class TestMissingStudentModule(TestCase):
 
         # We are updating a problem, so we write to courseware_studentmodulehistory
         # as well as courseware_studentmodule
-        with self.assertNumQueries(2):
+        with self.assertNumQueries(6):
             self.kvs.set(user_state_key('a_field'), 'a_value')
 
         self.assertEquals(1, len(self.field_data_cache.cache))
@@ -285,7 +281,7 @@ class StorageTestBase(object):
         self.kvs = DjangoKeyValueStore(self.field_data_cache)
 
     def test_set_and_get_existing_field(self):
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(2):
             self.kvs.set(self.key_factory('existing_field'), 'test_value')
         with self.assertNumQueries(0):
             self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field')))
@@ -302,14 +298,14 @@ class StorageTestBase(object):
 
     def test_set_existing_field(self):
         "Test that setting an existing field changes the value"
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(2):
             self.kvs.set(self.key_factory('existing_field'), 'new_value')
         self.assertEquals(1, self.storage_class.objects.all().count())
         self.assertEquals('new_value', json.loads(self.storage_class.objects.all()[0].value))
 
     def test_set_missing_field(self):
         "Test that setting a new field changes the value"
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(4):
             self.kvs.set(self.key_factory('missing_field'), 'new_value')
         self.assertEquals(2, self.storage_class.objects.all().count())
         self.assertEquals('old_value', json.loads(self.storage_class.objects.get(field_name='existing_field').value))
@@ -351,7 +347,7 @@ class StorageTestBase(object):
 
         # Each field is a separate row in the database, hence
         # a separate query
-        with self.assertNumQueries(len(kv_dict)):
+        with self.assertNumQueries(len(kv_dict)*3):
             self.kvs.set_many(kv_dict)
         for key in kv_dict:
             self.assertEquals(self.kvs.get(key), kv_dict[key])
@@ -359,8 +355,8 @@ class StorageTestBase(object):
     def test_set_many_failure(self):
         """Test that setting many regular fields with a DB error """
         kv_dict = self.construct_kv_dict()
+        with self.assertNumQueries(6):
             for key in kv_dict:
-            with self.assertNumQueries(1):
                 self.kvs.set(key, 'test value')
 
         with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]):
@@ -369,6 +365,7 @@ class StorageTestBase(object):
 
         exception = exception_context.exception
         self.assertEquals(len(exception.saved_field_names), 1)
+        self.assertEquals(exception.saved_field_names[0], 'existing_field')
 
 
 class TestUserStateSummaryStorage(StorageTestBase, TestCase):