Merge pull request #5020 from edx/hotfix/reduce-studio-mongo-calls

Hotfix - reduce number of Studio mongo calls

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -867,7 +867,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         # so we don't need to make an extra query to compute it.
         # set the branch to 'publish' in order to prevent extra lookups of draft versions
         with mongo_store.branch_setting(ModuleStoreEnum.Branch.published_only):
-            with check_mongo_calls(mongo_store, 4, 0):
+            with check_mongo_calls(4, 0):
                 course = mongo_store.get_course(course_id, depth=2)
 
             # make sure we pre-fetched a known sequential which should be at depth=2
@@ -879,7 +879,7 @@ class ContentStoreToyCourseTest(ContentStoreTestCase):
         # Now, test with the branch set to draft. No extra round trips b/c it doesn't go deep enough to get
         # beyond direct only categories
         with mongo_store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
-            with check_mongo_calls(mongo_store, 4, 0):
+            with check_mongo_calls(4, 0):
                 mongo_store.get_course(course_id, depth=2)
 
     def test_export_course_without_content_store(self):

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -201,10 +201,11 @@ class TestCourseListing(ModuleStoreTestCase):
 
         # Now count the db queries
         store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo)
-        with check_mongo_calls(store, USER_COURSES_COUNT):
+        with check_mongo_calls(USER_COURSES_COUNT):
             _accessible_courses_list_from_groups(self.request)
 
-        with check_mongo_calls(store, 1):
+        # TODO: LMS-11220: Document why this takes 6 calls
+        with check_mongo_calls(6):
             _accessible_courses_list(self.request)
 
     def test_get_course_list_with_same_course_id(self):

cms/djangoapps/contentstore/tests/test_import.py

@@ -157,15 +157,15 @@ class ContentStoreImportTest(ModuleStoreTestCase):
         store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo)
 
         # we try to refresh the inheritance tree for each update_item in the import
-        with check_exact_number_of_calls(store, store.refresh_cached_metadata_inheritance_tree, 28):
+        with check_exact_number_of_calls(store, "refresh_cached_metadata_inheritance_tree", 28):
 
             # _get_cached_metadata_inheritance_tree should be called only once
-            with check_exact_number_of_calls(store, store._get_cached_metadata_inheritance_tree, 1):
+            with check_exact_number_of_calls(store, "_get_cached_metadata_inheritance_tree", 1):
 
                 # with bulk-edit in progress, the inheritance tree should be recomputed only at the end of the import
                 # NOTE: On Jenkins, with memcache enabled, the number of calls here is only 1.
                 #       Locally, without memcache, the number of calls is actually 2 (once more during the publish step)
-                with check_number_of_calls(store, store._compute_metadata_inheritance_tree, 2):
+                with check_number_of_calls(store, "_compute_metadata_inheritance_tree", 2):
                     self.load_test_import_course()
 
     def test_rewrite_reference_list(self):

cms/djangoapps/contentstore/tests/utils.py

@@ -98,17 +98,23 @@ class CourseTestCase(ModuleStoreTestCase):
             nonstaff.is_authenticated = True
         return client, nonstaff
 
-    def populate_course(self):
+    def populate_course(self, branching=2):
         """
-        Add 2 chapters, 4 sections, 8 verticals, 16 problems to self.course (branching 2)
+        Add k chapters, k^2 sections, k^3 verticals, k^4 problems to self.course (where k = branching)
         """
         user_id = self.user.id
+        self.populated_usage_keys = {}
+
         def descend(parent, stack):
-            xblock_type = stack.pop(0)
-            for _ in range(2):
+            if not stack:
+                return
+
+            xblock_type = stack[0]
+            for _ in range(branching):
                 child = ItemFactory.create(category=xblock_type, parent_location=parent.location, user_id=user_id)
-                if stack:
-                    descend(child, stack)
+                print child.location
+                self.populated_usage_keys.setdefault(xblock_type, []).append(child.location)
+                descend(child, stack[1:])
 
         descend(self.course, ['chapter', 'sequential', 'vertical', 'problem'])
 

cms/djangoapps/contentstore/views/component.py

