Skip to content

E
edx-platform
Commit 025f046b by Braden MacDonald
Options

Fix errors with libraries when old mongo is default (first) modulestore

parent e6e443c7
Showing with 100 additions and 60 deletions
cms/djangoapps/contentstore/management/commands/fix_not_found.py
......@@ -21,7 +21,7 @@ class Command(BaseCommand):
course_key = CourseKey.from_string(args[0])
# for now only support on split mongo
# pylint: disable=protected-access
owning_store = modulestore()._get_modulestore_for_courseid(course_key)
owning_store = modulestore()._get_modulestore_for_courselike(course_key)
if hasattr(owning_store, 'fix_not_found'):
owning_store.fix_not_found(course_key, ModuleStoreEnum.UserID.mgmt_command)
else:
......
cms/djangoapps/contentstore/management/commands/tests/test_create_course.py
......@@ -65,4 +65,4 @@ class TestCreateCourse(ModuleStoreTestCase):
"Could not find course in {}".format(store)
)
# pylint: disable=protected-access
self.assertEqual(store, modulestore()._get_modulestore_for_courseid(new_key).get_modulestore_type())
self.assertEqual(store, modulestore()._get_modulestore_for_courselike(new_key).get_modulestore_type())
cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py
......@@ -81,7 +81,7 @@ class TestMigrateToSplit(ModuleStoreTestCase):
# default mapping in mixed modulestore. I left the test here so we can debate what it ought to do.
# self.assertEqual(
# ModuleStoreEnum.Type.split,
# modulestore()._get_modulestore_for_courseid(new_key).get_modulestore_type(),
# modulestore()._get_modulestore_for_courselike(new_key).get_modulestore_type(),
# "Split is not the new default for the course"
# )
......
cms/djangoapps/contentstore/tests/utils.py
......@@ -316,7 +316,7 @@ class CourseTestCase(ModuleStoreTestCase):
course2_item_loc = course2_id.make_usage_key(course1_item_loc.block_type, course1_item_loc.block_id)
if course1_item_loc.block_type == 'course':
# mongo uses the run as the name, split uses 'course'
store = self.store._get_modulestore_for_courseid(course2_id) # pylint: disable=protected-access
store = self.store._get_modulestore_for_courselike(course2_id) # pylint: disable=protected-access
new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_loc.run
course2_item_loc = course2_item_loc.replace(name=new_name)
course2_item = self.store.get_item(course2_item_loc)
......
common/lib/xmodule/xmodule/modulestore/mixed.py
......@@ -157,35 +157,39 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
self.mappings[course_key] = store
self.modulestores.append(store)
def _clean_course_id_for_mapping(self, course_id):
def _clean_locator_for_mapping(self, locator):
"""
In order for mapping to work, the course_id must be minimal--no version, no branch--
In order for mapping to work, the locator must be minimal--no version, no branch--
as we never store one version or one branch in one ms and another in another ms.
:param course_id: the CourseKey
:param locator: the CourseKey
"""
if hasattr(course_id, 'version_agnostic'):
course_id = course_id.version_agnostic()
if hasattr(course_id, 'branch'):
course_id = course_id.replace(branch=None)
return course_id
if hasattr(locator, 'version_agnostic'):
locator = locator.version_agnostic()
if hasattr(locator, 'branch'):
locator = locator.replace(branch=None)
return locator
def _get_modulestore_for_courseid(self, course_id=None):
def _get_modulestore_for_courselike(self, locator=None):
"""
For a given course_id, look in the mapping table and see if it has been pinned
For a given locator, look in the mapping table and see if it has been pinned
to a particular modulestore
If course_id is None, returns the first (ordered) store as the default
If locator is None, returns the first (ordered) store as the default
"""
if course_id is not None:
course_id = self._clean_course_id_for_mapping(course_id)
mapping = self.mappings.get(course_id, None)
if locator is not None:
locator = self._clean_locator_for_mapping(locator)
mapping = self.mappings.get(locator, None)
if mapping is not None:
return mapping
else:
if isinstance(locator, LibraryLocator):
has_locator = lambda store: hasattr(store, 'has_library') and store.has_library(locator)
else:
has_locator = lambda store: store.has_course(locator)
for store in self.modulestores:
if store.has_course(course_id):
self.mappings[course_id] = store
if has_locator(store):
self.mappings[locator] = store
return store
# return the default store
......@@ -206,7 +210,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Some course_keys are used without runs. This function calls the corresponding
fill_in_run function on the appropriate modulestore.
"""
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
if not hasattr(store, 'fill_in_run'):
return course_key
return store.fill_in_run(course_key)
......@@ -215,7 +219,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
"""
Does the course include the xblock who's id is reference?
"""
store = self._get_modulestore_for_courseid(usage_key.course_key)
store = self._get_modulestore_for_courselike(usage_key.course_key)
return store.has_item(usage_key, **kwargs)
@strip_key
......@@ -223,7 +227,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
"""
see parent doc
"""
store = self._get_modulestore_for_courseid(usage_key.course_key)
store = self._get_modulestore_for_courselike(usage_key.course_key)
return store.get_item(usage_key, depth, **kwargs)
@strip_key
......@@ -255,7 +259,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
if not isinstance(course_key, CourseKey):
raise Exception("Must pass in a course_key when calling get_items()")
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
return store.get_items(course_key, **kwargs)
@strip_key
......@@ -267,7 +271,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
for store in self.modulestores:
# filter out ones which were fetched from earlier stores but locations may not be ==
for course in store.get_courses(**kwargs):
course_id = self._clean_course_id_for_mapping(course.id)
course_id = self._clean_locator_for_mapping(course.id)
if course_id not in courses:
# course is indeed unique. save it in result
courses[course_id] = course
......@@ -283,11 +287,11 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
if not hasattr(store, 'get_libraries'):
continue
# filter out ones which were fetched from earlier stores but locations may not be ==
for course in store.get_libraries(**kwargs):
course_id = self._clean_course_id_for_mapping(course.location)
if course_id not in libraries:
# course is indeed unique. save it in result
libraries[course_id] = course
for library in store.get_libraries(**kwargs):
library_id = self._clean_locator_for_mapping(library.location)
if library_id not in libraries:
# library is indeed unique. save it in result
libraries[library_id] = library
return libraries.values()
def make_course_key(self, org, course, run):
......@@ -315,7 +319,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
:param course_key: must be a CourseKey
"""
assert isinstance(course_key, CourseKey)
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
try:
return store.get_course(course_key, depth=depth, **kwargs)
except ItemNotFoundError:
......@@ -352,7 +356,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
False, do a case sensitive search
"""
assert isinstance(course_id, CourseKey)
store = self._get_modulestore_for_courseid(course_id)
store = self._get_modulestore_for_courselike(course_id)
return store.has_course(course_id, ignore_case, **kwargs)
def delete_course(self, course_key, user_id):
......@@ -360,7 +364,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
See xmodule.modulestore.__init__.ModuleStoreWrite.delete_course
"""
assert isinstance(course_key, CourseKey)
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
return store.delete_course(course_key, user_id)
@contract(asset_metadata='AssetMetadata', user_id='int|long', import_only=bool)
......@@ -376,7 +380,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Returns:
True if info save was successful, else False
"""
store = self._get_modulestore_for_courseid(asset_metadata.asset_id.course_key)
store = self._get_modulestore_for_courselike(asset_metadata.asset_id.course_key)
return store.save_asset_metadata(asset_metadata, user_id, import_only)
@contract(asset_metadata_list='list(AssetMetadata)', user_id='int|long', import_only=bool)
......@@ -395,7 +399,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
"""
if len(asset_metadata_list) == 0:
return True
store = self._get_modulestore_for_courseid(asset_metadata_list[0].asset_id.course_key)
store = self._get_modulestore_for_courselike(asset_metadata_list[0].asset_id.course_key)
return store.save_asset_metadata_list(asset_metadata_list, user_id, import_only)
@strip_key
......@@ -410,7 +414,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Returns:
asset metadata (AssetMetadata) -or- None if not found
"""
store = self._get_modulestore_for_courseid(asset_key.course_key)
store = self._get_modulestore_for_courselike(asset_key.course_key)
return store.find_asset_metadata(asset_key, **kwargs)
@strip_key
......@@ -433,7 +437,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Returns:
List of AssetMetadata objects.
"""
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
return store.get_all_asset_metadata(course_key, asset_type, start, maxresults, sort, **kwargs)
@contract(asset_key='AssetKey', user_id='int|long')
......@@ -448,7 +452,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Returns:
Number of asset metadata entries deleted (0 or 1)
"""
store = self._get_modulestore_for_courseid(asset_key.course_key)
store = self._get_modulestore_for_courselike(asset_key.course_key)
return store.delete_asset_metadata(asset_key, user_id)
@contract(source_course_key='CourseKey', dest_course_key='CourseKey', user_id='int|long')
......@@ -461,8 +465,8 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
dest_course_key (CourseKey): identifier of course to copy to
user_id (int|long): user copying the asset metadata
"""
source_store = self._get_modulestore_for_courseid(source_course_key)
dest_store = self._get_modulestore_for_courseid(dest_course_key)
source_store = self._get_modulestore_for_courselike(source_course_key)
dest_store = self._get_modulestore_for_courselike(dest_course_key)
if source_store != dest_store:
with self.bulk_operations(dest_course_key):
# Get all the asset metadata in the source course.
......@@ -492,7 +496,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
NotFoundError if no such item exists
AttributeError is attr is one of the build in attrs.
"""
store = self._get_modulestore_for_courseid(asset_key.course_key)
store = self._get_modulestore_for_courselike(asset_key.course_key)
return store.set_asset_metadata_attrs(asset_key, {attr: value}, user_id)
@contract(asset_key='AssetKey', attr_dict=dict, user_id='int|long')
......@@ -509,7 +513,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
NotFoundError if no such item exists
AttributeError is attr is one of the build in attrs.
"""
store = self._get_modulestore_for_courseid(asset_key.course_key)
store = self._get_modulestore_for_courselike(asset_key.course_key)
return store.set_asset_metadata_attrs(asset_key, attr_dict, user_id)
@strip_key
......@@ -517,7 +521,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
"""
returns the parent locations for a given location
"""
store = self._get_modulestore_for_courseid(location.course_key)
store = self._get_modulestore_for_courselike(location.course_key)
return store.get_parent_location(location, **kwargs)
def get_block_original_usage(self, usage_key):
......@@ -540,7 +544,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
"mongo" for old-style MongoDB backed courses,
"split" for new-style split MongoDB backed courses.
"""
return self._get_modulestore_for_courseid(course_id).get_modulestore_type()
return self._get_modulestore_for_courselike(course_id).get_modulestore_type()
@strip_key
def get_orphans(self, course_key, **kwargs):
......@@ -549,7 +553,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't
use children to point to their dependents.
"""
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
return store.get_orphans(course_key, **kwargs)
def get_errored_courses(self):
......@@ -630,10 +634,10 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
* copy the assets
* migrate the courseware
"""
source_modulestore = self._get_modulestore_for_courseid(source_course_id)
source_modulestore = self._get_modulestore_for_courselike(source_course_id)
# for a temporary period of time, we may want to hardcode dest_modulestore as split if there's a split
# to have only course re-runs go to split. This code, however, uses the config'd priority
dest_modulestore = self._get_modulestore_for_courseid(dest_course_id)
dest_modulestore = self._get_modulestore_for_courselike(dest_course_id)
if source_modulestore == dest_modulestore:
return source_modulestore.clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs)
......@@ -806,7 +810,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
PublishState.private - content is editable and not deployed to LMS
"""
course_id = xblock.scope_ids.usage_id.course_key
store = self._get_modulestore_for_courseid(course_id)
store = self._get_modulestore_for_courselike(course_id)
return store.has_published_version(xblock)
@strip_key
......@@ -853,7 +857,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
Raises NotImplementedError if the found store does not support the given method.
"""
store = self._get_modulestore_for_courseid(course_key)
store = self._get_modulestore_for_courselike(course_key)
if hasattr(store, method):
return store
else:
......@@ -905,7 +909,7 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase):
A context manager for notifying the store of bulk operations.
If course_id is None, the default store is used.
"""
store = self._get_modulestore_for_courseid(course_id)
store = self._get_modulestore_for_courselike(course_id)
with store.bulk_operations(course_id):
yield
......
common/lib/xmodule/xmodule/modulestore/split_mongo/split.py
......@@ -944,6 +944,21 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
course_index = self.get_course_index(course_id, ignore_case)
return CourseLocator(course_index['org'], course_index['course'], course_index['run'], course_id.branch) if course_index else None
def has_library(self, library_id, ignore_case=False, **kwargs):
'''
Does this library exist in this modulestore. This method does not verify that the branch &/or
version in the library_id exists.
Returns the library_id of the course if it was found, else None.
'''
if not isinstance(library_id, LibraryLocator):
return None
index = self.get_course_index(library_id, ignore_case)
if index:
return LibraryLocator(index['org'], index['course'], library_id.branch)
return None
def has_item(self, usage_key):
"""
Returns True if usage_key exists in its course. Returns false if
......
common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py
......@@ -29,7 +29,7 @@ if not settings.configured:
settings.configure()
from opaque_keys.edx.locations import SlashSeparatedCourseKey
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator
from xmodule.exceptions import InvalidVersionError
from xmodule.modulestore import ModuleStoreEnum
from xmodule.modulestore.draft_and_published import UnsupportedRevisionError
......@@ -38,7 +38,7 @@ from xmodule.modulestore.mixed import MixedModuleStore
from xmodule.modulestore.search import path_to_location
from xmodule.modulestore.tests.factories import check_mongo_calls, check_exact_number_of_calls, \
mongo_uses_error_check
from xmodule.modulestore.tests.utils import create_modulestore_instance
from xmodule.modulestore.tests.utils import create_modulestore_instance, LocationMixin
from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
from xmodule.tests import DATA_DIR, CourseComparisonTest
......@@ -68,7 +68,7 @@ class TestMixedModuleStore(CourseComparisonTest):
'default_class': DEFAULT_CLASS,
'fs_root': DATA_DIR,
'render_template': RENDER_TEMPLATE,
'xblock_mixins': (EditInfoMixin, InheritanceMixin),
'xblock_mixins': (EditInfoMixin, InheritanceMixin, LocationMixin),
}
DOC_STORE_CONFIG = {
'host': HOST,
......@@ -102,7 +102,7 @@ class TestMixedModuleStore(CourseComparisonTest):
'OPTIONS': {
'data_dir': DATA_DIR,
'default_class': 'xmodule.hidden_module.HiddenDescriptor',
'xblock_mixins': (EditInfoMixin, InheritanceMixin),
'xblock_mixins': modulestore_options['xblock_mixins'],
}
},
]
......@@ -309,9 +309,9 @@ class TestMixedModuleStore(CourseComparisonTest):
self.store.mappings = {}
course_key = self.course_locations[self.MONGO_COURSEID].course_key
with check_exact_number_of_calls(self.store.default_modulestore, 'has_course', 1):
self.assertEqual(self.store.default_modulestore, self.store._get_modulestore_for_courseid(course_key))
self.assertEqual(self.store.default_modulestore, self.store._get_modulestore_for_courselike(course_key)) # pylint: disable=protected-access
self.assertIn(course_key, self.store.mappings)
self.assertEqual(self.store.default_modulestore, self.store._get_modulestore_for_courseid(course_key))
self.assertEqual(self.store.default_modulestore, self.store._get_modulestore_for_courselike(course_key)) # pylint: disable=protected-access
@ddt.data(*itertools.product(
(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split),
......@@ -881,6 +881,27 @@ class TestMixedModuleStore(CourseComparisonTest):
course = self.store.get_item(self.course_locations[self.XML_COURSEID1])
self.assertEqual(course.id, self.course_locations[self.XML_COURSEID1].course_key)
@ddt.data('draft', 'split')
def test_get_library(self, default_ms):
"""
Test that create_library and get_library work regardless of the default modulestore.
Other tests of MixedModulestore support are in test_libraries.py but this one must
be done here so we can test the configuration where Draft/old is the first modulestore.
"""
self.initdb(default_ms)
with self.store.default_store(ModuleStoreEnum.Type.split): # The CMS also wraps create_library like this
library = self.store.create_library("org", "lib", self.user_id, {"display_name": "Test Library"})
library_key = library.location.library_key
self.assertIsInstance(library_key, LibraryLocator)
# Now load with get_library and make sure it works:
library = self.store.get_library(library_key)
self.assertEqual(library.location.library_key, library_key)
# Clear the mappings so we can test get_library code path without mapping set:
self.store.mappings.clear()
library = self.store.get_library(library_key)
self.assertEqual(library.location.library_key, library_key)
# notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split
# still only 2)
# Draft: get_parent
......@@ -1007,7 +1028,7 @@ class TestMixedModuleStore(CourseComparisonTest):
self._create_block_hierarchy()
self.store.publish(self.course.location, self.user_id)
mongo_store = self.store._get_modulestore_for_courseid(course_id) # pylint: disable=protected-access
mongo_store = self.store._get_modulestore_for_courselike(course_id) # pylint: disable=protected-access
# add another parent (unit) "vertical_x1b" for problem "problem_x1a_1"
mongo_store.collection.update(
self.vertical_x1b.to_deprecated_son('_id.'),
......@@ -1249,7 +1270,7 @@ class TestMixedModuleStore(CourseComparisonTest):
self.store.publish(self.course.location, self.user_id)
# test that problem "problem_x1a_1" has only one published parent
mongo_store = self.store._get_modulestore_for_courseid(course_id) # pylint: disable=protected-access
mongo_store = self.store._get_modulestore_for_courselike(course_id) # pylint: disable=protected-access
with self.store.branch_setting(ModuleStoreEnum.Branch.published_only, course_id):
parent = mongo_store.get_parent_location(self.problem_x1a_1)
self.assertEqual(parent, self.vertical_x1a)
......@@ -1809,7 +1830,7 @@ class TestMixedModuleStore(CourseComparisonTest):
self.assertEquals(self.store.default_modulestore.get_modulestore_type(), store_type)
# verify internal helper method
store = self.store._get_modulestore_for_courseid() # pylint: disable=protected-access
store = self.store._get_modulestore_for_courselike() # pylint: disable=protected-access
self.assertEquals(store.get_modulestore_type(), store_type)
# verify store used for creating a course
......
lms/djangoapps/courseware/tests/test_module_render.py
......@@ -413,7 +413,7 @@ class TestTOC(ModuleStoreTestCase):
factory = RequestFactory()
self.request = factory.get(chapter_url)
self.request.user = UserFactory()
self.modulestore = self.store._get_modulestore_for_courseid(self.course_key)
self.modulestore = self.store._get_modulestore_for_courselike(self.course_key) # pylint: disable=protected-access, attribute-defined-outside-init
with self.modulestore.bulk_operations(self.course_key):
with check_mongo_calls(num_finds, num_sends):
self.toy_course = self.store.get_course(self.toy_loc, depth=2)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment