From 3f3d0d25d85e2b67ca213e77acc45702f6b56c8b Mon Sep 17 00:00:00 2001 From: Sagirov Eugeniy Date: Tue, 20 Sep 2022 13:05:07 +0300 Subject: [PATCH] feat!: Remove inheritance-related code from Old Mongo --- .../contentstore/tests/test_contentstore.py | 4 +- .../tests/test_courseware_index.py | 2 + .../contentstore/tests/test_import.py | 16 -- .../contentstore/tests/test_orphan.py | 2 +- .../contentstore/views/tests/test_item.py | 31 +--- .../course_api/blocks/tests/test_api.py | 4 +- .../course_api/tests/test_serializers.py | 2 +- lms/djangoapps/grades/tests/test_tasks.py | 8 +- .../grades/tests/test_transformer.py | 10 +- .../instructor_task/tests/test_base.py | 8 +- .../tests/test_tasks_helper.py | 5 +- .../lti_provider/tests/test_outcomes.py | 14 +- .../tests/test_course_overviews.py | 2 +- .../course_overviews/tests/test_signals.py | 2 +- xmodule/modulestore/mongo/base.py | 138 ++---------------- xmodule/modulestore/mongo/draft.py | 2 - .../tests/test_mixed_modulestore.py | 14 +- .../tests/test_mongo_call_count.py | 18 +-- xmodule/modulestore/tests/test_publish.py | 18 +-- 19 files changed, 64 insertions(+), 236 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 507ac7ebe1..847c18ed33 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1121,7 +1121,7 @@ class MiscCourseTests(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 self.store.branch_setting(ModuleStoreEnum.Branch.published_only, self.course.id): - with check_mongo_calls(3): + with check_mongo_calls(4): course = self.store.get_course(self.course.id, depth=2) # make sure we pre-fetched a known sequential which should be at depth=2 @@ -1133,7 +1133,7 @@ class MiscCourseTests(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 self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.course.id): - with check_mongo_calls(3): + with check_mongo_calls(4): self.store.get_course(self.course.id, depth=2) def _check_verticals(self, locations): diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index 9dbbfffdd0..54d2122aeb 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/cms/djangoapps/contentstore/tests/test_courseware_index.py @@ -33,6 +33,7 @@ from xmodule.modulestore.django import SignalHandler, modulestore # lint-amnest from xmodule.modulestore.tests.django_utils import ( # lint-amnesty, pylint: disable=wrong-import-order ModuleStoreTestCase, TEST_DATA_MONGO_MODULESTORE, + TEST_DATA_SPLIT_MODULESTORE, SharedModuleStoreTestCase, ) from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory # lint-amnesty, pylint: disable=wrong-import-order @@ -868,6 +869,7 @@ class GroupConfigurationSearchSplit(CourseTestCase, MixedWithOptionsTestCase): """ CREATE_USER = True INDEX_NAME = CoursewareSearchIndexer.INDEX_NAME + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE def setUp(self): super().setUp() diff --git a/cms/djangoapps/contentstore/tests/test_import.py b/cms/djangoapps/contentstore/tests/test_import.py index 4c63ecd735..0f80667938 100644 --- a/cms/djangoapps/contentstore/tests/test_import.py +++ b/cms/djangoapps/contentstore/tests/test_import.py @@ -18,7 +18,6 @@ from xmodule.exceptions import NotFoundError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import check_exact_number_of_calls, check_number_of_calls from xmodule.modulestore.xml_importer import import_course_from_xml TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -175,21 +174,6 @@ class ContentStoreImportTest(ModuleStoreTestCase): print(f"course tabs = {course.tabs}") self.assertEqual(course.tabs[1]['name'], 'Syllabus') - def test_import_performance_mongo(self): - 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, 'refresh_cached_metadata_inheritance_tree', 28): - - # _get_cached_metadata_inheritance_tree should be called once - 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 1. - # Locally, without memcache, the number of calls is 1 (publish no longer counted) - with check_number_of_calls(store, '_compute_metadata_inheritance_tree', 1): - self.load_test_import_course(create_if_not_present=False, module_store=store) - @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) def test_reimport(self, default_ms_type): with modulestore().default_store(default_ms_type): diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index e0489f73e6..d41a5c16e8 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -104,7 +104,7 @@ class TestOrphan(TestOrphanBase): @ddt.data( (ModuleStoreEnum.Type.split, 5, 3), - (ModuleStoreEnum.Type.mongo, 34, 12), + (ModuleStoreEnum.Type.mongo, 34, 11), ) @ddt.unpack def test_delete_orphans(self, default_store, max_mongo_calls, min_mongo_calls): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 055cea6401..b55e19c4f2 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -167,35 +167,6 @@ class GetItemTest(ItemTest): self.assertContains(resp, content_contains, status_code=expected_code) return resp - @ddt.data( - (1, 17, 15, 16, 12), - (2, 17, 15, 16, 12), - (3, 17, 15, 16, 12), - ) - @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])) - - @ddt.data( - (1, 30), - (2, 32), - (3, 34), - ) - @ddt.unpack - def test_container_get_query_count(self, branching_factor, unit_queries,): - self.populate_course(branching_factor) - with check_mongo_calls(unit_queries): - self.client.get(reverse_usage_url('xblock_container_handler', self.populated_usage_keys['vertical'][-1])) - def test_get_vertical(self): # Add a vertical resp = self.create_xblock(category='vertical') @@ -2565,7 +2536,7 @@ class TestXBlockInfo(ItemTest): @ddt.data( (ModuleStoreEnum.Type.split, 3, 3), - (ModuleStoreEnum.Type.mongo, 5, 7), + (ModuleStoreEnum.Type.mongo, 8, 12), ) @ddt.unpack def test_xblock_outline_handler_mongo_calls(self, store_type, chapter_queries, chapter_queries_1): diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index 8bf7b9d518..7ff7290eca 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -226,8 +226,8 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): ) @ddt.data( - (ModuleStoreEnum.Type.mongo, 5, True, 24), - (ModuleStoreEnum.Type.mongo, 5, False, 14), + (ModuleStoreEnum.Type.mongo, 19, True, 24), + (ModuleStoreEnum.Type.mongo, 19, False, 14), (ModuleStoreEnum.Type.split, 2, True, 24), (ModuleStoreEnum.Type.split, 2, False, 14), ) diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py index 7bba11d540..5ac8a119ac 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -155,7 +155,7 @@ class TestCourseDetailSerializer(TestCourseSerializer): # lint-amnesty, pylint: """ # 1 mongo call is made to get the course About overview text. - expected_mongo_calls = 1 + expected_mongo_calls = 2 serializer_class = CourseDetailSerializer def setUp(self): diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index f9c1beb930..f634e9b364 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -153,8 +153,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest assert mock_block_structure_create.call_count == 1 @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 41, True), - (ModuleStoreEnum.Type.mongo, 1, 41, False), + (ModuleStoreEnum.Type.mongo, 2, 41, True), + (ModuleStoreEnum.Type.mongo, 2, 41, False), (ModuleStoreEnum.Type.split, 2, 41, True), (ModuleStoreEnum.Type.split, 2, 41, False), ) @@ -167,7 +167,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest self._apply_recalculate_subsection_grade() @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 41), + (ModuleStoreEnum.Type.mongo, 2, 41), (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack @@ -213,7 +213,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest ) @ddt.data( - (ModuleStoreEnum.Type.mongo, 1, 41), + (ModuleStoreEnum.Type.mongo, 2, 41), (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index d30b333f1c..f7b27e7188 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -11,7 +11,7 @@ import ddt import pytz from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -from xmodule.modulestore.tests.factories import check_mongo_calls +from xmodule.modulestore.tests.factories import check_mongo_calls_range from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.course_blocks.api import get_course_blocks @@ -429,11 +429,11 @@ class MultiProblemModulestoreAccessTestCase(CourseStructureTestCase, SharedModul self.client.login(username=self.student.username, password=password) @ddt.data( - (ModuleStoreEnum.Type.split, 2), - (ModuleStoreEnum.Type.mongo, 2), + (ModuleStoreEnum.Type.split, 2, 2), + (ModuleStoreEnum.Type.mongo, 22, 15), ) @ddt.unpack - def test_modulestore_performance(self, store_type, expected_mongo_queries): + def test_modulestore_performance(self, store_type, max_mongo_calls, min_mongo_calls): """ Test that a constant number of mongo calls are made regardless of how many grade-related blocks are in the course. @@ -470,5 +470,5 @@ class MultiProblemModulestoreAccessTestCase(CourseStructureTestCase, SharedModul with self.store.default_store(store_type): blocks = self.build_course(course) clear_course_from_cache(blocks['course'].id) - with check_mongo_calls(expected_mongo_queries): + with check_mongo_calls_range(max_mongo_calls, min_mongo_calls): get_course_blocks(self.student, blocks['course'].location, self.transformers) diff --git a/lms/djangoapps/instructor_task/tests/test_base.py b/lms/djangoapps/instructor_task/tests/test_base.py index 75f6eb0b33..e94be80bf2 100644 --- a/lms/djangoapps/instructor_task/tests/test_base.py +++ b/lms/djangoapps/instructor_task/tests/test_base.py @@ -377,15 +377,13 @@ class TestReportMixin: """ csv data may contain numeric values that are converted to strings, and fractional numbers can be imprecise (e.g. 1 / 6 is sometimes '0.16666666666666666' and other times - '0.166666666667'). This function mutates the provided input (sorry) and returns - a new dictionary that contains only the numerically-valued items from it, rounded - to four decimal places. + '0.166666666667'). This function returns a new dictionary that contains only the + numerically-valued items from it, rounded to four decimal places. """ extracted = {} for key in list(dictionary): try: - float(dictionary[key]) - extracted[key] = round(float(dictionary.pop(key)), 4) + extracted[key] = round(float(dictionary[key]), 4) except ValueError: pass return extracted diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index e58fe277a3..5039e1949e 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -370,7 +370,7 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): self._verify_cell_data_for_user(verified_user.username, course.id, 'Certificate Eligible', 'Y', num_rows=2) @ddt.data( - (ModuleStoreEnum.Type.mongo, 4, 47), + (ModuleStoreEnum.Type.mongo, 6, 47), (ModuleStoreEnum.Type.split, 2, 48), ) @ddt.unpack @@ -1779,8 +1779,7 @@ class TestGradeReport(TestReportMixin, InstructorTaskModuleTestCase): self.define_option_problem('Unreleased', parent=self.unreleased_section) @patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False}) - @ddt.data(True, False) - def test_grade_report(self, persistent_grades_enabled): + def test_grade_report(self): self.submit_student_answer(self.student.username, 'Problem1', ['Option 1']) with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): diff --git a/lms/djangoapps/lti_provider/tests/test_outcomes.py b/lms/djangoapps/lti_provider/tests/test_outcomes.py index 4f9cc4779d..76228a1cce 100644 --- a/lms/djangoapps/lti_provider/tests/test_outcomes.py +++ b/lms/djangoapps/lti_provider/tests/test_outcomes.py @@ -371,7 +371,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): assert count == 3 def test_with_no_graded_assignments(self): - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) @@ -379,7 +379,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): def test_with_graded_unit(self): self.create_graded_assignment(self.unit, 'graded_unit', self.outcome_service) - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) @@ -388,7 +388,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): def test_with_graded_vertical(self): self.create_graded_assignment(self.vertical, 'graded_vertical', self.outcome_service) - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) @@ -398,7 +398,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): def test_with_graded_unit_and_vertical(self): self.create_graded_assignment(self.unit, 'graded_unit', self.outcome_service) self.create_graded_assignment(self.vertical, 'graded_vertical', self.outcome_service) - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) @@ -409,7 +409,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): def test_with_unit_used_twice(self): self.create_graded_assignment(self.unit, 'graded_unit', self.outcome_service) self.create_graded_assignment(self.unit, 'graded_unit2', self.outcome_service) - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) @@ -420,7 +420,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): def test_with_unit_graded_for_different_user(self): self.create_graded_assignment(self.unit, 'graded_unit', self.outcome_service) other_user = UserFactory.create() - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, other_user.id, self.course.id ) @@ -430,7 +430,7 @@ class TestAssignmentsForProblem(ModuleStoreTestCase): other_outcome_service = self.create_outcome_service('second_consumer') self.create_graded_assignment(self.unit, 'graded_unit', self.outcome_service) self.create_graded_assignment(self.unit, 'graded_unit2', other_outcome_service) - with check_mongo_calls(3): + with check_mongo_calls(7): assignments = outcomes.get_assignments_for_problem( self.unit, self.user_id, self.course.id ) diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py index 3af01b49a3..2c856ae70a 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py @@ -378,7 +378,7 @@ class CourseOverviewTestCase(CatalogIntegrationMixin, ModuleStoreTestCase, Cache course_overview = CourseOverview._create_or_update(course) # pylint: disable=protected-access assert course_overview.lowest_passing_grade is None - @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (ModuleStoreEnum.Type.split, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 5, 5), (ModuleStoreEnum.Type.split, 2, 2)) @ddt.unpack def test_versioning(self, modulestore_type, min_mongo_calls, max_mongo_calls): """ diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py index 9506a584bb..0f98eb26b7 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_signals.py @@ -28,7 +28,7 @@ class CourseOverviewSignalsTestCase(ModuleStoreTestCase): TODAY = datetime.datetime.utcnow() NEXT_WEEK = TODAY + datetime.timedelta(days=7) - @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + @ddt.data(ModuleStoreEnum.Type.split) def test_caching(self, modulestore_type): """ Tests that CourseOverview structures are actually getting cached. diff --git a/xmodule/modulestore/mongo/base.py b/xmodule/modulestore/mongo/base.py index 863d009555..65ee947531 100644 --- a/xmodule/modulestore/mongo/base.py +++ b/xmodule/modulestore/mongo/base.py @@ -40,7 +40,7 @@ from xmodule.error_module import ErrorBlock from xmodule.errortracker import exc_info_to_str, null_error_tracker from xmodule.exceptions import HeartbeatFailure from xmodule.mako_module import MakoDescriptorSystem -from xmodule.modulestore import BulkOperationsMixin, BulkOpsRecord, ModuleStoreEnum, ModuleStoreWriteBase +from xmodule.modulestore import BulkOperationsMixin, ModuleStoreEnum, ModuleStoreWriteBase from xmodule.modulestore.draft_and_published import DIRECT_ONLY_CATEGORIES, ModuleStoreDraftAndPublished from xmodule.modulestore.edit_info import EditInfoRuntimeMixin from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError, ReferentialIntegrityError @@ -176,10 +176,9 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li str(self.course_id), [str(key) for key in self.module_data.keys()], self.default_class, - [str(key) for key in self.cached_metadata.keys()], )) - def __init__(self, modulestore, course_key, module_data, default_class, cached_metadata, **kwargs): + def __init__(self, modulestore, course_key, module_data, default_class, **kwargs): """ modulestore: the module store that can be used to retrieve additional modules @@ -191,8 +190,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li default_class: The default_class to use when loading an XModuleDescriptor from the module_data - cached_metadata: the cache for handling inheritance computation. internal use only - resources_fs: a filesystem, as per MakoDescriptorSystem error_tracker: a function that logs errors for later display to users @@ -214,7 +211,6 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li # cdodge: other Systems have a course_id attribute defined. To keep things consistent, let's # define an attribute here as well, even though it's None self.course_id = course_key - self.cached_metadata = cached_metadata def load_item(self, location, for_parent=None): # lint-amnesty, pylint: disable=method-hidden """ @@ -248,16 +244,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li ] parent = None - if self.cached_metadata is not None: - # fish the parent out of here if it's available - parent_url = self.cached_metadata.get(str(location), {}).get('parent', {}).get( - ModuleStoreEnum.Branch.published_only if location.branch is None - else ModuleStoreEnum.Branch.draft_preferred - ) - if parent_url: - parent = self._convert_reference_to_key(parent_url) - - if not parent and category not in DETACHED_XBLOCK_TYPES.union(['course']): + if 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), @@ -283,15 +270,10 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): # li field_data = KvsFieldData(kvs) scope_ids = ScopeIds(None, category, location, location) module = self.construct_xblock_from_class(class_, scope_ids, field_data, for_parent=for_parent) - if self.cached_metadata is not None: - # parent container pointers don't differentiate between draft and non-draft - # so when we do the lookup, we should do so with a non-draft location - non_draft_loc = as_published(location) - # Convert the serialized fields values in self.cached_metadata - # to python values - metadata_to_inherit = self.cached_metadata.get(str(non_draft_loc), {}) - inherit_metadata(module, metadata_to_inherit) + non_draft_loc = as_published(location) + metadata_inheritance_tree = self.modulestore._compute_metadata_inheritance_tree(self.course_id) + inherit_metadata(module, metadata_inheritance_tree.get(str(non_draft_loc), {})) module._edit_info = json_data.get('edit_info') @@ -449,39 +431,16 @@ def as_published(location): return location.replace(revision=MongoRevisionKey.published) -class MongoBulkOpsRecord(BulkOpsRecord): - """ - Tracks whether there've been any writes per course and disables inheritance generation - """ - def __init__(self): - super().__init__() - self.dirty = False - - class MongoBulkOpsMixin(BulkOperationsMixin): """ Mongo bulk operation support """ - _bulk_ops_record_type = MongoBulkOpsRecord - - def _start_outermost_bulk_operation(self, bulk_ops_record, course_key, ignore_case=False): - """ - Prevent updating the meta-data inheritance cache for the given course - """ - # ensure it starts clean - bulk_ops_record.dirty = False def _end_outermost_bulk_operation(self, bulk_ops_record, structure_key): """ - Restart updating the meta-data inheritance cache for the given course or library. - Refresh the meta-data inheritance cache now since it was temporarily disabled. + The outermost nested bulk_operation call: do the actual end of the bulk operation. """ - dirty = False - if bulk_ops_record.dirty: - self.refresh_cached_metadata_inheritance_tree(structure_key) - dirty = True - bulk_ops_record.dirty = False # brand spanking clean now - return dirty + return True def _is_in_bulk_operation(self, course_id, ignore_case=False): # lint-amnesty, pylint: disable=arguments-differ """ @@ -739,62 +698,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo return metadata_to_inherit - def _get_cached_metadata_inheritance_tree(self, course_id, force_refresh=False): - ''' - Compute the metadata inheritance for the course. - ''' - tree = {} - - course_id = self.fill_in_run(course_id) - if not force_refresh: - # see if we are first in the request cache (if present) - if self.request_cache is not None and str(course_id) in self.request_cache.data.get('metadata_inheritance', {}): # lint-amnesty, pylint: disable=line-too-long - return self.request_cache.data['metadata_inheritance'][str(course_id)] - - # then look in any caching subsystem (e.g. memcached) - if self.metadata_inheritance_cache_subsystem is not None: - tree = self.metadata_inheritance_cache_subsystem.get(str(course_id), {}) - else: - logging.warning( - 'Running MongoModuleStore without a metadata_inheritance_cache_subsystem. This is \ - OK in localdev and testing environment. Not OK in production.' - ) - - if not tree: - # if not in subsystem, or we are on force refresh, then we have to compute - tree = self._compute_metadata_inheritance_tree(course_id) - - # now write out computed tree to caching subsystem (e.g. memcached), if available - if self.metadata_inheritance_cache_subsystem is not None: - self.metadata_inheritance_cache_subsystem.set(str(course_id), tree) - - # now populate a request_cache, if available. NOTE, we are outside of the - # scope of the above if: statement so that after a memcache hit, it'll get - # put into the request_cache - if self.request_cache is not None: - # we can't assume the 'metadatat_inheritance' part of the request cache dict has been - # defined - if 'metadata_inheritance' not in self.request_cache.data: - self.request_cache.data['metadata_inheritance'] = {} - self.request_cache.data['metadata_inheritance'][str(course_id)] = tree - - return tree - - def refresh_cached_metadata_inheritance_tree(self, course_id, runtime=None): - """ - Refresh the cached metadata inheritance tree for the org/course combination - for location - - If given a runtime, it replaces the cached_metadata in that runtime. NOTE: failure to provide - a runtime may mean that some objects report old values for inherited data. - """ - course_id = course_id.for_branch(None) - if not self._is_in_bulk_operation(course_id): - # below is done for side effects when runtime is None - cached_metadata = self._get_cached_metadata_inheritance_tree(course_id, force_refresh=True) - if runtime: - runtime.cached_metadata = cached_metadata - def _clean_item_data(self, item): """ Renames the '_id' field in item to 'location' @@ -857,7 +760,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo return data def _load_item(self, course_key, item, data_cache, - apply_cached_metadata=True, using_descriptor_system=None, for_parent=None): + using_descriptor_system=None, for_parent=None): """ Load an XModuleDescriptor from item, using the children stored in data_cache @@ -868,8 +771,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo data_dir (optional): The directory name to use as the root data directory for this XModule data_cache (dict): A dictionary mapping from UsageKeys to xblock field data (this is the xblock data loaded from the database) - apply_cached_metadata (bool): Whether to use the cached metadata for inheritance - purposes. using_descriptor_system (CachingDescriptorSystem): The existing CachingDescriptorSystem to add data to, and to load the XBlocks from. for_parent (:class:`XBlock`): The parent of the XBlock being loaded. @@ -880,10 +781,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo root = self.fs_root / data_dir resource_fs = _OSFS_INSTANCE.setdefault(root, OSFS(root, create=True)) - cached_metadata = {} - if apply_cached_metadata: - cached_metadata = self._get_cached_metadata_inheritance_tree(course_key) - if using_descriptor_system is None: services = {} if self.i18n_service: @@ -909,7 +806,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo resources_fs=resource_fs, error_tracker=self.error_tracker, render_template=self.render_template, - cached_metadata=cached_metadata, mixins=self.xblock_mixins, select=self.xblock_select, disabled_xblock_types=self.disabled_xblock_types, @@ -918,7 +814,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo else: system = using_descriptor_system system.module_data.update(data_cache) - system.cached_metadata.update(cached_metadata) item = system.load_item(location, for_parent=for_parent) @@ -945,22 +840,11 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo item, data_cache, using_descriptor_system=using_descriptor_system, - apply_cached_metadata=self._should_apply_cached_metadata(item, depth), for_parent=for_parent, ) for item in items ] - def _should_apply_cached_metadata(self, item, depth): - """ - Returns a boolean whether a particular query should trigger an application - of inherited metadata onto the item - """ - category = item['location']['category'] - apply_cached_metadata = category not in DETACHED_XBLOCK_TYPES and \ - not (category == 'course' and depth == 0) - return apply_cached_metadata - @autoretry_read() def get_course_summaries(self, **kwargs): """ @@ -1345,7 +1229,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo resources_fs=None, error_tracker=self.error_tracker, render_template=self.render_template, - cached_metadata={}, mixins=self.xblock_mixins, select=self.xblock_select, services=services, @@ -1532,9 +1415,6 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo # update the edit info of the instantiated xblock xblock._edit_info = payload['edit_info'] - - # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(xblock.scope_ids.usage_id.course_key, xblock.runtime) # fire signal that we've written to DB except ItemNotFoundError: if not allow_not_found: # lint-amnesty, pylint: disable=no-else-raise diff --git a/xmodule/modulestore/mongo/draft.py b/xmodule/modulestore/mongo/draft.py index 29c2536f33..2caca673fb 100644 --- a/xmodule/modulestore/mongo/draft.py +++ b/xmodule/modulestore/mongo/draft.py @@ -616,8 +616,6 @@ class DraftModuleStore(MongoModuleStore): first_tier = [as_func(location) for as_func in as_functions] self._breadth_first(_delete_item, first_tier) - # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(location.course_key) def _breadth_first(self, function, root_usages): """ diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 318f6dc57d..07c12949fa 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -518,7 +518,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -913,7 +913,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -942,7 +942,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: draft and published structures, definition (unnecessary) # sends: update published (why?), draft, and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 9, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 8, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -996,7 +996,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: structure (cached) # send: update structure and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 4, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1039,7 +1039,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # executed twice, possibly unnecessarily) # find: 2 reads of structure, definition (s/b lazy; so, unnecessary), # plus 1 wildcard find in draft mongo which has none - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 2, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) @ddt.unpack def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -1079,7 +1079,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # draft is 2: find out which ms owns course, get item # split: active_versions (mysql), structure, definition (to load course wiki string) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) @ddt.unpack def test_get_course(self, default_ms, num_mysql, max_find, max_send): """ @@ -1634,7 +1634,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # 8-9. get vertical, compute inheritance # 10-11. get other vertical_x1b (why?) and compute inheritance # Split: loading structure from mongo (also loads active version from MySQL, not tracked here) - @ddt.data((ModuleStoreEnum.Type.mongo, [0, 0], [12, 3], 0), (ModuleStoreEnum.Type.split, [1, 0], [2, 1], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, [0, 0], [15, 3], 0), (ModuleStoreEnum.Type.split, [1, 0], [2, 1], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): """ diff --git a/xmodule/modulestore/tests/test_mongo_call_count.py b/xmodule/modulestore/tests/test_mongo_call_count.py index ba618da8f3..01425d0e2d 100644 --- a/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/xmodule/modulestore/tests/test_mongo_call_count.py @@ -142,16 +142,16 @@ class CountMongoCallsCourseTraversal(TestCase): # These two lines show the way this traversal *should* be done # (if you'll eventually access all the fields and load all the definitions anyway). # 'lazy' does not matter in old Mongo. - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, False, True, 175), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, True, True, 175), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, False, True, 359), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, True, 359), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, False, True, 322), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, True, True, 322), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, False, True, 506), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, True, 506), # As shown in these two lines: whether or not the XBlock fields are accessed, # the same number of mongo calls are made in old Mongo for depth=None. - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, False, False, 175), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, True, False, 175), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, False, False, 359), - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, False, 359), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, False, False, 322), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, None, True, False, 322), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, False, False, 506), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, False, 506), # The line below shows the way this traversal *should* be done # (if you'll eventually access all the fields and load all the definitions anyway). (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 2), @@ -177,7 +177,7 @@ class CountMongoCallsCourseTraversal(TestCase): self._traverse_blocks_in_course(start_block, access_all_block_fields) @ddt.data( - (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176), + (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 324), (MIXED_SPLIT_MODULESTORE_BUILDER, 3), ) @ddt.unpack diff --git a/xmodule/modulestore/tests/test_publish.py b/xmodule/modulestore/tests/test_publish.py index c2a4b16848..68e4374860 100644 --- a/xmodule/modulestore/tests/test_publish.py +++ b/xmodule/modulestore/tests/test_publish.py @@ -20,7 +20,7 @@ from openedx.core.lib.tests import attr from xmodule.exceptions import InvalidVersionError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls, mongo_uses_error_check +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBootstrapper from xmodule.modulestore.tests.utils import ( DRAFT_MODULESTORE_SETUP, @@ -45,17 +45,17 @@ class TestPublish(SplitWMongoCourseBootstrapper): # 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(4, 2): + with check_mongo_calls(3, 2): super()._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_operations(self.old_course_key): # finds: 1 for parent to add child and 2 to get ancestors # sends: 1 for insert, 1 for parent (add child) - with check_mongo_calls(3, 2): + with check_mongo_calls(4, 2): self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', split=False) # lint-amnesty, pylint: disable=line-too-long - with check_mongo_calls(4, 2): + with check_mongo_calls(5, 2): self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', split=False) # lint-amnesty, pylint: disable=line-too-long # For each vertical (2) created: # - load draft @@ -64,7 +64,7 @@ class TestPublish(SplitWMongoCourseBootstrapper): # - load parent # - get ancestors # - load inheritable data - with check_mongo_calls(15, 6): + with check_mongo_calls(16, 6): self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', split=False) # lint-amnesty, pylint: disable=line-too-long self._create_item('vertical', 'Vert2', {}, {'display_name': 'Vertical 2'}, 'chapter', 'Chapter1', split=False) # lint-amnesty, pylint: disable=line-too-long # For each (4) item created @@ -73,7 +73,7 @@ class TestPublish(SplitWMongoCourseBootstrapper): # - compute what is parent # - load draft parent again & compute its parent chain up to course # count for updates increased to 16 b/c of edit_info updating - with check_mongo_calls(36, 16): + with check_mongo_calls(40, 16): self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False) # lint-amnesty, pylint: disable=line-too-long self._create_item( 'discussion', 'Discussion1', @@ -126,11 +126,7 @@ class TestPublish(SplitWMongoCourseBootstrapper): # delete the subtree of drafts (1 call), # update the published version of each node in subtree (4 calls), # update the ancestors up to course (2 calls) - if mongo_uses_error_check(self.draft_mongo): - max_find = 23 - else: - max_find = 22 - with check_mongo_calls(max_find, 7): + with check_mongo_calls(23, 7): self.draft_mongo.publish(item.location, self.user_id) # verify status