@@ -28,6 +28,7 @@ from opaque_keys.edx.keys import UsageKey
 
 from .access import has_course_access
 from django.utils.translation import ugettext as _
+from models.settings.course_grading import CourseGradingModel
 
 __all__ = ['OPEN_ENDED_COMPONENT_TYPES',
            'ADVANCED_COMPONENT_POLICY_KEY',

cms/djangoapps/contentstore/views/item.py

@@ -571,7 +571,7 @@ def _get_xblock(usage_key, user):
     """
     store = modulestore()
     try:
-        return store.get_item(usage_key)
+        return store.get_item(usage_key, depth=None)
     except ItemNotFoundError:
         if usage_key.category in CREATE_IF_NOT_FOUND:
             # Create a new one for certain categories only. Used for course info handouts.
@@ -596,6 +596,9 @@ def _get_module_info(xblock, rewrite_static_links=True):
             course_id=xblock.location.course_key
         )
 
+    # Pre-cache has changes for the entire course because we'll need it for the ancestor info
+    modulestore().has_changes(modulestore().get_course(xblock.location.course_key, depth=None))
+
     # Note that children aren't being returned until we have a use case.
     return create_xblock_info(xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=True)
 
@@ -756,7 +759,7 @@ def _compute_visibility_state(xblock, child_info, is_unit_with_changes):
         return VisibilityState.needs_attention
     is_unscheduled = xblock.start == DEFAULT_START_DATE
     is_live = datetime.now(UTC) > xblock.start
-    children = child_info and child_info['children']
+    children = child_info and child_info.get('children', [])
     if children and len(children) > 0:
         all_staff_only = True
         all_unscheduled = True

cms/djangoapps/contentstore/views/tests/test_item.py

 """Tests for items views."""
-import os
 import json
 from datetime import datetime, timedelta
 import ddt
-from unittest import skipUnless
 
 from mock import patch
 from pytz import UTC
@@ -26,7 +24,7 @@ from student.tests.factories import UserFactory
 from xmodule.capa_module import CapaDescriptor
 from xmodule.modulestore import ModuleStoreEnum
 from xmodule.modulestore.django import modulestore
-from xmodule.modulestore.tests.factories import ItemFactory
+from xmodule.modulestore.tests.factories import ItemFactory, check_mongo_calls
 from xmodule.x_module import STUDIO_VIEW, STUDENT_VIEW
 from xblock.exceptions import NoSuchHandlerError
 from opaque_keys.edx.keys import UsageKey, CourseKey
@@ -83,7 +81,8 @@ class ItemTest(CourseTestCase):
         return self.response_usage_key(resp)
 
 
-class GetItem(ItemTest):
+@ddt.ddt
+class GetItemTest(ItemTest):
     """Tests for '/xblock' GET url."""
 
     def _get_container_preview(self, usage_key):
@@ -100,6 +99,25 @@ class GetItem(ItemTest):
         self.assertIsNotNone(resources)
         return html, resources
 
+    @ddt.data(
+        (1, 21, 23, 35, 37),
+        (2, 22, 24, 38, 39),
+        (3, 23, 25, 41, 41),
+    )
+    @ddt.unpack
+    def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries):
+        self.populate_course(branching_factor)
+        # Retrieve it
+        with check_mongo_calls(chapter_queries):
+            self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['chapter'][-1]))
+        with check_mongo_calls(section_queries):
+            self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['sequential'][-1]))
+        with check_mongo_calls(unit_queries):
+            self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['vertical'][-1]))
+        with check_mongo_calls(problem_queries):
+            self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1]))
+
+
     def test_get_vertical(self):
         # Add a vertical
         resp = self.create_xblock(category='vertical')

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -24,6 +24,7 @@ from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from xblock.runtime import Mixologist
 from xblock.core import XBlock
+import functools
 
 log = logging.getLogger('edx.modulestore')
 
@@ -559,6 +560,36 @@ class ModuleStoreReadBase(ModuleStoreRead):
             raise ValueError(u"Cannot set default store to type {}".format(store_type))
         yield
 
+    @staticmethod
+    def memoize_request_cache(func):
+        """
+        Memoize a function call results on the request_cache if there's one. Creates the cache key by
+        joining the unicode of all the args with &; so, if your arg may use the default &, it may
+        have false hits
+        """
+        @functools.wraps(func)
+        def wrapper(self, *args, **kwargs):
+            if self.request_cache:
+                cache_key = '&'.join([hashvalue(arg) for arg in args])
+                if cache_key in self.request_cache.data.setdefault(func.__name__, {}):
+                    return self.request_cache.data[func.__name__][cache_key]
+
+                result = func(self, *args, **kwargs)
+
+                self.request_cache.data[func.__name__][cache_key] = result
+                return result
+            else:
+                return func(self, *args, **kwargs)
+        return wrapper
+
+def hashvalue(arg):
+    """
+    If arg is an xblock, use its location. otherwise just turn it into a string
+    """
+    if isinstance(arg, XBlock):
+        return unicode(arg.location)
+    else:
+        return unicode(arg)
 
 class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite):
     '''

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

@@ -589,13 +589,13 @@ class DraftModuleStore(MongoModuleStore):
         _internal([root_usage.to_deprecated_son() for root_usage in root_usages])
         self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe)
 
+    @MongoModuleStore.memoize_request_cache
     def has_changes(self, xblock):
         """
         Check if the xblock or its children have been changed since the last publish.
         :param xblock: xblock to check
         :return: True if the draft and published versions differ
         """
-
         # don't check children if this block has changes (is not public)
         if self.compute_publish_state(xblock) != PublishState.public:
             return True

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

@@ -10,6 +10,7 @@ from xmodule.modulestore.django import modulestore, clear_existing_modulestores
 from xmodule.modulestore import ModuleStoreEnum
 import datetime
 import pytz
+from request_cache.middleware import RequestCache
 from xmodule.tabs import CoursewareTab, CourseInfoTab, StaticTab, DiscussionTab, ProgressTab, WikiTab
 from xmodule.modulestore.tests.sample_courses import default_block_info_tree, TOY_BLOCK_INFO_TREE
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
@@ -275,6 +276,8 @@ class ModuleStoreTestCase(TestCase):
         # the next time they are accessed.
         # We do this at *both* setup and teardown just to be safe.
         clear_existing_modulestores()
+        # clear RequestCache to emulate its clearance after each http request.
+        RequestCache().clear_request_cache()
 
         # Call superclass implementation
         super(ModuleStoreTestCase, self)._post_teardown()

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

+import pprint
+import pymongo.message
+
 from factory import Factory, lazy_attribute_sequence, lazy_attribute
 from factory.containers import CyclicDefinitionError
 from uuid import uuid4
@@ -214,88 +217,83 @@ class ItemFactory(XModuleFactory):
 
 
 @contextmanager
-def check_exact_number_of_calls(object_with_method, method, num_calls, method_name=None):
+def check_exact_number_of_calls(object_with_method, method_name, num_calls):
     """
     Instruments the given method on the given object to verify the number of calls to the
     method is exactly equal to 'num_calls'.
     """
-    with check_number_of_calls(object_with_method, method, num_calls, num_calls, method_name):
+    with check_number_of_calls(object_with_method, method_name, num_calls, num_calls):
         yield
 
 
-@contextmanager
-def check_number_of_calls(object_with_method, method, maximum_calls, minimum_calls=1, method_name=None):
+def check_number_of_calls(object_with_method, method_name, maximum_calls, minimum_calls=1):
     """
     Instruments the given method on the given object to verify the number of calls to the method is
     less than or equal to the expected maximum_calls and greater than or equal to the expected minimum_calls.
     """
-    method_wrap = Mock(wraps=method)
-    wrap_patch = patch.object(object_with_method, method_name or method.__name__, method_wrap)
+    return check_sum_of_calls(object_with_method, [method_name], maximum_calls, minimum_calls)
+
 
-    try:
-        wrap_patch.start()
+@contextmanager
+def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1):
+    """
+    Instruments the given methods on the given object to verify that the total sum of calls made to the
+    methods falls between minumum_calls and maximum_calls.
+    """
+    mocks = {
+        method: Mock(wraps=getattr(object_, method))
+        for method in methods
+    }
+
+    with patch.multiple(object_, **mocks):
         yield
 
-    finally:
-        wrap_patch.stop()
+    call_count = sum(mock.call_count for mock in mocks.values())
+    calls = pprint.pformat({
+        method_name: mock.call_args_list
+        for method_name, mock in mocks.items()
+    })
+
+    # Assertion errors don't handle multi-line values, so pretty-print to std-out instead
+    if not minimum_calls <= call_count <= maximum_calls:
+        print "Expected between {} and {} calls, {} were made. Calls: {}".format(
+            minimum_calls,
+            maximum_calls,
+            call_count,
+            calls,
+        )
 
-        # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls
-        assert_greater_equal(method_wrap.call_count, minimum_calls)
+    # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls
+    assert_greater_equal(call_count, minimum_calls)
 
-        # now verify the number of actual calls is less than (or equal to) the expected maximum
-        assert_less_equal(method_wrap.call_count, maximum_calls)
+    # now verify the number of actual calls is less than (or equal to) the expected maximum
+    assert_less_equal(call_count, maximum_calls)
 
 
 @contextmanager
-def check_mongo_calls(mongo_store, num_finds=0, num_sends=None):
+def check_mongo_calls(num_finds=0, num_sends=None):
     """
     Instruments the given store to count the number of calls to find (incl find_one) and the number
     of calls to send_message which is for insert, update, and remove (if you provide num_sends). At the
     end of the with statement, it compares the counts to the num_finds and num_sends.
 
-    :param mongo_store: the MongoModulestore or subclass to watch or a SplitMongoModuleStore
     :param num_finds: the exact number of find calls expected
     :param num_sends: If none, don't instrument the send calls. If non-none, count and compare to
         the given int value.
     """
-    if mongo_store.get_modulestore_type() == ModuleStoreEnum.Type.mongo:
-        with check_exact_number_of_calls(mongo_store.collection, mongo_store.collection.find, num_finds):
-            if num_sends is not None:
-                with check_exact_number_of_calls(
-                    mongo_store.database.connection,
-                    mongo_store.database.connection._send_message,  # pylint: disable=protected-access
-                    num_sends,
-                ):
-                    yield
-            else:
+    with check_sum_of_calls(
+        pymongo.message,
+        ['query', 'get_more'],
+        num_finds,
+        num_finds
+    ):
+        if num_sends is not None:
+            with check_sum_of_calls(
+                pymongo.message,
+                ['insert', 'update', 'delete'],
+                num_sends,
+                num_sends
+            ):
                 yield
-    elif mongo_store.get_modulestore_type() == ModuleStoreEnum.Type.split:
-        collections = [
-            mongo_store.db_connection.course_index,
-            mongo_store.db_connection.structures,
-            mongo_store.db_connection.definitions,
-        ]
-        # could add else clause which raises exception or just rely on the below to suss that out
-        try:
-            find_wraps = []
-            wrap_patches = []
-            for collection in collections:
-                find_wrap = Mock(wraps=collection.find)
-                find_wraps.append(find_wrap)
-                wrap_patch = patch.object(collection, 'find', find_wrap)
-                wrap_patches.append(wrap_patch)
-                wrap_patch.start()
-            if num_sends is not None:
-                connection = mongo_store.db_connection.database.connection
-                with check_exact_number_of_calls(
-                    connection,
-                    connection._send_message,  # pylint: disable=protected-access
-                    num_sends,
-                ):
-                    yield
-            else:
-                yield
-        finally:
-            map(lambda wrap_patch: wrap_patch.stop(), wrap_patches)
-            call_count = sum([find_wrap.call_count for find_wrap in find_wraps])
-            assert_equal(call_count, num_finds)
+        else:
+            yield

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

@@ -270,8 +270,13 @@ class TestMixedModuleStore(unittest.TestCase):
             with self.assertRaises(DuplicateCourseError):
                 self.store.create_course('org_x', 'course_y', 'run_z', self.user_id)
 
+    # Draft:
+    #    - One lookup to locate an item that exists
+    #    - Two lookups to determine an item doesn't exist (one to check mongo, one to check split)
     # split has one lookup for the course and then one for the course items
-    @ddt.data(('draft', 1, 0), ('split', 2, 0))
+    # TODO: LMS-11220: Document why draft find count is [1, 1]
+    # TODO: LMS-11220: Document why split find count is [2, 2]
+    @ddt.data(('draft', [1, 1], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_has_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -279,15 +284,14 @@ class TestMixedModuleStore(unittest.TestCase):
 
         self.assertTrue(self.store.has_item(self.course_locations[self.XML_COURSEID1]))
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find.pop(0), max_send):
             self.assertTrue(self.store.has_item(self.problem_x1a_1))
 
         # try negative cases
         self.assertFalse(self.store.has_item(
             self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
         ))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find.pop(0), max_send):
             self.assertFalse(self.store.has_item(self.fake_location))
 
         # verify that an error is raised when the revision is not valid
@@ -296,7 +300,9 @@ class TestMixedModuleStore(unittest.TestCase):
 
     # draft is 2 to compute inheritance
     # split is 2 (would be 3 on course b/c it looks up the wiki_slug in definitions)
-    @ddt.data(('draft', 2, 0), ('split', 2, 0))
+    # TODO: LMS-11220: Document why draft find count is [2, 2]
+    # TODO: LMS-11220: Document why split find count is [3, 3]
+    @ddt.data(('draft', [2, 2], 0), ('split', [2, 2], 0))
     @ddt.unpack
     def test_get_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
@@ -304,8 +310,7 @@ class TestMixedModuleStore(unittest.TestCase):
 
         self.assertIsNotNone(self.store.get_item(self.course_locations[self.XML_COURSEID1]))
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find.pop(0), max_send):
             self.assertIsNotNone(self.store.get_item(self.problem_x1a_1))
 
         # try negative cases
@@ -313,7 +318,7 @@ class TestMixedModuleStore(unittest.TestCase):
             self.store.get_item(
                 self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem')
             )
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find.pop(0), max_send):
             with self.assertRaises(ItemNotFoundError):
                 self.store.get_item(self.fake_location)
 
@@ -322,6 +327,7 @@ class TestMixedModuleStore(unittest.TestCase):
             self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
     # compared to get_item for the course, draft asks for both draft and published
+    # TODO: LMS-11220: Document why split find count is 3
     @ddt.data(('draft', 8, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_items(self, default_ms, max_find, max_send):
@@ -334,9 +340,8 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(len(modules), 1)
         self.assertEqual(modules[0].location, course_locn)
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
         course_locn = self.course_locations[self.MONGO_COURSEID]
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             # NOTE: use get_course if you just want the course. get_items is expensive
             modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'})
         self.assertEqual(len(modules), 6)
@@ -352,7 +357,7 @@ class TestMixedModuleStore(unittest.TestCase):
     # split: 3 to get the course structure & the course definition (show_calculator is scope content)
     #  before the change. 1 during change to refetch the definition. 3 afterward (b/c it calls get_item to return the "new" object).
     #  2 sends to update index & structure (calculator is a setting field)
-    @ddt.data(('draft', 7, 5), ('split', 6, 2))
+    @ddt.data(('draft', 11, 5), ('split', 6, 2))
     @ddt.unpack
     def test_update_item(self, default_ms, max_find, max_send):
         """
@@ -368,12 +373,11 @@ class TestMixedModuleStore(unittest.TestCase):
             self.store.update_item(course, self.user_id)
 
         # now do it for a r/w db
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
         problem = self.store.get_item(self.problem_x1a_1)
         # if following raised, then the test is really a noop, change it
         self.assertNotEqual(problem.max_attempts, 2, "Default changed making test meaningless")
         problem.max_attempts = 2
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             problem = self.store.update_item(problem, self.user_id)
 
         self.assertEqual(problem.max_attempts, 2, "Update didn't persist")
@@ -434,7 +438,7 @@ class TestMixedModuleStore(unittest.TestCase):
         component = self.store.publish(component.location, self.user_id)
         self.assertFalse(self.store.has_changes(component))
 
-    @ddt.data(('draft', 7, 2), ('split', 13, 4))
+    @ddt.data(('draft', 8, 2), ('split', 13, 4))
     @ddt.unpack
     def test_delete_item(self, default_ms, max_find, max_send):
         """
@@ -446,14 +450,14 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(NotImplementedError):
             self.store.delete_item(self.xml_chapter_location, self.user_id)
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             self.store.delete_item(self.writable_chapter_location, self.user_id)
+
         # verify it's gone
         with self.assertRaises(ItemNotFoundError):
             self.store.get_item(self.writable_chapter_location)
 
-    @ddt.data(('draft', 8, 2), ('split', 13, 4))
+    @ddt.data(('draft', 9, 2), ('split', 13, 4))
     @ddt.unpack
     def test_delete_private_vertical(self, default_ms, max_find, max_send):
         """
@@ -484,8 +488,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertIn(vert_loc, course.children)
 
         # delete the vertical and ensure the course no longer points to it
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             self.store.delete_item(vert_loc, self.user_id)
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
         if hasattr(private_vert.location, 'version_guid'):
@@ -499,7 +502,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertFalse(self.store.has_item(leaf_loc))
         self.assertNotIn(vert_loc, course.children)
 
-    @ddt.data(('draft', 4, 1), ('split', 5, 2))
+    @ddt.data(('draft', 5, 1), ('split', 5, 2))
     @ddt.unpack
     def test_delete_draft_vertical(self, default_ms, max_find, max_send):
         """
@@ -528,24 +531,24 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.publish(private_vert.location, self.user_id)
         private_leaf.display_name = 'change me'
         private_leaf = self.store.update_item(private_leaf, self.user_id)
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
         # test succeeds if delete succeeds w/o error
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             self.store.delete_item(private_leaf.location, self.user_id)
 
-    @ddt.data(('draft', 2, 0), ('split', 3, 0))
+    # TODO: LMS-11220: Document why split find count is 5
+    # TODO: LMS-11220: Document why draft find count is 4
+    @ddt.data(('draft', 4, 0), ('split', 4, 0))
     @ddt.unpack
     def test_get_courses(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
         # we should have 3 total courses across all stores
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             courses = self.store.get_courses()
-        course_ids = [course.location for course in courses]
-        self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids))
-        self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids)
-        self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
-        self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
+            course_ids = [course.location for course in courses]
+            self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids))
+            self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids)
+            self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids)
+            self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids)
 
         with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred):
             draft_courses = self.store.get_courses(remove_branch=True)
@@ -588,8 +591,7 @@ class TestMixedModuleStore(unittest.TestCase):
         of getting an item whose scope.content fields are looked at.
         """
         self.initdb(default_ms)
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             course = self.store.get_item(self.course_locations[self.MONGO_COURSEID])
             self.assertEqual(course.id, self.course_locations[self.MONGO_COURSEID].course_key)
 
@@ -598,7 +600,8 @@ class TestMixedModuleStore(unittest.TestCase):
 
     # notice this doesn't test getting a public item via draft_preferred which draft would have 2 hits (split
     # still only 2)
-    @ddt.data(('draft', 1, 0), ('split', 2, 0))
+    # TODO: LMS-11220: Document why draft find count is 2
+    @ddt.data(('draft', 2, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_parent_locations(self, default_ms, max_find, max_send):
         """
@@ -607,10 +610,9 @@ class TestMixedModuleStore(unittest.TestCase):
         self.initdb(default_ms)
         self._create_block_hierarchy()
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             parent = self.store.get_parent_location(self.problem_x1a_1)
-        self.assertEqual(parent, self.vertical_x1a)
+            self.assertEqual(parent, self.vertical_x1a)
 
         parent = self.store.get_parent_location(self.xml_chapter_location)
         self.assertEqual(parent, self.course_locations[self.XML_COURSEID1])
@@ -692,7 +694,21 @@ class TestMixedModuleStore(unittest.TestCase):
             (child_to_delete_location, None, ModuleStoreEnum.RevisionOption.published_only),
         ])
 
-    @ddt.data(('draft', [10, 3], 0), ('split', [14, 6], 0))
+    # Mongo reads:
+    #    First location:
+    #        - count problem (1)
+    #        - For each level of ancestors: (5)
+    #          - Count ancestor
+    #          - retrieve ancestor
+    #        - compute inheritable data
+    #    Second location:
+    #        - load vertical
+    #        - load inheritance data
+
+    # TODO: LMS-11220: Document why draft send count is 5
+    # TODO: LMS-11220: Document why draft find count is 18
+    # TODO: LMS-11220: Document why split find count is 16
+    @ddt.data(('draft', [18, 5], 0), ('split', [14, 6], 0))
     @ddt.unpack
     def test_path_to_location(self, default_ms, num_finds, num_sends):
         """
@@ -713,7 +729,7 @@ class TestMixedModuleStore(unittest.TestCase):
 
             mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
             for location, expected in should_work:
-                with check_mongo_calls(mongo_store, num_finds.pop(0), num_sends):
+                with check_mongo_calls(num_finds.pop(0), num_sends):
                     self.assertEqual(path_to_location(self.store, location), expected)
 
         not_found = (
@@ -881,8 +897,7 @@ class TestMixedModuleStore(unittest.TestCase):
                 block_id=location.block_id
             )
 
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key)
         self.assertEqual(set(found_orphans), set(orphan_locations))
 
@@ -924,7 +939,9 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(self.user_id, block.subtree_edited_by)
         self.assertGreater(datetime.datetime.now(UTC), block.subtree_edited_on)
 
-    @ddt.data(('draft', 1, 0), ('split', 1, 0))
+    # TODO: LMS-11220: Document why split find count is 2
+    # TODO: LMS-11220: Document why draft find count is 2
+    @ddt.data(('draft', 2, 0), ('split', 2, 0))
     @ddt.unpack
     def test_get_courses_for_wiki(self, default_ms, max_find, max_send):
         """
@@ -941,8 +958,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertIn(self.course_locations[self.XML_COURSEID2].course_key, wiki_courses)
 
         # Test Mongo wiki
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             wiki_courses = self.store.get_courses_for_wiki('999')
         self.assertEqual(len(wiki_courses), 1)
         self.assertIn(
@@ -953,7 +969,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertEqual(len(self.store.get_courses_for_wiki('edX.simple.2012_Fall')), 0)
         self.assertEqual(len(self.store.get_courses_for_wiki('no_such_wiki')), 0)
 
-    @ddt.data(('draft', 2, 6), ('split', 7, 2))
+    @ddt.data(('draft', 3, 6), ('split', 7, 2))
     @ddt.unpack
     def test_unpublish(self, default_ms, max_find, max_send):
         """
@@ -971,8 +987,7 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertIsNotNone(published_xblock)
 
         # unpublish
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             self.store.unpublish(self.vertical_x1a, self.user_id)
 
         with self.assertRaises(ItemNotFoundError):
@@ -1000,8 +1015,7 @@ class TestMixedModuleStore(unittest.TestCase):
         # start off as Private
         item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state')
         item_location = item.location
-        mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID))
-        with check_mongo_calls(mongo_store, max_find, max_send):
+        with check_mongo_calls(max_find, max_send):
             self.assertEquals(self.store.compute_publish_state(item), PublishState.private)
 
         # Private -> Public

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

