diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 4f2dc29e4f..8a3bcb7007 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/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_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) - 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) + # 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() diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 2ecf717702..770391b142 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/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() - for key in kv_dict: - with self.assertNumQueries(1): + with self.assertNumQueries(6): + for key in kv_dict: 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):