Count & compare db hits between old and split mongo

LMS-11090

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

@@ -9,7 +9,7 @@ from xblock.core import XBlock
 from xmodule.tabs import StaticTab
 from decorator import contextmanager
 from mock import Mock, patch
-from nose.tools import assert_less_equal, assert_greater_equal
+from nose.tools import assert_less_equal, assert_greater_equal, assert_equal
 
 
 class Dummy(object):
@@ -259,11 +259,12 @@ def check_mongo_calls(mongo_store, num_finds=0, num_sends=None):
     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
+    :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:
                 with check_exact_number_of_calls(
@@ -274,3 +275,33 @@ def check_mongo_calls(mongo_store, num_finds=0, num_sends=None):
                     yield
             else:
                 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:
+                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)

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

@@ -19,6 +19,7 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
 # before importing the module
 # TODO remove this import and the configuration -- xmodule should not depend on django!
 from django.conf import settings
+from xmodule.modulestore.tests.factories import check_mongo_calls
 if not settings.configured:
     settings.configure()
 from xmodule.modulestore.mixed import MixedModuleStore
@@ -246,33 +247,50 @@ class TestMixedModuleStore(unittest.TestCase):
             SlashSeparatedCourseKey('foo', 'bar', '2012_Fall')), mongo_ms_type
         )
 
-    @ddt.data('draft', 'split')
-    def test_has_item(self, default_ms):
+    # split has one lookup for the course and then one for the course items
+    @ddt.data(('draft', 1, 0), ('split', 2, 0))
+    @ddt.unpack
+    def test_has_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
-        for course_locn in self.course_locations.itervalues():  # pylint: disable=maybe-no-member
-            self.assertTrue(self.store.has_item(course_locn))
+        self._create_block_hierarchy()
+
+        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):
+            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):
             self.assertFalse(self.store.has_item(self.fake_location))
 
         # verify that an error is raised when the revision is not valid
         with self.assertRaises(UnsupportedRevisionError):
             self.store.has_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
-    @ddt.data('draft', 'split')
-    def test_get_item(self, default_ms):
+    # 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))
+    @ddt.unpack
+    def test_get_item(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
-        for course_locn in self.course_locations.itervalues():  # pylint: disable=maybe-no-member
-            self.assertIsNotNone(self.store.get_item(course_locn))
+        self._create_block_hierarchy()
+
+        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):
+            self.assertIsNotNone(self.store.get_item(self.problem_x1a_1))
 
         # try negative cases
         with self.assertRaises(ItemNotFoundError):
             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 self.assertRaises(ItemNotFoundError):
                 self.store.get_item(self.fake_location)
 
@@ -280,16 +298,26 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(UnsupportedRevisionError):
             self.store.get_item(self.fake_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred)
 
-    @ddt.data('draft', 'split')
-    def test_get_items(self, default_ms):
+    # compared to get_item for the course, draft asks for both draft and published
+    @ddt.data(('draft', 8, 0), ('split', 2, 0))
+    @ddt.unpack
+    def test_get_items(self, default_ms, max_find, max_send):
         self.initdb(default_ms)
-        for course_locn in self.course_locations.itervalues():  # pylint: disable=maybe-no-member
-            locn = course_locn.course_key
+        self._create_block_hierarchy()
+
+        course_locn = self.course_locations[self.XML_COURSEID1]
         # NOTE: use get_course if you just want the course. get_items is expensive
-            modules = self.store.get_items(locn, category='course')
+        modules = self.store.get_items(course_locn.course_key, category='course')
         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):
+            # NOTE: use get_course if you just want the course. get_items is expensive
+            modules = self.store.get_items(course_locn.course_key, category='problem')
+        self.assertEqual(len(modules), 6)
+
         # verify that an error is raised when the revision is not valid
         with self.assertRaises(UnsupportedRevisionError):
             self.store.get_items(
@@ -297,26 +325,35 @@ class TestMixedModuleStore(unittest.TestCase):
                 revision=ModuleStoreEnum.RevisionOption.draft_preferred
             )
 
