Merge pull request #6657 from open-craft/fix-library-old-mongo-default

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

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)