Merge pull request #4494 from edx/nimisha/request-cache-for-branch-settings

Make overriding branch settings thread-safe LMS-11125

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -548,18 +548,6 @@ class ModuleStoreReadBase(ModuleStoreRead):
             raise ValueError(u"Cannot set default store to type {}".format(store_type))
         yield
 
-    @contextmanager
-    def branch_setting(self, branch_setting, course_id=None):
-        """
-        A context manager for temporarily setting a store's branch value
-        """
-        previous_branch_setting_func = getattr(self, 'branch_setting_func', None)
-        try:
-            self.branch_setting_func = lambda: branch_setting
-            yield
-        finally:
-            self.branch_setting_func = previous_branch_setting_func
-
 
 class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
     '''

common/lib/xmodule/xmodule/modulestore/draft_and_published.py

@@ -2,10 +2,64 @@
 This module provides an abstraction for Module Stores that support Draft and Published branches.
 """
 
+import threading
 from abc import ABCMeta, abstractmethod
+from contextlib import contextmanager
 from . import ModuleStoreEnum
 
-class ModuleStoreDraftAndPublished(object):
+
+class BranchSettingMixin(object):
+    """
+    A mixin to manage a module store's branch setting.
+    The order of override is (from higher precedence to lower):
+       1. thread-specific setting temporarily set using the branch_setting contextmanager
+       2. the return value of the branch_setting_func passed into this mixin's init method
+       3. the default branch setting being ModuleStoreEnum.Branch.published_only
+    """
+
+    def __init__(self, *args, **kwargs):
+        """
+        :param branch_setting_func: a function that returns the default branch setting for this object.
+            If not specified, ModuleStoreEnum.Branch.published_only is used as the default setting.
+        """
+        super(BranchSettingMixin, self).__init__(*args, **kwargs)
+        self.default_branch_setting_func = kwargs.pop(
+            'branch_setting_func',
+            lambda: ModuleStoreEnum.Branch.published_only
+        )
+
+        # cache the branch setting on a local thread to support a multi-threaded environment
+        self.thread_cache = threading.local()
+
+    @contextmanager
+    def branch_setting(self, branch_setting, course_id=None):  # pylint: disable=unused-argument
+        """
+        A context manager for temporarily setting a store's branch value on the current thread.
+        """
+        previous_thread_branch_setting = getattr(self.thread_cache, 'branch_setting', None)
+        try:
+            self.thread_cache.branch_setting = branch_setting
+            yield
+        finally:
+            self.thread_cache.branch_setting = previous_thread_branch_setting
+
+    def get_branch_setting(self, course_id=None):  # pylint: disable=unused-argument
+        """
+        Returns the current branch_setting on the store.
+
+        Returns the thread-local setting, if set.
+        Otherwise, returns the default value of the setting function set during the store's initialization.
+        """
+        # first check the thread-local cache
+        thread_local_branch_setting = getattr(self.thread_cache, 'branch_setting', None)
+        if thread_local_branch_setting:
+            return thread_local_branch_setting
+        else:
+            # return the default value
+            return self.default_branch_setting_func()
+
+
+class ModuleStoreDraftAndPublished(BranchSettingMixin):
     """
     A mixin for a read-write database backend that supports two branches, Draft and Published, with
     options to prefer Draft and fallback to Published.
@@ -14,7 +68,6 @@ class ModuleStoreDraftAndPublished(object):
 
     def __init__(self, *args, **kwargs):
         super(ModuleStoreDraftAndPublished, self).__init__(*args, **kwargs)
-        self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only)
 
     @abstractmethod
     def delete_item(self, location, user_id, revision=None, **kwargs):

common/lib/xmodule/xmodule/modulestore/mixed.py

@@ -142,7 +142,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         store = self._get_modulestore_for_courseid(usage_key.course_key)
         return store.get_item(usage_key, depth, **kwargs)
 
-    def get_items(self, course_key, settings=None, content=None, **kwargs):
+    def get_items(self, course_key, **kwargs):
         """
         Returns:
             list of XModuleDescriptor instances for the matching items within the course with
@@ -153,11 +153,12 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
 
         Args:
             course_key (CourseKey): the course identifier
-            settings (dict): fields to look for which have settings scope. Follows same syntax
-                and rules as kwargs below
-            content (dict): fields to look for which have content scope. Follows same syntax and
-                rules as kwargs below.
-            kwargs (key=value): what to look for within the course.
+            kwargs:
+                settings (dict): fields to look for which have settings scope. Follows same syntax
+                    and rules as kwargs below
+                content (dict): fields to look for which have content scope. Follows same syntax and
+                    rules as kwargs below.
+            additional kwargs (key=value): what to look for within the course.
                 Common qualifiers are ``category`` or any field name. if the target field is a list,
                 then it searches for the given value in the list not list equivalence.
                 Substring matching pass a regex object.
@@ -170,7 +171,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
             raise Exception("Must pass in a course_key when calling get_items()")
 
         store = self._get_modulestore_for_courseid(course_key)
-        return store.get_items(course_key, settings=settings, content=content, **kwargs)
+        return store.get_items(course_key, **kwargs)
 
     def get_courses(self):
         '''
