From f0c9aa3916f448bacf32848d15f1d2d421cbe638 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Mon, 1 Jul 2013 12:27:11 -0400 Subject: [PATCH] Provide `set_many` methods for Lms and Mongo KeyValueStores Refactor new set_many and update XBlock version number. --- .../xmodule/xmodule/modulestore/mongo/base.py | 8 ++ .../xmodule/modulestore/tests/factories.py | 3 + lms/djangoapps/courseware/model_data.py | 58 ++++++++++++- .../courseware/tests/test_model_data.py | 81 +++++++++++++++++-- requirements/edx/github.txt | 2 +- 5 files changed, 143 insertions(+), 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index ab63243aaf..7fc903623a 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -105,6 +105,14 @@ class MongoKeyValueStore(KeyValueStore): else: raise InvalidScopeError(key.scope) + def set_many(self, update_dict): + """set_many method. Implementations should accept an `update_dict` of + key-value pairs, and set all the `keys` to the given `value`s.""" + # It appears that `set` simply updates an in-memory db, rather than calling down + # to a real db; need to figure out if this is the case. + for key, value in update_dict.iteritems(): + self.set(key, value) + def delete(self, key): if key.scope == Scope.children: self._children = [] diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index be705149f3..e442944805 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -53,6 +53,9 @@ class XModuleCourseFactory(Factory): for k, v in kwargs.iteritems(): setattr(new_course, k, v) + # Save the data we've just created before we update mongo datastore + new_course.save() + # Update the data in the mongo datastore store.save_xmodule(new_course) return new_course diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 790f1fd721..dec4805ced 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -12,9 +12,14 @@ from .models import ( XModuleStudentPrefsField, XModuleStudentInfoField ) +import logging + +from django.db import DatabaseError from xblock.runtime import KeyValueStore, InvalidScopeError -from xblock.core import Scope +from xblock.core import KeyValueMultiSaveError, Scope + +log = logging.getLogger(__name__) class InvalidWriteError(Exception): @@ -244,7 +249,7 @@ class ModelDataCache(object): module_state_key=key.block_scope_id.url(), defaults={'state': json.dumps({}), 'module_type': key.block_scope_id.category, - }, + }, ) elif key.scope == Scope.content: field_object, _ = XModuleContentField.objects.get_or_create( @@ -345,6 +350,55 @@ class LmsKeyValueStore(KeyValueStore): field_object.save() + def set_many(self, kv_dict): + """ + Provide a bulk save mechanism. + + `kv_dict`: A dictionary of dirty fields that maps + xblock.DbModel._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.field_name in self._descriptor_model_data: + raise InvalidWriteError("Not allowed to overwrite descriptor model data", field.field_name) + + if field.scope not in self._allowed_scopes: + raise InvalidScopeError(field.scope) + + # if the field is valid + field_object = self._model_data_cache.find_or_create(field) + # if this field_object isn't already in the dictionary + # add it + 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 + 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.error('Error saving fields %r', field_objects[field_object]) + raise KeyValueMultiSaveError(saved_fields) + def delete(self, key): if key.field_name in self._descriptor_model_data: raise InvalidWriteError("Not allowed to deleted descriptor model data", key.field_name) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index e961f80939..07aaa66473 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -1,5 +1,5 @@ import json -from mock import Mock +from mock import Mock, patch from functools import partial from courseware.model_data import LmsKeyValueStore, InvalidWriteError @@ -15,6 +15,8 @@ from courseware.tests.factories import StudentPrefsFactory, StudentInfoFactory from xblock.core import Scope, BlockScope from xmodule.modulestore import Location from django.test import TestCase +from django.db import DatabaseError +from xblock.core import KeyValueMultiSaveError def mock_field(scope, name): @@ -93,7 +95,7 @@ class TestStudentModuleStorage(TestCase): def setUp(self): self.desc_md = {} - student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value'})) + student_module = StudentModuleFactory(state=json.dumps({'a_field': 'a_value', 'b_field': 'b_value'})) self.user = student_module.student self.mdc = ModelDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) self.kvs = LmsKeyValueStore(self.desc_md, self.mdc) @@ -110,13 +112,13 @@ class TestStudentModuleStorage(TestCase): "Test that setting an existing user_state field changes the value" self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) - self.assertEquals({'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) + self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_set_missing_field(self): "Test that setting a new user_state field changes the value" self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) - self.assertEquals({'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) + self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_delete_existing_field(self): "Test that deleting an existing field removes it from the StudentModule" @@ -128,7 +130,7 @@ class TestStudentModuleStorage(TestCase): "Test that deleting a missing field from an existing StudentModule raises a KeyError" self.assertRaises(KeyError, self.kvs.delete, user_state_key('not_a_field')) self.assertEquals(1, StudentModule.objects.all().count()) - self.assertEquals({'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state)) + self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value'}, json.loads(StudentModule.objects.all()[0].state)) def test_has_existing_field(self): "Test that `has` returns True for existing fields in StudentModules" @@ -138,6 +140,35 @@ class TestStudentModuleStorage(TestCase): "Test that `has` returns False for missing fields in StudentModule" self.assertFalse(self.kvs.has(user_state_key('not_a_field'))) + def construct_kv_dict(self): + """ construct a kv_dict that can be passed to set_many """ + key1 = user_state_key('field_a') + key2 = user_state_key('field_b') + new_value = 'new value' + newer_value = 'newer value' + return {key1: new_value, key2: newer_value} + + def test_set_many(self): + """Test setting many fields that are scoped to Scope.user_state """ + kv_dict = self.construct_kv_dict() + self.kvs.set_many(kv_dict) + + for key in kv_dict: + self.assertEquals(self.kvs.get(key), kv_dict[key]) + + def test_set_many_failure(self): + """Test failures when setting many fields that are scoped to Scope.user_state """ + kv_dict = self.construct_kv_dict() + # because we're patching the underlying save, we need to ensure the + # fields are in the cache + for key in kv_dict: + self.kvs.set(key, 'test_value') + + 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) + class TestMissingStudentModule(TestCase): def setUp(self): @@ -176,6 +207,10 @@ class TestMissingStudentModule(TestCase): class StorageTestBase(object): + """ + A base class for that gets subclassed when testing each of the scopes. + + """ factory = None scope = None key_factory = None @@ -188,7 +223,10 @@ class StorageTestBase(object): else: self.user = UserFactory.create() self.desc_md = {} - self.mdc = ModelDataCache([mock_descriptor([mock_field(self.scope, 'existing_field')])], course_id, self.user) + self.mock_descriptor = mock_descriptor([ + mock_field(self.scope, 'existing_field'), + mock_field(self.scope, 'other_existing_field')]) + self.mdc = ModelDataCache([self.mock_descriptor], course_id, self.user) self.kvs = LmsKeyValueStore(self.desc_md, self.mdc) def test_set_and_get_existing_field(self): @@ -234,6 +272,37 @@ class StorageTestBase(object): "Test that `has` return False for an existing Storage Field" self.assertFalse(self.kvs.has(self.key_factory('missing_field'))) + def construct_kv_dict(self): + key1 = self.key_factory('existing_field') + key2 = self.key_factory('other_existing_field') + new_value = 'new value' + newer_value = 'newer value' + return {key1: new_value, key2: newer_value} + + def test_set_many(self): + """Test that setting many regular fields at the same time works""" + kv_dict = self.construct_kv_dict() + + self.kvs.set_many(kv_dict) + for key in kv_dict: + self.assertEquals(self.kvs.get(key), kv_dict[key]) + + def test_set_many_failure(self): + """Test that setting many regular fields with a DB error """ + kv_dict = self.construct_kv_dict() + for key in kv_dict: + self.kvs.set(key, 'test value') + + with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): + with self.assertRaises(KeyValueMultiSaveError) as exception_context: + self.kvs.set_many(kv_dict) + + exception = exception_context.exception + self.assertEquals(len(exception.saved_field_names), 1) + self.assertEquals(exception.saved_field_names[0], 'existing_field') + + + class TestSettingsStorage(StorageTestBase, TestCase): factory = SettingsFactory diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index f64568dc10..62a44f1307 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -8,6 +8,6 @@ -e git://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@4d8735e883#egg=XBlock +-e git+https://github.com/edx/XBlock.git@0b71db6ee6f9b216d0dd85cd76b55f2354b93dff#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.1.3#egg=diff_cover