@@ -19,23 +19,35 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         # There are 12 created items and 7 parent updates
         # create course: finds: 1 to verify uniqueness, 1 to find parents
         # sends: 1 to create course, 1 to create overview
-        with check_mongo_calls(self.draft_mongo, 5, 2):
+        with check_mongo_calls(5, 2):
             super(TestPublish, self)._create_course(split=False)  # 2 inserts (course and overview)
 
         # with bulk will delay all inheritance computations which won't be added into the mongo_calls
         with self.draft_mongo.bulk_write_operations(self.old_course_key):
             # finds: 1 for parent to add child
             # sends: 1 for insert, 1 for parent (add child)
-            with check_mongo_calls(self.draft_mongo, 1, 2):
+            with check_mongo_calls(1, 2):
                 self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', split=False)
 
-            with check_mongo_calls(self.draft_mongo, 2, 2):
+            with check_mongo_calls(2, 2):
                 self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', split=False)
-            # update info propagation is 2 levels. create looks for draft and then published and then creates
-            with check_mongo_calls(self.draft_mongo, 8, 6):
+            # For each vertical (2) created:
+            #   - load draft
+            #   - load non-draft
+            #   - get last error
+            #   - load parent
+            #   - load inheritable data
+            with check_mongo_calls(10, 6):
                 self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', split=False)
                 self._create_item('vertical', 'Vert2', {}, {'display_name': 'Vertical 2'}, 'chapter', 'Chapter1', split=False)