@@ -519,7 +520,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
         A context manager for temporarily setting the branch value for the given course' store
         to the given branch_setting.  If course_id is None, the default store is used.
         """
-        store = self._get_modulestore_for_courseid(course_id)
+        store = self._verify_modulestore_support(course_id, 'branch_setting')
         with store.branch_setting(branch_setting, course_id):
             yield
 

common/lib/xmodule/xmodule/modulestore/mongo/draft.py

@@ -46,15 +46,6 @@ class DraftModuleStore(MongoModuleStore):
     This module also includes functionality to promote DRAFT modules (and their children)
     to published modules.
     """
-
-    def __init__(self, *args, **kwargs):
-        """
-        Args:
-            branch_setting_func: a function that returns the branch setting to use for this store's operations.
-                This should be an attribute from ModuleStoreEnum.Branch
-        """
-        super(DraftModuleStore, self).__init__(*args, **kwargs)
-
     def get_item(self, usage_key, depth=0, revision=None):
         """
         Returns an XModuleDescriptor instance for the item at usage_key.
@@ -103,7 +94,7 @@ class DraftModuleStore(MongoModuleStore):
         elif revision == ModuleStoreEnum.RevisionOption.draft_only:
             return get_draft()
 
-        elif self.branch_setting_func() == ModuleStoreEnum.Branch.published_only:
+        elif self.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
             return get_published()
 
         else:
@@ -137,7 +128,7 @@ class DraftModuleStore(MongoModuleStore):
         if revision == ModuleStoreEnum.RevisionOption.draft_only:
             return has_draft()
         elif revision == ModuleStoreEnum.RevisionOption.published_only \
-                or self.branch_setting_func() == ModuleStoreEnum.Branch.published_only:
+                or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
             return has_published()
         else:
             key = usage_key.to_deprecated_son(prefix='_id.')
@@ -282,7 +273,7 @@ class DraftModuleStore(MongoModuleStore):
         '''
         if revision is None:
             revision = ModuleStoreEnum.RevisionOption.published_only \
-                if self.branch_setting_func() == ModuleStoreEnum.Branch.published_only \
+                if self.get_branch_setting() == ModuleStoreEnum.Branch.published_only \
                 else ModuleStoreEnum.RevisionOption.draft_preferred
         return super(DraftModuleStore, self).get_parent_location(location, revision, **kwargs)
 
@@ -305,7 +296,7 @@ class DraftModuleStore(MongoModuleStore):
             super(DraftModuleStore, self).create_xmodule(location, definition_data, metadata, runtime, fields)
         )
 
-    def get_items(self, course_key, settings=None, content=None, revision=None, **kwargs):
+    def get_items(self, course_key, revision=None, **kwargs):
         """
         Performance Note: This is generally a costly operation, but useful for wildcard searches.
 
@@ -317,8 +308,6 @@ class DraftModuleStore(MongoModuleStore):
 
         Args:
             course_key (CourseKey): the course identifier
-            settings: not used
-            content: not used
             revision:
                 ModuleStoreEnum.RevisionOption.published_only - returns only Published items
                 ModuleStoreEnum.RevisionOption.draft_only - returns only Draft items
@@ -351,7 +340,7 @@ class DraftModuleStore(MongoModuleStore):
         if revision == ModuleStoreEnum.RevisionOption.draft_only:
             return draft_items()
         elif revision == ModuleStoreEnum.RevisionOption.published_only \
-                or self.branch_setting_func() == ModuleStoreEnum.Branch.published_only:
+                or self.get_branch_setting() == ModuleStoreEnum.Branch.published_only:
             return published_items([])
         else:
             draft_items = draft_items()
@@ -747,7 +736,7 @@ class DraftModuleStore(MongoModuleStore):
         for non_draft in to_process_non_drafts:
             to_process_dict[Location._from_deprecated_son(non_draft["_id"], course_key.run)] = non_draft
 
-        if self.branch_setting_func() == ModuleStoreEnum.Branch.draft_preferred:
+        if self.get_branch_setting() == ModuleStoreEnum.Branch.draft_preferred:
             # now query all draft content in another round-trip
             query = []
             for item in items:
@@ -801,7 +790,7 @@ class DraftModuleStore(MongoModuleStore):
         """
         Raises an exception if the current branch setting does not match the expected branch setting.
         """
-        actual_branch_setting = self.branch_setting_func()
+        actual_branch_setting = self.get_branch_setting()
         if actual_branch_setting != expected_branch_setting:
             raise InvalidBranchSetting(
                 expected_setting=expected_branch_setting,

common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py

 import pymongo
 from uuid import uuid4
 import ddt
-from mock import patch
 from importlib import import_module
 from collections import namedtuple
 import unittest
@@ -172,7 +171,8 @@ class TestMixedModuleStore(unittest.TestCase):
         def create_sub_tree(parent, block_info):
             block = self.store.create_child(
                 self.user_id, parent.location.version_agnostic(),
-                block_info.category, block_id=block_info.display_name
+                block_info.category, block_id=block_info.display_name,
+                fields={'display_name': block_info.display_name},
             )
             for tree in block_info.sub_tree:
                 create_sub_tree(block, tree)
@@ -802,6 +802,83 @@ class TestMixedModuleStore(unittest.TestCase):
             wiki_courses
         )
 
+    @ddt.data('draft')
+    def test_branch_setting(self, default_ms):
+        """
+        Test the branch_setting context manager
+        """
+        self.initdb(default_ms)
+        self._create_block_hierarchy()
+
+        problem_location = self.problem_x1a_1
+        problem_original_name = 'Problem_x1a_1'
+        problem_new_name = 'New Problem Name'
+        course_key = problem_location.course_key
+
+        def assertNumProblems(display_name, expected_number):
+            """
+            Asserts the number of problems with the given display name is the given expected number.
+            """
+            self.assertEquals(
+                len(self.store.get_items(course_key, settings={'display_name': display_name})),
+                expected_number
+            )
+
+        def assertProblemNameEquals(expected_display_name):
+            """
+            Asserts the display_name of the xblock at problem_location matches the given expected value.
+            """
+            # check the display_name of the problem
+            problem = self.store.get_item(problem_location)
+            self.assertEquals(problem.display_name, expected_display_name)
+
+            # there should be only 1 problem with the expected_display_name
+            assertNumProblems(expected_display_name, 1)
+
+        # verify Draft problem
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+            assertProblemNameEquals(problem_original_name)
+
+        # verify Published problem doesn't exist
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            with self.assertRaises(ItemNotFoundError):
+                self.store.get_item(problem_location)
+
+        # PUBLISH the problem
+        self.store.publish(problem_location, self.user_id)
+
+        # verify Published problem
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            assertProblemNameEquals(problem_original_name)
+
+        # verify Draft-preferred
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+            assertProblemNameEquals(problem_original_name)
+
+        # EDIT name
+        problem = self.store.get_item(problem_location)
+        problem.display_name = problem_new_name
+        self.store.update_item(problem, self.user_id)
+
+        # verify Draft problem has new name
+        with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+            assertProblemNameEquals(problem_new_name)
+
+        # verify Published problem still has old name
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            assertProblemNameEquals(problem_original_name)
+            # there should be no published problems with the new name
+            assertNumProblems(problem_new_name, 0)
+
+        # PUBLISH the problem
+        self.store.publish(problem_location, self.user_id)
+
+        # verify Published problem has new name
+        with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            assertProblemNameEquals(problem_new_name)
+            # there should be no published problems with the old name
+            assertNumProblems(problem_original_name, 0)
+
 
 #=============================================================================================================
 # General utils for not using django settings

common/lib/xmodule/xmodule/modulestore/tests/test_xml.py

@@ -107,3 +107,19 @@ class TestXMLModuleStore(unittest.TestCase):
             SlashSeparatedCourseKey('edX', 'toy', '2012_Fall'),
             locator_key_fields=SlashSeparatedCourseKey.KEY_FIELDS
         )
+
+    def test_branch_setting(self):
+        """
+        Test the branch setting context manager
+        """
+        store = XMLModuleStore(DATA_DIR, course_dirs=['toy'])
+        course_key = store.get_courses()[0]
+
+        # XML store allows published_only branch setting
+        with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key):
+            store.get_item(course_key.location)
+
+        # XML store does NOT allow draft_preferred branch setting
+        with self.assertRaises(ValueError):
+            with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
+                pass

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -13,6 +13,7 @@ from fs.osfs import OSFS
 from importlib import import_module
 from lxml import etree
 from path import path
+from contextlib import contextmanager
 
 from xmodule.error_module import ErrorDescriptor
 from xmodule.errortracker import make_error_tracker, exc_info_to_str
@@ -409,9 +410,6 @@ class XMLModuleStore(ModuleStoreReadBase):
 
         self.i18n_service = i18n_service
 
-        # The XML Module Store is a read-only store and only handles published content
-        self.branch_setting_func = lambda: ModuleStoreEnum.RevisionOption.published_only
-
         # If we are specifically asked for missing courses, that should
         # be an error.  If we are asked for "all" courses, find the ones
         # that have a course.xml. We sort the dirs in alpha order so we always
@@ -826,3 +824,11 @@ class XMLModuleStore(ModuleStoreReadBase):
         """
         return {ModuleStoreEnum.Type.xml: True}
 
+    @contextmanager
+    def branch_setting(self, branch_setting, course_id=None):  # pylint: disable=unused-argument
+        """
+        A context manager for temporarily setting the branch value for the store to the given branch_setting.
+        """
+        if branch_setting != ModuleStoreEnum.Branch.published_only:
+            raise ValueError(u"Cannot set branch setting to {} on a ReadOnly store".format(branch_setting))
+        yield

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -162,8 +162,8 @@ def import_from_xml(
     # method on XmlModuleStore.
     course_items = []
 
-    with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
-        for course_key in xml_module_store.modules.keys():
+    for course_key in xml_module_store.modules.keys():
+        with store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, course_key):
 
             if target_course_id is not None:
                 dest_course_id = target_course_id