From 8247ace5957694a8a66b7f9336c260d402ffdf0d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 16 Mar 2015 09:41:40 -0400 Subject: [PATCH] Revert "Merge pull request #7258 from cpennington/lms-field-data-query-counts" This reverts commit 4856ef78d39719834318ea063b64ddf8d2f1e571, reversing changes made to 7abbbb443a4eb6b5b6cc1650e0d677119bb2293d. --- lms/djangoapps/courseware/model_data.py | 56 ++++----- .../courseware/tests/test_model_data.py | 108 +++++------------- 2 files changed, 58 insertions(+), 106 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 3e660d1848..eb0993ee4e 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -266,7 +266,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) @@ -275,26 +275,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, ) @@ -360,39 +362,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): @@ -407,7 +409,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 3d203bed6b..80b4c04512 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -106,68 +106,48 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'})) self.user = student_module.student self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - - # 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) def test_get_existing_field(self): "Test that getting an existing field in an existing StudentModule works" - # This should only read from the cache, not the database - with self.assertNumQueries(0): - self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) + self.assertEquals('a_value', self.kvs.get(user_state_key('a_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing StudentModule raises a KeyError" - # This should only read from the cache, not the database - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) + self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) def test_set_existing_field(self): "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): - self.kvs.set(user_state_key('a_field'), 'new_value') + 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)) def test_set_missing_field(self): "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): - self.kvs.set(user_state_key('not_a_field'), 'new_value') + 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)) def test_delete_existing_field(self): "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): - self.kvs.delete(user_state_key('a_field')) + 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')) def test_delete_missing_field(self): "Test that deleting a missing field from an existing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) + self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_has_existing_field(self): "Test that `has` returns True for existing fields in StudentModules" - with self.assertNumQueries(0): - self.assertTrue(self.kvs.has(user_state_key('a_field'))) + self.assertTrue(self.kvs.has(user_state_key('a_field'))) def test_has_missing_field(self): "Test that `has` returns False for missing fields in StudentModule" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) + self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -180,12 +160,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): def test_set_many(self): "Test setting many fields that are scoped to Scope.user_state" kv_dict = self.construct_kv_dict() - - # 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): - self.kvs.set_many(kv_dict) + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -210,26 +185,19 @@ class TestMissingStudentModule(TestCase): self.user = UserFactory.create(username='user') self.assertEqual(self.user.id, 1) # check our assumption hard-coded in the key functions above. - - # The descriptor has no fields, so FDC shouldn't send any queries - with self.assertNumQueries(0): - self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) + self.field_data_cache = FieldDataCache([mock_descriptor()], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_get_field_from_missing_student_module(self): "Test that getting a field from a missing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) + self.assertRaises(KeyError, self.kvs.get, user_state_key('a_field')) def test_set_field_in_missing_student_module(self): "Test that setting a field in a missing StudentModule creates the student module" self.assertEquals(0, len(self.field_data_cache.cache)) self.assertEquals(0, StudentModule.objects.all().count()) - # We are updating a problem, so we write to courseware_studentmodulehistory - # as well as courseware_studentmodule - with self.assertNumQueries(2): - self.kvs.set(user_state_key('a_field'), 'a_value') + self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, len(self.field_data_cache.cache)) self.assertEquals(1, StudentModule.objects.all().count()) @@ -242,13 +210,11 @@ class TestMissingStudentModule(TestCase): def test_delete_field_from_missing_student_module(self): "Test that deleting a field from a missing StudentModule raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) + self.assertRaises(KeyError, self.kvs.delete, user_state_key('a_field')) def test_has_field_for_missing_student_module(self): "Test that `has` returns False for missing StudentModules" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(user_state_key('a_field'))) + self.assertFalse(self.kvs.has(user_state_key('a_field'))) class StorageTestBase(object): @@ -274,64 +240,51 @@ class StorageTestBase(object): self.mock_descriptor = mock_descriptor([ mock_field(self.scope, 'existing_field'), mock_field(self.scope, 'other_existing_field')]) - # Each field is stored as a separate row in the table, - # but we can query them in a single query - with self.assertNumQueries(1): - self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) + self.field_data_cache = FieldDataCache([self.mock_descriptor], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - with self.assertNumQueries(1): - 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'))) + self.kvs.set(self.key_factory('existing_field'), 'test_value') + self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_existing_field(self): "Test that getting an existing field in an existing Storage Field works" - with self.assertNumQueries(0): - self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) + self.assertEquals('old_value', self.kvs.get(self.key_factory('existing_field'))) def test_get_missing_field(self): "Test that getting a missing field from an existing Storage Field raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) + self.assertRaises(KeyError, self.kvs.get, self.key_factory('missing_field')) def test_set_existing_field(self): "Test that setting an existing field changes the value" - with self.assertNumQueries(1): - self.kvs.set(self.key_factory('existing_field'), 'new_value') + 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): - self.kvs.set(self.key_factory('missing_field'), 'new_value') + 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)) self.assertEquals('new_value', json.loads(self.storage_class.objects.get(field_name='missing_field').value)) def test_delete_existing_field(self): "Test that deleting an existing field removes it" - with self.assertNumQueries(1): - self.kvs.delete(self.key_factory('existing_field')) + self.kvs.delete(self.key_factory('existing_field')) self.assertEquals(0, self.storage_class.objects.all().count()) def test_delete_missing_field(self): "Test that deleting a missing field from an existing Storage Field raises a KeyError" - with self.assertNumQueries(0): - self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) + self.assertRaises(KeyError, self.kvs.delete, self.key_factory('missing_field')) self.assertEquals(1, self.storage_class.objects.all().count()) def test_has_existing_field(self): "Test that `has` returns True for an existing Storage Field" - with self.assertNumQueries(0): - self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) + self.assertTrue(self.kvs.has(self.key_factory('existing_field'))) def test_has_missing_field(self): "Test that `has` return False for an existing Storage Field" - with self.assertNumQueries(0): - self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) + self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) def construct_kv_dict(self): """Construct a kv_dict that can be passed to set_many""" @@ -345,10 +298,7 @@ class StorageTestBase(object): """Test that setting many regular fields at the same time works""" kv_dict = self.construct_kv_dict() - # Each field is a separate row in the database, hence - # a separate query - with self.assertNumQueries(len(kv_dict)): - self.kvs.set_many(kv_dict) + self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -356,8 +306,7 @@ class StorageTestBase(object): """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): - self.kvs.set(key, 'test value') + self.kvs.set(key, 'test value') with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): with self.assertRaises(KeyValueMultiSaveError) as exception_context: @@ -365,6 +314,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):