Provide `set_many` methods for Lms and Mongo KeyValueStores

Refactor new set_many and update XBlock version number.

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 = []

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

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)

lms/djangoapps/courseware/tests/test_model_data.py

 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

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