From 4312c0e763658c4326da8e1f4ce93b48b977160c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 14:29:19 -0400 Subject: [PATCH 01/50] Enable PyContracts during tests --- .../contentstore/views/tests/test_assets.py | 15 ++++++++------- .../courseware/tests/test_module_render.py | 4 ++-- lms/djangoapps/lms_xblock/test/test_runtime.py | 17 +++++++++-------- lms/djangoapps/open_ended_grading/tests.py | 3 ++- manage.py | 3 ++- pavelib/utils/test/suites/nose_suite.py | 2 +- 6 files changed, 24 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_assets.py b/cms/djangoapps/contentstore/views/tests/test_assets.py index daa7450f2b..22f26fd16f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/views/tests/test_assets.py @@ -309,13 +309,14 @@ class DownloadTestCase(AssetsTestCase): def test_metadata_found_in_modulestore(self): # Insert asset metadata into the modulestore (with no accompanying asset). asset_key = self.course.id.make_asset_key(AssetMetadata.GENERAL_ASSET_TYPE, 'pic1.jpg') - asset_md = AssetMetadata(asset_key, { - 'internal_name': 'EKMND332DDBK', - 'basename': 'pix/archive', - 'locked': False, - 'curr_version': '14', - 'prev_version': '13' - }) + asset_md = AssetMetadata( + asset_key, + internal_name='EKMND332DDBK', + pathname='pix/archive', + locked=False, + curr_version='14', + prev_version='13', + ) modulestore().save_asset_metadata(asset_md, 15) # Get the asset metadata and have it be found in the modulestore. # Currently, no asset metadata should be found in the modulestore. The code is not yet storing it there. diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index b8f4bebc93..0534c668ec 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -48,7 +48,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory, check_mongo_calls -from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW, CombinedSystem +from xmodule.x_module import XModuleDescriptor, XModule, STUDENT_VIEW, CombinedSystem, DescriptorSystem TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @@ -1063,7 +1063,7 @@ class TestAnonymousStudentId(ModuleStoreTestCase, LoginEnrollmentTestCase): location=location, static_asset_path=None, _runtime=Mock( - spec=Runtime, + spec=DescriptorSystem, resources_fs=None, mixologist=Mock(_mixins=(), name='mixologist'), name='runtime', diff --git a/lms/djangoapps/lms_xblock/test/test_runtime.py b/lms/djangoapps/lms_xblock/test/test_runtime.py index a8fe5a9df2..6b3bb748ec 100644 --- a/lms/djangoapps/lms_xblock/test/test_runtime.py +++ b/lms/djangoapps/lms_xblock/test/test_runtime.py @@ -11,6 +11,7 @@ from urlparse import urlparse from opaque_keys.edx.locations import SlashSeparatedCourseKey from lms.djangoapps.lms_xblock.runtime import quote_slashes, unquote_slashes, LmsModuleSystem from xblock.fields import ScopeIds +from xmodule.x_module import DescriptorSystem TEST_STRINGS = [ '', @@ -48,12 +49,12 @@ class TestHandlerUrl(TestCase): self.course_key = SlashSeparatedCourseKey("org", "course", "run") self.runtime = LmsModuleSystem( static_url='/static', - track_function=Mock(), - get_module=Mock(), - render_template=Mock(), + track_function=Mock(name='track_function'), + get_module=Mock(name='get_module'), + render_template=Mock(name='render_template'), replace_urls=str, course_id=self.course_key, - descriptor_runtime=Mock(), + descriptor_runtime=Mock(spec=DescriptorSystem, name='descriptor_runtime'), ) def test_trailing_characters(self): @@ -120,13 +121,13 @@ class TestUserServiceAPI(TestCase): self.runtime = LmsModuleSystem( static_url='/static', - track_function=Mock(), - get_module=Mock(), - render_template=Mock(), + track_function=Mock(name="track_function"), + get_module=Mock(name="get_module"), + render_template=Mock(name="render_template"), replace_urls=str, course_id=self.course_id, get_real_user=mock_get_real_user, - descriptor_runtime=Mock(), + descriptor_runtime=Mock(spec=DescriptorSystem, name="descriptor_runtime"), ) self.scope = 'course' self.key = 'key1' diff --git a/lms/djangoapps/open_ended_grading/tests.py b/lms/djangoapps/open_ended_grading/tests.py index 4614e8bf47..4f7114242f 100644 --- a/lms/djangoapps/open_ended_grading/tests.py +++ b/lms/djangoapps/open_ended_grading/tests.py @@ -24,6 +24,7 @@ from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem from student.roles import CourseStaffRole from student.models import unique_id_for_user from xmodule import peer_grading_module +from xmodule.x_module import DescriptorSystem from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -288,7 +289,7 @@ class TestPeerGradingService(ModuleStoreTestCase, LoginEnrollmentTestCase): open_ended_grading_interface=test_util_open_ended.OPEN_ENDED_GRADING_INTERFACE, mixins=settings.XBLOCK_MIXINS, error_descriptor_class=ErrorDescriptor, - descriptor_runtime=None, + descriptor_runtime=Mock(spec=DescriptorSystem, name="descriptor_runtime"), ) self.descriptor = peer_grading_module.PeerGradingDescriptor(self.system, field_data, ScopeIds(None, None, None, None)) self.descriptor.xmodule_runtime = self.system diff --git a/manage.py b/manage.py index c14d386e87..266e656128 100755 --- a/manage.py +++ b/manage.py @@ -113,4 +113,5 @@ if __name__ == "__main__": from django.core.management import execute_from_command_line - execute_from_command_line([sys.argv[0]] + django_args) + sys.argv[1:] = django_args + execute_from_command_line(sys.argv) diff --git a/pavelib/utils/test/suites/nose_suite.py b/pavelib/utils/test/suites/nose_suite.py index c0597e668c..130cad4e95 100644 --- a/pavelib/utils/test/suites/nose_suite.py +++ b/pavelib/utils/test/suites/nose_suite.py @@ -120,7 +120,7 @@ class SystemTestSuite(NoseTestSuite): @property def cmd(self): cmd = ( - './manage.py {system} test --verbosity={verbosity} ' + './manage.py {system} --contracts test --verbosity={verbosity} ' '{test_id} {test_opts} --traceback --settings=test {extra} ' '--with-xunit --xunit-file={xunit_report}'.format( system=self.root, From 4182f8749045a5b239f6a96281eb23c31ae7ed81 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 9 Mar 2015 15:54:16 -0400 Subject: [PATCH 02/50] Add the candidate XBlockUserStateClient interface --- lms/djangoapps/xblock_user_state/__init__.py | 0 lms/djangoapps/xblock_user_state/interface.py | 254 ++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 lms/djangoapps/xblock_user_state/__init__.py create mode 100644 lms/djangoapps/xblock_user_state/interface.py diff --git a/lms/djangoapps/xblock_user_state/__init__.py b/lms/djangoapps/xblock_user_state/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/lms/djangoapps/xblock_user_state/interface.py b/lms/djangoapps/xblock_user_state/interface.py new file mode 100644 index 0000000000..5d001de4d1 --- /dev/null +++ b/lms/djangoapps/xblock_user_state/interface.py @@ -0,0 +1,254 @@ +""" +A baseclass for a generic client for accessing XBlock Scope.user_state field data. +""" + +from abc import abstractmethod + +from contracts import contract, new_contract, ContractsMeta +from opaque_keys.edx.keys import UsageKey +from xblock.fields import Scope, ScopeBase + +new_contract('UsageKey', UsageKey) + +class XBlockUserStateClient(object): + """ + First stab at an interface for accessing XBlock User State. This will have + use StudentModule as a backing store in the default case. + + Scope/Goals: + 1. Mediate access to all student-specific state stored by XBlocks. + a. This includes "preferences" and "user_info" (i.e. UserScope.ONE) + b. This includes XBlock Asides. + c. This may later include user_state_summary (i.e. UserScope.ALL). + d. This may include group state in the future. + e. This may include other key types + UserScope.ONE (e.g. Definition) + 2. Assume network service semantics. + At some point, this will probably be calling out to an external service. + Even if it doesn't, we want to be able to implement circuit breakers, so + that a failure in StudentModule doesn't bring down the whole site. + This also implies that the client is running as a user, and whatever is + backing it is smart enough to do authorization checks. + 3. This does not yet cover export-related functionality. + + Open Questions: + 1. Is it sufficient to just send the block_key in and extract course + + version info from it? + 2. Do we want to use the username as the identifier? Privacy implications? + Ease of debugging? + 3. Would a get_many_by_type() be useful? + """ + + __metaclass__ = ContractsMeta + + class ServiceUnavailable(Exception): + """ + This error is raised if the service backing this client is currently unavailable. + """ + pass + + class PermissionDenied(Exception): + """ + This error is raised if the caller is not allowed to access the requested data. + """ + pass + + class DoesNotExist(Exception): + """ + This error is raised if the caller has requested data that does not exist. + """ + pass + + @contract( + username="basestring", + block_key=UsageKey, + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + returns="dict(basestring: *)" + ) + def get(self, username, block_key, scope=Scope.user_state, fields=None): + """ + Retrieve the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be retrieved + block_key (UsageKey): The UsageKey identifying which xblock state to load. + scope (Scope): The scope to load data from + fields: A list of field values to retrieve. If None, retrieve all stored fields. + + Returns + dict: A dictionary mapping field names to values + """ + return next(self.get_many(username, [block_key], scope, fields=fields))[1] + + @contract( + username="basestring", + block_key=UsageKey, + state="dict(basestring: *)", + scope=ScopeBase, + returns=None, + ) + def set(self, username, block_key, state, scope=Scope.user_state): + """ + Set fields for a particular XBlock. + + Arguments: + username: The name of the user whose state should be retrieved + block_key (UsageKey): The UsageKey identifying which xblock state to load. + state (dict): A dictionary mapping field names to values + scope (Scope): The scope to store data to + """ + self.set_many(username, {block_key: state}, scope) + + @contract( + username="basestring", + block_key=UsageKey, + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + returns=None, + ) + def delete(self, username, block_key, scope=Scope.user_state, fields=None): + """ + Delete the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock state to delete. + scope (Scope): The scope to delete data from + fields: A list of fields to delete. If None, delete all stored fields. + """ + return self.delete_many(username, [block_key], scope, fields=fields) + + @contract( + username="basestring", + block_key=UsageKey, + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + returns="dict(basestring: datetime)", + ) + 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 queried + 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, query all 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)", + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + ) + @abstractmethod + def get_many(self, username, block_keys, scope=Scope.user_state, fields=None): + """ + Retrieve the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be retrieved + block_keys ([UsageKey]): A list of UsageKeys identifying which xblock states to load. + scope (Scope): The scope to load data from + fields: A list of field values to retrieve. If None, retrieve all stored fields. + + Yields: + (UsageKey, field_state) tuples for each specified UsageKey in block_keys. + field_state is a dict mapping field names to values. + """ + raise NotImplementedError() + + @contract( + username="basestring", + block_keys_to_state="dict(UsageKey: dict(basestring: *))", + scope=ScopeBase, + returns=None, + ) + @abstractmethod + def set_many(self, username, block_keys_to_state, scope=Scope.user_state): + """ + Set fields for a particular XBlock. + + Arguments: + username: The name of the user whose state should be retrieved + block_keys_to_state (dict): A dict mapping UsageKeys to state dicts. + Each state dict maps field names to values. These state dicts + are overlaid over the stored state. To delete fields, use + :meth:`delete` or :meth:`delete_many`. + scope (Scope): The scope to load data from + """ + raise NotImplementedError() + + @contract( + username="basestring", + block_keys="seq(UsageKey)|set(UsageKey)", + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + returns=None, + ) + @abstractmethod + def delete_many(self, username, block_keys, scope=Scope.user_state, fields=None): + """ + Delete the stored XBlock state for a many xblock usages. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock state to delete. + scope (Scope): The scope to delete data from + fields: A list of fields to delete. If None, delete all stored fields. + """ + raise NotImplementedError() + + @contract( + username="basestring", + block_keys="seq(UsageKey)|set(UsageKey)", + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None", + ) + @abstractmethod + 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 queried + 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. + """ + raise NotImplementedError() + + def get_history(self, username, block_key, scope=Scope.user_state): + """We don't guarantee that history for many blocks will be fast.""" + raise NotImplementedError() + + def iter_all_for_block(self, block_key, scope=Scope.user_state, batch_size=None): + """ + You get no ordering guarantees. Fetching will happen in batch_size + increments. If you're using this method, you should be running in an + async task. + """ + raise NotImplementedError() + + def iter_all_for_course(self, course_key, block_type=None, scope=Scope.user_state, batch_size=None): + """ + You get no ordering guarantees. Fetching will happen in batch_size + increments. If you're using this method, you should be running in an + async task. + """ + raise NotImplementedError() From fd0c47bc4222fca6728057740050055a924cc774 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 9 Mar 2015 16:26:38 -0400 Subject: [PATCH 03/50] Extract query chunking from FieldDataCache --- lms/djangoapps/courseware/model_data.py | 62 ++++++++++++++----------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 8a3bcb7007..6f5c844fb7 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -42,6 +42,32 @@ def chunks(items, chunk_size): return (items[i:i + chunk_size] for i in xrange(0, len(items), chunk_size)) +def _query(model_class, select_for_update, **kwargs): + """ + Queries model_class with **kwargs, optionally adding select_for_update if + `select_for_update` is True. + """ + query = model_class.objects + if select_for_update: + query = query.select_for_update() + query = query.filter(**kwargs) + return query + +def _chunked_query(model_class, select_for_update, chunk_field, items, chunk_size=500, **kwargs): + """ + Queries model_class with `chunk_field` set to chunks of size `chunk_size`, + and all other parameters from `**kwargs`. + + This works around a limitation in sqlite3 on the number of parameters + that can be put into a single query. + """ + res = chain.from_iterable( + _query(model_class, select_for_update, **dict([(chunk_field, chunk)] + kwargs.items())) + for chunk in chunks(items, chunk_size) + ) + return res + + class FieldDataCache(object): """ A cache of django model objects needed to supply the data @@ -142,30 +168,6 @@ class FieldDataCache(object): cache.add_descriptor_descendents(descriptor, depth, descriptor_filter) return cache - def _query(self, model_class, **kwargs): - """ - Queries model_class with **kwargs, optionally adding select_for_update if - self.select_for_update is set - """ - query = model_class.objects - if self.select_for_update: - query = query.select_for_update() - query = query.filter(**kwargs) - return query - - def _chunked_query(self, model_class, chunk_field, items, chunk_size=500, **kwargs): - """ - Queries model_class with `chunk_field` set to chunks of size `chunk_size`, - and all other parameters from `**kwargs` - - This works around a limitation in sqlite3 on the number of parameters - that can be put into a single query - """ - res = chain.from_iterable( - self._query(model_class, **dict([(chunk_field, chunk)] + kwargs.items())) - for chunk in chunks(items, chunk_size) - ) - return res def _all_usage_ids(self, descriptors): """ @@ -199,31 +201,35 @@ class FieldDataCache(object): Queries the database for all of the fields in the specified scope """ if scope == Scope.user_state: - return self._chunked_query( + return _chunked_query( StudentModule, + self.select_for_update, 'module_state_key__in', self._all_usage_ids(descriptors), course_id=self.course_id, student=self.user.pk, ) elif scope == Scope.user_state_summary: - return self._chunked_query( + return _chunked_query( XModuleUserStateSummaryField, + self.select_for_update, 'usage_id__in', self._all_usage_ids(descriptors), field_name__in=set(field.name for field in fields), ) elif scope == Scope.preferences: - return self._chunked_query( + return _chunked_query( XModuleStudentPrefsField, + self.select_for_update, 'module_type__in', self._all_block_types(descriptors), student=self.user.pk, field_name__in=set(field.name for field in fields), ) elif scope == Scope.user_info: - return self._query( + return _query( XModuleStudentInfoField, + self.select_for_update, student=self.user.pk, field_name__in=set(field.name for field in fields), ) From 90597276726b1450dcf614f70bd39ed034cf8877 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 9 Mar 2015 16:37:51 -0400 Subject: [PATCH 04/50] Extract cache instantiation into classes per-scope --- lms/djangoapps/courseware/model_data.py | 100 ++++++++++++++++++------ 1 file changed, 77 insertions(+), 23 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 6f5c844fb7..2682e58e2f 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -68,6 +68,63 @@ def _chunked_query(model_class, select_for_update, chunk_field, items, chunk_siz return res +class UserStateCache(object): + """ + Cache for Scope.user_state xblock field data. + """ + def __init__(self, user, course_id, usage_keys, select_for_update=False): + self._data = _chunked_query( + StudentModule, + select_for_update, + 'module_state_key__in', + usage_keys, + course_id=course_id, + student=user.pk, + ) + + +class UserStateSummaryCache(object): + """ + Cache for Scope.user_state_summary xblock field data. + """ + def __init__(self, usage_keys, fields, select_for_update=False): + self._data = _chunked_query( + XModuleUserStateSummaryField, + select_for_update, + 'usage_id__in', + usage_keys, + field_name__in=set(field.name for field in fields), + ) + + +class PreferencesCache(object): + """ + Cache for Scope.preferences xblock field data. + """ + def __init__(self, user, block_types, fields, select_for_update=False): + self._data = _chunked_query( + XModuleStudentPrefsField, + select_for_update, + 'module_type__in', + block_types, + student=user.pk, + field_name__in=set(field.name for field in fields), + ) + + +class UserInfoCache(object): + """ + Cache for Scope.user_info xblock field data + """ + def __init__(self, user, fields, select_for_update=False): + self._data = _query( + XModuleStudentInfoField, + select_for_update, + student=user.pk, + field_name__in=set(field.name for field in fields), + ) + + class FieldDataCache(object): """ A cache of django model objects needed to supply the data @@ -201,38 +258,35 @@ class FieldDataCache(object): Queries the database for all of the fields in the specified scope """ if scope == Scope.user_state: - return _chunked_query( - StudentModule, - self.select_for_update, - 'module_state_key__in', + self.user_state_cache = UserStateCache( + self.user, + self.course_id, self._all_usage_ids(descriptors), - course_id=self.course_id, - student=self.user.pk, + self.select_for_update, ) + return self.user_state_cache._data elif scope == Scope.user_state_summary: - return _chunked_query( - XModuleUserStateSummaryField, - self.select_for_update, - 'usage_id__in', + self.user_state_summary_cache = UserStateSummaryCache( self._all_usage_ids(descriptors), - field_name__in=set(field.name for field in fields), + fields, + self.select_for_update, ) + return self.user_state_summary_cache._data elif scope == Scope.preferences: - return _chunked_query( - XModuleStudentPrefsField, - self.select_for_update, - 'module_type__in', + self.preferences_cache = PreferencesCache( + self.user, self._all_block_types(descriptors), - student=self.user.pk, - field_name__in=set(field.name for field in fields), - ) - elif scope == Scope.user_info: - return _query( - XModuleStudentInfoField, + fields, self.select_for_update, - student=self.user.pk, - field_name__in=set(field.name for field in fields), ) + return self.preferences_cache._data + elif scope == Scope.user_info: + self.user_info_cache = UserInfoCache( + self.user, + fields, + self.select_for_update, + ) + return self.user_info_cache._data else: return [] From 7353d40b16e7bb589281c1a678c82db9c75678c2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 8 Apr 2015 16:18:58 -0400 Subject: [PATCH 05/50] Push field_object iteration inside _retrieve_fields, and rename to _cache_fields --- lms/djangoapps/courseware/model_data.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 2682e58e2f..0825e93c27 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -164,8 +164,7 @@ class FieldDataCache(object): """ if self.user.is_authenticated(): for scope, fields in self._fields_to_cache(descriptors).items(): - for field_object in self._retrieve_fields(scope, fields, descriptors): - self.cache[self._cache_key_from_field_object(scope, field_object)] = field_object + self._cache_fields(scope, fields, descriptors) def add_descriptor_descendents(self, descriptor, depth=None, descriptor_filter=lambda descriptor: True): """ @@ -253,7 +252,7 @@ class FieldDataCache(object): return block_types - def _retrieve_fields(self, scope, fields, descriptors): + def _cache_fields(self, scope, fields, descriptors): """ Queries the database for all of the fields in the specified scope """ @@ -264,14 +263,14 @@ class FieldDataCache(object): self._all_usage_ids(descriptors), self.select_for_update, ) - return self.user_state_cache._data + field_objects = self.user_state_cache._data elif scope == Scope.user_state_summary: self.user_state_summary_cache = UserStateSummaryCache( self._all_usage_ids(descriptors), fields, self.select_for_update, ) - return self.user_state_summary_cache._data + field_objects = self.user_state_summary_cache._data elif scope == Scope.preferences: self.preferences_cache = PreferencesCache( self.user, @@ -279,16 +278,19 @@ class FieldDataCache(object): fields, self.select_for_update, ) - return self.preferences_cache._data + field_objects = self.preferences_cache._data elif scope == Scope.user_info: self.user_info_cache = UserInfoCache( self.user, fields, self.select_for_update, ) - return self.user_info_cache._data + field_objects = self.user_info_cache._data else: - return [] + field_objects = [] + + for field_object in field_objects: + self.cache[self._cache_key_from_field_object(scope, field_object)] = field_object def _fields_to_cache(self, descriptors): """ From 645d2a727f501088fad1bf2718a169efbf7aa2a7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 8 Apr 2015 16:29:49 -0400 Subject: [PATCH 06/50] Move field_object -> cache_key transformations to the scope-specific caches --- lms/djangoapps/courseware/model_data.py | 49 ++++++++++++------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 0825e93c27..b48e9b8ad1 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -81,13 +81,17 @@ class UserStateCache(object): course_id=course_id, student=user.pk, ) + self.course_id = course_id + + def cache_key_for_field_object(self, field_object): + return (Scope.user_state, field_object.module_state_key.map_into_course(self.course_id)) class UserStateSummaryCache(object): """ Cache for Scope.user_state_summary xblock field data. """ - def __init__(self, usage_keys, fields, select_for_update=False): + def __init__(self, course_id, usage_keys, fields, select_for_update=False): self._data = _chunked_query( XModuleUserStateSummaryField, select_for_update, @@ -95,6 +99,10 @@ class UserStateSummaryCache(object): usage_keys, field_name__in=set(field.name for field in fields), ) + self.course_id = course_id + + def cache_key_for_field_object(self, field_object): + return (Scope.user_state_summary, field_object.usage_id.map_into_course(self.course_id), field_object.field_name) class PreferencesCache(object): @@ -111,6 +119,9 @@ class PreferencesCache(object): field_name__in=set(field.name for field in fields), ) + def cache_key_for_field_object(self, field_object): + return (Scope.preferences, field_object.module_type, field_object.field_name) + class UserInfoCache(object): """ @@ -124,6 +135,9 @@ class UserInfoCache(object): field_name__in=set(field.name for field in fields), ) + def cache_key_for_field_object(self, field_object): + return (Scope.user_info, field_object.field_name) + class FieldDataCache(object): """ @@ -257,40 +271,37 @@ class FieldDataCache(object): Queries the database for all of the fields in the specified scope """ if scope == Scope.user_state: - self.user_state_cache = UserStateCache( + cache = self.user_state_cache = UserStateCache( self.user, self.course_id, self._all_usage_ids(descriptors), self.select_for_update, ) - field_objects = self.user_state_cache._data elif scope == Scope.user_state_summary: - self.user_state_summary_cache = UserStateSummaryCache( + cache = self.user_state_summary_cache = UserStateSummaryCache( + self.course_id, self._all_usage_ids(descriptors), fields, self.select_for_update, ) - field_objects = self.user_state_summary_cache._data elif scope == Scope.preferences: - self.preferences_cache = PreferencesCache( + cache = self.preferences_cache = PreferencesCache( self.user, self._all_block_types(descriptors), fields, self.select_for_update, ) - field_objects = self.preferences_cache._data elif scope == Scope.user_info: - self.user_info_cache = UserInfoCache( + cache = self.user_info_cache = UserInfoCache( self.user, fields, self.select_for_update, ) - field_objects = self.user_info_cache._data else: - field_objects = [] + return - for field_object in field_objects: - self.cache[self._cache_key_from_field_object(scope, field_object)] = field_object + for field_object in cache._data: + self.cache[cache.cache_key_for_field_object(field_object)] = field_object def _fields_to_cache(self, descriptors): """ @@ -315,20 +326,6 @@ class FieldDataCache(object): elif key.scope == Scope.user_info: return (key.scope, key.field_name) - def _cache_key_from_field_object(self, scope, field_object): - """ - Return the key used in the FieldDataCache for the specified scope and - field - """ - if scope == Scope.user_state: - return (scope, field_object.module_state_key.map_into_course(self.course_id)) - elif scope == Scope.user_state_summary: - return (scope, field_object.usage_id.map_into_course(self.course_id), field_object.field_name) - elif scope == Scope.preferences: - return (scope, field_object.module_type, field_object.field_name) - elif scope == Scope.user_info: - return (scope, field_object.field_name) - def find(self, key): ''' Look for a model data object using an DjangoKeyValueStore.Key object From 0d37ec8099ba28ba76d34a754c91fb2944898b87 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 8 Apr 2015 16:48:43 -0400 Subject: [PATCH 07/50] Change the central cache to store at two levels: first by scope, then by cache key --- lms/djangoapps/courseware/model_data.py | 29 +++++++++++-------- .../courseware/tests/test_model_data.py | 4 +-- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index b48e9b8ad1..6bfa2b65dc 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -84,7 +84,7 @@ class UserStateCache(object): self.course_id = course_id def cache_key_for_field_object(self, field_object): - return (Scope.user_state, field_object.module_state_key.map_into_course(self.course_id)) + return field_object.module_state_key.map_into_course(self.course_id) class UserStateSummaryCache(object): @@ -102,7 +102,7 @@ class UserStateSummaryCache(object): self.course_id = course_id def cache_key_for_field_object(self, field_object): - return (Scope.user_state_summary, field_object.usage_id.map_into_course(self.course_id), field_object.field_name) + return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) class PreferencesCache(object): @@ -120,7 +120,7 @@ class PreferencesCache(object): ) def cache_key_for_field_object(self, field_object): - return (Scope.preferences, field_object.module_type, field_object.field_name) + return (field_object.module_type, field_object.field_name) class UserInfoCache(object): @@ -136,7 +136,7 @@ class UserInfoCache(object): ) def cache_key_for_field_object(self, field_object): - return (Scope.user_info, field_object.field_name) + return field_object.field_name class FieldDataCache(object): @@ -158,7 +158,12 @@ class FieldDataCache(object): select_for_update: True if rows should be locked until end of transaction asides: The list of aside types to load, or None to prefetch no asides. ''' - self.cache = {} + self.cache = { + Scope.user_state: {}, + Scope.user_info: {}, + Scope.preferences: {}, + Scope.user_state_summary: {}, + } self.select_for_update = select_for_update if asides is None: @@ -301,7 +306,7 @@ class FieldDataCache(object): return for field_object in cache._data: - self.cache[cache.cache_key_for_field_object(field_object)] = field_object + self.cache[scope][cache.cache_key_for_field_object(field_object)] = field_object def _fields_to_cache(self, descriptors): """ @@ -318,13 +323,13 @@ class FieldDataCache(object): Return the key used in the FieldDataCache for the specified KeyValueStore key """ if key.scope == Scope.user_state: - return (key.scope, key.block_scope_id) + return key.block_scope_id elif key.scope == Scope.user_state_summary: - return (key.scope, key.block_scope_id, key.field_name) + return (key.block_scope_id, key.field_name) elif key.scope == Scope.preferences: - return (key.scope, BlockTypeKeyV1(key.block_family, key.block_scope_id), key.field_name) + return (BlockTypeKeyV1(key.block_family, key.block_scope_id), key.field_name) elif key.scope == Scope.user_info: - return (key.scope, key.field_name) + return key.field_name def find(self, key): ''' @@ -339,7 +344,7 @@ class FieldDataCache(object): # user we were constructed for. assert key.user_id == self.user.id - return self.cache.get(self._cache_key_from_kvs_key(key)) + return self.cache[key.scope].get(self._cache_key_from_kvs_key(key)) def find_or_create(self, key): ''' @@ -379,7 +384,7 @@ class FieldDataCache(object): ) cache_key = self._cache_key_from_kvs_key(key) - self.cache[cache_key] = field_object + self.cache[key.scope][cache_key] = field_object return field_object diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index ca62da4e80..3355970562 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -230,7 +230,7 @@ class TestMissingStudentModule(TestCase): 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, sum(len(cache) for cache in self.field_data_cache.cache.values())) self.assertEquals(0, StudentModule.objects.all().count()) # We are updating a problem, so we write to courseware_studentmodulehistory @@ -238,7 +238,7 @@ class TestMissingStudentModule(TestCase): with self.assertNumQueries(6): self.kvs.set(user_state_key('a_field'), 'a_value') - self.assertEquals(1, len(self.field_data_cache.cache)) + self.assertEquals(1, sum(len(cache) for cache in self.field_data_cache.cache.values())) self.assertEquals(1, StudentModule.objects.all().count()) student_module = StudentModule.objects.all()[0] From 4463f711482dc9a7d31ae0d1b53da60dbb4c0ac3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 13:56:32 -0400 Subject: [PATCH 08/50] Separate caching for particular fields from instantiating the cache --- lms/djangoapps/courseware/model_data.py | 58 +++++++++++++++++-------- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 6bfa2b65dc..a0ec75c84b 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -73,15 +73,21 @@ class UserStateCache(object): Cache for Scope.user_state xblock field data. """ def __init__(self, user, course_id, usage_keys, select_for_update=False): + self.course_id = course_id + self._cache = {} + self.user = user + self.usage_keys = usage_keys + self.select_for_update = select_for_update + + def cache_fields(self, fields): self._data = _chunked_query( StudentModule, - select_for_update, + self.select_for_update, 'module_state_key__in', - usage_keys, - course_id=course_id, - student=user.pk, + self.usage_keys, + course_id=self.course_id, + student=self.user.pk, ) - self.course_id = course_id def cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) @@ -91,15 +97,20 @@ class UserStateSummaryCache(object): """ Cache for Scope.user_state_summary xblock field data. """ - def __init__(self, course_id, usage_keys, fields, select_for_update=False): + def __init__(self, course_id, usage_keys, select_for_update=False): + self.usage_keys = usage_keys + self.course_id = course_id + self._cache = {} + self.select_for_update = select_for_update + + def cache_fields(self, fields): self._data = _chunked_query( XModuleUserStateSummaryField, - select_for_update, + self.select_for_update, 'usage_id__in', - usage_keys, + self.usage_keys, field_name__in=set(field.name for field in fields), ) - self.course_id = course_id def cache_key_for_field_object(self, field_object): return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) @@ -109,13 +120,19 @@ class PreferencesCache(object): """ Cache for Scope.preferences xblock field data. """ - def __init__(self, user, block_types, fields, select_for_update=False): + def __init__(self, user, block_types, select_for_update=False): + self.user = user + self.block_types = block_types + self.select_for_update = select_for_update + self._cache = {} + + def cache_fields(self, fields): self._data = _chunked_query( XModuleStudentPrefsField, - select_for_update, + self.select_for_update, 'module_type__in', - block_types, - student=user.pk, + self.block_types, + student=self.user.pk, field_name__in=set(field.name for field in fields), ) @@ -127,11 +144,16 @@ class UserInfoCache(object): """ Cache for Scope.user_info xblock field data """ - def __init__(self, user, fields, select_for_update=False): + def __init__(self, user, select_for_update=False): + self._cache = {} + self.user = user + self.select_for_update = select_for_update + + def cache_fields(self, fields): self._data = _query( XModuleStudentInfoField, - select_for_update, - student=user.pk, + self.select_for_update, + student=self.user.pk, field_name__in=set(field.name for field in fields), ) @@ -286,25 +308,23 @@ class FieldDataCache(object): cache = self.user_state_summary_cache = UserStateSummaryCache( self.course_id, self._all_usage_ids(descriptors), - fields, self.select_for_update, ) elif scope == Scope.preferences: cache = self.preferences_cache = PreferencesCache( self.user, self._all_block_types(descriptors), - fields, self.select_for_update, ) elif scope == Scope.user_info: cache = self.user_info_cache = UserInfoCache( self.user, - fields, self.select_for_update, ) else: return + cache.cache_fields(fields) for field_object in cache._data: self.cache[scope][cache.cache_key_for_field_object(field_object)] = field_object From 21bcf55dba4b0b3ef3e5e83e8fc1699336d82b04 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 16:49:28 -0400 Subject: [PATCH 09/50] Extract _all_block_types and _all_usage_keys out of cache class --- lms/djangoapps/courseware/model_data.py | 63 +++++++++++++------------ 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index a0ec75c84b..910321e8fc 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -68,6 +68,36 @@ def _chunked_query(model_class, select_for_update, chunk_field, items, chunk_siz return res +def _all_usage_keys(descriptors, aside_types): + """ + Return a set of all usage_ids for the `descriptors` and for + as all asides in `aside_types` for those descriptors. + """ + usage_ids = set() + for descriptor in descriptors: + usage_ids.add(descriptor.scope_ids.usage_id) + + for aside_type in aside_types: + usage_ids.add(AsideUsageKeyV1(descriptor.scope_ids.usage_id, aside_type)) + + return usage_ids + + +def _all_block_types(descriptors, aside_types): + """ + Return a set of all block_types for the supplied `descriptors` and for + the asides types in `aside_types` associated with those descriptors. + """ + block_types = set() + for descriptor in descriptors: + block_types.add(BlockTypeKeyV1(descriptor.entry_point, descriptor.scope_ids.block_type)) + + for aside_type in aside_types: + block_types.add(BlockTypeKeyV1(XBlockAside.entry_point, aside_type)) + + return block_types + + class UserStateCache(object): """ Cache for Scope.user_state xblock field data. @@ -266,33 +296,6 @@ class FieldDataCache(object): return cache - def _all_usage_ids(self, descriptors): - """ - Return a set of all usage_ids for the descriptors that this FieldDataCache is caching - against, and well as all asides for those descriptors. - """ - usage_ids = set() - for descriptor in descriptors: - usage_ids.add(descriptor.scope_ids.usage_id) - - for aside_type in self.asides: - usage_ids.add(AsideUsageKeyV1(descriptor.scope_ids.usage_id, aside_type)) - - return usage_ids - - def _all_block_types(self, descriptors): - """ - Return a set of all block_types that are cached by this FieldDataCache. - """ - block_types = set() - for descriptor in descriptors: - block_types.add(BlockTypeKeyV1(descriptor.entry_point, descriptor.scope_ids.block_type)) - - for aside_type in self.asides: - block_types.add(BlockTypeKeyV1(XBlockAside.entry_point, aside_type)) - - return block_types - def _cache_fields(self, scope, fields, descriptors): """ Queries the database for all of the fields in the specified scope @@ -301,19 +304,19 @@ class FieldDataCache(object): cache = self.user_state_cache = UserStateCache( self.user, self.course_id, - self._all_usage_ids(descriptors), + _all_usage_keys(descriptors, self.asides), self.select_for_update, ) elif scope == Scope.user_state_summary: cache = self.user_state_summary_cache = UserStateSummaryCache( self.course_id, - self._all_usage_ids(descriptors), + _all_usage_keys(descriptors, self.asides), self.select_for_update, ) elif scope == Scope.preferences: cache = self.preferences_cache = PreferencesCache( self.user, - self._all_block_types(descriptors), + _all_block_types(descriptors, self.asides), self.select_for_update, ) elif scope == Scope.user_info: From 01cf2a322538a73622fdb35bf19407e73c68bf2c Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 13:59:04 -0400 Subject: [PATCH 10/50] Store cache objects, rather than dictionaries generated by cache objects --- lms/djangoapps/courseware/model_data.py | 127 ++++++++++++++---------- 1 file changed, 77 insertions(+), 50 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 910321e8fc..dcdd476b38 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -102,19 +102,18 @@ class UserStateCache(object): """ Cache for Scope.user_state xblock field data. """ - def __init__(self, user, course_id, usage_keys, select_for_update=False): + def __init__(self, user, course_id, select_for_update=False): self.course_id = course_id self._cache = {} self.user = user - self.usage_keys = usage_keys self.select_for_update = select_for_update - def cache_fields(self, fields): + def cache_fields(self, fields, descriptors, aside_types): self._data = _chunked_query( StudentModule, self.select_for_update, 'module_state_key__in', - self.usage_keys, + _all_usage_keys(descriptors, aside_types), course_id=self.course_id, student=self.user.pk, ) @@ -122,46 +121,61 @@ class UserStateCache(object): def cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) + def get(self, cache_key): + return self._cache.get(cache_key) + + def set(self, cache_key, value): + self._cache[cache_key] = value + + def __len__(self): + return len(self._cache) class UserStateSummaryCache(object): """ Cache for Scope.user_state_summary xblock field data. """ - def __init__(self, course_id, usage_keys, select_for_update=False): - self.usage_keys = usage_keys + def __init__(self, course_id, select_for_update=False): self.course_id = course_id self._cache = {} self.select_for_update = select_for_update - def cache_fields(self, fields): + def cache_fields(self, fields, descriptors, aside_types): self._data = _chunked_query( XModuleUserStateSummaryField, self.select_for_update, 'usage_id__in', - self.usage_keys, + _all_usage_keys(descriptors, aside_types), field_name__in=set(field.name for field in fields), ) def cache_key_for_field_object(self, field_object): return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) + def get(self, cache_key): + return self._cache.get(cache_key) + + def set(self, cache_key, value): + self._cache[cache_key] = value + + def __len__(self): + return len(self._cache) + class PreferencesCache(object): """ Cache for Scope.preferences xblock field data. """ - def __init__(self, user, block_types, select_for_update=False): + def __init__(self, user, select_for_update=False): self.user = user - self.block_types = block_types self.select_for_update = select_for_update self._cache = {} - def cache_fields(self, fields): + def cache_fields(self, fields, descriptors, aside_types): self._data = _chunked_query( XModuleStudentPrefsField, self.select_for_update, 'module_type__in', - self.block_types, + _all_block_types(descriptors, aside_types), student=self.user.pk, field_name__in=set(field.name for field in fields), ) @@ -169,6 +183,15 @@ class PreferencesCache(object): def cache_key_for_field_object(self, field_object): return (field_object.module_type, field_object.field_name) + def get(self, cache_key): + return self._cache.get(cache_key) + + def set(self, cache_key, value): + self._cache[cache_key] = value + + def __len__(self): + return len(self._cache) + class UserInfoCache(object): """ @@ -179,7 +202,7 @@ class UserInfoCache(object): self.user = user self.select_for_update = select_for_update - def cache_fields(self, fields): + def cache_fields(self, fields, descriptors, aside_types): self._data = _query( XModuleStudentInfoField, self.select_for_update, @@ -190,6 +213,15 @@ class UserInfoCache(object): def cache_key_for_field_object(self, field_object): return field_object.field_name + def get(self, cache_key): + return self._cache.get(cache_key) + + def set(self, cache_key, value): + self._cache[cache_key] = value + + def __len__(self): + return len(self._cache) + class FieldDataCache(object): """ @@ -210,12 +242,6 @@ class FieldDataCache(object): select_for_update: True if rows should be locked until end of transaction asides: The list of aside types to load, or None to prefetch no asides. ''' - self.cache = { - Scope.user_state: {}, - Scope.user_info: {}, - Scope.preferences: {}, - Scope.user_state_summary: {}, - } self.select_for_update = select_for_update if asides is None: @@ -227,6 +253,25 @@ class FieldDataCache(object): self.course_id = course_id self.user = user + self.cache = { + Scope.user_state: UserStateCache( + self.user, + self.course_id, + self.select_for_update, + ), + Scope.user_info: UserInfoCache( + self.user, + self.select_for_update, + ), + Scope.preferences: PreferencesCache( + self.user, + self.select_for_update, + ), + Scope.user_state_summary: UserStateSummaryCache( + self.course_id, + self.select_for_update, + ), + } self.add_descriptors_to_cache(descriptors) def add_descriptors_to_cache(self, descriptors): @@ -235,6 +280,9 @@ class FieldDataCache(object): """ if self.user.is_authenticated(): for scope, fields in self._fields_to_cache(descriptors).items(): + if scope not in self.cache: + continue + self._cache_fields(scope, fields, descriptors) def add_descriptor_descendents(self, descriptor, depth=None, descriptor_filter=lambda descriptor: True): @@ -300,36 +348,9 @@ class FieldDataCache(object): """ Queries the database for all of the fields in the specified scope """ - if scope == Scope.user_state: - cache = self.user_state_cache = UserStateCache( - self.user, - self.course_id, - _all_usage_keys(descriptors, self.asides), - self.select_for_update, - ) - elif scope == Scope.user_state_summary: - cache = self.user_state_summary_cache = UserStateSummaryCache( - self.course_id, - _all_usage_keys(descriptors, self.asides), - self.select_for_update, - ) - elif scope == Scope.preferences: - cache = self.preferences_cache = PreferencesCache( - self.user, - _all_block_types(descriptors, self.asides), - self.select_for_update, - ) - elif scope == Scope.user_info: - cache = self.user_info_cache = UserInfoCache( - self.user, - self.select_for_update, - ) - else: - return - - cache.cache_fields(fields) - for field_object in cache._data: - self.cache[scope][cache.cache_key_for_field_object(field_object)] = field_object + self.cache[scope].cache_fields(fields, descriptors, self.asides) + for field_object in self.cache[scope]._data: + self.cache[scope].set(self.cache[scope].cache_key_for_field_object(field_object), field_object) def _fields_to_cache(self, descriptors): """ @@ -367,6 +388,9 @@ class FieldDataCache(object): # user we were constructed for. assert key.user_id == self.user.id + if key.scope not in self.cache: + return None + return self.cache[key.scope].get(self._cache_key_from_kvs_key(key)) def find_or_create(self, key): @@ -406,8 +430,11 @@ class FieldDataCache(object): student_id=key.user_id, ) + if key.scope not in self.cache: + return + cache_key = self._cache_key_from_kvs_key(key) - self.cache[key.scope][cache_key] = field_object + self.cache[key.scope].set(cache_key, field_object) return field_object From 7909bee511f767d795d79d62bf13db72173ba888 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 15:04:40 -0400 Subject: [PATCH 11/50] Stop leaking private _data members from per-scope caches --- lms/djangoapps/courseware/model_data.py | 27 ++++++++++++------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index dcdd476b38..cf94be9000 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -109,7 +109,7 @@ class UserStateCache(object): self.select_for_update = select_for_update def cache_fields(self, fields, descriptors, aside_types): - self._data = _chunked_query( + data = _chunked_query( StudentModule, self.select_for_update, 'module_state_key__in', @@ -117,6 +117,8 @@ class UserStateCache(object): course_id=self.course_id, student=self.user.pk, ) + for field_object in data: + self._cache[self.cache_key_for_field_object(field_object)] = field_object def cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) @@ -140,13 +142,15 @@ class UserStateSummaryCache(object): self.select_for_update = select_for_update def cache_fields(self, fields, descriptors, aside_types): - self._data = _chunked_query( + data = _chunked_query( XModuleUserStateSummaryField, self.select_for_update, 'usage_id__in', _all_usage_keys(descriptors, aside_types), field_name__in=set(field.name for field in fields), ) + for field_object in data: + self._cache[self.cache_key_for_field_object(field_object)] = field_object def cache_key_for_field_object(self, field_object): return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) @@ -171,7 +175,7 @@ class PreferencesCache(object): self._cache = {} def cache_fields(self, fields, descriptors, aside_types): - self._data = _chunked_query( + data = _chunked_query( XModuleStudentPrefsField, self.select_for_update, 'module_type__in', @@ -179,6 +183,8 @@ class PreferencesCache(object): student=self.user.pk, field_name__in=set(field.name for field in fields), ) + for field_object in data: + self._cache[self.cache_key_for_field_object(field_object)] = field_object def cache_key_for_field_object(self, field_object): return (field_object.module_type, field_object.field_name) @@ -203,12 +209,14 @@ class UserInfoCache(object): self.select_for_update = select_for_update def cache_fields(self, fields, descriptors, aside_types): - self._data = _query( + data = _query( XModuleStudentInfoField, self.select_for_update, student=self.user.pk, field_name__in=set(field.name for field in fields), ) + for field_object in data: + self._cache[self.cache_key_for_field_object(field_object)] = field_object def cache_key_for_field_object(self, field_object): return field_object.field_name @@ -283,7 +291,7 @@ class FieldDataCache(object): if scope not in self.cache: continue - self._cache_fields(scope, fields, descriptors) + self.cache[scope].cache_fields(fields, descriptors, self.asides) def add_descriptor_descendents(self, descriptor, depth=None, descriptor_filter=lambda descriptor: True): """ @@ -343,15 +351,6 @@ class FieldDataCache(object): cache.add_descriptor_descendents(descriptor, depth, descriptor_filter) return cache - - def _cache_fields(self, scope, fields, descriptors): - """ - Queries the database for all of the fields in the specified scope - """ - self.cache[scope].cache_fields(fields, descriptors, self.asides) - for field_object in self.cache[scope]._data: - self.cache[scope].set(self.cache[scope].cache_key_for_field_object(field_object), field_object) - def _fields_to_cache(self, descriptors): """ Returns a map of scopes to fields in that scope that should be cached From 29606a170d4b8eb49f3f2b7c9db082c57cfc8e6d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 15:19:47 -0400 Subject: [PATCH 12/50] Extract common django-orm-backed-cache functionality --- lms/djangoapps/courseware/model_data.py | 232 +++++++++++++++++------- 1 file changed, 167 insertions(+), 65 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index cf94be9000..355e791c11 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -3,6 +3,7 @@ Classes to provide the LMS runtime data storage to XBlocks """ import json +from abc import abstractmethod, ABCMeta from collections import defaultdict from itertools import chain from .models import ( @@ -98,137 +99,238 @@ def _all_block_types(descriptors, aside_types): return block_types -class UserStateCache(object): +class DjangoOrmFieldCache(object): + """ + Baseclass for Scope-specific field cache objects that are based on + single-row-per-field Django ORM objects. + """ + __metaclass__ = ABCMeta + + def __init__(self): + self._cache = {} + + def cache_fields(self, fields, xblocks, aside_types): + """ + Load all fields specified by ``fields`` for the supplied ``xblocks`` + and ``aside_types`` into this cache. + + Arguments: + fields (list of str): Field names to cache. + xblocks (list of :class:`XBlock`): XBlocks to cache fields for. + aside_types (list of str): Aside types to cache fields for. + """ + for field_object in self._read_objects(fields, xblocks, aside_types): + self._cache[self._cache_key_for_field_object(field_object)] = field_object + + def get(self, cache_key): + return self._cache.get(cache_key) + + def set(self, cache_key, value): + self._cache[cache_key] = value + + def __len__(self): + return len(self._cache) + + @abstractmethod + 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 + ``kvs_key``). + + Arguments: + kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + value: What value to record in the field + """ + raise NotImplementedError() + + @abstractmethod + def _read_objects(self, fields, xblocks, aside_types): + """ + Return an iterator for all objects stored in the underlying datastore + for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated + with them. + + Arguments: + fields (list of str): Field names to return values for + xblocks (list of :class:`~XBlock`): XBlocks to load fields for + aside_types (list of str): Asides to load field for (which annotate the supplied + xblocks). + """ + raise NotImplementedError() + + @abstractmethod + def _cache_key_for_field_object(self, field_object): + """ + Return the key used in this DjangoOrmFieldCache to store the specified field_object. + + Arguments: + field_object: A Django model instance that stores the data for fields in this cache + """ + raise NotImplementedError() + + @abstractmethod + def _cache_key_for_kvs_key(self, key): + """ + Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + raise NotImplementedError() + + +class UserStateCache(DjangoOrmFieldCache): """ Cache for Scope.user_state xblock field data. """ def __init__(self, user, course_id, select_for_update=False): + super(UserStateCache, self).__init__() self.course_id = course_id - self._cache = {} self.user = user self.select_for_update = select_for_update - def cache_fields(self, fields, descriptors, aside_types): - data = _chunked_query( + def _read_objects(self, fields, xblocks, aside_types): + """ + Return an iterator for all objects stored in the underlying datastore + for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated + with them. + + Arguments: + fields (list of str): Field names to return values for + xblocks (list of :class:`~XBlock`): XBlocks to load fields for + aside_types (list of str): Asides to load field for (which annotate the supplied + xblocks). + """ + return _chunked_query( StudentModule, self.select_for_update, 'module_state_key__in', - _all_usage_keys(descriptors, aside_types), + _all_usage_keys(xblocks, aside_types), course_id=self.course_id, student=self.user.pk, ) - for field_object in data: - self._cache[self.cache_key_for_field_object(field_object)] = field_object - def cache_key_for_field_object(self, field_object): + def _cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) - def get(self, cache_key): - return self._cache.get(cache_key) - def set(self, cache_key, value): - self._cache[cache_key] = value - - def __len__(self): - return len(self._cache) - -class UserStateSummaryCache(object): +class UserStateSummaryCache(DjangoOrmFieldCache): """ Cache for Scope.user_state_summary xblock field data. """ def __init__(self, course_id, select_for_update=False): + super(UserStateSummaryCache, self).__init__() self.course_id = course_id - self._cache = {} self.select_for_update = select_for_update - def cache_fields(self, fields, descriptors, aside_types): - data = _chunked_query( + def _read_objects(self, fields, xblocks, aside_types): + """ + Return an iterator for all objects stored in the underlying datastore + for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated + with them. + + Arguments: + fields (list of str): Field names to return values for + xblocks (list of :class:`~XBlock`): XBlocks to load fields for + aside_types (list of str): Asides to load field for (which annotate the supplied + xblocks). + """ + return _chunked_query( XModuleUserStateSummaryField, self.select_for_update, 'usage_id__in', - _all_usage_keys(descriptors, aside_types), + _all_usage_keys(xblocks, aside_types), field_name__in=set(field.name for field in fields), ) - for field_object in data: - self._cache[self.cache_key_for_field_object(field_object)] = field_object - def cache_key_for_field_object(self, field_object): + def _cache_key_for_field_object(self, field_object): + """ + Return the key used in this DjangoOrmFieldCache to store the specified field_object. + + Arguments: + field_object: A Django model instance that stores the data for fields in this cache + """ return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) - def get(self, cache_key): - return self._cache.get(cache_key) - def set(self, cache_key, value): - self._cache[cache_key] = value - - def __len__(self): - return len(self._cache) - - -class PreferencesCache(object): +class PreferencesCache(DjangoOrmFieldCache): """ Cache for Scope.preferences xblock field data. """ def __init__(self, user, select_for_update=False): + super(PreferencesCache, self).__init__() self.user = user self.select_for_update = select_for_update - self._cache = {} - def cache_fields(self, fields, descriptors, aside_types): - data = _chunked_query( + def _read_objects(self, fields, xblocks, aside_types): + """ + Return an iterator for all objects stored in the underlying datastore + for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated + with them. + + Arguments: + fields (list of str): Field names to return values for + xblocks (list of :class:`~XBlock`): XBlocks to load fields for + aside_types (list of str): Asides to load field for (which annotate the supplied + xblocks). + """ + return _chunked_query( XModuleStudentPrefsField, self.select_for_update, 'module_type__in', - _all_block_types(descriptors, aside_types), + _all_block_types(xblocks, aside_types), student=self.user.pk, field_name__in=set(field.name for field in fields), ) - for field_object in data: - self._cache[self.cache_key_for_field_object(field_object)] = field_object - def cache_key_for_field_object(self, field_object): + def _cache_key_for_field_object(self, field_object): + """ + Return the key used in this DjangoOrmFieldCache to store the specified field_object. + + Arguments: + field_object: A Django model instance that stores the data for fields in this cache + """ return (field_object.module_type, field_object.field_name) - def get(self, cache_key): - return self._cache.get(cache_key) - def set(self, cache_key, value): - self._cache[cache_key] = value - - def __len__(self): - return len(self._cache) - - -class UserInfoCache(object): +class UserInfoCache(DjangoOrmFieldCache): """ Cache for Scope.user_info xblock field data """ def __init__(self, user, select_for_update=False): - self._cache = {} + super(UserInfoCache, self).__init__() self.user = user self.select_for_update = select_for_update - def cache_fields(self, fields, descriptors, aside_types): - data = _query( + def _read_objects(self, fields, xblocks, aside_types): + """ + Return an iterator for all objects stored in the underlying datastore + for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated + with them. + + Arguments: + fields (list of str): Field names to return values for + xblocks (list of :class:`~XBlock`): XBlocks to load fields for + aside_types (list of str): Asides to load field for (which annotate the supplied + xblocks). + """ + return _query( XModuleStudentInfoField, self.select_for_update, student=self.user.pk, field_name__in=set(field.name for field in fields), ) - for field_object in data: - self._cache[self.cache_key_for_field_object(field_object)] = field_object - def cache_key_for_field_object(self, field_object): + def _cache_key_for_field_object(self, field_object): + """ + Return the key used in this DjangoOrmFieldCache to store the specified field_object. + + Arguments: + field_object: A Django model instance that stores the data for fields in this cache + """ return field_object.field_name - def get(self, cache_key): - return self._cache.get(cache_key) - - def set(self, cache_key, value): - self._cache[cache_key] = value - - def __len__(self): - return len(self._cache) class FieldDataCache(object): From 5df3c22651579896a589a3f23da6cc5d5f86e174 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 9 Apr 2015 15:56:15 -0400 Subject: [PATCH 13/50] Push cache_key transformations inside the cache objects --- lms/djangoapps/courseware/model_data.py | 305 +++++++++++++----------- 1 file changed, 165 insertions(+), 140 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 355e791c11..a0d2123e88 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -16,6 +16,7 @@ import logging from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.block_types import BlockTypeKeyV1 from opaque_keys.edx.asides import AsideUsageKeyV1 +from contracts import contract from django.db import DatabaseError @@ -99,6 +100,127 @@ def _all_block_types(descriptors, aside_types): return block_types +class DjangoKeyValueStore(KeyValueStore): + """ + This KeyValueStore will read and write data in the following scopes to django models + Scope.user_state_summary + Scope.user_state + Scope.preferences + Scope.user_info + + Access to any other scopes will raise an InvalidScopeError + + Data for Scope.user_state is stored as StudentModule objects via the django orm. + + Data for the other scopes is stored in individual objects that are named for the + scope involved and have the field name as a key + + If the key isn't found in the expected table during a read or a delete, then a KeyError will be raised + """ + + _allowed_scopes = ( + Scope.user_state_summary, + Scope.user_state, + Scope.preferences, + Scope.user_info, + ) + + def __init__(self, field_data_cache): + self._field_data_cache = field_data_cache + + def get(self, key): + if key.scope not in self._allowed_scopes: + raise InvalidScopeError(key) + + field_object = self._field_data_cache.find(key) + if field_object is None: + raise KeyError(key.field_name) + + if key.scope == Scope.user_state: + return json.loads(field_object.state)[key.field_name] + else: + return json.loads(field_object.value) + + def set(self, key, value): + """ + Set a single value in the KeyValueStore + """ + self.set_many({key: value}) + + def set_many(self, kv_dict): + """ + Provide a bulk save mechanism. + + `kv_dict`: A dictionary of dirty fields that maps + xblock.KvsFieldData._key : value + + """ + saved_fields = [] + # 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 field.scope == Scope.user_state: + state = json.loads(field_object.state) + 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[field]) + + for field_object in field_objects: + try: + # Save the field object that we made above + field_object.save() + # If save is successful on this scope, add the saved fields to + # the list of successful saves + saved_fields.extend([field.field_name for field in field_objects[field_object]]) + except DatabaseError: + log.exception('Error saving fields %r', field_objects[field_object]) + raise KeyValueMultiSaveError(saved_fields) + + def delete(self, key): + if key.scope not in self._allowed_scopes: + raise InvalidScopeError(key) + + field_object = self._field_data_cache.find(key) + if field_object is None: + raise KeyError(key.field_name) + + if key.scope == Scope.user_state: + state = json.loads(field_object.state) + del state[key.field_name] + field_object.state = json.dumps(state) + field_object.save() + else: + field_object.delete() + + def has(self, key): + if key.scope not in self._allowed_scopes: + raise InvalidScopeError(key) + + field_object = self._field_data_cache.find(key) + if field_object is None: + return False + + if key.scope == Scope.user_state: + return key.field_name in json.loads(field_object.state) + else: + return True + + class DjangoOrmFieldCache(object): """ Baseclass for Scope-specific field cache objects that are based on @@ -122,11 +244,13 @@ class DjangoOrmFieldCache(object): for field_object in self._read_objects(fields, xblocks, aside_types): self._cache[self._cache_key_for_field_object(field_object)] = field_object - def get(self, cache_key): - return self._cache.get(cache_key) + @contract(kvs_key=DjangoKeyValueStore.Key) + def get(self, kvs_key): + return self._cache.get(self._cache_key_for_kvs_key(kvs_key)) - def set(self, cache_key, value): - self._cache[cache_key] = value + @contract(kvs_key=DjangoKeyValueStore.Key) + def set(self, kvs_key, value): + self._cache[self._cache_key_for_kvs_key(kvs_key)] = value def __len__(self): return len(self._cache) @@ -214,6 +338,15 @@ class UserStateCache(DjangoOrmFieldCache): def _cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) + def _cache_key_for_kvs_key(self, key): + """ + Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + return key.block_scope_id + class UserStateSummaryCache(DjangoOrmFieldCache): """ @@ -253,6 +386,15 @@ class UserStateSummaryCache(DjangoOrmFieldCache): """ return (field_object.usage_id.map_into_course(self.course_id), field_object.field_name) + def _cache_key_for_kvs_key(self, key): + """ + Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + return (key.block_scope_id, key.field_name) + class PreferencesCache(DjangoOrmFieldCache): """ @@ -293,6 +435,15 @@ class PreferencesCache(DjangoOrmFieldCache): """ return (field_object.module_type, field_object.field_name) + def _cache_key_for_kvs_key(self, key): + """ + Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + return (BlockTypeKeyV1(key.block_family, key.block_scope_id), key.field_name) + class UserInfoCache(DjangoOrmFieldCache): """ @@ -331,6 +482,14 @@ class UserInfoCache(DjangoOrmFieldCache): """ return field_object.field_name + def _cache_key_for_kvs_key(self, key): + """ + Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + return key.field_name class FieldDataCache(object): @@ -463,18 +622,6 @@ class FieldDataCache(object): scope_map[field.scope].add(field) return scope_map - def _cache_key_from_kvs_key(self, key): - """ - Return the key used in the FieldDataCache for the specified KeyValueStore key - """ - if key.scope == Scope.user_state: - return key.block_scope_id - elif key.scope == Scope.user_state_summary: - return (key.block_scope_id, key.field_name) - elif key.scope == Scope.preferences: - return (BlockTypeKeyV1(key.block_family, key.block_scope_id), key.field_name) - elif key.scope == Scope.user_info: - return key.field_name def find(self, key): ''' @@ -492,7 +639,7 @@ class FieldDataCache(object): if key.scope not in self.cache: return None - return self.cache[key.scope].get(self._cache_key_from_kvs_key(key)) + return self.cache[key.scope].get(key) def find_or_create(self, key): ''' @@ -534,127 +681,5 @@ class FieldDataCache(object): if key.scope not in self.cache: return - cache_key = self._cache_key_from_kvs_key(key) - self.cache[key.scope].set(cache_key, field_object) + self.cache[key.scope].set(key, field_object) return field_object - - -class DjangoKeyValueStore(KeyValueStore): - """ - This KeyValueStore will read and write data in the following scopes to django models - Scope.user_state_summary - Scope.user_state - Scope.preferences - Scope.user_info - - Access to any other scopes will raise an InvalidScopeError - - Data for Scope.user_state is stored as StudentModule objects via the django orm. - - Data for the other scopes is stored in individual objects that are named for the - scope involved and have the field name as a key - - If the key isn't found in the expected table during a read or a delete, then a KeyError will be raised - """ - - _allowed_scopes = ( - Scope.user_state_summary, - Scope.user_state, - Scope.preferences, - Scope.user_info, - ) - - def __init__(self, field_data_cache): - self._field_data_cache = field_data_cache - - def get(self, key): - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) - - field_object = self._field_data_cache.find(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - return json.loads(field_object.state)[key.field_name] - else: - return json.loads(field_object.value) - - def set(self, key, value): - """ - Set a single value in the KeyValueStore - """ - self.set_many({key: value}) - - def set_many(self, kv_dict): - """ - Provide a bulk save mechanism. - - `kv_dict`: A dictionary of dirty fields that maps - xblock.KvsFieldData._key : value - - """ - saved_fields = [] - # 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 field.scope == Scope.user_state: - state = json.loads(field_object.state) - 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[field]) - - for field_object in field_objects: - try: - # Save the field object that we made above - field_object.save() - # If save is successful on this scope, add the saved fields to - # the list of successful saves - saved_fields.extend([field.field_name for field in field_objects[field_object]]) - except DatabaseError: - log.exception('Error saving fields %r', field_objects[field_object]) - raise KeyValueMultiSaveError(saved_fields) - - def delete(self, key): - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) - - field_object = self._field_data_cache.find(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - state = json.loads(field_object.state) - del state[key.field_name] - field_object.state = json.dumps(state) - field_object.save() - else: - field_object.delete() - - def has(self, key): - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) - - field_object = self._field_data_cache.find(key) - if field_object is None: - return False - - if key.scope == Scope.user_state: - return key.field_name in json.loads(field_object.state) - else: - return True From 951c19d7895d9e33792bc4e2f0842f16f773d4f4 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 13:23:43 -0400 Subject: [PATCH 14/50] Push `get` down into FieldDataCache, from DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 28 ++++++++++++++++++------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index a0d2123e88..8d6057dde7 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -132,14 +132,7 @@ class DjangoKeyValueStore(KeyValueStore): if key.scope not in self._allowed_scopes: raise InvalidScopeError(key) - field_object = self._field_data_cache.find(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - return json.loads(field_object.state)[key.field_name] - else: - return json.loads(field_object.value) + return self._field_data_cache.get(key) def set(self, key, value): """ @@ -622,6 +615,25 @@ class FieldDataCache(object): scope_map[field.scope].add(field) return scope_map + @contract(key=DjangoKeyValueStore.Key) + def get(self, key): + ''' + Load the field value specified by `key`. + + Arguments: + key (`DjangoKeyValueStore.Key`): The field value to load + + Returns: The found value + Raises: KeyError if key isn't found in the cache + ''' + field_object = self.find(key) + if field_object is None: + raise KeyError(key.field_name) + + if key.scope == Scope.user_state: + return json.loads(field_object.state)[key.field_name] + else: + return json.loads(field_object.value) def find(self, key): ''' From ebb3c906ff80e2abfc805307b29fa439350ac213 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 13:48:14 -0400 Subject: [PATCH 15/50] Push `set_many` down into FieldDataCache from DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 86 +++++++++++++++---------- 1 file changed, 51 insertions(+), 35 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 8d6057dde7..79d6dceb62 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -16,7 +16,7 @@ import logging from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.block_types import BlockTypeKeyV1 from opaque_keys.edx.asides import AsideUsageKeyV1 -from contracts import contract +from contracts import contract, new_contract from django.db import DatabaseError @@ -148,41 +148,12 @@ class DjangoKeyValueStore(KeyValueStore): xblock.KvsFieldData._key : value """ - saved_fields = [] - # 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) + for key in kv_dict: + # Check key for validity + if key.scope not in self._allowed_scopes: + raise InvalidScopeError(key) - # 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 field.scope == Scope.user_state: - state = json.loads(field_object.state) - 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[field]) - - for field_object in field_objects: - try: - # Save the field object that we made above - field_object.save() - # If save is successful on this scope, add the saved fields to - # the list of successful saves - saved_fields.extend([field.field_name for field in field_objects[field_object]]) - except DatabaseError: - log.exception('Error saving fields %r', field_objects[field_object]) - raise KeyValueMultiSaveError(saved_fields) + self._field_data_cache.set_many(kv_dict) def delete(self, key): if key.scope not in self._allowed_scopes: @@ -213,6 +184,9 @@ class DjangoKeyValueStore(KeyValueStore): else: return True +new_contract("DjangoKeyValueStore", DjangoKeyValueStore) +new_contract("DjangoKeyValueStore_Key", DjangoKeyValueStore.Key) + class DjangoOrmFieldCache(object): """ @@ -635,6 +609,48 @@ class FieldDataCache(object): else: return json.loads(field_object.value) + @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") + def set_many(self, kv_dict): + """ + Set all of the fields specified by the keys of `kv_dict` to the values + in that dict. + + Arguments: + kv_dict (dict): dict mapping from `DjangoKeyValueStore.Key`s to field values + Raises: DatabaseError if any fields fail to save + """ + saved_fields = [] + # field_objects maps a field_object to a list of associated fields + field_objects = dict() + for field in kv_dict: + # If the field is valid and isn't already in the dictionary, add it. + field_object = self.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 field.scope == Scope.user_state: + state = json.loads(field_object.state) + 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[field]) + + for field_object in field_objects: + try: + # Save the field object that we made above + field_object.save() + # If save is successful on this scope, add the saved fields to + # the list of successful saves + saved_fields.extend([field.field_name for field in field_objects[field_object]]) + except DatabaseError: + log.exception('Error saving fields %r', field_objects[field_object]) + raise KeyValueMultiSaveError(saved_fields) + def find(self, key): ''' Look for a model data object using an DjangoKeyValueStore.Key object From 9338b6a4804c3ffbe51bfcdd362d2355ab27e6eb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 13:50:16 -0400 Subject: [PATCH 16/50] Push `delete` down into FieldDataCache from DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 34 +++++++++++++++++-------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 79d6dceb62..128f031bac 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -159,17 +159,7 @@ class DjangoKeyValueStore(KeyValueStore): if key.scope not in self._allowed_scopes: raise InvalidScopeError(key) - field_object = self._field_data_cache.find(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - state = json.loads(field_object.state) - del state[key.field_name] - field_object.state = json.dumps(state) - field_object.save() - else: - field_object.delete() + self._field_data_cache.delete(key) def has(self, key): if key.scope not in self._allowed_scopes: @@ -651,6 +641,28 @@ class FieldDataCache(object): log.exception('Error saving fields %r', field_objects[field_object]) raise KeyValueMultiSaveError(saved_fields) + @contract(key=DjangoKeyValueStore.Key) + def delete(self, key): + """ + Delete the value specified by `key`. + + Arguments: + key (`DjangoKeyValueStore.Key`): The field value to delete + + Raises: KeyError if key isn't found in the cache + """ + field_object = self.find(key) + if field_object is None: + raise KeyError(key.field_name) + + if key.scope == Scope.user_state: + state = json.loads(field_object.state) + del state[key.field_name] + field_object.state = json.dumps(state) + field_object.save() + else: + field_object.delete() + def find(self, key): ''' Look for a model data object using an DjangoKeyValueStore.Key object From 3bcd5ceb505cf338e4aa976c671392c99f1dffa7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 14:29:51 -0400 Subject: [PATCH 17/50] Push `has` down into FieldDataCache from DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 28 ++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 128f031bac..85c15e6355 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -165,14 +165,8 @@ class DjangoKeyValueStore(KeyValueStore): if key.scope not in self._allowed_scopes: raise InvalidScopeError(key) - field_object = self._field_data_cache.find(key) - if field_object is None: - return False + return self._field_data_cache.has(key) - if key.scope == Scope.user_state: - return key.field_name in json.loads(field_object.state) - else: - return True new_contract("DjangoKeyValueStore", DjangoKeyValueStore) new_contract("DjangoKeyValueStore_Key", DjangoKeyValueStore.Key) @@ -663,6 +657,26 @@ class FieldDataCache(object): else: field_object.delete() + @contract(key=DjangoKeyValueStore.Key, returns=bool) + def has(self, key): + """ + Return whether the specified `key` is set. + + Arguments: + key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: bool + """ + field_object = self.find(key) + if field_object is None: + return False + + if key.scope == Scope.user_state: + return key.field_name in json.loads(field_object.state) + else: + return True + + def find(self, key): ''' Look for a model data object using an DjangoKeyValueStore.Key object From d1fae17ca235425167a489c6edb1cd57db1c3b03 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 15:51:43 -0400 Subject: [PATCH 18/50] Move the logic from `find` into the methods that use it --- lms/djangoapps/courseware/model_data.py | 59 ++++++++++++++++--------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 85c15e6355..c03ae4ac89 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -584,7 +584,16 @@ class FieldDataCache(object): Returns: The found value Raises: KeyError if key isn't found in the cache ''' - field_object = self.find(key) + + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + + if key.scope not in self.cache: + raise KeyError(key.field_name) + + field_object = self.cache[key.scope].get(key) if field_object is None: raise KeyError(key.field_name) @@ -603,6 +612,7 @@ class FieldDataCache(object): kv_dict (dict): dict mapping from `DjangoKeyValueStore.Key`s to field values Raises: DatabaseError if any fields fail to save """ + saved_fields = [] # field_objects maps a field_object to a list of associated fields field_objects = dict() @@ -645,7 +655,16 @@ class FieldDataCache(object): Raises: KeyError if key isn't found in the cache """ - field_object = self.find(key) + + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + + if key.scope not in self.cache: + raise KeyError(key.field_name) + + field_object = self.cache[key.scope].get(key) if field_object is None: raise KeyError(key.field_name) @@ -667,7 +686,16 @@ class FieldDataCache(object): Returns: bool """ - field_object = self.find(key) + + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + + if key.scope not in self.cache: + return False + + field_object = self.cache[key.scope].get(key) if field_object is None: return False @@ -676,31 +704,21 @@ class FieldDataCache(object): else: return True - - def find(self, key): + def find_or_create(self, key): ''' - Look for a model data object using an DjangoKeyValueStore.Key object - - key: An `DjangoKeyValueStore.Key` object selecting the object to find - - returns the found object, or None if the object doesn't exist + Find a model data object in this cache, or create a new one if it doesn't + exist ''' + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): # If we're getting user data, we expect that the key matches the # user we were constructed for. assert key.user_id == self.user.id if key.scope not in self.cache: - return None + return - return self.cache[key.scope].get(key) - - def find_or_create(self, key): - ''' - Find a model data object in this cache, or create it if it doesn't - exist - ''' - field_object = self.find(key) + field_object = self.cache[key.scope].get(key) if field_object is not None: return field_object @@ -732,8 +750,5 @@ class FieldDataCache(object): student_id=key.user_id, ) - if key.scope not in self.cache: - return - self.cache[key.scope].set(key, field_object) return field_object From 43d41d6648e8f1230f46adb045e9f4c29ae9a101 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 16:07:06 -0400 Subject: [PATCH 19/50] Move `delete` logic down into per-scope caches --- lms/djangoapps/courseware/model_data.py | 51 +++++++++++++++++++------ 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index c03ae4ac89..fc0700ea2d 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -203,6 +203,25 @@ class DjangoOrmFieldCache(object): def set(self, kvs_key, value): self._cache[self._cache_key_for_kvs_key(kvs_key)] = value + @contract(kvs_key=DjangoKeyValueStore.Key) + def delete(self, kvs_key): + """ + Delete the value specified by `kvs_key`. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + Raises: KeyError if key isn't found in the cache + """ + + cache_key = self._cache_key_for_kvs_key(kvs_key) + field_object = self._cache.get(cache_key) + if field_object is None: + raise KeyError(kvs_key.field_name) + + field_object.delete() + del self._cache[cache_key] + def __len__(self): return len(self._cache) @@ -298,6 +317,26 @@ class UserStateCache(DjangoOrmFieldCache): """ return key.block_scope_id + @contract(kvs_key=DjangoKeyValueStore.Key) + def delete(self, kvs_key): + """ + Delete the value specified by `kvs_key`. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + 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: + 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() + class UserStateSummaryCache(DjangoOrmFieldCache): """ @@ -664,17 +703,7 @@ class FieldDataCache(object): if key.scope not in self.cache: raise KeyError(key.field_name) - field_object = self.cache[key.scope].get(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - state = json.loads(field_object.state) - del state[key.field_name] - field_object.state = json.dumps(state) - field_object.save() - else: - field_object.delete() + self.cache[key.scope].delete(key) @contract(key=DjangoKeyValueStore.Key, returns=bool) def has(self, key): From 6ce7cc771a76bb926131146efd540de171b90912 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 15 Apr 2015 16:11:36 -0400 Subject: [PATCH 20/50] Move `has` logic down into per-scope caches --- lms/djangoapps/courseware/model_data.py | 36 +++++++++++++++++++------ 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index fc0700ea2d..57c3ea27ba 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -222,6 +222,18 @@ class DjangoOrmFieldCache(object): field_object.delete() del self._cache[cache_key] + @contract(kvs_key=DjangoKeyValueStore.Key, returns=bool) + def has(self, kvs_key): + """ + Return whether the specified `kvs_key` is set. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: bool + """ + return self._cache_key_for_kvs_key(kvs_key) in self._cache + def __len__(self): return len(self._cache) @@ -337,6 +349,21 @@ class UserStateCache(DjangoOrmFieldCache): field_object.state = json.dumps(state) field_object.save() + @contract(kvs_key=DjangoKeyValueStore.Key, returns=bool) + def has(self, kvs_key): + """ + Return whether the specified `kvs_key` is set. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: bool + """ + field_object = self._cache.get(self._cache_key_for_kvs_key(kvs_key)) + if field_object is None: + return False + + return kvs_key.field_name in json.loads(field_object.state) class UserStateSummaryCache(DjangoOrmFieldCache): """ @@ -724,14 +751,7 @@ class FieldDataCache(object): if key.scope not in self.cache: return False - field_object = self.cache[key.scope].get(key) - if field_object is None: - return False - - if key.scope == Scope.user_state: - return key.field_name in json.loads(field_object.state) - else: - return True + return self.cache[key.scope].has(key) def find_or_create(self, key): ''' From 7a9923e28979f0c20be74543f21c076408066c44 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 16 Apr 2015 09:24:26 -0400 Subject: [PATCH 21/50] Inline find_or_create --- lms/djangoapps/courseware/model_data.py | 104 ++++++++++----------- lms/djangoapps/courseware/module_render.py | 15 +-- 2 files changed, 56 insertions(+), 63 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 57c3ea27ba..a2659d6f10 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -682,23 +682,64 @@ class FieldDataCache(object): saved_fields = [] # field_objects maps a field_object to a list of associated fields field_objects = dict() - for field in kv_dict: + for key in kv_dict: # If the field is valid and isn't already in the dictionary, add it. - field_object = self.find_or_create(field) + + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + + if key.scope not in self.cache: + continue + + field_object = self.cache[key.scope].get(key) + + if field_object is None: + if key.scope == Scope.user_state: + field_object, __ = StudentModule.objects.get_or_create( + course_id=self.course_id, + student_id=key.user_id, + module_state_key=key.block_scope_id, + defaults={ + 'state': json.dumps({}), + 'module_type': key.block_scope_id.block_type, + }, + ) + elif key.scope == Scope.user_state_summary: + 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.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.objects.get_or_create( + field_name=key.field_name, + student_id=key.user_id, + ) + + self.cache[key.scope].set(key, field_object) + if field_object not in field_objects.keys(): field_objects[field_object] = [] + # Update the list of associated fields - field_objects[field_object].append(field) + field_objects[field_object].append(key) # Special case when scope is for the user state, because this scope saves fields in a single row - if field.scope == Scope.user_state: + if key.scope == Scope.user_state: state = json.loads(field_object.state) - state[field.field_name] = kv_dict[field] + state[key.field_name] = kv_dict[key] 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[field]) + field_object.value = json.dumps(kv_dict[key]) for field_object in field_objects: try: @@ -706,7 +747,7 @@ class FieldDataCache(object): field_object.save() # If save is successful on this scope, add the saved fields to # the list of successful saves - saved_fields.extend([field.field_name for field in field_objects[field_object]]) + saved_fields.extend([key.field_name for key in field_objects[field_object]]) except DatabaseError: log.exception('Error saving fields %r', field_objects[field_object]) raise KeyValueMultiSaveError(saved_fields) @@ -752,52 +793,3 @@ class FieldDataCache(object): return False return self.cache[key.scope].has(key) - - def find_or_create(self, key): - ''' - Find a model data object in this cache, or create a new one if it doesn't - exist - ''' - - if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): - # If we're getting user data, we expect that the key matches the - # user we were constructed for. - assert key.user_id == self.user.id - - if key.scope not in self.cache: - return - - field_object = self.cache[key.scope].get(key) - - if field_object is not None: - return field_object - - if key.scope == Scope.user_state: - field_object, __ = StudentModule.objects.get_or_create( - course_id=self.course_id, - student_id=key.user_id, - module_state_key=key.block_scope_id, - defaults={ - 'state': json.dumps({}), - 'module_type': key.block_scope_id.block_type, - }, - ) - elif key.scope == Scope.user_state_summary: - 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.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.objects.get_or_create( - field_name=key.field_name, - student_id=key.user_id, - ) - - self.cache[key.scope].set(key, field_object) - return field_object diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index f381edd3fc..b0b99f8dd1 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -423,13 +423,14 @@ def get_module_system_for_user(user, field_data_cache, block_scope_id=descriptor.location, field_name='grade' ) - - student_module = field_data_cache.find_or_create(key) - # Update the grades - student_module.grade = event.get('value') - student_module.max_grade = event.get('max_value') - # Save all changes to the underlying KeyValueStore - student_module.save() + StudentModule.objects.filter( + student__id=user_id, + module_state_key=descriptor.location, + course_id=course_id, + ).update( + grade=event.get('value'), + max_grade=event.get('max_value') + ) # Bin score into range and increment stats score_bucket = get_score_bucket(student_module.grade, student_module.max_grade) From a0555d4cdce5981721a0151a42a01b24d99d14ec Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 12:47:08 -0400 Subject: [PATCH 22/50] Add more documentation to DjangoOrmFieldCache --- lms/djangoapps/courseware/model_data.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index a2659d6f10..638d8d7474 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -197,10 +197,26 @@ class DjangoOrmFieldCache(object): @contract(kvs_key=DjangoKeyValueStore.Key) def get(self, kvs_key): + """ + Return the django model object specified by `kvs_key` from + the cache. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: A django orm object from the cache + """ return self._cache.get(self._cache_key_for_kvs_key(kvs_key)) @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): + """ + Set the specified `kvs_key` to the django model object `value`. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + value: The django orm object to be stored in the cache + """ self._cache[self._cache_key_for_kvs_key(kvs_key)] = value @contract(kvs_key=DjangoKeyValueStore.Key) From 97c2513ed847715d168ef68ba54c9512faeadd71 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 14:39:38 -0400 Subject: [PATCH 23/50] Push `set_many` object creation down into per-type caches --- lms/djangoapps/courseware/model_data.py | 136 ++++++++++++++++-------- 1 file changed, 93 insertions(+), 43 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 638d8d7474..662fa88e73 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -211,13 +211,19 @@ class DjangoOrmFieldCache(object): @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): """ - Set the specified `kvs_key` to the django model object `value`. + Set the specified `kvs_key` to the field value `value`. Arguments: kvs_key (`DjangoKeyValueStore.Key`): The field value to delete - value: The django orm object to be stored in the cache + value: The field value to store """ - self._cache[self._cache_key_for_kvs_key(kvs_key)] = value + 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(kvs_key, value) @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): @@ -250,11 +256,17 @@ class DjangoOrmFieldCache(object): """ return self._cache_key_for_kvs_key(kvs_key) in self._cache + @contract(kvs_key=DjangoKeyValueStore.Key) + def _set_field_value(self, kvs_key, value): + cache_key = self._cache_key_for_kvs_key(kvs_key) + field_object = self._cache.get(cache_key) + field_object.value = json.dumps(value) + def __len__(self): return len(self._cache) @abstractmethod - def _create_object(self, kvs_key, value): + def _create_object(self, kvs_key): """ Create a new object to add to the cache (which should record the specified field ``value`` for the field identified by @@ -262,7 +274,6 @@ 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() @@ -312,6 +323,26 @@ class UserStateCache(DjangoOrmFieldCache): self.user = user self.select_for_update = select_for_update + def _create_object(self, kvs_key): + """ + Create a new object to add to the cache (which should record + the specified field ``value`` for the field identified by + ``kvs_key``). + + Arguments: + kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + """ + 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 _read_objects(self, fields, xblocks, aside_types): """ Return an iterator for all objects stored in the underlying datastore @@ -381,6 +412,15 @@ class UserStateCache(DjangoOrmFieldCache): return kvs_key.field_name in json.loads(field_object.state) + @contract(kvs_key=DjangoKeyValueStore.Key) + def _set_field_value(self, kvs_key, value): + cache_key = self._cache_key_for_kvs_key(kvs_key) + field_object = self._cache.get(cache_key) + state = json.loads(field_object.state) + state[kvs_key.field_name] = value + field_object.state = json.dumps(state) + + class UserStateSummaryCache(DjangoOrmFieldCache): """ Cache for Scope.user_state_summary xblock field data. @@ -390,6 +430,21 @@ class UserStateSummaryCache(DjangoOrmFieldCache): self.course_id = course_id self.select_for_update = select_for_update + def _create_object(self, kvs_key): + """ + Create a new object to add to the cache (which should record + the specified field ``value`` for the field identified by + ``kvs_key``). + + Arguments: + kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + """ + field_object, __ = XModuleUserStateSummaryField.objects.get_or_create( + field_name=kvs_key.field_name, + usage_id=kvs_key.block_scope_id + ) + return field_object + def _read_objects(self, fields, xblocks, aside_types): """ Return an iterator for all objects stored in the underlying datastore @@ -438,6 +493,22 @@ class PreferencesCache(DjangoOrmFieldCache): self.user = user self.select_for_update = select_for_update + def _create_object(self, kvs_key): + """ + Create a new object to add to the cache (which should record + the specified field ``value`` for the field identified by + ``kvs_key``). + + Arguments: + kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + """ + field_object, __ = XModuleStudentPrefsField.objects.get_or_create( + field_name=kvs_key.field_name, + module_type=BlockTypeKeyV1(kvs_key.block_family, kvs_key.block_scope_id), + student_id=kvs_key.user_id, + ) + return field_object + def _read_objects(self, fields, xblocks, aside_types): """ Return an iterator for all objects stored in the underlying datastore @@ -487,6 +558,21 @@ class UserInfoCache(DjangoOrmFieldCache): self.user = user self.select_for_update = select_for_update + def _create_object(self, kvs_key): + """ + Create a new object to add to the cache (which should record + the specified field ``value`` for the field identified by + ``kvs_key``). + + Arguments: + kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + """ + field_object, __ = XModuleStudentInfoField.objects.get_or_create( + field_name=kvs_key.field_name, + student_id=kvs_key.user_id, + ) + return field_object + def _read_objects(self, fields, xblocks, aside_types): """ Return an iterator for all objects stored in the underlying datastore @@ -709,37 +795,10 @@ class FieldDataCache(object): if key.scope not in self.cache: continue + self.cache[key.scope].set(key, kv_dict[key]) + field_object = self.cache[key.scope].get(key) - if field_object is None: - if key.scope == Scope.user_state: - field_object, __ = StudentModule.objects.get_or_create( - course_id=self.course_id, - student_id=key.user_id, - module_state_key=key.block_scope_id, - defaults={ - 'state': json.dumps({}), - 'module_type': key.block_scope_id.block_type, - }, - ) - elif key.scope == Scope.user_state_summary: - 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.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.objects.get_or_create( - field_name=key.field_name, - student_id=key.user_id, - ) - - self.cache[key.scope].set(key, field_object) if field_object not in field_objects.keys(): field_objects[field_object] = [] @@ -747,15 +806,6 @@ class FieldDataCache(object): # Update the list of associated fields field_objects[field_object].append(key) - # Special case when scope is for the user state, because this scope saves fields in a single row - if key.scope == Scope.user_state: - state = json.loads(field_object.state) - state[key.field_name] = kv_dict[key] - 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]) for field_object in field_objects: try: From 67d66184a73562a841cf4b54e3b9e07171f48a68 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 15:42:19 -0400 Subject: [PATCH 24/50] Push `set_many` save() calls into per-type caches --- lms/djangoapps/courseware/model_data.py | 35 +++++-------------- .../courseware/tests/test_model_data.py | 2 +- 2 files changed, 9 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 662fa88e73..8ea7f5e724 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -223,7 +223,8 @@ class DjangoOrmFieldCache(object): if field_object is None: self._cache[cache_key] = field_object = self._create_object(kvs_key) - self._set_field_value(kvs_key, value) + self._set_field_value(field_object, kvs_key, value) + field_object.save() @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): @@ -257,9 +258,7 @@ class DjangoOrmFieldCache(object): return self._cache_key_for_kvs_key(kvs_key) in self._cache @contract(kvs_key=DjangoKeyValueStore.Key) - def _set_field_value(self, kvs_key, value): - cache_key = self._cache_key_for_kvs_key(kvs_key) - field_object = self._cache.get(cache_key) + def _set_field_value(self, field_object, kvs_key, value): field_object.value = json.dumps(value) def __len__(self): @@ -413,9 +412,7 @@ class UserStateCache(DjangoOrmFieldCache): return kvs_key.field_name in json.loads(field_object.state) @contract(kvs_key=DjangoKeyValueStore.Key) - def _set_field_value(self, kvs_key, value): - cache_key = self._cache_key_for_kvs_key(kvs_key) - field_object = self._cache.get(cache_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) @@ -782,8 +779,6 @@ class FieldDataCache(object): """ saved_fields = [] - # field_objects maps a field_object to a list of associated fields - field_objects = dict() for key in kv_dict: # If the field is valid and isn't already in the dictionary, add it. @@ -795,27 +790,13 @@ class FieldDataCache(object): if key.scope not in self.cache: continue - self.cache[key.scope].set(key, kv_dict[key]) - - field_object = self.cache[key.scope].get(key) - - - if field_object not in field_objects.keys(): - field_objects[field_object] = [] - - # Update the list of associated fields - field_objects[field_object].append(key) - - - for field_object in field_objects: try: - # Save the field object that we made above - field_object.save() - # If save is successful on this scope, add the saved fields to + self.cache[key.scope].set(key, kv_dict[key]) + # If save is successful on this field, add it to # the list of successful saves - saved_fields.extend([key.field_name for key in field_objects[field_object]]) + saved_fields.append(key.field_name) except DatabaseError: - log.exception('Error saving fields %r', field_objects[field_object]) + log.exception('Error saving field %r', key) raise KeyValueMultiSaveError(saved_fields) @contract(key=DjangoKeyValueStore.Key) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 3355970562..83607cb6b1 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -373,7 +373,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') + self.assertEquals(exception.saved_field_names[0], 'other_existing_field') class TestUserStateSummaryStorage(StorageTestBase, TestCase): From 12740fa4acb619149eb7004e17b73f120a37b571 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 16:00:21 -0400 Subject: [PATCH 25/50] Make test_model_data test_set_many_failure slightly more robust --- lms/djangoapps/courseware/tests/test_model_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 83607cb6b1..f75e0cdb89 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -373,7 +373,7 @@ class StorageTestBase(object): exception = exception_context.exception self.assertEquals(len(exception.saved_field_names), 1) - self.assertEquals(exception.saved_field_names[0], 'other_existing_field') + self.assertIn(exception.saved_field_names[0], ('existing_field', 'other_existing_field')) class TestUserStateSummaryStorage(StorageTestBase, TestCase): From 407db1693882f6f9ce8b3d6d192fba7217fb7aca Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 16:00:48 -0400 Subject: [PATCH 26/50] Implement per-type set methods in terms of set_many methods --- lms/djangoapps/courseware/model_data.py | 47 +++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 8ea7f5e724..c1380be801 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -217,14 +217,26 @@ class DjangoOrmFieldCache(object): kvs_key (`DjangoKeyValueStore.Key`): The field value to delete value: The field value to store """ - cache_key = self._cache_key_for_kvs_key(kvs_key) - field_object = self._cache.get(cache_key) + self.set_many({kvs_key: value}) - if field_object is None: - self._cache[cache_key] = field_object = self._create_object(kvs_key) + @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") + def set_many(self, kv_dict): + """ + Set the specified fields to the supplied values. - self._set_field_value(field_object, kvs_key, value) - field_object.save() + Arguments: + kv_dict (dict): A dictionary mapping :class:`~DjangoKeyValueStore.Key` + objects to values to set. + """ + for kvs_key, value in sorted(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) + + self._set_field_value(field_object, kvs_key, value) + field_object.save() @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): @@ -375,6 +387,29 @@ class UserStateCache(DjangoOrmFieldCache): """ return key.block_scope_id + @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") + def set_many(self, kv_dict): + """ + Set the specified fields to the supplied values. + + Arguments: + kv_dict (dict): A dictionary mapping :class:`~DjangoKeyValueStore.Key` + objects to values to set. + """ + dirty_field_objects = set() + 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) + + self._set_field_value(field_object, kvs_key, value) + dirty_field_objects.add(field_object) + + for field_object in sorted(dirty_field_objects): + field_object.save() + @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): """ From 60e436c290581082d76210f0675102a7b37caf35 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 16:10:47 -0400 Subject: [PATCH 27/50] Push `get` down into per-type caches --- lms/djangoapps/courseware/model_data.py | 36 ++++++++++++++++++------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index c1380be801..680ceab3e6 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -206,7 +206,13 @@ class DjangoOrmFieldCache(object): Returns: A django orm object from the cache """ - return self._cache.get(self._cache_key_for_kvs_key(kvs_key)) + cache_key = self._cache_key_for_kvs_key(kvs_key) + if cache_key not in self._cache: + raise KeyError(kvs_key.field_name) + + field_object = self._cache[cache_key] + + return json.loads(field_object.value) @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): @@ -410,6 +416,25 @@ class UserStateCache(DjangoOrmFieldCache): for field_object in sorted(dirty_field_objects): field_object.save() + @contract(kvs_key=DjangoKeyValueStore.Key) + def get(self, kvs_key): + """ + Return the django model object specified by `kvs_key` from + the cache. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: A django orm object from the cache + """ + cache_key = self._cache_key_for_kvs_key(kvs_key) + 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] + @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): """ @@ -793,14 +818,7 @@ class FieldDataCache(object): if key.scope not in self.cache: raise KeyError(key.field_name) - field_object = self.cache[key.scope].get(key) - if field_object is None: - raise KeyError(key.field_name) - - if key.scope == Scope.user_state: - return json.loads(field_object.state)[key.field_name] - else: - return json.loads(field_object.value) + return self.cache[key.scope].get(key) @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") def set_many(self, kv_dict): From c3bb2e9b3ac1ab1a72bc5632e46a8efb8e55be9d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 21 Apr 2015 16:42:57 -0400 Subject: [PATCH 28/50] Use per-type cache set_many calls in FieldDataCache set_many --- lms/djangoapps/courseware/model_data.py | 42 +++++++++++++++++-------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 680ceab3e6..dabd1785b8 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -234,6 +234,7 @@ class DjangoOrmFieldCache(object): kv_dict (dict): A dictionary mapping :class:`~DjangoKeyValueStore.Key` objects to values to set. """ + saved_fields = [] for kvs_key, value in sorted(kv_dict.items()): cache_key = self._cache_key_for_kvs_key(kvs_key) field_object = self._cache.get(cache_key) @@ -242,7 +243,13 @@ class DjangoOrmFieldCache(object): self._cache[cache_key] = field_object = self._create_object(kvs_key) self._set_field_value(field_object, kvs_key, value) - field_object.save() + + try: + field_object.save() + saved_fields.append(kvs_key.field_name) + except DatabaseError: + log.exception("Saving field %r failed", kvs_key.field_name) + raise KeyValueMultiSaveError(saved_fields) @contract(kvs_key=DjangoKeyValueStore.Key) def delete(self, kvs_key): @@ -402,7 +409,7 @@ class UserStateCache(DjangoOrmFieldCache): kv_dict (dict): A dictionary mapping :class:`~DjangoKeyValueStore.Key` objects to values to set. """ - dirty_field_objects = set() + dirty_field_objects = defaultdict(set) 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) @@ -411,10 +418,16 @@ class UserStateCache(DjangoOrmFieldCache): self._cache[cache_key] = field_object = self._create_object(kvs_key) self._set_field_value(field_object, kvs_key, value) - dirty_field_objects.add(field_object) + dirty_field_objects[field_object].add(kvs_key.field_name) - for field_object in sorted(dirty_field_objects): - field_object.save() + 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) @contract(kvs_key=DjangoKeyValueStore.Key) def get(self, kvs_key): @@ -832,8 +845,8 @@ class FieldDataCache(object): """ saved_fields = [] - for key in kv_dict: - # If the field is valid and isn't already in the dictionary, add it. + by_scope = defaultdict(dict) + for key, value in kv_dict.iteritems(): if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): # If we're getting user data, we expect that the key matches the @@ -843,14 +856,17 @@ class FieldDataCache(object): if key.scope not in self.cache: continue + by_scope[key.scope][key] = value + + for scope, set_many_data in by_scope.iteritems(): try: - self.cache[key.scope].set(key, kv_dict[key]) - # If save is successful on this field, add it to + self.cache[scope].set_many(set_many_data) + # If save is successful on these fields, add it to # the list of successful saves - saved_fields.append(key.field_name) - except DatabaseError: - log.exception('Error saving field %r', key) - raise KeyValueMultiSaveError(saved_fields) + saved_fields.extend(key.field_name for key in set_many_data) + except KeyValueMultiSaveError as exc: + log.exception('Error saving fields %r', [key.field_name for key in set_many_data]) + raise KeyValueMultiSaveError(saved_fields + exc.saved_field_names) @contract(key=DjangoKeyValueStore.Key) def delete(self, key): From 3430024d225cc58005ee481ce33a6f46ac2248eb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 23 Apr 2015 12:12:04 -0400 Subject: [PATCH 29/50] Add a temporary set_grade method to the FieldDataCache and UserStateCache --- lms/djangoapps/courseware/model_data.py | 26 +++++++++++++++++++++- lms/djangoapps/courseware/module_render.py | 24 ++++++++------------ 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index dabd1785b8..109550c2ce 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -13,7 +13,7 @@ from .models import ( XModuleStudentInfoField ) import logging -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.block_types import BlockTypeKeyV1 from opaque_keys.edx.asides import AsideUsageKeyV1 from contracts import contract, new_contract @@ -490,6 +490,18 @@ class UserStateCache(DjangoOrmFieldCache): state[kvs_key.field_name] = value field_object.state = json.dumps(state) + @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): + """ + UNSUPPORTED METHOD + + 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() + class UserStateSummaryCache(DjangoOrmFieldCache): """ @@ -909,3 +921,15 @@ class FieldDataCache(object): return False return self.cache[key.scope].has(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): + """ + UNSUPPORTED METHOD + + Set the score and max_score for the specified user and xblock usage. + """ + assert not self.user.is_anonymous() + assert user_id == self.user.id + assert usage_key.course_key == self.course_id + self.cache[Scope.user_state].set_score(user_id, usage_key, score, max_score) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index b0b99f8dd1..a3845e2ff7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -416,24 +416,18 @@ def get_module_system_for_user(user, field_data_cache, """ user_id = event.get('user_id', user.id) - # Construct the key for the module - key = KeyValueStore.Key( - scope=Scope.user_state, - user_id=user_id, - block_scope_id=descriptor.location, - field_name='grade' - ) - StudentModule.objects.filter( - student__id=user_id, - module_state_key=descriptor.location, - course_id=course_id, - ).update( - grade=event.get('value'), - max_grade=event.get('max_value') + grade = event.get('value') + max_grade = event.get('max_value') + + field_data_cache.set_score( + user_id, + descriptor.location, + grade, + max_grade, ) # Bin score into range and increment stats - score_bucket = get_score_bucket(student_module.grade, student_module.max_grade) + score_bucket = get_score_bucket(grade, max_grade) tags = [ u"org:{}".format(course_id.org), From 8c959528bc1b721bb3d57e56945aff587e27d28d Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 23 Apr 2015 15:55:45 -0400 Subject: [PATCH 30/50] Add a last_modified method to FieldDataCache --- lms/djangoapps/courseware/model_data.py | 37 ++++++++++++++++++++++++ lms/djangoapps/mobile_api/users/views.py | 10 +++---- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 109550c2ce..80df3db3ea 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -282,6 +282,23 @@ class DjangoOrmFieldCache(object): """ return self._cache_key_for_kvs_key(kvs_key) in self._cache + @contract(kvs_key=DjangoKeyValueStore.Key, returns="datetime|None") + def last_modified(self, kvs_key): + """ + Return when the supplied field was changed. + + Arguments: + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + + 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 + @contract(kvs_key=DjangoKeyValueStore.Key) def _set_field_value(self, field_object, kvs_key, value): field_object.value = json.dumps(value) @@ -933,3 +950,23 @@ class FieldDataCache(object): assert user_id == self.user.id assert usage_key.course_key == self.course_id self.cache[Scope.user_state].set_score(user_id, usage_key, score, max_score) + + @contract(key=DjangoKeyValueStore.Key, returns="datetime|None") + def last_modified(self, key): + """ + Return when the supplied field was changed. + + Arguments: + key (`DjangoKeyValueStore.Key`): The field value to delete + + Returns: datetime if there was a modified date, or None otherwise + """ + if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): + # If we're getting user data, we expect that the key matches the + # user we were constructed for. + assert key.user_id == self.user.id + + if key.scope not in self.cache: + return None + + return self.cache[key.scope].last_modified(key) diff --git a/lms/djangoapps/mobile_api/users/views.py b/lms/djangoapps/mobile_api/users/views.py index fa1e6c68ee..2f90d96de1 100644 --- a/lms/djangoapps/mobile_api/users/views.py +++ b/lms/djangoapps/mobile_api/users/views.py @@ -149,12 +149,10 @@ class UserCourseStatus(views.APIView): block_scope_id=course.location, field_name=None ) - student_module = field_data_cache.find(key) - if student_module: - original_store_date = student_module.modified - if modification_date < original_store_date: - # old modification date so skip update - return self._get_course_info(request, course) + original_store_date = field_data_cache.last_modified(key) + if original_store_date is not None and modification_date < original_store_date: + # old modification date so skip update + return self._get_course_info(request, course) save_positions_recursively_up(request.user, request, field_data_cache, module) return self._get_course_info(request, course) From 3ea74743ee786ab9b708c9cb320b5a238a20ec7a Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 09:21:25 -0400 Subject: [PATCH 31/50] Flatten DjangoOrmFieldCache methods into UserStateCache --- lms/djangoapps/courseware/model_data.py | 117 ++++++++++++++---------- 1 file changed, 71 insertions(+), 46 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 80df3db3ea..806cafbe00 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -354,68 +354,56 @@ class DjangoOrmFieldCache(object): raise NotImplementedError() -class UserStateCache(DjangoOrmFieldCache): +class UserStateCache(object): """ Cache for Scope.user_state xblock field data. """ def __init__(self, user, course_id, select_for_update=False): - super(UserStateCache, self).__init__() + self._cache = {} self.course_id = course_id self.user = user self.select_for_update = select_for_update - def _create_object(self, kvs_key): + def cache_fields(self, fields, xblocks, aside_types): # pylint: disable=unused-argument """ - Create a new object to add to the cache (which should record - the specified field ``value`` for the field identified by - ``kvs_key``). + Load all fields specified by ``fields`` for the supplied ``xblocks`` + and ``aside_types`` into this cache. Arguments: - kvs_key (:class:`DjangoKeyValueStore.Key`): Which field to create an entry for + fields (list of str): Field names to cache. + xblocks (list of :class:`XBlock`): XBlocks to cache fields for. + aside_types (list of str): Aside types to cache fields for. """ - 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 + for field_object in self._read_objects(fields, xblocks, aside_types): + self._cache[self._cache_key_for_field_object(field_object)] = field_object - def _read_objects(self, fields, xblocks, aside_types): + @contract(kvs_key=DjangoKeyValueStore.Key) + def set(self, kvs_key, value): """ - Return an iterator for all objects stored in the underlying datastore - for the ``fields`` on the ``xblocks`` and the ``aside_types`` associated - with them. + Set the specified `kvs_key` to the field value `value`. Arguments: - fields (list of str): Field names to return values for - xblocks (list of :class:`~XBlock`): XBlocks to load fields for - aside_types (list of str): Asides to load field for (which annotate the supplied - xblocks). + kvs_key (`DjangoKeyValueStore.Key`): The field value to delete + value: The field value to store """ - return _chunked_query( - StudentModule, - self.select_for_update, - 'module_state_key__in', - _all_usage_keys(xblocks, aside_types), - course_id=self.course_id, - student=self.user.pk, - ) + self.set_many({kvs_key: value}) - def _cache_key_for_field_object(self, field_object): - return field_object.module_state_key.map_into_course(self.course_id) - - def _cache_key_for_kvs_key(self, key): + @contract(kvs_key=DjangoKeyValueStore.Key, returns="datetime|None") + def last_modified(self, kvs_key): """ - Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. + Return when the supplied field was changed. Arguments: - key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + kvs_key (`DjangoKeyValueStore.Key`): The key representing the cached field + + Returns: datetime if there was a modified date, or None otherwise """ - return key.block_scope_id + 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 @contract(kv_dict="dict(DjangoKeyValueStore_Key: *)") def set_many(self, kv_dict): @@ -501,12 +489,6 @@ class UserStateCache(DjangoOrmFieldCache): return kvs_key.field_name in json.loads(field_object.state) - @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) - @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): """ @@ -519,6 +501,49 @@ class UserStateCache(DjangoOrmFieldCache): field_object.max_grade = max_score field_object.save() + def __len__(self): + return len(self._cache) + + def _read_objects(self, fields, descriptors, aside_types): + return _chunked_query( + StudentModule, + self.select_for_update, + 'module_state_key__in', + _all_usage_keys(descriptors, aside_types), + course_id=self.course_id, + student=self.user.pk, + ) + + def _cache_key_for_field_object(self, field_object): + return field_object.module_state_key.map_into_course(self.course_id) + + 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. + + Arguments: + key (:class:`~DjangoKeyValueStore.Key`): The key representing the cached field + """ + 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): """ From 57d5fa28b58c1f86db052c755460119844ddc326 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 09:28:27 -0400 Subject: [PATCH 32/50] Reorder methods in UserStateCache --- lms/djangoapps/courseware/model_data.py | 28 ++++++++++++++----------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 806cafbe00..3fb9b2acaf 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -501,9 +501,25 @@ class UserStateCache(object): 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) + 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 _read_objects(self, fields, descriptors, aside_types): return _chunked_query( StudentModule, @@ -517,18 +533,6 @@ class UserStateCache(object): def _cache_key_for_field_object(self, field_object): return field_object.module_state_key.map_into_course(self.course_id) - 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. From c9dbd2c30858736e63a5cc0a672ad6a3c6c57227 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 09:30:06 -0400 Subject: [PATCH 33/50] Add an empty implementation of XBlockUserStateClient backed by StudentModule --- .../courseware/user_state_client.py | 63 +++++++++++++++++++ lms/djangoapps/xblock_user_state/interface.py | 1 + 2 files changed, 64 insertions(+) create mode 100644 lms/djangoapps/courseware/user_state_client.py diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py new file mode 100644 index 0000000000..19f522d34b --- /dev/null +++ b/lms/djangoapps/courseware/user_state_client.py @@ -0,0 +1,63 @@ +""" +An implementation of :class:`XBlockUserStateClient`, which stores XBlock Scope.user_state +data in a Django ORM model. +""" + +from xblock_user_state.interface import DjangoXBlockUserStateClient + + +class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): + """ + An interface that uses the Django ORM StudentModule as a backend. + """ + + class ServiceUnavailable(XBlockUserStateClient.ServiceUnavailable): + """ + This error is raised if the service backing this client is currently unavailable. + """ + pass + + class PermissionDenied(XBlockUserStateClient.PermissionDenied): + """ + This error is raised if the caller is not allowed to access the requested data. + """ + pass + + class DoesNotExist(XBlockUserStateClient.DoesNotExist): + """ + This error is raised if the caller has requested data that does not exist. + """ + pass + + def get(username, block_key, scope=Scope.user_state): + return self.get_many(username, [block_key], scope) + + def set(username, block_key, state, scope=Scope.user_state): + self.set_many(username, {block_key: state}, scope) + + def get_many(username, block_keys, scope=Scope.user_state): + """Returns dict of block_id -> state.""" + raise NotImplementedError() + + def set_many(username, block_keys_to_state, scope=Scope.user_state): + raise NotImplementedError() + + def get_history(username, block_key, scope=Scope.user_state): + """We don't guarantee that history for many blocks will be fast.""" + raise NotImplementedError() + + def iter_all_for_block(block_key, scope=Scope.user_state, batch_size=None): + """ + You get no ordering guarantees. Fetching will happen in batch_size + increments. If you're using this method, you should be running in an + async task. + """ + raise NotImplementedError() + + def iter_all_for_course(course_key, block_type=None, scope=Scope.user_state, batch_size=None): + """ + You get no ordering guarantees. Fetching will happen in batch_size + increments. If you're using this method, you should be running in an + async task. + """ + raise NotImplementedError() diff --git a/lms/djangoapps/xblock_user_state/interface.py b/lms/djangoapps/xblock_user_state/interface.py index 5d001de4d1..2c7254d00f 100644 --- a/lms/djangoapps/xblock_user_state/interface.py +++ b/lms/djangoapps/xblock_user_state/interface.py @@ -10,6 +10,7 @@ from xblock.fields import Scope, ScopeBase new_contract('UsageKey', UsageKey) + class XBlockUserStateClient(object): """ First stab at an interface for accessing XBlock User State. This will have From 4c3fb2d07ed18acf92c07306310dbe1f3f4ce580 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 09:32:13 -0400 Subject: [PATCH 34/50] Enforce user-state only for StudentModule backend Client --- lms/djangoapps/courseware/user_state_client.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index 19f522d34b..d46a57dcb8 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -37,13 +37,19 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): def get_many(username, block_keys, scope=Scope.user_state): """Returns dict of block_id -> state.""" + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() def set_many(username, block_keys_to_state, scope=Scope.user_state): + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() def get_history(username, block_key, scope=Scope.user_state): """We don't guarantee that history for many blocks will be fast.""" + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() def iter_all_for_block(block_key, scope=Scope.user_state, batch_size=None): @@ -52,6 +58,8 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): increments. If you're using this method, you should be running in an async task. """ + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() def iter_all_for_course(course_key, block_type=None, scope=Scope.user_state, batch_size=None): @@ -60,4 +68,6 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): increments. If you're using this method, you should be running in an async task. """ + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() From 1ec4ae7c26014a8998db066e25f91964e2111e03 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 09:53:13 -0400 Subject: [PATCH 35/50] Add more documentation to XBlockUserStateClient interface --- .../courseware/user_state_client.py | 60 +++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index d46a57dcb8..d41ab12d19 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -29,19 +29,71 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): """ pass - def get(username, block_key, scope=Scope.user_state): - return self.get_many(username, [block_key], scope) + def get(username, block_key, scope=Scope.user_state, fields=None): + """ + Retrieve the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be retrieved + block_key (UsageKey): The UsageKey identifying which xblock state to load. + scope (Scope): The scope to load data from + fields: A list of field values to retrieve. If None, retrieve all stored fields. + + Returns: + dict: A dictionary mapping field names to values + + Raises: + DoesNotExist if no entry is found. + """ + try: + _usage_key, state = next(self.get_many(username, [block_key], scope, fields=fields)) + except StopIteration: + raise self.DoesNotExist() + + return state def set(username, block_key, state, scope=Scope.user_state): + """ + Set fields for a particular XBlock. + + Arguments: + username: The name of the user whose state should be retrieved + block_key (UsageKey): The UsageKey identifying which xblock state to load. + state (dict): A dictionary mapping field names to values + scope (Scope): The scope to load data from + """ self.set_many(username, {block_key: state}, scope) - def get_many(username, block_keys, scope=Scope.user_state): - """Returns dict of block_id -> state.""" + def get_many(username, block_keys, scope=Scope.user_state, fields=None): + """ + Retrieve the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be retrieved + block_keys ([UsageKey]): A list of UsageKeys identifying which xblock states to load. + scope (Scope): The scope to load data from + fields: A list of field values to retrieve. If None, retrieve all stored fields. + + Yields: + (UsageKey, field_state) tuples for each specified UsageKey in block_keys. + field_state is a dict mapping field names to values. + """ if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() def set_many(username, block_keys_to_state, scope=Scope.user_state): + """ + Set fields for a particular XBlock. + + Arguments: + username: The name of the user whose state should be retrieved + block_keys_to_state (dict): A dict mapping UsageKeys to state dicts. + Each state dict maps field names to values. These state dicts + are overlaid over the stored state. To delete fields, use + :meth:`delete` or :meth:`delete_many`. + scope (Scope): The scope to load data from + """ if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() From 674b68b7e3d47a0486579d249e3df91383cc6351 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 10:30:10 -0400 Subject: [PATCH 36/50] Add implementation of get_many and set_many to DjangoXBlockUserStateClient --- .../courseware/user_state_client.py | 156 ++++++++++++++++-- 1 file changed, 143 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index d41ab12d19..cccfd65f62 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -3,10 +3,20 @@ An implementation of :class:`XBlockUserStateClient`, which stores XBlock Scope.u data in a Django ORM model. """ -from xblock_user_state.interface import DjangoXBlockUserStateClient +import itertools +from operator import attrgetter + +try: + import simplejson as json +except ImportError: + import json + +from xblock.fields import Scope +from xblock_user_state.interface import XBlockUserStateClient +from courseware.models import StudentModule -class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): +class DjangoXBlockUserStateClient(XBlockUserStateClient): """ An interface that uses the Django ORM StudentModule as a backend. """ @@ -29,7 +39,11 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): """ pass - def get(username, block_key, scope=Scope.user_state, fields=None): + def __init__(self, user): + self._student_module_cache = {} + self.user = user + + def get(self, username, block_key, scope=Scope.user_state, fields=None): """ Retrieve the stored XBlock state for a single xblock usage. @@ -45,6 +59,7 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): Raises: DoesNotExist if no entry is found. """ + assert self.user.username == username try: _usage_key, state = next(self.get_many(username, [block_key], scope, fields=fields)) except StopIteration: @@ -52,19 +67,68 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): return state - def set(username, block_key, state, scope=Scope.user_state): + def set(self, username, block_key, state, scope=Scope.user_state): """ Set fields for a particular XBlock. Arguments: username: The name of the user whose state should be retrieved - block_key (UsageKey): The UsageKey identifying which xblock state to load. + block_key (UsageKey): The UsageKey identifying which xblock state to update. state (dict): A dictionary mapping field names to values scope (Scope): The scope to load data from """ + assert self.user.username == username self.set_many(username, {block_key: state}, scope) - def get_many(username, block_keys, scope=Scope.user_state, fields=None): + def delete(self, username, block_key, scope=Scope.user_state, fields=None): + """ + Delete the stored XBlock state for a single xblock usage. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock state to delete. + scope (Scope): The scope to delete data from + fields: A list of fields to delete. If None, delete all stored fields. + """ + assert self.user.username == username + return self.delete_many(username, [block_key], scope, fields=fields) + + def _get_field_objects(self, username, block_keys): + """ + Retrieve the :class:`~StudentModule`s for the supplied ``username`` and ``block_keys``. + + Arguments: + username (str): The name of the user to load `StudentModule`s for. + block_keys (list of :class:`~UsageKey`): The set of XBlocks to load data for. + """ + course_key_func = attrgetter('course_key') + by_course = itertools.groupby( + sorted(block_keys, key=course_key_func), + course_key_func, + ) + + 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, + 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) + + + def get_many(self, username, block_keys, scope=Scope.user_state, fields=None): """ Retrieve the stored XBlock state for a single xblock usage. @@ -78,11 +142,19 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): (UsageKey, field_state) tuples for each specified UsageKey in block_keys. field_state is a dict mapping field names to values. """ + assert self.user.username == username if scope != Scope.user_state: - raise ValueError("Only Scope.user_state is supported") - raise NotImplementedError() + raise ValueError("Only Scope.user_state is supported, not {}".format(scope)) - def set_many(username, block_keys_to_state, scope=Scope.user_state): + modules = self._get_field_objects(username, block_keys) + for module, usage_key in modules: + if module.state is None: + state = {} + else: + state = json.loads(module.state) + yield (usage_key, state) + + def set_many(self, username, block_keys_to_state, scope=Scope.user_state): """ Set fields for a particular XBlock. @@ -94,17 +166,75 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): :meth:`delete` or :meth:`delete_many`. scope (Scope): The scope to load data from """ + assert self.user.username == username if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") - raise NotImplementedError() - def get_history(username, block_key, scope=Scope.user_state): + 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() + + for usage_key, state in block_keys_to_state.items(): + student_module, created = StudentModule.objects.get_or_create( + student=self.user, + course_id=usage_key.course_key, + module_state_key=usage_key, + defaults={ + 'state': json.dumps(state), + 'module_type': usage_key.block_type, + }, + ) + + if not created: + if student_module.state is None: + current_state = {} + else: + current_state = json.loads(student_module.state) + current_state.update(state) + student_module.state = json.dumps(current_state) + # We just read this object, so we know that we can do an update + student_module.save(force_update=True) + + def delete_many(self, username, block_keys, scope=Scope.user_state, fields=None): + """ + Delete the stored XBlock state for a many xblock usages. + + Arguments: + username: The name of the user whose state should be deleted + block_key (UsageKey): The UsageKey identifying which xblock state to delete. + scope (Scope): The scope to delete data from + fields: A list of fields to delete. If None, delete all stored fields. + """ + assert self.user.username == username + if scope != Scope.user_state: + raise ValueError("Only Scope.user_state is supported") + + student_modules = self._get_field_objects(username, block_keys) + for student_module, _ in student_modules: + if fields is None: + field_object.state = "{}" + else: + current_state = json.loads(field_object.state) + for field in fields: + if field in current_state: + del current_state[field] + + student_module.state = json.dumps(current_state) + # We just read this object, so we know that we can do an update + student_module.save(force_update=True) + + 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 if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() - def iter_all_for_block(block_key, scope=Scope.user_state, batch_size=None): + def iter_all_for_block(self, block_key, scope=Scope.user_state, batch_size=None): """ You get no ordering guarantees. Fetching will happen in batch_size increments. If you're using this method, you should be running in an @@ -114,7 +244,7 @@ class DjangoXBlockUserStateClient(DjangoXBlockUserStateClient): raise ValueError("Only Scope.user_state is supported") raise NotImplementedError() - def iter_all_for_course(course_key, block_type=None, scope=Scope.user_state, batch_size=None): + def iter_all_for_course(self, course_key, block_type=None, scope=Scope.user_state, batch_size=None): """ You get no ordering guarantees. Fetching will happen in batch_size increments. If you're using this method, you should be running in an From 917ea90dc30dc8b0fa44124d1b6a07d0ff5c5300 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 10:38:38 -0400 Subject: [PATCH 37/50] Inline some private methods in UserStateCache --- lms/djangoapps/courseware/model_data.py | 26 +++++++++++-------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 3fb9b2acaf..cdcbd4e32f 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -374,8 +374,17 @@ class UserStateCache(object): xblocks (list of :class:`XBlock`): XBlocks to cache fields for. aside_types (list of str): Aside types to cache fields for. """ - for field_object in self._read_objects(fields, xblocks, aside_types): - self._cache[self._cache_key_for_field_object(field_object)] = field_object + query = _chunked_query( + StudentModule, + self.select_for_update, + 'module_state_key__in', + _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 @contract(kvs_key=DjangoKeyValueStore.Key) def set(self, kvs_key, value): @@ -520,19 +529,6 @@ class UserStateCache(object): ) return field_object - def _read_objects(self, fields, descriptors, aside_types): - return _chunked_query( - StudentModule, - self.select_for_update, - 'module_state_key__in', - _all_usage_keys(descriptors, aside_types), - course_id=self.course_id, - student=self.user.pk, - ) - - def _cache_key_for_field_object(self, field_object): - return field_object.module_state_key.map_into_course(self.course_id) - def _cache_key_for_kvs_key(self, key): """ Return the key used in this DjangoOrmFieldCache for the specified KeyValueStore key. From 8d9d6ce0ec70bc1e3941a14c6e63767169801f8e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 11:20:23 -0400 Subject: [PATCH 38/50] Clarify the interface used by xqueue_callback to load an XBlock --- lms/djangoapps/courseware/module_render.py | 12 ++++++------ .../courseware/tests/test_module_render.py | 6 ++---- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index a3845e2ff7..8776b2b4ae 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -52,7 +52,7 @@ from xblock.exceptions import NoSuchHandlerError, NoSuchViewError from xblock.django.request import django_to_webob_request, webob_to_django_response from xmodule.error_module import ErrorDescriptor, NonStaffErrorDescriptor from xmodule.exceptions import NotFoundError, ProcessingError -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import modulestore, ModuleI18nService @@ -724,12 +724,12 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours return descriptor -def find_target_student_module(request, user_id, course_id, mod_id): +def load_single_xblock(request, user_id, course_id, usage_key_string): """ - Retrieve target StudentModule + Load a single XBlock identified by usage_key_string. """ course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) - usage_key = course_id.make_usage_key_from_deprecated_string(mod_id) + usage_key = course_id.make_usage_key_from_deprecated_string(usage_key_string) user = User.objects.get(id=user_id) field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course_id, @@ -740,7 +740,7 @@ def find_target_student_module(request, user_id, course_id, mod_id): ) instance = get_module(user, request, usage_key, field_data_cache, grade_bucket_type='xqueue') if instance is None: - msg = "No module {0} for user {1}--access denied?".format(mod_id, user) + msg = "No module {0} for user {1}--access denied?".format(usage_key_string, user) log.debug(msg) raise Http404 return instance @@ -764,7 +764,7 @@ def xqueue_callback(request, course_id, userid, mod_id, dispatch): if not isinstance(header, dict) or 'lms_key' not in header: raise Http404 - instance = find_target_student_module(request, userid, course_id, mod_id) + instance = load_single_xblock(request, userid, course_id, mod_id) # Transfer 'queuekey' from xqueue response header to the data. # This is required to use the interface defined by 'handle_ajax' diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 0534c668ec..bdac1694d6 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -160,8 +160,7 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase): } # Patch getmodule to return our mock module - with patch('courseware.module_render.find_target_student_module') as get_fake_module: - get_fake_module.return_value = self.mock_module + with patch('courseware.module_render.load_single_xblock', return_value=self.mock_module): # call xqueue_callback with our mocked information request = self.request_factory.post(self.callback_url, data) render.xqueue_callback(request, self.course_key, self.mock_user.id, self.mock_module.id, self.dispatch) @@ -176,8 +175,7 @@ class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase): 'xqueue_body': 'hello world', } - with patch('courseware.module_render.find_target_student_module') as get_fake_module: - get_fake_module.return_value = self.mock_module + with patch('courseware.module_render.load_single_xblock', return_value=self.mock_module): # Test with missing xqueue data with self.assertRaises(Http404): request = self.request_factory.post(self.callback_url, {}) From e6db0af1f1d906f79adb2973f8e6db86a576f321 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 11:22:13 -0400 Subject: [PATCH 39/50] Use current OpaqueKeys methods when loading a block for the XQueue callback --- lms/djangoapps/courseware/module_render.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 8776b2b4ae..827f33921d 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -728,11 +728,12 @@ def load_single_xblock(request, user_id, course_id, usage_key_string): """ Load a single XBlock identified by usage_key_string. """ - course_id = SlashSeparatedCourseKey.from_deprecated_string(course_id) - usage_key = course_id.make_usage_key_from_deprecated_string(usage_key_string) + usage_key = UsageKey.from_string(usage_key_string) + course_key = CourseKey.from_string(course_id) + usage_key = usage_key.map_into_course(course_key) user = User.objects.get(id=user_id) field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - course_id, + course_key, user, modulestore().get_item(usage_key), depth=0, From cacdbc35d7514ffbb141b3ba97ed58094347afd3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 24 Apr 2015 12:09:24 -0400 Subject: [PATCH 40/50] Remove the ability to `select_for_updates` from FieldDataCache. The only consumer of that functionality (the XQueue callback) already does retries, so newly introduce integrity errors (due to multiple commiters trying to update a StudentModule) won't break the XQueue processing pipeline. --- lms/djangoapps/courseware/model_data.py | 44 +++++----------------- lms/djangoapps/courseware/module_render.py | 1 - 2 files changed, 9 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index cdcbd4e32f..7abcbc4450 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -44,18 +44,7 @@ def chunks(items, chunk_size): return (items[i:i + chunk_size] for i in xrange(0, len(items), chunk_size)) -def _query(model_class, select_for_update, **kwargs): - """ - Queries model_class with **kwargs, optionally adding select_for_update if - `select_for_update` is True. - """ - query = model_class.objects - if select_for_update: - query = query.select_for_update() - query = query.filter(**kwargs) - return query - -def _chunked_query(model_class, select_for_update, chunk_field, items, chunk_size=500, **kwargs): +def _chunked_query(model_class, chunk_field, items, chunk_size=500, **kwargs): """ Queries model_class with `chunk_field` set to chunks of size `chunk_size`, and all other parameters from `**kwargs`. @@ -64,7 +53,7 @@ def _chunked_query(model_class, select_for_update, chunk_field, items, chunk_siz that can be put into a single query. """ res = chain.from_iterable( - _query(model_class, select_for_update, **dict([(chunk_field, chunk)] + kwargs.items())) + model_class.objects.filter(**dict([(chunk_field, chunk)] + kwargs.items())) for chunk in chunks(items, chunk_size) ) return res @@ -358,11 +347,10 @@ class UserStateCache(object): """ Cache for Scope.user_state xblock field data. """ - def __init__(self, user, course_id, select_for_update=False): + def __init__(self, user, course_id): self._cache = {} self.course_id = course_id self.user = user - self.select_for_update = select_for_update def cache_fields(self, fields, xblocks, aside_types): # pylint: disable=unused-argument """ @@ -376,7 +364,6 @@ class UserStateCache(object): """ query = _chunked_query( StudentModule, - self.select_for_update, 'module_state_key__in', _all_usage_keys(xblocks, aside_types), course_id=self.course_id, @@ -549,10 +536,9 @@ class UserStateSummaryCache(DjangoOrmFieldCache): """ Cache for Scope.user_state_summary xblock field data. """ - def __init__(self, course_id, select_for_update=False): + def __init__(self, course_id): super(UserStateSummaryCache, self).__init__() self.course_id = course_id - self.select_for_update = select_for_update def _create_object(self, kvs_key): """ @@ -583,7 +569,6 @@ class UserStateSummaryCache(DjangoOrmFieldCache): """ return _chunked_query( XModuleUserStateSummaryField, - self.select_for_update, 'usage_id__in', _all_usage_keys(xblocks, aside_types), field_name__in=set(field.name for field in fields), @@ -612,10 +597,9 @@ class PreferencesCache(DjangoOrmFieldCache): """ Cache for Scope.preferences xblock field data. """ - def __init__(self, user, select_for_update=False): + def __init__(self, user): super(PreferencesCache, self).__init__() self.user = user - self.select_for_update = select_for_update def _create_object(self, kvs_key): """ @@ -647,7 +631,6 @@ class PreferencesCache(DjangoOrmFieldCache): """ return _chunked_query( XModuleStudentPrefsField, - self.select_for_update, 'module_type__in', _all_block_types(xblocks, aside_types), student=self.user.pk, @@ -677,10 +660,9 @@ class UserInfoCache(DjangoOrmFieldCache): """ Cache for Scope.user_info xblock field data """ - def __init__(self, user, select_for_update=False): + def __init__(self, user): super(UserInfoCache, self).__init__() self.user = user - self.select_for_update = select_for_update def _create_object(self, kvs_key): """ @@ -709,9 +691,7 @@ class UserInfoCache(DjangoOrmFieldCache): aside_types (list of str): Asides to load field for (which annotate the supplied xblocks). """ - return _query( - XModuleStudentInfoField, - self.select_for_update, + return XModuleStudentInfoField.objects.filter( student=self.user.pk, field_name__in=set(field.name for field in fields), ) @@ -751,11 +731,9 @@ class FieldDataCache(object): descriptors: A list of XModuleDescriptors. course_id: The id of the current course user: The user for which to cache data - select_for_update: True if rows should be locked until end of transaction + select_for_update: Ignored asides: The list of aside types to load, or None to prefetch no asides. ''' - self.select_for_update = select_for_update - if asides is None: self.asides = [] else: @@ -769,19 +747,15 @@ class FieldDataCache(object): Scope.user_state: UserStateCache( self.user, self.course_id, - self.select_for_update, ), Scope.user_info: UserInfoCache( self.user, - self.select_for_update, ), Scope.preferences: PreferencesCache( self.user, - self.select_for_update, ), Scope.user_state_summary: UserStateSummaryCache( self.course_id, - self.select_for_update, ), } self.add_descriptors_to_cache(descriptors) @@ -849,7 +823,7 @@ class FieldDataCache(object): the supplied descriptor. If depth is None, load all descendent StudentModules descriptor_filter is a function that accepts a descriptor and return wether the StudentModule should be cached - select_for_update: Flag indicating whether the rows should be locked until end of transaction + select_for_update: Ignored """ cache = FieldDataCache([], course_id, user, select_for_update, asides=asides) cache.add_descriptor_descendents(descriptor, depth, descriptor_filter) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 827f33921d..7de942c7ec 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -737,7 +737,6 @@ def load_single_xblock(request, user_id, course_id, usage_key_string): user, modulestore().get_item(usage_key), depth=0, - select_for_update=True ) instance = get_module(user, request, usage_key, field_data_cache, grade_bucket_type='xqueue') if instance is None: From 257660ed856b7ca8d88a70a0bb65375a02cdb5ca Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 28 Apr 2015 12:53:48 -0400 Subject: [PATCH 41/50] Move query-chunking into StudentModule and related ORM-objects --- lms/djangoapps/courseware/model_data.py | 33 ++----------------- lms/djangoapps/courseware/models.py | 42 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 7abcbc4450..a8bc8e1276 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -5,7 +5,6 @@ Classes to provide the LMS runtime data storage to XBlocks import json from abc import abstractmethod, ABCMeta from collections import defaultdict -from itertools import chain from .models import ( StudentModule, XModuleUserStateSummaryField, @@ -36,29 +35,6 @@ class InvalidWriteError(Exception): """ -def chunks(items, chunk_size): - """ - Yields the values from items in chunks of size chunk_size - """ - items = list(items) - return (items[i:i + chunk_size] for i in xrange(0, len(items), chunk_size)) - - -def _chunked_query(model_class, chunk_field, items, chunk_size=500, **kwargs): - """ - Queries model_class with `chunk_field` set to chunks of size `chunk_size`, - and all other parameters from `**kwargs`. - - This works around a limitation in sqlite3 on the number of parameters - that can be put into a single query. - """ - res = chain.from_iterable( - model_class.objects.filter(**dict([(chunk_field, chunk)] + kwargs.items())) - for chunk in chunks(items, chunk_size) - ) - return res - - def _all_usage_keys(descriptors, aside_types): """ Return a set of all usage_ids for the `descriptors` and for @@ -362,8 +338,7 @@ 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 = _chunked_query( - StudentModule, + query = StudentModule.objects.chunked_filter( 'module_state_key__in', _all_usage_keys(xblocks, aside_types), course_id=self.course_id, @@ -567,8 +542,7 @@ class UserStateSummaryCache(DjangoOrmFieldCache): aside_types (list of str): Asides to load field for (which annotate the supplied xblocks). """ - return _chunked_query( - XModuleUserStateSummaryField, + return XModuleUserStateSummaryField.objects.chunked_filter( 'usage_id__in', _all_usage_keys(xblocks, aside_types), field_name__in=set(field.name for field in fields), @@ -629,8 +603,7 @@ class PreferencesCache(DjangoOrmFieldCache): aside_types (list of str): Asides to load field for (which annotate the supplied xblocks). """ - return _chunked_query( - XModuleStudentPrefsField, + return XModuleStudentPrefsField.objects.chunked_filter( 'module_type__in', _all_block_types(xblocks, aside_types), student=self.user.pk, diff --git a/lms/djangoapps/courseware/models.py b/lms/djangoapps/courseware/models.py index 030eecbfdc..3c2cd72026 100644 --- a/lms/djangoapps/courseware/models.py +++ b/lms/djangoapps/courseware/models.py @@ -13,6 +13,7 @@ ASSUMPTIONS: modules have unique IDs, even across different module_types """ import logging +import itertools from django.contrib.auth.models import User from django.conf import settings @@ -29,10 +30,49 @@ from xmodule_django.models import CourseKeyField, LocationKeyField, BlockTypeKey log = logging.getLogger("edx.courseware") +def chunks(items, chunk_size): + """ + Yields the values from items in chunks of size chunk_size + """ + items = list(items) + return (items[i:i + chunk_size] for i in xrange(0, len(items), chunk_size)) + + +class ChunkingManager(models.Manager): + """ + :class:`~Manager` that adds an additional method :meth:`chunked_filter` to provide + the ability to make select queries with specific chunk sizes. + """ + def chunked_filter(self, chunk_field, items, **kwargs): + """ + Queries model_class with `chunk_field` set to chunks of size `chunk_size`, + and all other parameters from `**kwargs`. + + This works around a limitation in sqlite3 on the number of parameters + that can be put into a single query. + + Arguments: + chunk_field (str): The name of the field to chunk the query on. + items: The values for of chunk_field to select. This is chunked into ``chunk_size`` + chunks, and passed as the value for the ``chunk_field`` keyword argument to + :meth:`~Manager.filter`. This implies that ``chunk_field`` should be an + ``__in`` key. + chunk_size (int): The size of chunks to pass. Defaults to 500. + """ + chunk_size = kwargs.pop('chunk_size', 500) + res = itertools.chain.from_iterable( + self.filter(**dict([(chunk_field, chunk)] + kwargs.items())) + for chunk in chunks(items, chunk_size) + ) + return res + + class StudentModule(models.Model): """ Keeps student state for a particular module in a particular course. """ + objects = ChunkingManager() + MODEL_TAGS = ['course_id', 'module_type'] # For a homework problem, contains a JSON @@ -142,6 +182,8 @@ class XBlockFieldBase(models.Model): """ Base class for all XBlock field storage. """ + objects = ChunkingManager() + class Meta(object): # pylint: disable=missing-docstring abstract = True From 2e06e592ca05c39968309fbe5a8a8e6ac6662667 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 28 Apr 2015 14:53:40 -0400 Subject: [PATCH 42/50] Use DjangoXBlockUserStateClient to implement UserStateCache --- lms/djangoapps/courseware/model_data.py | 174 +++++++++--------- .../courseware/tests/test_model_data.py | 42 +++-- .../courseware/user_state_client.py | 78 ++++++-- lms/djangoapps/mobile_api/users/views.py | 2 +- 4 files changed, 172 insertions(+), 124 deletions(-) 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: From 2bfbd57ec18cd1a1c47f2c75d699bcba59b0a7d6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 29 Apr 2015 11:39:48 -0400 Subject: [PATCH 43/50] Add a test case for an XBlock that has no student state fields, but sets a score --- .../courseware/tests/test_module_render.py | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index bdac1694d6..fff12aae4d 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -78,6 +78,27 @@ class EmptyXModuleDescriptor(XModuleDescriptor): # pylint: disable=abstract-met module_class = EmptyXModule +class GradedStatelessXBlock(XBlock): + """ + This XBlock exists to test grade storage for blocks that don't store + student state in a scoped field. + """ + + @XBlock.json_handler + def set_score(self, json_data, suffix): # pylint: disable=unused-argument + """ + Set the score for this testing XBlock. + """ + self.runtime.publish( + self, + 'grade', + { + 'value': json_data['grade'], + 'max_value': 1 + } + ) + + @attr('shard_1') @ddt.ddt class ModuleRenderTestCase(ModuleStoreTestCase, LoginEnrollmentTestCase): @@ -335,8 +356,7 @@ class TestHandleXBlockCallback(ModuleStoreTestCase, LoginEnrollmentTestCase): self.course_key = self.create_toy_course() self.location = self.course_key.make_usage_key('chapter', 'Overview') self.toy_course = modulestore().get_course(self.course_key) - self.mock_user = UserFactory() - self.mock_user.id = 1 + self.mock_user = UserFactory.create() self.request_factory = RequestFactory() # Construct a mock module for the modulestore to return @@ -476,6 +496,33 @@ class TestHandleXBlockCallback(ModuleStoreTestCase, LoginEnrollmentTestCase): 'bad_dispatch', ) + @XBlock.register_temp_plugin(GradedStatelessXBlock, identifier='stateless_scorer') + def test_score_without_student_state(self): + course = CourseFactory.create() + block = ItemFactory.create(category='stateless_scorer', parent=course) + + request = self.request_factory.post( + 'dummy_url', + data=json.dumps({"grade": 0.75}), + content_type='application/json' + ) + request.user = self.mock_user + + response = render.handle_xblock_callback( + request, + unicode(course.id), + quote_slashes(unicode(block.scope_ids.usage_id)), + 'set_score', + '', + ) + self.assertEquals(response.status_code, 200) + student_module = StudentModule.objects.get( + student=self.mock_user, + module_state_key=block.scope_ids.usage_id, + ) + self.assertEquals(student_module.grade, 0.75) + self.assertEquals(student_module.max_grade, 1) + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_XBLOCK_VIEW_ENDPOINT': True}) def test_xblock_view_handler(self): args = [ From 7d2909c5b54adeda601f84c0d15585ddd4c95611 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 29 Apr 2015 13:10:37 -0400 Subject: [PATCH 44/50] Add contracts to DjangoXBlockUserStateClient interface methods --- .../courseware/user_state_client.py | 40 +++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index 811cecb59a..eae3e2c590 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -11,9 +11,13 @@ try: except ImportError: import json -from xblock.fields import Scope +from xblock.fields import Scope, ScopeBase from xblock_user_state.interface import XBlockUserStateClient from courseware.models import StudentModule +from contracts import contract, new_contract +from opaque_keys.edx.keys import UsageKey + +new_contract('UsageKey', UsageKey) class DjangoXBlockUserStateClient(XBlockUserStateClient): @@ -42,6 +46,12 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): def __init__(self, user): self.user = user + @contract( + username="basestring", + block_key=UsageKey, + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None" + ) def get(self, username, block_key, scope=Scope.user_state, fields=None): """ Retrieve the stored XBlock state for a single xblock usage. @@ -66,6 +76,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): return state + @contract(username="basestring", block_key=UsageKey, state="dict(basestring: *)", scope=ScopeBase) def set(self, username, block_key, state, scope=Scope.user_state): """ Set fields for a particular XBlock. @@ -79,6 +90,12 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): assert self.user.username == username self.set_many(username, {block_key: state}, scope) + @contract( + username="basestring", + block_key=UsageKey, + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None" + ) def delete(self, username, block_key, scope=Scope.user_state, fields=None): """ Delete the stored XBlock state for a single xblock usage. @@ -92,7 +109,12 @@ 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") + @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. @@ -140,7 +162,12 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): usage_key = student_module.module_state_key.map_into_course(student_module.course_id) yield (student_module, usage_key) - + @contract( + username="basestring", + block_keys="seq(UsageKey)|set(UsageKey)", + scope=ScopeBase, + fields="seq(basestring)|set(basestring)|None" + ) def get_many(self, username, block_keys, scope=Scope.user_state, fields=None): """ Retrieve the stored XBlock state for a single xblock usage. @@ -167,6 +194,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): state = json.loads(module.state) yield (usage_key, state) + @contract(username="basestring", block_keys_to_state="dict(UsageKey: dict(basestring: *))", scope=ScopeBase) def set_many(self, username, block_keys_to_state, scope=Scope.user_state): """ Set fields for a particular XBlock. @@ -208,6 +236,12 @@ 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 delete_many(self, username, block_keys, scope=Scope.user_state, fields=None): """ Delete the stored XBlock state for a many xblock usages. From 13dc390f7a323bdfb533649bedd6a36029c5cdcf Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Thu, 30 Apr 2015 14:23:25 -0400 Subject: [PATCH 45/50] Use a specialized method to clean up DjangoKeyValueStore --- lms/djangoapps/courseware/model_data.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 28719cc6a3..5532e0ef89 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -95,9 +95,7 @@ class DjangoKeyValueStore(KeyValueStore): self._field_data_cache = field_data_cache def get(self, key): - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) - + self._raise_unless_scope_is_allowed(key) return self._field_data_cache.get(key) def set(self, key, value): @@ -116,23 +114,23 @@ class DjangoKeyValueStore(KeyValueStore): """ for key in kv_dict: # Check key for validity - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) + self._raise_unless_scope_is_allowed(key) self._field_data_cache.set_many(kv_dict) def delete(self, key): - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) - + self._raise_unless_scope_is_allowed(key) self._field_data_cache.delete(key) def has(self, key): + self._raise_unless_scope_is_allowed(key) + return self._field_data_cache.has(key) + + def _raise_unless_scope_is_allowed(self, key): + """Raise an InvalidScopeError if key.scope is not in self._allowed_scopes.""" if key.scope not in self._allowed_scopes: raise InvalidScopeError(key) - return self._field_data_cache.has(key) - new_contract("DjangoKeyValueStore", DjangoKeyValueStore) new_contract("DjangoKeyValueStore_Key", DjangoKeyValueStore.Key) From 3e8631c214185381c0733eac277271c55ba837a6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 1 May 2015 16:20:02 -0400 Subject: [PATCH 46/50] Make the use of StudentModules explicit in variable names in user_state_client.py --- .../courseware/user_state_client.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lms/djangoapps/courseware/user_state_client.py b/lms/djangoapps/courseware/user_state_client.py index eae3e2c590..31137c9975 100644 --- a/lms/djangoapps/courseware/user_state_client.py +++ b/lms/djangoapps/courseware/user_state_client.py @@ -136,7 +136,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): } @contract(username="basestring", block_keys="seq(UsageKey)|set(UsageKey)") - def _get_field_objects(self, username, block_keys): + def _get_student_modules(self, username, block_keys): """ Retrieve the :class:`~StudentModule`s for the supplied ``username`` and ``block_keys``. @@ -186,7 +186,7 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported, not {}".format(scope)) - modules = self._get_field_objects(username, block_keys) + modules = self._get_student_modules(username, block_keys) for module, usage_key in modules: if module.state is None: state = {} @@ -256,12 +256,12 @@ class DjangoXBlockUserStateClient(XBlockUserStateClient): if scope != Scope.user_state: raise ValueError("Only Scope.user_state is supported") - student_modules = self._get_field_objects(username, block_keys) + student_modules = self._get_student_modules(username, block_keys) for student_module, _ in student_modules: if fields is None: - field_object.state = "{}" + student_module.state = "{}" else: - current_state = json.loads(field_object.state) + current_state = json.loads(student_module.state) for field in fields: if field in current_state: del current_state[field] @@ -295,13 +295,13 @@ 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) - for field_object, usage_key in field_objects: - if field_object.state is None: + student_modules = self._get_student_modules(username, block_keys) + for student_module, usage_key in student_modules: + if student_module.state is None: continue - for field in json.loads(field_object.state): - yield (usage_key, field, field_object.modified) + for field in json.loads(student_module.state): + yield (usage_key, field, student_module.modified) def get_history(self, username, block_key, scope=Scope.user_state): """We don't guarantee that history for many blocks will be fast.""" From bd1e9aa129c3f58f1888c2a6332bfb62c14a241b Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 18 May 2015 12:01:46 -0400 Subject: [PATCH 47/50] Record valid scopes when raising InvalidScopeError --- .../lib/xmodule/xmodule/modulestore/mongo/base.py | 15 ++++++++++++--- .../modulestore/split_mongo/split_mongo_kvs.py | 12 +++++++----- lms/djangoapps/courseware/model_data.py | 2 +- requirements/edx/github.txt | 2 +- 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index f005a7cded..c85c554e22 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -119,7 +119,10 @@ class MongoKeyValueStore(InheritanceKeyValueStore): elif key.scope == Scope.content: return self._data[key.field_name] else: - raise InvalidScopeError(key) + raise InvalidScopeError( + key, + (Scope.children, Scope.parent, Scope.settings, Scope.content), + ) def set(self, key, value): if key.scope == Scope.children: @@ -129,7 +132,10 @@ class MongoKeyValueStore(InheritanceKeyValueStore): elif key.scope == Scope.content: self._data[key.field_name] = value else: - raise InvalidScopeError(key) + raise InvalidScopeError( + key, + (Scope.children, Scope.settings, Scope.content), + ) def delete(self, key): if key.scope == Scope.children: @@ -141,7 +147,10 @@ class MongoKeyValueStore(InheritanceKeyValueStore): if key.field_name in self._data: del self._data[key.field_name] else: - raise InvalidScopeError(key) + raise InvalidScopeError( + key, + (Scope.children, Scope.settings, Scope.content), + ) def has(self, key): if key.scope in (Scope.children, Scope.parent): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index 2124732242..8720b7c3d9 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -18,6 +18,8 @@ class SplitMongoKVS(InheritanceKeyValueStore): known to the MongoModuleStore (data, children, and metadata) """ + VALID_SCOPES = (Scope.parent, Scope.children, Scope.settings, Scope.content) + @contract(parent="BlockUsageLocator | None") def __init__(self, definition, initial_values, default_values, parent, field_decorator=None): """ @@ -59,7 +61,7 @@ class SplitMongoKVS(InheritanceKeyValueStore): else: raise KeyError() else: - raise InvalidScopeError(key) + raise InvalidScopeError(key, self.VALID_SCOPES) if key.field_name in self._fields: field_value = self._fields[key.field_name] @@ -71,8 +73,8 @@ class SplitMongoKVS(InheritanceKeyValueStore): def set(self, key, value): # handle any special cases - if key.scope not in [Scope.children, Scope.settings, Scope.content]: - raise InvalidScopeError(key) + if key.scope not in self.VALID_SCOPES: + raise InvalidScopeError(key, self.VALID_SCOPES) if key.scope == Scope.content: self._load_definition() @@ -90,8 +92,8 @@ class SplitMongoKVS(InheritanceKeyValueStore): def delete(self, key): # handle any special cases - if key.scope not in [Scope.children, Scope.settings, Scope.content]: - raise InvalidScopeError(key) + if key.scope not in self.VALID_SCOPES: + raise InvalidScopeError(key, self.VALID_SCOPES) if key.scope == Scope.content: self._load_definition() diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 5532e0ef89..7dfa1002b2 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -129,7 +129,7 @@ class DjangoKeyValueStore(KeyValueStore): def _raise_unless_scope_is_allowed(self, key): """Raise an InvalidScopeError if key.scope is not in self._allowed_scopes.""" if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) + raise InvalidScopeError(key, self._allowed_scopes) new_contract("DjangoKeyValueStore", DjangoKeyValueStore) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index aa652261f2..5e6adfa953 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -29,7 +29,7 @@ git+https://github.com/pmitros/pyfs.git@96e1922348bfe6d99201b9512a9ed946c87b7e0b git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c4aee6e76b9abe61cc808 # Our libraries: --e git+https://github.com/edx/XBlock.git@1934a2978cdd3e2414486c74b76e3040ff1fb138#egg=XBlock +-e git+https://github.com/cpennington/XBlock.git@e9c4d441d1eaf7c9f176b873fe23e24c1152dba6#egg=XBlock -e git+https://github.com/edx/codejail.git@6b17c33a89bef0ac510926b1d7fea2748b73aadd#egg=codejail -e git+https://github.com/edx/js-test-tool.git@v0.1.6#egg=js_test_tool -e git+https://github.com/edx/event-tracking.git@0.2.0#egg=event-tracking From 827a8f815a3105e7a50e3ddfffca00b214ff1e9b Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 18 May 2015 14:01:26 -0400 Subject: [PATCH 48/50] Improve documentation of courseware.model_data --- lms/djangoapps/courseware/model_data.py | 37 +++++++++++++++++++------ 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 7dfa1002b2..e027a13c04 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -1,5 +1,24 @@ """ -Classes to provide the LMS runtime data storage to XBlocks +Classes to provide the LMS runtime data storage to XBlocks. + +:class:`DjangoKeyValueStore`: An XBlock :class:`~KeyValueStore` which + stores a subset of xblocks scopes as Django ORM objects. It wraps + :class:`~FieldDataCache` to provide an XBlock-friendly interface. + + +:class:`FieldDataCache`: A object which provides a read-through prefetch cache + of data to support XBlock fields within a limited set of scopes. + +The remaining classes in this module provide read-through prefetch cache implementations +for specific scopes. The individual classes provide the knowledge of what are the essential +pieces of information for each scope, and thus how to cache, prefetch, and create new field data +entries. + +UserStateCache: A cache for Scope.user_state +UserStateSummaryCache: A cache for Scope.user_state_summary +PreferencesCache: A cache for Scope.preferences +UserInfoCache: A cache for Scope.user_info +DjangoOrmFieldCache: A base-class for single-row-per-field caches. """ import json @@ -681,7 +700,7 @@ class UserInfoCache(DjangoOrmFieldCache): class FieldDataCache(object): """ A cache of django model objects needed to supply the data - for a module and its decendants + for a module and its descendants """ def __init__(self, descriptors, course_id, user, select_for_update=False, asides=None): ''' @@ -736,13 +755,13 @@ class FieldDataCache(object): def add_descriptor_descendents(self, descriptor, depth=None, descriptor_filter=lambda descriptor: True): """ - Add all descendents of `descriptor` to this FieldDataCache. + Add all descendants of `descriptor` to this FieldDataCache. Arguments: descriptor: An XModuleDescriptor - depth is the number of levels of descendent modules to load StudentModules for, in addition to - the supplied descriptor. If depth is None, load all descendent StudentModules - descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + depth is the number of levels of descendant modules to load StudentModules for, in addition to + the supplied descriptor. If depth is None, load all descendant StudentModules + descriptor_filter is a function that accepts a descriptor and return whether the field data should be cached """ @@ -782,9 +801,9 @@ class FieldDataCache(object): course_id: the course in the context of which we want StudentModules. user: the django user for whom to load modules. descriptor: An XModuleDescriptor - depth is the number of levels of descendent modules to load StudentModules for, in addition to - the supplied descriptor. If depth is None, load all descendent StudentModules - descriptor_filter is a function that accepts a descriptor and return wether the StudentModule + depth is the number of levels of descendant modules to load StudentModules for, in addition to + the supplied descriptor. If depth is None, load all descendant StudentModules + descriptor_filter is a function that accepts a descriptor and return whether the field data should be cached select_for_update: Ignored """ From 4625036d31c2a23888ae6196fbf8a1f20c65e39e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 20 May 2015 14:55:28 -0400 Subject: [PATCH 49/50] Standardize on triple double-quotes for docstrings --- lms/djangoapps/courseware/model_data.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index e027a13c04..7e2c508bad 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -703,7 +703,7 @@ class FieldDataCache(object): for a module and its descendants """ def __init__(self, descriptors, course_id, user, select_for_update=False, asides=None): - ''' + """ Find any courseware.models objects that are needed by any descriptor in descriptors. Attempts to minimize the number of queries to the database. Note: Only modules that have store_state = True or have shared @@ -715,7 +715,7 @@ class FieldDataCache(object): user: The user for which to cache data select_for_update: Ignored asides: The list of aside types to load, or None to prefetch no asides. - ''' + """ if asides is None: self.asides = [] else: @@ -823,7 +823,7 @@ class FieldDataCache(object): @contract(key=DjangoKeyValueStore.Key) def get(self, key): - ''' + """ Load the field value specified by `key`. Arguments: @@ -831,7 +831,7 @@ class FieldDataCache(object): Returns: The found value Raises: KeyError if key isn't found in the cache - ''' + """ if key.scope.user == UserScope.ONE and not self.user.is_anonymous(): # If we're getting user data, we expect that the key matches the From 122039ac58c97814a5116e7d41439dca44c258cb Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 20 May 2015 15:06:17 -0400 Subject: [PATCH 50/50] Add __len__ to FieldDataCache --- lms/djangoapps/courseware/model_data.py | 3 +++ lms/djangoapps/courseware/tests/test_model_data.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 7e2c508bad..c47ee65b41 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -951,3 +951,6 @@ class FieldDataCache(object): return None return self.cache[key.scope].last_modified(key) + + def __len__(self): + return sum(len(cache) for cache in self.cache.values()) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index df04376065..0f87c49de3 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -242,7 +242,7 @@ class TestMissingStudentModule(TestCase): 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, sum(len(cache) for cache in self.field_data_cache.cache.values())) + self.assertEquals(0, len(self.field_data_cache)) self.assertEquals(0, StudentModule.objects.all().count()) # We are updating a problem, so we write to courseware_studentmodulehistory