-    @ddt.data('draft', 'split')
-    def test_update_item(self, default_ms):
+    # draft: 2 to look in draft and then published and then 5 for updating ancestors.
+    # 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', 7, 2))
+    @ddt.unpack
+    def test_update_item(self, default_ms, max_find, max_send):
         """
         Update should fail for r/o dbs and succeed for r/w ones
         """
         self.initdb(default_ms)
+        self._create_block_hierarchy()
         course = self.store.get_course(self.course_locations[self.XML_COURSEID1].course_key)
         # if following raised, then the test is really a noop, change it
         self.assertFalse(course.show_calculator, "Default changed making test meaningless")
         course.show_calculator = True
         with self.assertRaises(NotImplementedError):  # ensure it doesn't allow writing
             self.store.update_item(course, self.user_id)
+
         # now do it for a r/w db
-        course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key)
+        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.assertFalse(course.show_calculator, "Default changed making test meaningless")
-        course.show_calculator = True
-        self.store.update_item(course, self.user_id)
-        course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key)
-        self.assertTrue(course.show_calculator)
+        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):
+            problem = self.store.update_item(problem, self.user_id)
+
+        self.assertEqual(problem.max_attempts, 2, "Update didn't persist")
 
     @ddt.data('draft', 'split')
     def test_has_changes_direct_only(self, default_ms):
@@ -374,20 +411,33 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.publish(component.location, self.user_id)
         self.assertFalse(self.store.has_changes(component.location))
 
-    @ddt.data('draft', 'split')
-    def test_delete_item(self, default_ms):
+    @ddt.data(('draft', 7, 2), ('split', 3, 2))
+    @ddt.unpack
+    def test_delete_item(self, default_ms, max_find, max_send):
         """
         Delete should reject on r/o db and work on r/w one
         """
         self.initdb(default_ms)
+
         # r/o try deleting the chapter (is here to ensure it can't be deleted)
         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):
             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', 3, 2))
+    @ddt.unpack
+    def test_delete_private_vertical(self, default_ms, max_find, max_send):
+        """
+        Because old mongo treated verticals as the first layer which could be draft, it has some interesting
+        behavioral properties which this deletion test gets at.
+        """
+        self.initdb(default_ms)
         # create and delete a private vertical with private children
         private_vert = self.store.create_child(
             # don't use course_location as it may not be the repr
@@ -411,8 +461,9 @@ class TestMixedModuleStore(unittest.TestCase):
         course = self.store.get_course(self.course_locations[self.MONGO_COURSEID].course_key, 0)
         self.assertIn(vert_loc, course.children)
 
-        # update the component to force it to draft w/o forcing the unit to draft
         # 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):
             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'):
@@ -426,6 +477,14 @@ class TestMixedModuleStore(unittest.TestCase):
         self.assertFalse(self.store.has_item(leaf_loc))
         self.assertNotIn(vert_loc, course.children)
 
+    @ddt.data(('draft', 4, 1), ('split', 3, 2))
+    @ddt.unpack
+    def test_delete_draft_vertical(self, default_ms, max_find, max_send):
+        """
+        Test deleting a draft vertical which has a published version.
+        """
+        self.initdb(default_ms)
+
         # reproduce bug STUD-1965
         # create and delete a private vertical with private children
         private_vert = self.store.create_child(
@@ -447,13 +506,18 @@ class TestMixedModuleStore(unittest.TestCase):
         self.store.publish(private_vert.location.version_agnostic(), 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):
             self.store.delete_item(private_leaf.location, self.user_id)
 
-    @ddt.data('draft', 'split')
-    def test_get_courses(self, default_ms):
+    @ddt.data(('draft', 3, 0), ('split', 6, 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):
             courses = self.store.get_courses()
         course_ids = [
             course.location.version_agnostic()
@@ -489,20 +553,39 @@ class TestMixedModuleStore(unittest.TestCase):
         with self.assertRaises(AttributeError):
             xml_store.create_course("org", "course", "run", self.user_id)
 
-    @ddt.data('draft', 'split')
-    def test_get_course(self, default_ms):
+    # draft is 2 to compute inheritance
+    # split is 3 b/c it gets the definition to check whether wiki is set
+    @ddt.data(('draft', 2, 0), ('split', 3, 0))
+    @ddt.unpack
+    def test_get_course(self, default_ms, max_find, max_send):
+        """
+        This test is here for the performance comparison not functionality. It tests the performance
+        of getting an item whose scope.content fields are looked at.
+        """
         self.initdb(default_ms)
-        for course_location in self.course_locations.itervalues():  # pylint: disable=maybe-no-member
-            # NOTE: use get_course if you just want the course. get_items is expensive
-            course = self.store.get_course(course_location.course_key)
-            self.assertIsNotNone(course)
-            self.assertEqual(course.id, course_location.course_key)
-
-    @ddt.data('draft', 'split')
-    def test_get_parent_locations(self, 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):
+            course = self.store.get_item(self.course_locations[self.MONGO_COURSEID])
+            self.assertEqual(course.id, self.course_locations[self.MONGO_COURSEID].course_key)
+
+        course = self.store.get_item(self.course_locations[self.XML_COURSEID1])
+        self.assertEqual(course.id, self.course_locations[self.XML_COURSEID1].course_key)
+
+    # 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))
+    @ddt.unpack
+    def test_get_parent_locations(self, default_ms, max_find, max_send):
+        """
+        Test a simple get parent for a direct only category (i.e, always published)
+        """
         self.initdb(default_ms)
-        parent = self.store.get_parent_location(self.writable_chapter_location)
-        self.assertEqual(parent, self.course_locations[self.MONGO_COURSEID])
+        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):
+            parent = self.store.get_parent_location(self.problem_x1a_1)
+        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])
@@ -669,8 +752,12 @@ class TestMixedModuleStore(unittest.TestCase):
         # It does not discard the child vertical, even though that child is a draft (with no published version)
         self.assertEqual(1, len(reverted_parent.children))
 
-    @ddt.data('draft', 'split')
-    def test_get_orphans(self, default_ms):
+    @ddt.data(('draft', 1, 0), ('split', 2, 0))
+    @ddt.unpack
+    def test_get_orphans(self, default_ms, max_find, max_send):
+        """
+        Test finding orphans.
+        """
         self.initdb(default_ms)
         course_id = self.course_locations[self.MONGO_COURSEID].course_key
 
@@ -700,6 +787,8 @@ 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):
             found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key)
         self.assertEqual(set(found_orphans), set(orphan_locations))
 
@@ -741,8 +830,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', 'split')
-    def test_get_courses_for_wiki(self, default_ms):
+    @ddt.data(('draft', 1, 0), ('split', 1, 0))
+    @ddt.unpack
+    def test_get_courses_for_wiki(self, default_ms, max_find, max_send):
         """
         Test the get_courses_for_wiki method
         """
@@ -757,6 +847,8 @@ 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):
             wiki_courses = self.store.get_courses_for_wiki('999')
         self.assertEqual(len(wiki_courses), 1)
         self.assertIn(
@@ -767,8 +859,9 @@ 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', 'split')
-    def test_unpublish(self, default_ms):
+    @ddt.data(('draft', 2, 0), ('split', 5, 0))
+    @ddt.unpack
+    def test_unpublish(self, default_ms, max_find, max_send):
         """
         Test calling unpublish
         """
@@ -784,6 +877,8 @@ 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):
             self.store.unpublish(self.vertical_x1a, self.user_id)
 
         with self.assertRaises(ItemNotFoundError):
@@ -799,8 +894,9 @@ class TestMixedModuleStore(unittest.TestCase):
         )
         self.assertIsNotNone(draft_xblock)
 
-    @ddt.data('draft', 'split')
-    def test_compute_publish_state(self, default_ms):
+    @ddt.data(('draft', 1, 0), ('split', 4, 0))
+    @ddt.unpack
+    def test_compute_publish_state(self, default_ms, max_find, max_send):
         """
         Test the compute_publish_state method
         """
@@ -810,6 +906,8 @@ 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.version_agnostic()
+        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):
             self.assertEquals(self.store.compute_publish_state(item), PublishState.private)
 
         # Private -> Public