diff --git a/cms/djangoapps/contentstore/management/commands/fix_not_found.py b/cms/djangoapps/contentstore/management/commands/fix_not_found.py index ec4bdc9dd3..87e3406183 100644 --- a/cms/djangoapps/contentstore/management/commands/fix_not_found.py +++ b/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: diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py index bd7098e780..e83f7d2f35 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_create_course.py +++ b/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()) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index b253a90935..def308c08b 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/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" # ) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index af938715ce..27bb81bf27 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/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) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index ba78d76ed4..96471c9915 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7a7887b91d..866d916d9c 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 4c4a4696df..ef19575d6f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index b751f8f4fc..b4156ccc35 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/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)