diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index cdb0948b20..dc84c0cef2 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -95,17 +95,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']) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index f8e8ae2f4f..3ffe8b135f 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -27,6 +27,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', diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 6da18ae47a..677cea25f9 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -570,7 +570,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. @@ -595,6 +595,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) @@ -755,7 +758,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 diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index c613ab59d6..a7fac49877 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -1,9 +1,7 @@ """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') diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 681c9f7975..2bacbd6814 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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') @@ -577,6 +578,36 @@ class ModuleStoreReadBase(ModuleStoreRead): """ pass + @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): ''' diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 95882cdb3b..217399d883 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -589,6 +589,7 @@ 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 subtree rooted at xblock has any drafts and thus may possibly have changes diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index e7a329be40..2a5b4617ad 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/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() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 64831faea4..4686ec3890 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -354,8 +354,9 @@ class TestMixedModuleStore(unittest.TestCase): ) # draft: 2 to look in draft and then published and then 5 for updating ancestors. - # split: 1 for the course index, 1 for the course structure before the change, 1 for the structure after the change - # 2 sends: insert structure, update index_entry + # 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', 11, 5), ('split', 3, 2)) @ddt.unpack def test_update_item(self, default_ms, max_find, max_send): @@ -438,7 +439,6 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_changes(component)) # TODO: LMS-11220: Document why split find count is 4 - # TODO: LMS-11220: Document why draft find count is 8 # TODO: LMS-11220: Document why split send count is 3 @ddt.data(('draft', 8, 2), ('split', 4, 3)) @ddt.unpack @@ -459,7 +459,7 @@ class TestMixedModuleStore(unittest.TestCase): with self.assertRaises(ItemNotFoundError): self.store.get_item(self.writable_chapter_location) - # TODO: LMS-11220: Document why draft find count is 9 + # TODO: LMS-11220: Document why split find count is 4 # TODO: LMS-11220: Document why split send count is 3 @ddt.data(('draft', 9, 2), ('split', 4, 3)) @ddt.unpack @@ -506,8 +506,7 @@ class TestMixedModuleStore(unittest.TestCase): self.assertFalse(self.store.has_item(leaf_loc)) self.assertNotIn(vert_loc, course.children) - # TODO: LMS-11220: Document why split send count is 2 - # TODO: LMS-11220: Document why draft find count is 5 + # TODO: LMS-11220: Document why split find count is 2 @ddt.data(('draft', 5, 1), ('split', 2, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, max_find, max_send): @@ -712,9 +711,9 @@ class TestMixedModuleStore(unittest.TestCase): # - load vertical # - load inheritance data - # TODO: LMS-11220: Document why draft send count is 6 - # TODO: LMS-11220: Document why draft find count is 18 - # TODO: LMS-11220: Document why split find count is 16 + # TODO: LMS-11220: Document why draft send count is 5 + # TODO: LMS-11220: Document why draft find count is [19, 6] + # TODO: LMS-11220: Document why split find count is [2, 2] @ddt.data(('draft', [19, 6], 0), ('split', [2, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): @@ -1045,6 +1044,7 @@ class TestMixedModuleStore(unittest.TestCase): # Sends: # - insert structure # - write index entry + # TODO: LMS-11220: Document why split find count is 3 @ddt.data(('draft', 3, 6), ('split', 3, 2)) @ddt.unpack def test_unpublish(self, default_ms, max_find, max_send): diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index e14a51b8ff..aa223d724f 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -326,15 +326,16 @@ class TestTOC(ModuleStoreTestCase): self.request = factory.get(chapter_url) self.request.user = UserFactory() self.modulestore = self.store._get_modulestore_for_courseid(self.course_key) - self.toy_course = self.store.get_course(self.toy_loc, depth=2) 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 ) - # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) + # TODO: LMS-11220: Document why split find count is 9 + # TODO: LMS-11220: Document why mongo find count is 4 + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 0), (ModuleStoreEnum.Type.split, 9, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms): @@ -361,8 +362,9 @@ class TestTOC(ModuleStoreTestCase): for toc_section in expected: self.assertIn(toc_section, actual) - # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) + # TODO: LMS-11220: Document why split find count is 9 + # TODO: LMS-11220: Document why mongo find count is 4 + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 0), (ModuleStoreEnum.Type.split, 9, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms):