Skip to content

E
edx-platform
Commit 8247ace5 by Calen Pennington
Options

Revert "Merge pull request #7258 from cpennington/lms-field-data-query-counts" 

This reverts commit 4856ef78, reversing
changes made to 7abbbb44.
parent 4856ef78
Showing with 59 additions and 107 deletions
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_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):
......@@ -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()
......
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):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment