diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index a8bc8e1276..28719cc6a3 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -24,6 +24,7 @@ from xblock.exceptions import KeyValueMultiSaveError, InvalidScopeError from xblock.fields import Scope, UserScope from xmodule.modulestore.django import modulestore from xblock.core import XBlockAside +from courseware.user_state_client import DjangoXBlockUserStateClient log = logging.getLogger(__name__) @@ -204,18 +205,28 @@ class DjangoOrmFieldCache(object): cache_key = self._cache_key_for_kvs_key(kvs_key) field_object = self._cache.get(cache_key) - if field_object is None: - self._cache[cache_key] = field_object = self._create_object(kvs_key) - - self._set_field_value(field_object, kvs_key, value) - try: - field_object.save() - saved_fields.append(kvs_key.field_name) + serialized_value = json.dumps(value) + # It is safe to force an insert or an update, because + # a) we should have retrieved the object as part of the + # prefetch step, so if it isn't in our cache, it doesn't exist yet. + # b) no other code should be modifying these models out of band of + # this cache. + if field_object is None: + field_object = self._create_object(kvs_key, serialized_value) + field_object.save(force_insert=True) + self._cache[cache_key] = field_object + else: + field_object.value = serialized_value + field_object.save(force_update=True) + except DatabaseError: log.exception("Saving field %r failed", kvs_key.field_name) raise KeyValueMultiSaveError(saved_fields) + finally: + saved_fields.append(kvs_key.field_name) + @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): """ @@ -264,15 +275,11 @@ class DjangoOrmFieldCache(object): else: return field_object.modified - @contract(kvs_key=DjangoKeyValueStore.Key) - def _set_field_value(self, field_object, kvs_key, value): - field_object.value = json.dumps(value) - def __len__(self): return len(self._cache) @abstractmethod - def _create_object(self, kvs_key): + def _create_object(self, kvs_key, value): """ Create a new object to add to the cache (which should record the specified field ``value`` for the field identified by @@ -280,6 +287,7 @@ class DjangoOrmFieldCache(object): Arguments: kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + value: What value to record in the field """ raise NotImplementedError() @@ -324,9 +332,10 @@ class UserStateCache(object): Cache for Scope.user_state xblock field data. """ def __init__(self, user, course_id): - self._cache = {} + self._cache = defaultdict(dict) self.course_id = course_id self.user = user + self._client = DjangoXBlockUserStateClient(self.user) def cache_fields(self, fields, xblocks, aside_types): # pylint: disable=unused-argument """ @@ -338,15 +347,12 @@ class UserStateCache(object): xblocks (list of :class:`XBlock`): XBlocks to cache fields for. aside_types (list of str): Aside types to cache fields for. """ - query = StudentModule.objects.chunked_filter( - 'module_state_key__in', + block_field_state = self._client.get_many( + self.user.username, _all_usage_keys(xblocks, aside_types), - course_id=self.course_id, - student=self.user.pk, ) - for field_object in query: - cache_key = field_object.module_state_key.map_into_course(self.course_id) - self._cache[cache_key] = field_object + for usage_key, field_state in block_field_state: + self._cache[usage_key] = field_state @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): @@ -369,12 +375,11 @@ class UserStateCache(object): Returns: datetime if there was a modified date, or None otherwise """ - field_object = self._cache.get(self._cache_key_for_kvs_key(kvs_key)) - - if field_object is None: - return None - else: - return field_object.modified + return self._client.get_mod_date( + self.user.username, + kvs_key.block_scope_id, + fields=[kvs_key.field_name], + ).get(kvs_key.field_name) @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") def set_many(self, kv_dict): @@ -385,25 +390,21 @@ class UserStateCache(object): kv_dict (dict): A dictionary mapping :class:`~DjangoKeyValueStore.Key` objects to values to set. """ - dirty_field_objects = defaultdict(set) + pending_updates = defaultdict(dict) for kvs_key, value in kv_dict.items(): cache_key = self._cache_key_for_kvs_key(kvs_key) - field_object = self._cache.get(cache_key) - if field_object is None: - self._cache[cache_key] = field_object = self._create_object(kvs_key) + pending_updates[cache_key][kvs_key.field_name] = value - self._set_field_value(field_object, kvs_key, value) - dirty_field_objects[field_object].add(kvs_key.field_name) - - saved_fields = [] - for field_object, fields in sorted(dirty_field_objects.iteritems()): - try: - field_object.save() - saved_fields.extend(fields) - except DatabaseError: - log.exception("Saving fields %r failed", fields) - raise KeyValueMultiSaveError(saved_fields) + try: + self._client.set_many( + self.user.username, + pending_updates + ) + except DatabaseError: + raise KeyValueMultiSaveError([]) + finally: + self._cache.update(pending_updates) @contract(kvs_key=DjangoKeyValueStore.Key) def get(self, kvs_key): @@ -420,9 +421,7 @@ class UserStateCache(object): if cache_key not in self._cache: raise KeyError(kvs_key.field_name) - field_object = self._cache[cache_key] - - return json.loads(field_object.state)[kvs_key.field_name] + return self._cache[cache_key][kvs_key.field_name] @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): @@ -434,15 +433,17 @@ class UserStateCache(object): Raises: KeyError if key isn't found in the cache """ - - field_object = self._cache.get(self._cache_key_for_kvs_key(kvs_key)) - if field_object is None: + cache_key = self._cache_key_for_kvs_key(kvs_key) + if cache_key not in self._cache: raise KeyError(kvs_key.field_name) - state = json.loads(field_object.state) - del state[kvs_key.field_name] - field_object.state = json.dumps(state) - field_object.save() + field_state = self._cache[cache_key] + + if kvs_key.field_name not in field_state: + raise KeyError(kvs_key.field_name) + + self._client.delete(self.user.username, cache_key, fields=[kvs_key.field_name]) + del field_state[kvs_key.field_name] @contract(kvs_key=DjangoKeyValueStore.Key, returns=bool) def has(self, kvs_key): @@ -454,11 +455,12 @@ class UserStateCache(object): Returns: bool """ - field_object = self._cache.get(self._cache_key_for_kvs_key(kvs_key)) - if field_object is None: - return False + cache_key = self._cache_key_for_kvs_key(kvs_key) - return kvs_key.field_name in json.loads(field_object.state) + return ( + cache_key in self._cache and + kvs_key.field_name in self._cache[cache_key] + ) @contract(user_id=int, usage_key=UsageKey, score="number|None", max_score="number|None") def set_score(self, user_id, usage_key, score, max_score): @@ -467,30 +469,23 @@ class UserStateCache(object): Set the score and max_score for the specified user and xblock usage. """ - field_object = self._cache[usage_key] - field_object.grade = score - field_object.max_grade = max_score - field_object.save() - - @contract(kvs_key=DjangoKeyValueStore.Key) - def _set_field_value(self, field_object, kvs_key, value): - field_object.value = json.dumps(value) + student_module, created = StudentModule.objects.get_or_create( + student_id=user_id, + module_state_key=usage_key, + course_id=usage_key.course_key, + defaults={ + 'grade': score, + 'max_grade': max_score, + } + ) + if not created: + student_module.grade = score + student_module.max_grade = max_score + student_module.save() def __len__(self): return len(self._cache) - def _create_object(self, kvs_key): - field_object, __ = StudentModule.objects.get_or_create( - course_id=self.course_id, - student_id=kvs_key.user_id, - module_state_key=kvs_key.block_scope_id, - defaults={ - 'state': json.dumps({}), - 'module_type': kvs_key.block_scope_id.block_type, - }, - ) - return field_object - def _cache_key_for_kvs_key(self, key): """ Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. @@ -500,12 +495,6 @@ class UserStateCache(object): """ return key.block_scope_id - @contract(kvs_key=DjangoKeyValueStore.Key) - def _set_field_value(self, field_object, kvs_key, value): - state = json.loads(field_object.state) - state[kvs_key.field_name] = value - field_object.state = json.dumps(state) - class UserStateSummaryCache(DjangoOrmFieldCache): """ @@ -515,7 +504,7 @@ class UserStateSummaryCache(DjangoOrmFieldCache): super(UserStateSummaryCache, self).__init__() self.course_id = course_id - def _create_object(self, kvs_key): + def _create_object(self, kvs_key, value): """ Create a new object to add to the cache (which should record the specified field ``value`` for the field identified by @@ -523,12 +512,13 @@ class UserStateSummaryCache(DjangoOrmFieldCache): Arguments: kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + value: The value to assign to the new field object """ - field_object, __ = XModuleUserStateSummaryField.objects.get_or_create( + return XModuleUserStateSummaryField( field_name=kvs_key.field_name, - usage_id=kvs_key.block_scope_id + usage_id=kvs_key.block_scope_id, + value=value, ) - return field_object def _read_objects(self, fields, xblocks, aside_types): """ @@ -575,7 +565,7 @@ class PreferencesCache(DjangoOrmFieldCache): super(PreferencesCache, self).__init__() self.user = user - def _create_object(self, kvs_key): + def _create_object(self, kvs_key, value): """ Create a new object to add to the cache (which should record the specified field ``value`` for the field identified by @@ -583,13 +573,14 @@ class PreferencesCache(DjangoOrmFieldCache): Arguments: kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + value: The value to assign to the new field object """ - field_object, __ = XModuleStudentPrefsField.objects.get_or_create( + return XModuleStudentPrefsField( field_name=kvs_key.field_name, module_type=BlockTypeKeyV1(kvs_key.block_family, kvs_key.block_scope_id), student_id=kvs_key.user_id, + value=value, ) - return field_object def _read_objects(self, fields, xblocks, aside_types): """ @@ -637,7 +628,7 @@ class UserInfoCache(DjangoOrmFieldCache): super(UserInfoCache, self).__init__() self.user = user - def _create_object(self, kvs_key): + def _create_object(self, kvs_key, value): """ Create a new object to add to the cache (which should record the specified field ``value`` for the field identified by @@ -645,12 +636,13 @@ class UserInfoCache(DjangoOrmFieldCache): Arguments: kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + value: The value to assign to the new field object """ - field_object, __ = XModuleStudentInfoField.objects.get_or_create( + return XModuleStudentInfoField( field_name=kvs_key.field_name, student_id=kvs_key.user_id, + value=value, ) - return field_object def _read_objects(self, fields, xblocks, aside_types): """ diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index f75e0cdb89..df04376065 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -134,7 +134,10 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): 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 + # as well as courseware_studentmodule. We also need to read the database + # to discover if something other than the DjangoXBlockUserStateClient + # has written to the StudentModule (such as UserStateCache setting the score + # on the StudentModule). with self.assertNumQueries(3): self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) @@ -143,7 +146,10 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): 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 + # as well as courseware_studentmodule. We also need to read the database + # to discover if something other than the DjangoXBlockUserStateClient + # has written to the StudentModule (such as UserStateCache setting the score + # on the StudentModule). with self.assertNumQueries(3): self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) @@ -152,7 +158,10 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): 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 + # as well as courseware_studentmodule. We also need to read the database + # to discover if something other than the DjangoXBlockUserStateClient + # has written to the StudentModule (such as UserStateCache setting the score + # on the StudentModule). with self.assertNumQueries(3): self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) @@ -190,6 +199,9 @@ 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 + # We also need to read the database to discover if something other than the + # DjangoXBlockUserStateClient has written to the StudentModule (such as + # UserStateCache setting the score on the StudentModule). with self.assertNumQueries(3): self.kvs.set_many(kv_dict) @@ -207,7 +219,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): with patch('django.db.models.Model.save', side_effect=DatabaseError): with self.assertRaises(KeyValueMultiSaveError) as exception_context: self.kvs.set_many(kv_dict) - self.assertEquals(len(exception_context.exception.saved_field_names), 0) + self.assertEquals(exception_context.exception.saved_field_names, []) @attr('shard_1') @@ -234,8 +246,11 @@ class TestMissingStudentModule(TestCase): 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(6): + # as well as courseware_studentmodule. We also need to read the database + # to discover if something other than the DjangoXBlockUserStateClient + # has written to the StudentModule (such as UserStateCache setting the score + # on the StudentModule). + with self.assertNumQueries(3): self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, sum(len(cache) for cache in self.field_data_cache.cache.values())) @@ -289,7 +304,7 @@ class StorageTestBase(object): self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - with self.assertNumQueries(2): + 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'))) @@ -306,14 +321,14 @@ class StorageTestBase(object): def test_set_existing_field(self): "Test that setting an existing field changes the value" - with self.assertNumQueries(2): + with self.assertNumQueries(1): 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(4): + with self.assertNumQueries(1): 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)) @@ -355,7 +370,7 @@ class StorageTestBase(object): # Each field is a separate row in the database, hence # a separate query - with self.assertNumQueries(len(kv_dict) * 3): + with self.assertNumQueries(len(kv_dict)): self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -363,8 +378,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: + 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]): @@ -372,8 +387,7 @@ class StorageTestBase(object): self.kvs.set_many(kv_dict) exception = exception_context.exception - self.assertEquals(len(exception.saved_field_names), 1) - self.assertIn(exception.saved_field_names[0], ('existing_field', 'other_existing_field')) + self.assertEquals(exception.saved_field_names, ['existing_field', 'other_existing_field']) class TestUserStateSummaryStorage(StorageTestBase, TestCase): diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index cccfd65f62..811cecb59a 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -40,7 +40,6 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): pass def __init__(self, user): - self._student_module_cache = {} self.user = user def get(self, username, block_key, scope=Scope.user_state, fields=None): @@ -93,6 +92,28 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): assert self.user.username == username return self.delete_many(username, [block_key], scope, fields=fields) + @contract(username="basestring", block_key=UsageKey, scope=ScopeBase, fields="seq(basestring)|set(basestring)|None") + def get_mod_date(self, username, block_key, scope=Scope.user_state, fields=None): + """ + Get the last modification date for fields from the specified blocks. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock modification dates to retrieve. + scope (Scope): The scope to retrieve from. + fields: A list of fields to query. If None, delete all stored fields. + Specific implementations are free to return the same modification date + for all fields, if they don't store changes individually per field. + Implementations may omit fields for which data has not been stored. + + Returns: list a dict of {field_name: modified_date} for each selected field. + """ + results = self.get_mod_date_many(username, [block_key], scope, fields=fields) + return { + field: date for (_, field, date) in results + } + + @contract(username="basestring", block_keys="seq(UsageKey)|set(UsageKey)") def _get_field_objects(self, username, block_keys): """ Retrieve the :class:`~StudentModule`s for the supplied ``username`` and ``block_keys``. @@ -108,23 +129,15 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): ) for course_key, usage_keys in by_course: - not_cached = [] - for usage_key in usage_keys: - if usage_key in self._student_module_cache: - yield self._student_module_cache[usage_key] - else: - not_cached.append(usage_key) - query = StudentModule.objects.chunked_filter( 'module_state_key__in', - not_cached, + usage_keys, student__username=username, course_id=course_key, ) for student_module in query: usage_key = student_module.module_state_key.map_into_course(student_module.course_id) - self._student_module_cache[usage_key] = student_module yield (student_module, usage_key) @@ -170,14 +183,10 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") - field_objects = self._get_field_objects(username, block_keys_to_state.keys()) - for field_object in field_objects: - usage_key = field_object.module_state_key.map_into_course(field_object.course_id) - current_state = json.loads(field_object.state) - current_state.update(block_keys_to_state.pop(usage_key)) - field_object.state = json.dumps(current_state) - field_object.save() - + # We do a find_or_create for every block (rather than re-using field objects + # that were queried in get_many) so that if the score has + # been changed by some other piece of the code, we don't overwrite + # that score. for usage_key, state in block_keys_to_state.items(): student_module, created = StudentModule.objects.get_or_create( student=self.user, @@ -227,6 +236,39 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): # We just read this object, so we know that we can do an update student_module.save(force_update=True) + @contract( + username="basestring", + block_keys="seq(UsageKey)|set(UsageKey)", + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None" + ) + def get_mod_date_many(self, username, block_keys, scope=Scope.user_state, fields=None): + """ + Get the last modification date for fields from the specified blocks. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock modification dates to retrieve. + scope (Scope): The scope to retrieve from. + fields: A list of fields to query. If None, delete all stored fields. + Specific implementations are free to return the same modification date + for all fields, if they don't store changes individually per field. + Implementations may omit fields for which data has not been stored. + + Yields: tuples of (block, field_name, modified_date) for each selected field. + """ + assert self.user.username == username + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") + + field_objects = self._get_field_objects(username, block_keys) + for field_object, usage_key in field_objects: + if field_object.state is None: + continue + + for field in json.loads(field_object.state): + yield (usage_key, field, field_object.modified) + def get_history(self, username, block_key, scope=Scope.user_state): """We don't guarantee that history for many blocks will be fast.""" assert self.user.username == username diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index 2f90d96de1..446d7a934d 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -147,7 +147,7 @@ class UserCourseStatus(views.APIView): scope=Scope.user_state, user_id=request.user.id, block_scope_id=course.location, - field_name=None + field_name='position' ) original_store_date = field_data_cache.last_modified(key) if original_store_date is not None and modification_date < original_store_date: