Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make overriding branch settings thread-safe LMS-11125

Browse Source
This commit is contained in:
Nimisha Asthagiri
2014-07-21 01:32:33 -04:00
parent 9b8c47ab4e
commit 3eecc9639a
8 changed files with 177 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
common/lib/xmodule/xmodule/modulestore/__init__.py
View File

@@ -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):
'''

57
common/lib/xmodule/xmodule/modulestore/draft_and_published.py
View File

@@ -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):

17
common/lib/xmodule/xmodule/modulestore/mixed.py
View File

@@ -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

25
common/lib/xmodule/xmodule/modulestore/mongo/draft.py
View File

@@ -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,

81
common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
View File

@@ -1,7 +1,6 @@
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

16
common/lib/xmodule/xmodule/modulestore/tests/test_xml.py
View File

@@ -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

12
common/lib/xmodule/xmodule/modulestore/xml.py
View File

@@ -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

4
common/lib/xmodule/xmodule/modulestore/xml_importer.py
View File

@@ -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