Merge pull request #11095 from edx/mushtaq/improve_get_item

Get only those block items which have their path to root

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

@@ -51,6 +51,7 @@ from xmodule.modulestore.edit_info import EditInfoRuntimeMixin
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError
 from xmodule.modulestore.inheritance import InheritanceMixin, inherit_metadata, InheritanceKeyValueStore
 from xmodule.modulestore.xml import CourseLocationManager
+from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES
 from xmodule.services import SettingsService
 
 log = logging.getLogger(__name__)
@@ -77,8 +78,6 @@ BLOCK_TYPES_WITH_CHILDREN = list(set(
 # at module level, cache one instance of OSFS per filesystem root.
 _OSFS_INSTANCE = {}
 
-_DETACHED_CATEGORIES = [name for name, __ in XBlock.load_tagged_classes("detached")]
-
 
 class MongoRevisionKey(object):
     """
@@ -269,7 +268,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin):
                     )
                     if parent_url:
                         parent = self._convert_reference_to_key(parent_url)
-                if not parent and category not in _DETACHED_CATEGORIES + ['course']:
+
+                if not parent and category not in DETACHED_XBLOCK_TYPES.union(['course']):
                     # try looking it up just-in-time (but not if we're working with a detached block).
                     parent = self.modulestore.get_parent_location(
                         as_published(location),
@@ -965,7 +965,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo
         of inherited metadata onto the item
         """
         category = item['location']['category']
-        apply_cached_metadata = category not in _DETACHED_CATEGORIES and \
+        apply_cached_metadata = category not in DETACHED_XBLOCK_TYPES and \
             not (category == 'course' and depth == 0)
         return apply_cached_metadata
 

common/lib/xmodule/xmodule/modulestore/split_mongo/split.py

@@ -83,6 +83,7 @@ from ..exceptions import ItemNotFoundError
 from .caching_descriptor_system import CachingDescriptorSystem
 from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, DuplicateKeyError
 from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope
+from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES
 from xmodule.error_module import ErrorDescriptor
 from collections import defaultdict
 from types import NoneType
@@ -1133,7 +1134,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 log.debug("Found more than one item for '{}'".format(usage_key))
             return items[0]
 
-    def get_items(self, course_locator, settings=None, content=None, qualifiers=None, **kwargs):
+    def get_items(self, course_locator, settings=None, content=None, qualifiers=None, include_orphans=True, **kwargs):
         """
         Returns:
             list of XModuleDescriptor instances for the matching items within the course with
@@ -1153,6 +1154,11 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
                 For substring matching pass a regex object.
                 For split,
                 you can search by ``edited_by``, ``edited_on`` providing a function testing limits.
+            include_orphans (boolean): Returns all items in a course, including orphans if present.
+                True - This would return all items irrespective of course in tree checking. It may fetch orphans
+                if present in the course.
+                False - if we want only those items which are in the course tree. This would ensure no orphans are
+                fetched.
         """
         if not isinstance(course_locator, CourseLocator) or course_locator.deprecated:
             # The supplied CourseKey is of the wrong type, so it can't possibly be stored in this modulestore.
@@ -1195,31 +1201,85 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase):
         # don't expect caller to know that children are in fields
         if 'children' in qualifiers:
             settings['children'] = qualifiers.pop('children')
+
+        # No need of these caches unless include_orphans is set to False
+        path_cache = None
+        parents_cache = None
+
+        if not include_orphans:
+            path_cache = {}
+            parents_cache = self.build_block_key_to_parents_mapping(course.structure)
+
         for block_id, value in course.structure['blocks'].iteritems():
             if _block_matches_all(value):
-                items.append(block_id)
+                if not include_orphans:
+                    if (  # pylint: disable=bad-continuation
+                        block_id.type in DETACHED_XBLOCK_TYPES or
+                        self.has_path_to_root(block_id, course, path_cache, parents_cache)
+                    ):
+                        items.append(block_id)
+                else:
+                    items.append(block_id)
 
         if len(items) > 0:
             return self._load_items(course, items, depth=0, **kwargs)
         else:
             return []
 
-    def has_path_to_root(self, block_key, course):
+    def build_block_key_to_parents_mapping(self, structure):
+        """
+        Given a structure, builds block_key to parents mapping for all block keys in structure
+        and returns it
+
+        :param structure: db json of course structure
+
+        :return dict: a dictionary containing mapping of block_keys against their parents.
+        """
+        children_to_parents = defaultdict(list)
+        for parent_key, value in structure['blocks'].iteritems():
+            for child_key in value.fields.get('children', []):
+                children_to_parents[child_key].append(parent_key)
+
+        return children_to_parents
+
+    def has_path_to_root(self, block_key, course, path_cache=None, parents_cache=None):
         """
         Check recursively if an xblock has a path to the course root
 
         :param block_key: BlockKey of the component whose path is to be checked
         :param course: actual db json of course from structures
+        :param path_cache: a dictionary that records which modules have a path to the root so that we don't have to
+        double count modules if we're computing this for a list of modules in a course.
+        :param parents_cache: a dictionary containing mapping of block_key to list of its parents. Optionally, this
+        should be built for course structure to make this method faster.
 
         :return Bool: whether or not component has path to the root
         """
 
-        xblock_parents = self._get_parents_from_structure(block_key, course.structure)
+        if path_cache and block_key in path_cache:
+            return path_cache[block_key]
+
+        if parents_cache is None:
+            xblock_parents = self._get_parents_from_structure(block_key, course.structure)
+        else:
+            xblock_parents = parents_cache[block_key]
+
         if len(xblock_parents) == 0 and block_key.type in ["course", "library"]:
             # Found, xblock has the path to the root
+            if path_cache is not None:
+                path_cache[block_key] = True
+
             return True
 
-        return any(self.has_path_to_root(xblock_parent, course) for xblock_parent in xblock_parents)
+        has_path = any(
+            self.has_path_to_root(xblock_parent, course, path_cache, parents_cache)
+            for xblock_parent in xblock_parents
+        )
+
+        if path_cache is not None:
+            path_cache[block_key] = has_path
+
+        return has_path
 
     def get_parent_location(self, locator, **kwargs):
         """

common/lib/xmodule/xmodule/modulestore/store_utilities.py

@@ -3,6 +3,9 @@ import logging
 from collections import namedtuple
 
 import uuid
+from xblock.core import XBlock
+
+DETACHED_XBLOCK_TYPES = set(name for name, __ in XBlock.load_tagged_classes("detached"))
 
 
 def _prefix_only_url_replace_regex(pattern):

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

@@ -42,6 +42,7 @@ from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, DI
 from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem
 from xmodule.modulestore.mixed import MixedModuleStore
 from xmodule.modulestore.search import path_to_location, navigation_index
+from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES
 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, LocationMixin, mock_tab_from_json
@@ -436,6 +437,68 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup):
                 revision=ModuleStoreEnum.RevisionOption.draft_preferred
             )
 
+    @ddt.data((ModuleStoreEnum.Type.split, 2, False), (ModuleStoreEnum.Type.mongo, 3, True))
+    @ddt.unpack
+    def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orphan_in_items):
+        """
+        Test `include_orphans` option helps in returning only those items which are present in course tree.
+        It tests that orphans are not fetched when calling `get_item` with `include_orphans`.
+
+        Params:
+            expected_items_in_tree:
+                Number of items that will be returned after `get_items` would be called with `include_orphans`.
+                In split, it would not get orphan items.
+                In mongo, it would still get orphan items because `include_orphans` would not have any impact on mongo
+                    modulestore which will return same number of items as called without `include_orphans` kwarg.
+
+            orphan_in_items:
+                When `get_items` is called with `include_orphans` kwarg, then check if an orphan is returned or not.
+                False when called in split modulestore because in split get_items is expected to not retrieve orphans
+                    now because of `include_orphans`.
+                True when called in mongo modulstore because `include_orphans` does not have any effect on mongo.
+        """
+        self.initdb(default_ms)
+
+        test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id)
+        course_key = test_course.id
+
+        items = self.store.get_items(course_key)
+        # Check items found are either course or about type
+        self.assertTrue(set(['course', 'about']).issubset(set([item.location.block_type for item in items])))
+        # Assert that about is a detached category found in get_items
+        self.assertIn(
+            [item.location.block_type for item in items if item.location.block_type == 'about'][0],
+            DETACHED_XBLOCK_TYPES
+        )
+        self.assertEqual(len(items), 2)
+
+        # Check that orphans are not found
+        orphans = self.store.get_orphans(course_key)
+        self.assertEqual(len(orphans), 0)
+
+        # Add an orphan to test course
+        orphan = course_key.make_usage_key('chapter', 'OrphanChapter')
+        self.store.create_item(self.user_id, orphan.course_key, orphan.block_type, block_id=orphan.block_id)
+
+        # Check that now an orphan is found
+        orphans = self.store.get_orphans(course_key)
+        self.assertIn(orphan, orphans)
+        self.assertEqual(len(orphans), 1)
+
+        # Check now `get_items` retrieves an extra item added above which is an orphan.
+        items = self.store.get_items(course_key)
+        self.assertIn(orphan, [item.location for item in items])
+        self.assertEqual(len(items), 3)
+
+        # Check now `get_items` with `include_orphans` kwarg does not retrieves an orphan block.
+        items_in_tree = self.store.get_items(course_key, include_orphans=False)
+
+        # Check that course and about blocks are found in get_items
+        self.assertTrue(set(['course', 'about']).issubset(set([item.location.block_type for item in items_in_tree])))
+        # Check orphan is found or not - this is based on mongo/split modulestore. It should be found in mongo.
+        self.assertEqual(orphan in [item.location for item in items_in_tree], orphan_in_items)
+        self.assertEqual(len(items_in_tree), expected_items_in_tree)
+
     # draft: get draft, get ancestors up to course (2-6), compute inheritance
     #    sends: update problem and then each ancestor up to course (edit info)
     # split: active_versions, definitions (calculator field), structures