-            with check_mongo_calls(self.draft_mongo, 20, 12):
+            # For each (4) item created
+            #   - load draft
+            #   - load non-draft
+            #   - get last error
+            #   - load parent
+            #   - load inheritable data
+            #   - load parent
+            with check_mongo_calls(24, 12):
                 self._create_item('html', 'Html1', "<p>Goodbye</p>", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False)
                 self._create_item(
                     'discussion', 'Discussion1',
@@ -63,7 +75,7 @@ class TestPublish(SplitWMongoCourseBoostrapper):
                     split=False
                 )
 
-            with check_mongo_calls(self.draft_mongo, 0, 2):
+            with check_mongo_calls(0, 2):
                 # 2 finds b/c looking for non-existent parents
                 self._create_item('static_tab', 'staticuno', "<p>tab</p>", {'display_name': 'Tab uno'}, None, None, split=False)
                 self._create_item('course_info', 'updates', "<ol><li><h2>Sep 22</h2><p>test</p></li></ol>", {}, None, None, split=False)
@@ -76,10 +88,11 @@ class TestPublish(SplitWMongoCourseBoostrapper):
         vert_location = self.old_course_key.make_usage_key('vertical', block_id='Vert1')
         item = self.draft_mongo.get_item(vert_location, 2)
         # Vert1 has 3 children; so, publishes 4 nodes which may mean 4 inserts & 1 bulk remove
+        # TODO: LMS-11220: Document why find count is 25
         # 25-June-2014 find calls are 19. Probably due to inheritance recomputation?
         # 02-July-2014 send calls are 7. 5 from above, plus 2 for updating subtree edit info for Chapter1 and course
         #              find calls are 22. 19 from above, plus 3 for finding the parent of Vert1, Chapter1, and course
-        with check_mongo_calls(self.draft_mongo, 22, 7):
+        with check_mongo_calls(25, 7):
             self.draft_mongo.publish(item.location, self.user_id)
 
         # verify status

common/lib/xmodule/xmodule/modulestore/xml_importer.py

@@ -607,7 +607,7 @@ def _import_course_draft(
                     _import_module(descriptor)
 
                 except Exception:
-                    logging.exception('There while importing draft descriptor %s', descriptor)
+                    logging.exception('while importing draft descriptor %s', descriptor)
 
 
 def allowed_metadata_by_category(category):

lms/djangoapps/courseware/tests/test_module_render.py

@@ -326,7 +326,7 @@ class TestTOC(ModuleStoreTestCase):
         self.request = factory.get(chapter_url)
         self.request.user = UserFactory()
         self.modulestore = self.store._get_modulestore_for_courseid(self.course_key)
-        with check_mongo_calls(self.modulestore, num_finds, num_sends):
+        with check_mongo_calls(num_finds, num_sends):
             self.toy_course = self.store.get_course(self.toy_loc, depth=2)
             self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
                 self.toy_loc, self.request.user, self.toy_course, depth=2)
@@ -352,7 +352,7 @@ class TestTOC(ModuleStoreTestCase):
                             'format': '', 'due': None, 'active': False}],
                           'url_name': 'secret:magic', 'display_name': 'secret:magic'}])
 
-            with check_mongo_calls(self.modulestore, 0, 0):
+            with check_mongo_calls(0, 0):
                 actual = render.toc_for_course(
                     self.request.user, self.request, self.toy_course, self.chapter, None, self.field_data_cache
                 )