diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index 34b3ae7053..f6ef12fddb 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -125,7 +125,7 @@ class CloneCourseTest(CourseTestCase): ) # try to hit the generic exception catch - with patch('xmodule.modulestore.split_mongo.mongo_connection.MongoConnection.insert_course_index', Mock(side_effect=Exception)): # lint-amnesty, pylint: disable=line-too-long + with patch('xmodule.modulestore.split_mongo.mongo_connection.MongoPersistenceBackend.insert_course_index', Mock(side_effect=Exception)): # lint-amnesty, pylint: disable=line-too-long split_course4_id = CourseLocator(org="edx3", course="split3", run="rerun_fail") fields = {'display_name': 'total failure'} CourseRerunState.objects.initiated(split_course3_id, split_course4_id, self.user, fields['display_name']) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index acd2fbfd82..dcffbd7954 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -157,8 +157,8 @@ class TestCourseListing(ModuleStoreTestCase): self.assertEqual(len(list(courses_iter)), 0) @ddt.data( - (ModuleStoreEnum.Type.split, 3), - (ModuleStoreEnum.Type.mongo, 2) + (ModuleStoreEnum.Type.split, 2), + (ModuleStoreEnum.Type.mongo, 1) ) @ddt.unpack def test_staff_course_listing(self, default_store, mongo_calls): @@ -239,8 +239,8 @@ class TestCourseListing(ModuleStoreTestCase): ) @ddt.data( - (ModuleStoreEnum.Type.split, 3, 3), - (ModuleStoreEnum.Type.mongo, 2, 2) + (ModuleStoreEnum.Type.split, 2, 2), + (ModuleStoreEnum.Type.mongo, 1, 1) ) @ddt.unpack def test_course_listing_performance(self, store, courses_list_from_group_calls, courses_list_calls): @@ -289,7 +289,7 @@ class TestCourseListing(ModuleStoreTestCase): # Calls: # 1) query old mongo # 2) get_more on old mongo - # 3) query split (but no courses so no fetching of data) + # 3) query split (handled with MySQL only) @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) def test_course_listing_errored_deleted_courses(self, store): diff --git a/cms/djangoapps/contentstore/tests/test_orphan.py b/cms/djangoapps/contentstore/tests/test_orphan.py index 48745753f4..1e19bc21fc 100644 --- a/cms/djangoapps/contentstore/tests/test_orphan.py +++ b/cms/djangoapps/contentstore/tests/test_orphan.py @@ -103,7 +103,7 @@ class TestOrphan(TestOrphanBase): self.assertIn(str(location), orphans) @ddt.data( - (ModuleStoreEnum.Type.split, 9, 5), + (ModuleStoreEnum.Type.split, 5, 3), (ModuleStoreEnum.Type.mongo, 34, 12), ) @ddt.unpack diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index b2182b9ec9..dff64735d5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -391,13 +391,13 @@ class TestCourseIndexArchived(CourseTestCase): @ddt.data( # Staff user has course staff access - (True, 'staff', None, 3, 18), - (False, 'staff', None, 3, 18), + (True, 'staff', None, 1, 20), + (False, 'staff', None, 1, 20), # Base user has global staff access - (True, 'user', ORG, 3, 18), - (False, 'user', ORG, 3, 18), - (True, 'user', None, 3, 18), - (False, 'user', None, 3, 18), + (True, 'user', ORG, 1, 20), + (False, 'user', ORG, 1, 20), + (True, 'user', None, 1, 20), + (False, 'user', None, 1, 20), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 0d93d8b454..a5367abc24 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -2528,7 +2528,7 @@ class TestXBlockInfo(ItemTest): self.validate_course_xblock_info(json_response, course_outline=True) @ddt.data( - (ModuleStoreEnum.Type.split, 4, 4), + (ModuleStoreEnum.Type.split, 3, 3), (ModuleStoreEnum.Type.mongo, 5, 7), ) @ddt.unpack diff --git a/cms/envs/common.py b/cms/envs/common.py index 5036425d25..1b01d6c4f1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1456,6 +1456,7 @@ INSTALLED_APPS = [ # For CMS 'cms.djangoapps.contentstore.apps.ContentstoreConfig', + 'common.djangoapps.split_modulestore_django.apps.SplitModulestoreDjangoBackendAppConfig', 'openedx.core.djangoapps.contentserver', 'cms.djangoapps.course_creators', diff --git a/common/djangoapps/split_modulestore_django/__init__.py b/common/djangoapps/split_modulestore_django/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/split_modulestore_django/admin.py b/common/djangoapps/split_modulestore_django/admin.py new file mode 100644 index 0000000000..4dbddce9fa --- /dev/null +++ b/common/djangoapps/split_modulestore_django/admin.py @@ -0,0 +1,18 @@ +""" +Admin registration for Split Modulestore Django Backend +""" +from django.contrib import admin +from simple_history.admin import SimpleHistoryAdmin + +from .models import SplitModulestoreCourseIndex + + +@admin.register(SplitModulestoreCourseIndex) +class SplitModulestoreCourseIndexAdmin(SimpleHistoryAdmin): + """ + Admin config for course indexes + """ + list_display = ('course_id', 'draft_version', 'published_version', 'library_version', 'wiki_slug', 'last_update') + search_fields = ('course_id', 'wiki_slug') + ordering = ('course_id', ) + readonly_fields = ('id', 'objectid', 'course_id', 'org', ) diff --git a/common/djangoapps/split_modulestore_django/apps.py b/common/djangoapps/split_modulestore_django/apps.py new file mode 100644 index 0000000000..022533938d --- /dev/null +++ b/common/djangoapps/split_modulestore_django/apps.py @@ -0,0 +1,12 @@ +""" +Define this module as a Django app +""" +from django.apps import AppConfig + + +class SplitModulestoreDjangoBackendAppConfig(AppConfig): + """ + Django app that provides a backend for Split Modulestore instead of MongoDB. + """ + name = 'common.djangoapps.split_modulestore_django' + verbose_name = "Split Modulestore Django Backend" diff --git a/common/djangoapps/split_modulestore_django/migrations/0001_initial.py b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py new file mode 100644 index 0000000000..598090f8b0 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/migrations/0001_initial.py @@ -0,0 +1,65 @@ +# Generated by Django 2.2.20 on 2021-05-07 18:29 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import opaque_keys.edx.django.models +import simple_history.models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='SplitModulestoreCourseIndex', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('objectid', models.CharField(max_length=24, unique=True)), + ('course_id', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255, unique=True)), + ('org', models.CharField(db_index=True, max_length=255)), + ('draft_version', models.CharField(blank=True, max_length=24)), + ('published_version', models.CharField(blank=True, max_length=24)), + ('library_version', models.CharField(blank=True, max_length=24)), + ('wiki_slug', models.CharField(db_index=True, blank=True, max_length=255)), + ('base_store', models.CharField(choices=[('mongodb', 'MongoDB'), ('django', 'Django - not implemented yet')], max_length=20)), + ('edited_on', models.DateTimeField()), + ('last_update', models.DateTimeField()), + ('edited_by_id', models.IntegerField(null=True)), + ], + options={'ordering': ['course_id'], 'verbose_name_plural': 'Split modulestore course indexes'}, + ), + migrations.CreateModel( + name='HistoricalSplitModulestoreCourseIndex', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('objectid', models.CharField(db_index=True, max_length=24)), + ('course_id', opaque_keys.edx.django.models.LearningContextKeyField(db_index=True, max_length=255)), + ('org', models.CharField(db_index=True, max_length=255)), + ('draft_version', models.CharField(blank=True, max_length=24)), + ('published_version', models.CharField(blank=True, max_length=24)), + ('library_version', models.CharField(blank=True, max_length=24)), + ('wiki_slug', models.CharField(db_index=True, blank=True, max_length=255)), + ('base_store', models.CharField(choices=[('mongodb', 'MongoDB'), ('django', 'Django - not implemented yet')], max_length=20)), + ('edited_on', models.DateTimeField()), + ('last_update', models.DateTimeField()), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('edited_by_id', models.IntegerField(null=True)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'historical split modulestore course index', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py b/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py new file mode 100644 index 0000000000..148e1b6b75 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/migrations/0002_data_migration.py @@ -0,0 +1,41 @@ +from django.db import migrations, models +from django.db.utils import IntegrityError + + +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore + +from ..models import SplitModulestoreCourseIndex as SplitModulestoreCourseIndex_Real + + +def forwards_func(apps, schema_editor): + """ + Copy all course index data from MongoDB to MySQL. + """ + db_alias = schema_editor.connection.alias + SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") + split_modulestore = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split) + + for course_index in split_modulestore.db_connection.find_matching_course_indexes(force_mongo=True): + data = SplitModulestoreCourseIndex_Real.fields_from_v1_schema(course_index) + + SplitModulestoreCourseIndex(**data).save(using=db_alias) + +def reverse_func(apps, schema_editor): + """ + Reverse the data migration, deleting all entries in this table. + """ + db_alias = schema_editor.connection.alias + SplitModulestoreCourseIndex = apps.get_model("split_modulestore_django", "SplitModulestoreCourseIndex") + SplitModulestoreCourseIndex.objects.using(db_alias).all().delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('split_modulestore_django', '0001_initial'), + ] + + operations = [ + migrations.RunPython(forwards_func, reverse_func), + ] diff --git a/common/djangoapps/split_modulestore_django/migrations/__init__.py b/common/djangoapps/split_modulestore_django/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/split_modulestore_django/models.py b/common/djangoapps/split_modulestore_django/models.py new file mode 100644 index 0000000000..b3f8b6d145 --- /dev/null +++ b/common/djangoapps/split_modulestore_django/models.py @@ -0,0 +1,164 @@ +""" +Django model to store the "course index" data +""" +from bson.objectid import ObjectId +from django.contrib.auth import get_user_model +from django.db import models +from opaque_keys.edx.locator import CourseLocator, LibraryLocator +from opaque_keys.edx.django.models import LearningContextKeyField +from simple_history.models import HistoricalRecords + +from xmodule.modulestore import ModuleStoreEnum + +User = get_user_model() + + +class SplitModulestoreCourseIndex(models.Model): + """ + A "course index" for a course in "split modulestore." + + This model/table mostly stores the current version of each course. + (Well, twice for each course - "draft" and "published" branch versions are + tracked separately.) + + This MySQL table / django model is designed to replace the "active_versions" + MongoDB collection. They contain the same information. + + It also stores the "wiki_slug" to facilitate looking up a course + by it's wiki slug, which is required due to the nuances of the + django-wiki integration. + + .. no_pii: + """ + # For compatibility with MongoDB, each course index must have an ObjectId. We still have an integer primary key too. + objectid = models.CharField(max_length=24, null=False, blank=False, unique=True) + + # The ID of this course (or library). Must start with "course-v1:" or "library-v1:" + course_id = LearningContextKeyField(max_length=255, db_index=True, unique=True, null=False) + # Extract the "org" value from the course_id key so that we can search by org. + # This gets set automatically by clean() + org = models.CharField(max_length=255, db_index=True) + + # Version fields: The ObjectId of the current entry in the "structures" collection, for this course. + # The version is stored separately for each "branch". + # Note that there are only three branch names allowed. Draft/published are used for courses, while "library" is used + # for content libraries. + + # ModuleStoreEnum.BranchName.draft = 'draft-branch' + draft_version = models.CharField(max_length=24, null=False, blank=True) + # ModuleStoreEnum.BranchName.published = 'published-branch' + published_version = models.CharField(max_length=24, null=False, blank=True) + # ModuleStoreEnum.BranchName.library = 'library' + library_version = models.CharField(max_length=24, null=False, blank=True) + + # Wiki slug for this course + wiki_slug = models.CharField(max_length=255, db_index=True, blank=True) + + # Base store - whether the "structures" and "definitions" data are in MongoDB or object storage (S3) + BASE_STORE_MONGO = "mongodb" + BASE_STORE_DJANGO = "django" + BASE_STORE_CHOICES = [ + (BASE_STORE_MONGO, "MongoDB"), # For now, MongoDB is the only implemented option + (BASE_STORE_DJANGO, "Django - not implemented yet"), + ] + base_store = models.CharField(max_length=20, blank=False, choices=BASE_STORE_CHOICES) + + # Edit history: + # ID of the user that made the latest edit. This is not a ForeignKey because some values (like + # ModuleStoreEnum.UserID.*) are not real user IDs. + edited_by_id = models.IntegerField(null=True) + edited_on = models.DateTimeField() + # last_update is different from edited_on, and is used only to prevent collisions? + last_update = models.DateTimeField() + + # Keep track of the history of this table: + history = HistoricalRecords() + + def __str__(self): + return f"Course Index ({self.course_id})" + + class Meta: + ordering = ["course_id"] + verbose_name_plural = "Split modulestore course indexes" + + def as_v1_schema(self): + """ Return in the same format as was stored in MongoDB """ + versions = {} + for branch in ("draft", "published", "library"): + # The current version of this branch, a hex-encoded ObjectID - or an empty string: + version_str = getattr(self, f"{branch}_version") + if version_str: + versions[getattr(ModuleStoreEnum.BranchName, branch)] = ObjectId(version_str) + return { + "_id": ObjectId(self.objectid), + "org": self.course_id.org, + "course": self.course_id.course, + "run": self.course_id.run, # pylint: disable=no-member + "edited_by": self.edited_by_id, + "edited_on": self.edited_on, + "last_update": self.last_update, + "versions": versions, + "schema_version": 1, # This matches schema version 1, see SplitMongoModuleStore.SCHEMA_VERSION + "search_targets": {"wiki_slug": self.wiki_slug}, + } + + @staticmethod + def fields_from_v1_schema(values): + """ Convert the MongoDB-style dict shape to a dict of fields that match this model """ + if values["run"] == LibraryLocator.RUN and ModuleStoreEnum.BranchName.library in values["versions"]: + # This is a content library: + locator = LibraryLocator(org=values["org"], library=values["course"]) + else: + # This is a course: + locator = CourseLocator(org=values["org"], course=values["course"], run=values["run"]) + result = { + "course_id": locator, + "org": values["org"], + "edited_by_id": values["edited_by"], + "edited_on": values["edited_on"], + "base_store": SplitModulestoreCourseIndex.BASE_STORE_MONGO, + } + if "_id" in values: + result["objectid"] = str(values["_id"]) # Convert ObjectId to its hex representation + if "last_update" in values: + result["last_update"] = values["last_update"] + if "search_targets" in values and "wiki_slug" in values["search_targets"]: + result["wiki_slug"] = values["search_targets"]["wiki_slug"] + for branch in ("draft", "published", "library"): + version = values["versions"].get(getattr(ModuleStoreEnum.BranchName, branch)) + if version: + result[f"{branch}_version"] = str(version) # Convert version from ObjectId to hex string + return result + + @staticmethod + def field_name_for_branch(branch_name): + """ Given a full branch name, get the name of the field in this table that stores that branch's version """ + if branch_name == ModuleStoreEnum.BranchName.draft: + return "draft_version" + if branch_name == ModuleStoreEnum.BranchName.published: + return "published_version" + if branch_name == ModuleStoreEnum.BranchName.library: + return "library_version" + raise ValueError(f"Unknown branch name: {branch_name}") + + def clean(self): + """ + Validation for this model + """ + super().clean() + # Check that course_id is a supported type: + course_id_str = str(self.course_id) + if not course_id_str.startswith("course-v1:") and not course_id_str.startswith("library-v1:"): + raise ValueError( + f"Split modulestore cannot store course[like] object with key {course_id_str}" + " - only course-v1/library-v1 prefixed keys are supported." + ) + # Set the "org" field automatically - ensure it always matches the "org" in the course_id + self.org = self.course_id.org + + def save(self, *args, **kwargs): + """ Save this model """ + # Override to ensure that full_clean()/clean() is always called, so that the checks in clean() above are run. + # But don't validate_unique(), it just runs extra queries and the database enforces it anyways. + self.full_clean(validate_unique=False) + return super().save(*args, **kwargs) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py index c868ee89f2..e51dc9e0df 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/mongo_connection.py @@ -13,11 +13,14 @@ from contextlib import contextmanager from time import time from django.core.cache import caches, InvalidCacheBackendError +from django.db.transaction import TransactionManagementError import pymongo import pytz from mongodb_proxy import autoretry_read # Import this just to export it from pymongo.errors import DuplicateKeyError # pylint: disable=unused-import + +from common.djangoapps.split_modulestore_django.models import SplitModulestoreCourseIndex from xmodule.exceptions import HeartbeatFailure from xmodule.modulestore import BlockData from xmodule.modulestore.split_mongo import BlockKey @@ -243,7 +246,7 @@ class CourseStructureCache: self.cache.set(key, compressed_pickled_data, None) -class MongoConnection: +class MongoPersistenceBackend: """ Segregation of pymongo functions from the data modeling mechanisms for split modulestore. """ @@ -429,15 +432,18 @@ class MongoConnection: return courses_queries - def insert_course_index(self, course_index, course_context=None): + def insert_course_index(self, course_index, course_context=None, last_update_already_set=False): """ Create the course_index in the db """ with TIMER.timer("insert_course_index", course_context): - course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Set last_update which is used to avoid collisions, unless a subclass already set it before calling super() + if not last_update_already_set: + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Insert the new index: self.course_index.insert_one(course_index) - def update_course_index(self, course_index, from_index=None, course_context=None): + def update_course_index(self, course_index, from_index=None, course_context=None, last_update_already_set=False): """ Update the db record for course_index. @@ -457,7 +463,10 @@ class MongoConnection: 'course': course_index['course'], 'run': course_index['run'], } - course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Set last_update which is used to avoid collisions, unless a subclass already set it before calling super() + if not last_update_already_set: + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Update the course index: self.course_index.replace_one(query, course_index, upsert=False,) def delete_course_index(self, course_key): @@ -551,3 +560,167 @@ class MongoConnection: if connections: connection.close() + + +class DjangoFlexPersistenceBackend(MongoPersistenceBackend): + """ + Backend for split mongo that can read/write from MySQL and/or S3 instead of Mongo, + either partially replacing MongoDB or fully replacing it. + """ + + # Structures and definitions are only supported in MongoDB for now. + # Course indexes are read from MySQL and written to both MongoDB and MySQL + + def get_course_index(self, key, ignore_case=False): + """ + Get the course_index from the persistence mechanism whose id is the given key + """ + if key.version_guid and not key.org: + # I don't think it was intentional, but with the MongoPersistenceBackend, using a key with only a version + # guid and no org/course/run value would not raise an error, but would always return None. So we need to be + # compatible with that. + # e.g. test_split_modulestore.py:SplitModuleCourseTests.test_get_course -> get_course(key with only version) + # > _load_items > cache_items > begin bulk operations > get_course_index > results in this situation. + log.warning("DjangoFlexPersistenceBackend: get_course_index without org/course/run will always return None") + return None + # We never include the branch or the version in the course key in the SplitModulestoreCourseIndex table: + key = key.for_branch(None).version_agnostic() + if not ignore_case: + query = {"course_id": key} + else: + # Case insensitive search is important when creating courses to reject course IDs that differ only by + # capitalization. + query = {"course_id__iexact": key} + try: + return SplitModulestoreCourseIndex.objects.get(**query).as_v1_schema() + except SplitModulestoreCourseIndex.DoesNotExist: + return None + + def find_matching_course_indexes( # pylint: disable=arguments-differ + self, + branch=None, + search_targets=None, + org_target=None, + course_context=None, + course_keys=None, + force_mongo=False, + ): + """ + Find the course_index matching particular conditions. + + Arguments: + branch: If specified, this branch must exist in the returned courses + search_targets: If specified, this must be a dictionary specifying field values + that must exist in the search_targets of the returned courses + org_target: If specified, this is an ORG filter so that only course_indexs are + returned for the specified ORG + """ + if force_mongo: + # For data migration purposes, this argument will read from MongoDB instead of MySQL + return super().find_matching_course_indexes( + branch=branch, search_targets=search_targets, org_target=org_target, + course_context=course_context, course_keys=course_keys, + ) + queryset = SplitModulestoreCourseIndex.objects.all() + if course_keys: + queryset = queryset.filter(course_id__in=course_keys) + if search_targets: + if "wiki_slug" in search_targets: + queryset = queryset.filter(wiki_slug=search_targets.pop("wiki_slug")) + if search_targets: # If there are any search targets besides wiki_slug (which we've handled by this point): + raise ValueError(f"Unsupported search_targets: {', '.join(search_targets.keys())}") + if org_target: + queryset = queryset.filter(org=org_target) + if branch is not None: + branch_field = SplitModulestoreCourseIndex.field_name_for_branch(branch) + queryset = queryset.exclude(**{branch_field: ""}) + + return (course_index.as_v1_schema() for course_index in queryset) + + def insert_course_index(self, course_index, course_context=None): # pylint: disable=arguments-differ + """ + Create the course_index in the db + """ + course_index['last_update'] = datetime.datetime.now(pytz.utc) + new_index = SplitModulestoreCourseIndex(**SplitModulestoreCourseIndex.fields_from_v1_schema(course_index)) + new_index.save() + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super().insert_course_index(course_index, course_context, last_update_already_set=True) + + def update_course_index(self, course_index, from_index=None, course_context=None): # pylint: disable=arguments-differ + """ + Update the db record for course_index. + + Arguments: + from_index: If set, only update an index if it matches the one specified in `from_index`. + + Exceptions: + SplitModulestoreCourseIndex.DoesNotExist: If the given object_id is not valid + """ + # "last_update not only tells us when this course was last updated but also helps prevent collisions" + # This code is just copying the behavior of the existing MongoPersistenceBackend + # See https://github.com/edx/edx-platform/pull/5200 for context + course_index['last_update'] = datetime.datetime.now(pytz.utc) + # Find the SplitModulestoreCourseIndex entry that we'll be updating: + index_obj = SplitModulestoreCourseIndex.objects.get(objectid=course_index["_id"]) + + # Check for collisions: + if from_index and index_obj.last_update != from_index["last_update"]: + # "last_update not only tells us when this course was last updated but also helps prevent collisions" + log.warning( + "Collision in Split Mongo when applying course index. This can happen in dev if django debug toolbar " + "is enabled, as it slows down parallel queries. New index was: %s", + course_index, + ) + return # Collision; skip this update + + # Apply updates to the index entry. While doing so, track which branch versions were changed (if any). + changed_branches = [] + for attr, value in SplitModulestoreCourseIndex.fields_from_v1_schema(course_index).items(): + if attr in ("objectid", "course_id"): + # Enforce these attributes as immutable. + if getattr(index_obj, attr) != value: + raise ValueError( + f"Attempted to change the {attr} key of a course index entry ({index_obj.course_id})" + ) + else: + if attr.endswith("_version"): + # Model fields ending in _version are branches. If the branch version has changed, convert the field + # name to a branch name and report it in the history below. + if getattr(index_obj, attr) != value: + changed_branches.append(attr[:-8]) + setattr(index_obj, attr, value) + if changed_branches: + # For the django simple history, indicate what was changed. Unfortunately at this point we only really know + # which branch(es) were changed, not anything more useful than that. + index_obj._change_reason = f'Updated {" and ".join(changed_branches)} branch' # pylint: disable=protected-access + + # Save the course index entry and create a historical record: + index_obj.save() + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super().update_course_index(course_index, from_index, course_context, last_update_already_set=True) + + def delete_course_index(self, course_key): + """ + Delete the course_index from the persistence mechanism whose id is the given course_index + """ + SplitModulestoreCourseIndex.objects.filter(course_id=course_key).delete() + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super().delete_course_index(course_key) + + def _drop_database(self, database=True, collections=True, connections=True): + """ + Reset data for testing. + """ + try: + SplitModulestoreCourseIndex.objects.all().delete() + except TransactionManagementError as err: + # If the test doesn't use 'with self.allow_transaction_exception():', then this error can occur and it may + # be non-obvious why, so give a very clear explanation of how to fix it. See the docstring of + # allow_transaction_exception() for more details. + raise RuntimeError( + "post-test cleanup failed with TransactionManagementError. " + "Use 'with self.allow_transaction_exception():' from ModuleStoreTestCase/...IsolationMixin to fix it." + ) from err + # TEMP: Also write to MongoDB, so we can switch back to using it if this new MySQL version doesn't work well: + super()._drop_database(database, collections, connections) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index ac50ace8a7..7878b85d07 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -101,7 +101,7 @@ from xmodule.modulestore.exceptions import ( VersionConflictError ) from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope -from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.partitions.partitions_service import PartitionService @@ -651,7 +651,7 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): super().__init__(contentstore, **kwargs) - self.db_connection = MongoConnection(**doc_store_config) + self.db_connection = DjangoFlexPersistenceBackend(**doc_store_config) if default_class is not None: module_path, __, class_name = default_class.rpartition('.') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 908ad97baa..bd94c642d8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -12,7 +12,7 @@ from unittest.mock import patch from django.conf import settings from django.contrib.auth.models import AnonymousUser -from django.db import connections +from django.db import connections, transaction from django.test import TestCase from django.test.utils import override_settings @@ -331,6 +331,30 @@ class ModuleStoreIsolationMixin(CacheIsolationMixin, SignalIsolationMixin): cls.end_cache_isolation() cls.enable_all_signals() + @staticmethod + def allow_transaction_exception(): + """ + Context manager to wrap modulestore-using test code that may throw an exception. + + (Use this if a modulestore test is failing with TransactionManagementError during cleanup.) + + Details: + Some test cases that purposely throw an exception may normally cause the end_modulestore_isolation() cleanup + step to fail with + TransactionManagementError: + An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block. + This happens because the test is wrapped in an implicit transaction and when the exception occurs, django won't + allow any subsequent database queries in the same transaction - in particular, the queries needed to clean up + split modulestore's SplitModulestoreCourseIndex table after the test. + + By wrapping the inner part of the test in this atomic() call, we create a savepoint so that if an exception is + thrown, Django merely rolls back to the savepoint and the overall transaction continues, including the eventual + cleanup step. + + This method mostly exists to provide this docstring/explanation; the code itself is trivial. + """ + return transaction.atomic() + class ModuleStoreTestUsersMixin(): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py index 6cb17268a7..d607c0e7e9 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_libraries.py @@ -23,17 +23,21 @@ class TestLibraries(MixedSplitTestCase): def test_create_library(self): """ - Test that we can create a library, and see how many mongo calls it uses to do so. + Test that we can create a library, and see how many database calls it uses to do so. Expected mongo calls, in order: - find_one({'org': '...', 'run': 'library', 'course': '...'}) - insert(definition: {'block_type': 'library', 'fields': {}}) + -> insert(definition: {'block_type': 'library', 'fields': {}}) + -> insert_structure(bulk) + -> insert_course_index(bulk) - insert_structure(bulk) - insert_course_index(bulk) - get_course_index(bulk) + Expected MySQL calls in order: + -> SELECT from SplitModulestoreCourseIndex case insensitive search for existing libraries + -> SELECT from SplitModulestoreCourseIndex lookup library with that exact ID + -> SELECT from XBlockConfiguration (?) + -> INSERT into SplitModulestoreCourseIndex to save the new library + -> INSERT a historical record of the SplitModulestoreCourseIndex """ - with check_mongo_calls(2, 3): + with check_mongo_calls(0, 3), self.assertNumQueries(5): LibraryFactory.create(modulestore=self.store) def test_duplicate_library(self): 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 27b014a3a1..f847630403 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -366,8 +366,9 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # Draft: # problem: One lookup to locate an item that exists # fake: one w/ wildcard version - # split has one lookup for the course and then one for the course items - @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [2, 2], 0)) + # split: has one lookup for the course and then one for the course items + # but the active_versions check is done in MySQL + @ddt.data((ModuleStoreEnum.Type.mongo, [1, 1], 0), (ModuleStoreEnum.Type.split, [1, 1], 0)) @ddt.unpack def test_has_item(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -390,17 +391,17 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # split: # problem: active_versions, structure # non-existent problem: ditto - @ddt.data((ModuleStoreEnum.Type.mongo, [3, 2], 0), (ModuleStoreEnum.Type.split, [2, 2], 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [3, 2], 0), (ModuleStoreEnum.Type.split, 1, [1, 1], 0)) @ddt.unpack - def test_get_item(self, default_ms, max_find, max_send): + def test_get_item(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) self._create_block_hierarchy() - with check_mongo_calls(max_find.pop(0), max_send): + with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql): assert self.store.get_item(self.problem_x1a_1) is not None # lint-amnesty, pylint: disable=no-member # try negative cases - with check_mongo_calls(max_find.pop(0), max_send): + with check_mongo_calls(max_find.pop(0), max_send), self.assertNumQueries(num_mysql): with pytest.raises(ItemNotFoundError): self.store.get_item(self.fake_location) @@ -411,15 +412,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # Draft: # wildcard query, 6! load pertinent items for inheritance calls, load parents, course root fetch (why) # Split: - # active_versions (with regex), structure, and spurious active_versions refetch - @ddt.data((ModuleStoreEnum.Type.mongo, 14, 0), (ModuleStoreEnum.Type.split, 4, 0)) + # mysql: fetch course's active version from SplitModulestoreCourseIndex, spurious refetch x2 + # find: get structure + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 14, 0), (ModuleStoreEnum.Type.split, 3, 1, 0)) @ddt.unpack - def test_get_items(self, default_ms, max_find, max_send): + def test_get_items(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) self._create_block_hierarchy() course_locn = self.course_locations[self.MONGO_COURSEID] - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): modules = self.store.get_items(course_locn.course_key, qualifiers={'category': 'problem'}) assert len(modules) == 6 @@ -513,13 +515,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert (orphan in [item.location for item in items_in_tree]) == orphan_in_items assert len(items_in_tree) == expected_items_in_tree - # draft: get draft, get ancestors up to course (2-6), compute inheritance + # draft: + # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) - # split: active_versions, definitions (calculator field), structures - # 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 7, 5), (ModuleStoreEnum.Type.split, 3, 2)) + # split: + # 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, 4, 2, 2)) @ddt.unpack - def test_update_item(self, default_ms, max_find, max_send): + def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ Update should succeed for r/w dbs """ @@ -529,7 +534,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # if following raised, then the test is really a noop, change it assert problem.max_attempts != 2, 'Default changed making test meaningless' problem.max_attempts = 2 - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): problem = self.store.update_item(problem, self.user_id) assert problem.max_attempts == 2, "Update didn't persist" @@ -909,11 +914,12 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # get item (to delete subtree), get inheritance again. # Sends: delete item, update parent # Split + # 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, 7, 2), (ModuleStoreEnum.Type.split, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 7, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) @ddt.unpack - def test_delete_item(self, default_ms, max_find, max_send): + def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ Delete should reject on r/o db and work on r/w one """ @@ -922,7 +928,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): max_find += 1 with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred, self.writable_chapter_location.course_key): # lint-amnesty, pylint: disable=line-too-long - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.delete_item(self.writable_chapter_location, self.user_id) # verify it's gone @@ -933,15 +939,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): self.store.get_item(self.writable_chapter_location, revision=ModuleStoreEnum.RevisionOption.published_only) # Draft: - # queries: find parent (definition.children), count versions of item, get parent, count grandparents, - # inheritance items, draft item, draft child, inheritance + # find: find parent (definition.children), count versions of item, get parent, count grandparents, + # inheritance items, draft item, draft child, inheritance # sends: delete draft vertical and update parent # Split: - # queries: active_versions, draft and published structures, definition (unnecessary) + # 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, 9, 2), (ModuleStoreEnum.Type.split, 4, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 9, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) @ddt.unpack - def test_delete_private_vertical(self, default_ms, max_find, max_send): + def test_delete_private_vertical(self, default_ms, num_mysql, 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. @@ -972,7 +979,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert vert_loc in course.children # delete the vertical and ensure the course no longer points to it - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): 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'): @@ -990,11 +997,12 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # find: active_version & structure (cached) + # 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, 4, 1), (ModuleStoreEnum.Type.split, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 4, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) @ddt.unpack - def test_delete_draft_vertical(self, default_ms, max_find, max_send): + def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ Test deleting a draft vertical which has a published version. """ @@ -1024,23 +1032,23 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # test succeeds if delete succeeds w/o error if default_ms == ModuleStoreEnum.Type.mongo and mongo_uses_error_check(self.store): max_find += 1 - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.delete_item(private_leaf.location, self.user_id) # Draft: - # 1) find all courses (wildcard), - # 2) get each course 1 at a time (1 course), - # 3) wildcard split if it has any (1) but it doesn't + # mysql: 1 select on SplitModulestoreCourseIndex since this searches both modulestores + # find: 1 find all courses (wildcard), 1 find to get each course 1 at a time (1 course) # Split: - # 1) wildcard split search, - # 2-4) active_versions, structure, definition (s/b lazy; so, unnecessary) - # 5) wildcard draft mongo which has none - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 6, 0)) + # mysql: 3 selects on SplitModulestoreCourseIndex - 1 to get all courses, 2 to get specific course (this query is + # 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, 3, 3, 0)) @ddt.unpack - def test_get_courses(self, default_ms, max_find, max_send): + def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) # we should have one course across all stores - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): courses = self.store.get_courses() course_ids = [course.location for course in courses] assert len(courses) == 1, f'Not one course: {course_ids}' @@ -1074,16 +1082,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert len(mongo_course.children) == 1 # draft is 2: find out which ms owns course, get item - # split: active_versions, structure, definition (to load course wiki string) - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 0), (ModuleStoreEnum.Type.split, 3, 0)) + # 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.unpack - def test_get_course(self, default_ms, max_find, max_send): + def test_get_course(self, default_ms, num_mysql, 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) - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): course = self.store.get_item(self.course_locations[self.MONGO_COURSEID]) assert course.id == self.course_locations[self.MONGO_COURSEID].course_key @@ -1112,16 +1120,16 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # still only 2) # Draft: get_parent # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_get_parent_locations(self, default_ms, max_find, max_send): + def test_get_parent_locations(self, default_ms, num_mysql, max_find, max_send): """ Test a simple get parent for a direct only category (i.e, always published) """ self.initdb(default_ms) self._create_block_hierarchy() - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): parent = self.store.get_parent_location(self.problem_x1a_1) # lint-amnesty, pylint: disable=no-member assert parent == self.vertical_x1a # lint-amnesty, pylint: disable=no-member @@ -1629,10 +1637,10 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # 7-8. get sequential, compute inheritance # 8-9. get vertical, compute inheritance # 10-11. get other vertical_x1b (why?) and compute inheritance - # Split: active_versions & structure - @ddt.data((ModuleStoreEnum.Type.mongo, [12, 3], 0), (ModuleStoreEnum.Type.split, [3, 2], 0)) + # Split: loading structure from mongo (also loads active version from MySQL, not tracked here) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, [12, 3], 0), (ModuleStoreEnum.Type.split, 1, [2, 1], 0)) @ddt.unpack - def test_path_to_location(self, default_ms, num_finds, num_sends): + def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): """ Make sure that path_to_location works """ @@ -1651,7 +1659,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): for location, expected in should_work: # each iteration has different find count, pop this iter's find count - with check_mongo_calls(num_finds.pop(0), num_sends): + with check_mongo_calls(num_finds.pop(0), num_sends), self.assertNumQueries(num_mysql): path = path_to_location(self.store, location) assert path == expected @@ -1868,10 +1876,10 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert False, "SplitMongoModuleStore was not found in MixedModuleStore" # Draft: get all items which can be or should have parents - # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Split: active_versions (mysql), structure (mongo) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_get_orphans(self, default_ms, max_find, max_send): + def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ Test finding orphans. """ @@ -1903,7 +1911,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): block_id=location.block_id ) - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID].course_key) self.assertCountEqual(found_orphans, orphan_locations) @@ -2004,17 +2012,17 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert self.user_id == block.subtree_edited_by assert datetime.datetime.now(UTC) > block.subtree_edited_on - # Draft: wildcard search of draft and split - # Split: wildcard search of draft and split - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Draft: wildcard search of draft (find) and split (mysql) + # Split: wildcard search of draft (find) and split (mysql) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_get_courses_for_wiki(self, default_ms, max_find, max_send): + def test_get_courses_for_wiki(self, default_ms, num_mysql, max_find, max_send): """ Test the get_courses_for_wiki method """ self.initdb(default_ms) # Test Mongo wiki - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): wiki_courses = self.store.get_courses_for_wiki('999') assert len(wiki_courses) == 1 assert self.course_locations[self.MONGO_COURSEID].course_key.replace(branch=None) in wiki_courses @@ -2028,13 +2036,18 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # 1. delete all of the published nodes in subtree # 2. insert vertical as published (deleted in step 1) w/ the deleted problems as children # 3-6. insert the 3 problems and 1 html as published - # Split: active_versions, 2 structures (pre & post published?) - # Sends: - # - insert structure - # - write index entry - @ddt.data((ModuleStoreEnum.Type.mongo, 2, 6), (ModuleStoreEnum.Type.split, 3, 2)) + # Split: + # MySQL SplitModulestoreCourseIndex: + # 1. Select by course ID + # 2. Select by objectid + # 3-4. Update index version, update historical record + # Find: 2 structures (pre & post published?) + # Sends: + # 1. insert structure + # 2. write index entry + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 6), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack - def test_unpublish(self, default_ms, max_find, max_send): + def test_unpublish(self, default_ms, num_mysql, max_find, max_send): """ Test calling unpublish """ @@ -2052,7 +2065,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert published_xblock is not None # unpublish - with check_mongo_calls(max_find, max_send): + with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): self.store.unpublish(self.vertical_x1a, self.user_id) # lint-amnesty, pylint: disable=no-member with pytest.raises(ItemNotFoundError): @@ -2069,10 +2082,10 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): assert draft_xblock is not None # Draft: specific query for revision None - # Split: active_versions, structure - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + # Split: active_versions from MySQL, structure from mongo + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack - def test_has_published_version(self, default_ms, max_find, max_send): + def test_has_published_version(self, default_ms, mysql_queries, max_find, max_send): """ Test the has_published_version method """ @@ -2082,7 +2095,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): # start off as Private item = self.store.create_child(self.user_id, self.writable_chapter_location, 'problem', 'test_compute_publish_state') # lint-amnesty, pylint: disable=line-too-long item_location = item.location - with check_mongo_calls(max_find, max_send): + with self.assertNumQueries(mysql_queries), check_mongo_calls(max_find, max_send): assert not self.store.has_published_version(item) # Private -> Public @@ -3771,7 +3784,7 @@ class TestAsidesWithMixedModuleStore(CommonMixedModuleStoreSetup): assert asides2[0].field11 == 'aside1_default_value1' assert asides2[0].field12 == 'aside1_default_value2' - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 1, 0)) @XBlockAside.register_temp_plugin(AsideFoo, 'test_aside1') @patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types', lambda self, block: ['test_aside1']) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py index 8402d2be88..ba618da8f3 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py @@ -154,14 +154,14 @@ class CountMongoCallsCourseTraversal(TestCase): (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 0, True, False, 359), # 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, 3), - (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 38), - (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 38), - (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 38), - (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 3), - (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 3), - (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 3), - (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 3) + (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, True, 2), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, True, 37), + (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, True, 37), + (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, True, 37), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, False, False, 2), + (MIXED_SPLIT_MODULESTORE_BUILDER, None, True, False, 2), + (MIXED_SPLIT_MODULESTORE_BUILDER, 0, False, False, 2), + (MIXED_SPLIT_MODULESTORE_BUILDER, 0, True, False, 2), ) @ddt.unpack def test_number_mongo_calls(self, store_builder, depth, lazy, access_all_block_fields, num_mongo_calls): @@ -178,7 +178,7 @@ class CountMongoCallsCourseTraversal(TestCase): @ddt.data( (MIXED_OLD_MONGO_MODULESTORE_BUILDER, 176), - (MIXED_SPLIT_MODULESTORE_BUILDER, 4), + (MIXED_SPLIT_MODULESTORE_BUILDER, 3), ) @ddt.unpack def test_lazy_when_course_previously_cached(self, store_builder, num_mongo_calls): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index efcf5ef68c..c2a4b16848 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -163,6 +163,7 @@ class TestPublish(SplitWMongoCourseBootstrapper): assert self.draft_mongo.has_item(other_child_loc), 'Oops, lost moved item' +@pytest.mark.django_db # required if using split modulestore class DraftPublishedOpTestCourseSetup(unittest.TestCase): """ This class exists to test XML import and export between different modulestore diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index 1739f48761..3550ef69ea 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -53,6 +53,7 @@ TEST_ASSISTANT_USER_ID = ModuleStoreEnum.UserID.test - 12 @attr('mongo') +@pytest.mark.django_db class SplitModuleTest(unittest.TestCase): ''' The base set of tests manually populates a db w/ courses which have diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py index da0a6d0d7a..6690770a86 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore_bulk_operations.py @@ -12,7 +12,7 @@ import ddt from bson.objectid import ObjectId from opaque_keys.edx.locator import CourseLocator -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import MongoPersistenceBackend from xmodule.modulestore.split_mongo.split import SplitBulkWriteMixin VERSION_GUID_DICT = { @@ -30,7 +30,7 @@ class TestBulkWriteMixin(unittest.TestCase): # lint-amnesty, pylint: disable=mi self.bulk = SplitBulkWriteMixin() self.bulk.SCHEMA_VERSION = 1 self.clear_cache = self.bulk._clear_cache = Mock(name='_clear_cache') - self.conn = self.bulk.db_connection = MagicMock(name='db_connection', spec=MongoConnection) + self.conn = self.bulk.db_connection = MagicMock(name='db_connection', spec=MongoPersistenceBackend) self.conn.get_course_index.return_value = {'initial': 'index'} self.course_key = CourseLocator('org', 'course', 'run-a', branch='test') diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py index 3460470186..008c1cc2a6 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_mongo_mongo_connection.py @@ -1,4 +1,4 @@ -""" Test the behavior of split_mongo/MongoConnection """ +""" Test the behavior of split_mongo/MongoPersistenceBackend """ import unittest @@ -8,7 +8,7 @@ import pytest from pymongo.errors import ConnectionFailure from xmodule.exceptions import HeartbeatFailure -from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection +from xmodule.modulestore.split_mongo.mongo_connection import MongoPersistenceBackend class TestHeartbeatFailureException(unittest.TestCase): @@ -20,7 +20,7 @@ class TestHeartbeatFailureException(unittest.TestCase): # pylint: disable=W0613 with patch('mongodb_proxy.MongoProxy') as mock_proxy: mock_proxy.return_value.admin.command.side_effect = ConnectionFailure('Test') - useless_conn = MongoConnection('useless', 'useless', 'useless') + useless_conn = MongoPersistenceBackend('useless', 'useless', 'useless') with pytest.raises(HeartbeatFailure): useless_conn.heartbeat() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py index 2798bcc9fe..fa55146808 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py @@ -19,6 +19,7 @@ from xmodule.x_module import XModuleMixin @pytest.mark.mongo +@pytest.mark.django_db class SplitWMongoCourseBootstrapper(unittest.TestCase): """ Helper for tests which need to construct split mongo & old mongo based courses to get interesting internal structure. # lint-amnesty, pylint: disable=line-too-long diff --git a/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py b/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py index dc3ad0813f..f1bd4b6f8d 100644 --- a/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py +++ b/common/lib/xmodule/xmodule/tests/test_course_metadata_utils.py @@ -34,6 +34,7 @@ _LAST_WEEK = _TODAY - timedelta(days=7) _NEXT_WEEK = _TODAY + timedelta(days=7) +@pytest.mark.django_db class CourseMetadataUtilsTestCase(TestCase): """ Tests for course_metadata_utils. diff --git a/lms/djangoapps/badges/backends/tests/test_badgr_backend.py b/lms/djangoapps/badges/backends/tests/test_badgr_backend.py index 3a500c117b..1817fa92a8 100644 --- a/lms/djangoapps/badges/backends/tests/test_badgr_backend.py +++ b/lms/djangoapps/badges/backends/tests/test_badgr_backend.py @@ -114,7 +114,8 @@ class BadgrBackendTestCase(ModuleStoreTestCase, EventTrackingTestCase): Verify badge spec creation works. """ self.handler._get_access_token = Mock(return_value='12345') - self.handler._create_badge(self.badge_class) + with self.allow_transaction_exception(): + self.handler._create_badge(self.badge_class) args, kwargs = post.call_args assert args[0] == 'https://example.com/v2/issuers/test-issuer/badgeclasses' assert kwargs['files']['image'][0] == self.badge_class.image.name diff --git a/lms/djangoapps/badges/tests/test_models.py b/lms/djangoapps/badges/tests/test_models.py index 154dd74704..41a49c6629 100644 --- a/lms/djangoapps/badges/tests/test_models.py +++ b/lms/djangoapps/badges/tests/test_models.py @@ -196,8 +196,8 @@ class BadgeClassTest(ModuleStoreTestCase): Verify handing incomplete data for required fields when making a badge class raises an Integrity error. """ image = get_image('good') - pytest.raises(IntegrityError, BadgeClass.get_badge_class, slug='new_slug', issuing_component='new_component', - image_file_handle=image) + with pytest.raises(IntegrityError), self.allow_transaction_exception(): + BadgeClass.get_badge_class(slug='new_slug', issuing_component='new_component', image_file_handle=image) def test_get_for_user(self): """ diff --git a/lms/djangoapps/bulk_enroll/tests/test_views.py b/lms/djangoapps/bulk_enroll/tests/test_views.py index 1ce5b1775b..70e37b15c7 100644 --- a/lms/djangoapps/bulk_enroll/tests/test_views.py +++ b/lms/djangoapps/bulk_enroll/tests/test_views.py @@ -107,7 +107,8 @@ class BulkEnrollmentTest(ModuleStoreTestCase, LoginEnrollmentTestCase, APITestCa """ Test that non global staff users are forbidden from API use. """ self.staff.is_staff = False self.staff.save() - response = self.request_bulk_enroll() + with self.allow_transaction_exception(): + response = self.request_bulk_enroll() assert response.status_code == 403 def test_missing_params(self): diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 8cfca804d3..0970ab8c99 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -271,22 +271,22 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 33 + QUERY_COUNT = 34 TEST_DATA = { - ('no_overrides', 1, True, False): (QUERY_COUNT, 3), - ('no_overrides', 2, True, False): (QUERY_COUNT, 3), - ('no_overrides', 3, True, False): (QUERY_COUNT, 3), - ('ccx', 1, True, False): (QUERY_COUNT, 3), - ('ccx', 2, True, False): (QUERY_COUNT, 3), - ('ccx', 3, True, False): (QUERY_COUNT, 3), - ('ccx', 1, True, True): (QUERY_COUNT + 2, 3), - ('ccx', 2, True, True): (QUERY_COUNT + 2, 3), - ('ccx', 3, True, True): (QUERY_COUNT + 2, 3), - ('no_overrides', 1, False, False): (QUERY_COUNT, 3), - ('no_overrides', 2, False, False): (QUERY_COUNT, 3), - ('no_overrides', 3, False, False): (QUERY_COUNT, 3), - ('ccx', 1, False, False): (QUERY_COUNT, 3), - ('ccx', 2, False, False): (QUERY_COUNT, 3), - ('ccx', 3, False, False): (QUERY_COUNT, 3), + ('no_overrides', 1, True, False): (QUERY_COUNT, 2), + ('no_overrides', 2, True, False): (QUERY_COUNT, 2), + ('no_overrides', 3, True, False): (QUERY_COUNT, 2), + ('ccx', 1, True, False): (QUERY_COUNT, 2), + ('ccx', 2, True, False): (QUERY_COUNT, 2), + ('ccx', 3, True, False): (QUERY_COUNT, 2), + ('ccx', 1, True, True): (QUERY_COUNT + 3, 2), + ('ccx', 2, True, True): (QUERY_COUNT + 3, 2), + ('ccx', 3, True, True): (QUERY_COUNT + 3, 2), + ('no_overrides', 1, False, False): (QUERY_COUNT, 2), + ('no_overrides', 2, False, False): (QUERY_COUNT, 2), + ('no_overrides', 3, False, False): (QUERY_COUNT, 2), + ('ccx', 1, False, False): (QUERY_COUNT, 2), + ('ccx', 2, False, False): (QUERY_COUNT, 2), + ('ccx', 3, False, False): (QUERY_COUNT, 2), } diff --git a/lms/djangoapps/ccx/tests/test_models.py b/lms/djangoapps/ccx/tests/test_models.py index 006ba60de5..97c8761b59 100644 --- a/lms/djangoapps/ccx/tests/test_models.py +++ b/lms/djangoapps/ccx/tests/test_models.py @@ -46,7 +46,7 @@ class TestCCX(ModuleStoreTestCase): def test_ccx_course_caching(self): """verify that caching the propery works to limit queries""" - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -72,7 +72,7 @@ class TestCCX(ModuleStoreTestCase): """verify that caching the start property works to limit queries""" now = datetime.now(utc) self.set_ccx_override('start', now) - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. @@ -97,7 +97,7 @@ class TestCCX(ModuleStoreTestCase): """verify that caching the due property works to limit queries""" expected = datetime.now(utc) self.set_ccx_override('due', expected) - with check_mongo_calls(3): + with check_mongo_calls(2): # these statements are used entirely to demonstrate the # instance-level caching of these values on CCX objects. The # check_mongo_calls context is the point here. diff --git a/lms/djangoapps/course_api/blocks/tests/test_api.py b/lms/djangoapps/course_api/blocks/tests/test_api.py index e7b3fbdd9a..5297f7f9f2 100644 --- a/lms/djangoapps/course_api/blocks/tests/test_api.py +++ b/lms/djangoapps/course_api/blocks/tests/test_api.py @@ -224,23 +224,17 @@ class TestGetBlocksQueryCounts(TestGetBlocksQueryCountsBase): ) @ddt.data( - *product( - ((ModuleStoreEnum.Type.mongo, 5), (ModuleStoreEnum.Type.split, 3)), - (True, False), - ) + (ModuleStoreEnum.Type.mongo, 5, True, 23), + (ModuleStoreEnum.Type.mongo, 5, False, 13), + (ModuleStoreEnum.Type.split, 2, True, 24), + (ModuleStoreEnum.Type.split, 2, False, 14), ) @ddt.unpack - def test_query_counts_uncached(self, store_type_tuple, with_storage_backing): - store_type, expected_mongo_queries = store_type_tuple + def test_query_counts_uncached(self, store_type, expected_mongo_queries, with_storage_backing, num_sql_queries): with override_waffle_switch(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing): course = self._create_course(store_type) clear_course_from_cache(course.id) - if with_storage_backing: - num_sql_queries = 23 - else: - num_sql_queries = 13 - self._get_blocks( course, expected_mongo_queries, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 4cc29a5e11..e6acde7d22 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -1014,9 +1014,9 @@ class TestTOC(ModuleStoreTestCase): # Split makes 2 queries to load the course to depth 2: # - 1 for the structure # - 1 for 5 definitions - # Split makes 1 query to render the toc: - # - 1 for the active version at the start of the bulk operation - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + # Split makes 1 MySQL query to render the toc: + # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): @@ -1054,9 +1054,9 @@ class TestTOC(ModuleStoreTestCase): # Split makes 2 queries to load the course to depth 2: # - 1 for the structure # - 1 for 5 definitions - # Split makes 1 query to render the toc: - # - 1 for the active version at the start of the bulk operation - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 1)) + # Split makes 1 MySQL query to render the toc: + # - 1 MySQL for the active version at the start of the bulk operation (no mongo calls) + @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0, 0), (ModuleStoreEnum.Type.split, 2, 0, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, setup_finds, setup_sends, toc_finds): with self.store.default_store(default_ms): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index ddafdde656..350bf95eda 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -389,7 +389,7 @@ class IndexQueryTestCase(ModuleStoreTestCase): @ddt.data( (ModuleStoreEnum.Type.mongo, 10, 164), - (ModuleStoreEnum.Type.split, 4, 160), + (ModuleStoreEnum.Type.split, 3, 161), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index 6c4caa6be1..e3fb0132ed 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -162,9 +162,9 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta): @ddt.data( ('vertical_block', ModuleStoreEnum.Type.mongo, 13), - ('vertical_block', ModuleStoreEnum.Type.split, 6), + ('vertical_block', ModuleStoreEnum.Type.split, 4), ('html_block', ModuleStoreEnum.Type.mongo, 14), - ('html_block', ModuleStoreEnum.Type.split, 6), + ('html_block', ModuleStoreEnum.Type.split, 4), ) @ddt.unpack def test_courseware_html(self, block_name, default_store, mongo_calls): @@ -192,7 +192,7 @@ class RenderXBlockTestMixin(MasqueradeMixin, metaclass=ABCMeta): @ddt.data( (ModuleStoreEnum.Type.mongo, 5), - (ModuleStoreEnum.Type.split, 5), + (ModuleStoreEnum.Type.split, 4), ) @ddt.unpack def test_success_enrolled_staff(self, default_store, mongo_calls): diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index efb0189809..efc96a325d 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -402,7 +402,7 @@ class ViewsQueryCountTestCase( @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 4, 39), - (ModuleStoreEnum.Type.split, 3, 13, 39), + (ModuleStoreEnum.Type.split, 3, 8, 44), ) @ddt.unpack @count_queries @@ -411,7 +411,7 @@ class ViewsQueryCountTestCase( @ddt.data( (ModuleStoreEnum.Type.mongo, 3, 3, 35), - (ModuleStoreEnum.Type.split, 3, 10, 35), + (ModuleStoreEnum.Type.split, 3, 6, 39), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 61103af01c..ffcffbef6c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -413,10 +413,10 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}}), (2, ModuleStoreEnum.Type.mongo, 2, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (2, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), - (2, ModuleStoreEnum.Type.split, 3, + (2, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), + (2, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}, "Test Topic 2": {"id": "test_topic_2"}}), - (10, ModuleStoreEnum.Type.split, 3, {"Test Topic 1": {"id": "test_topic_1"}}), + (10, ModuleStoreEnum.Type.split, 2, {"Test Topic 1": {"id": "test_topic_1"}}), ) @ddt.unpack def test_bulk_response(self, modules_count, module_store, mongo_calls, topics): diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 0cf438e231..fb1da7cb68 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -483,15 +483,15 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, False, 1, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, False, 50, 2, 2, 22, 8), # Enabling Enterprise integration should have no effect on the number of mongo queries made. (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 21, 7), (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 21, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 21, 8), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 21, 8), + (ModuleStoreEnum.Type.split, True, 1, 2, 2, 22, 8), + (ModuleStoreEnum.Type.split, True, 50, 2, 2, 22, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index c68b17f37b..cfee479e46 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -164,8 +164,8 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39, True), (ModuleStoreEnum.Type.mongo, 1, 39, False), - (ModuleStoreEnum.Type.split, 3, 39, True), - (ModuleStoreEnum.Type.split, 3, 39, False), + (ModuleStoreEnum.Type.split, 2, 40, True), + (ModuleStoreEnum.Type.split, 2, 40, False), ) @ddt.unpack def test_query_counts(self, default_store, num_mongo_calls, num_sql_calls, create_multiple_subsections): @@ -178,7 +178,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 39), - (ModuleStoreEnum.Type.split, 3, 39), + (ModuleStoreEnum.Type.split, 2, 40), ) @ddt.unpack def test_query_counts_dont_change_with_more_content(self, default_store, num_mongo_calls, num_sql_calls): @@ -224,7 +224,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 22), - (ModuleStoreEnum.Type.split, 3, 22), + (ModuleStoreEnum.Type.split, 2, 23), ) @ddt.unpack def test_persistent_grades_not_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): @@ -239,7 +239,7 @@ class RecalculateSubsectionGradeTest(HasCourseWithProblemsMixin, ModuleStoreTest @ddt.data( (ModuleStoreEnum.Type.mongo, 1, 40), - (ModuleStoreEnum.Type.split, 3, 40), + (ModuleStoreEnum.Type.split, 2, 41), ) @ddt.unpack def test_persistent_grades_enabled_on_course(self, default_store, num_mongo_queries, num_sql_queries): diff --git a/lms/djangoapps/grades/tests/test_transformer.py b/lms/djangoapps/grades/tests/test_transformer.py index f2bf8741b8..98685ea2d5 100644 --- a/lms/djangoapps/grades/tests/test_transformer.py +++ b/lms/djangoapps/grades/tests/test_transformer.py @@ -430,7 +430,7 @@ class MultiProblemModulestoreAccessTestCase(CourseStructureTestCase, SharedModul self.client.login(username=self.student.username, password=password) @ddt.data( - (ModuleStoreEnum.Type.split, 3), + (ModuleStoreEnum.Type.split, 2), (ModuleStoreEnum.Type.mongo, 2), ) @ddt.unpack diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 0713bd1f56..0840f74265 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -252,12 +252,12 @@ class TestSetDueDateExtension(ModuleStoreTestCase): def test_set_due_date_extension_invalid_date(self): extended = datetime.datetime(2009, 1, 1, 0, 0, tzinfo=UTC) - with pytest.raises(tools.DashboardError): + with pytest.raises(tools.DashboardError), self.allow_transaction_exception(): tools.set_due_date_extension(self.course, self.week1, self.user, extended) def test_set_due_date_extension_no_date(self): extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=UTC) - with pytest.raises(tools.DashboardError): + with pytest.raises(tools.DashboardError), self.allow_transaction_exception(): tools.set_due_date_extension(self.course, self.week3, self.user, extended) def test_reset_due_date_extension(self): diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 90a35f1d6b..6be624e9fd 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -673,6 +673,6 @@ class TestInstructorDashboardPerformance(ModuleStoreTestCase, LoginEnrollmentTes # check MongoDB calls count url = reverse('spoc_gradebook', kwargs={'course_id': self.course.id}) - with check_mongo_calls(9): + with check_mongo_calls(6): response = self.client.get(url) assert response.status_code == 200 diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 7420dfa9d5..c3dd3eb9af 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -371,11 +371,11 @@ 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), - (ModuleStoreEnum.Type.split, 3), + (ModuleStoreEnum.Type.mongo, 4, 46), + (ModuleStoreEnum.Type.split, 2, 47), ) @ddt.unpack - def test_query_counts(self, store_type, mongo_count): + def test_query_counts(self, store_type, mongo_count, expected_query_count): with self.store.default_store(store_type): experiment_group_a = Group(2, 'Expériment Group A') experiment_group_b = Group(3, 'Expériment Group B') @@ -401,7 +401,6 @@ class TestInstructorGradeReport(InstructorGradeReportTestCase): RequestCache.clear_all_namespaces() - expected_query_count = 46 with patch('lms.djangoapps.instructor_task.tasks_helper.runner._get_current_task'): with check_mongo_calls(mongo_count): with self.assertNumQueries(expected_query_count): diff --git a/lms/envs/common.py b/lms/envs/common.py index fdf5867418..b3d43bb36c 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2910,6 +2910,7 @@ INSTALLED_APPS = [ 'lms.djangoapps.courseware', 'lms.djangoapps.coursewarehistoryextended', 'common.djangoapps.student.apps.StudentConfig', + 'common.djangoapps.split_modulestore_django.apps.SplitModulestoreDjangoBackendAppConfig', 'lms.djangoapps.static_template_view', 'lms.djangoapps.staticbook', diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index 46e5b8ed70..df5d89c5f5 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -276,11 +276,11 @@ class BookmarkModelTests(BookmarksTestsBase): (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 4), (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 6), (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 7), - (ModuleStoreEnum.Type.split, 'course', [], 3), - (ModuleStoreEnum.Type.split, 'chapter_1', [], 2), - (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 2), - (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 2), - (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 2), + (ModuleStoreEnum.Type.split, 'course', [], 2), + (ModuleStoreEnum.Type.split, 'chapter_1', [], 1), + (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 1), + (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 1), + (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 1), ) @ddt.unpack def test_path_and_queries_on_create(self, store_type, block_to_bookmark, ancestors_attrs, expected_mongo_calls): @@ -376,11 +376,11 @@ class BookmarkModelTests(BookmarksTestsBase): # (ModuleStoreEnum.Type.mongo, 6, 3, 3), Too slow. (ModuleStoreEnum.Type.mongo, 2, 4, 4), # (ModuleStoreEnum.Type.mongo, 4, 4, 4), - (ModuleStoreEnum.Type.split, 2, 2, 2), - (ModuleStoreEnum.Type.split, 4, 2, 2), - (ModuleStoreEnum.Type.split, 2, 3, 2), - # (ModuleStoreEnum.Type.split, 4, 3, 2), - (ModuleStoreEnum.Type.split, 2, 4, 2), + (ModuleStoreEnum.Type.split, 2, 2, 1), + (ModuleStoreEnum.Type.split, 4, 2, 1), + (ModuleStoreEnum.Type.split, 2, 3, 1), + # (ModuleStoreEnum.Type.split, 4, 3, 1), + (ModuleStoreEnum.Type.split, 2, 4, 1), ) @ddt.unpack def test_get_path_queries(self, store_type, children_per_block, depth, expected_mongo_calls): diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py index f033e6bc1e..615d1b5d8a 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py @@ -108,10 +108,10 @@ class XBlockCacheTaskTests(BookmarksTestsBase): (ModuleStoreEnum.Type.mongo, 4, 3, 5), (ModuleStoreEnum.Type.mongo, 2, 4, 6), # (ModuleStoreEnum.Type.mongo, 4, 4, 6), Too slow. - (ModuleStoreEnum.Type.split, 2, 2, 3), - (ModuleStoreEnum.Type.split, 4, 2, 3), - (ModuleStoreEnum.Type.split, 2, 3, 3), - (ModuleStoreEnum.Type.split, 2, 4, 3), + (ModuleStoreEnum.Type.split, 2, 2, 2), + (ModuleStoreEnum.Type.split, 4, 2, 2), + (ModuleStoreEnum.Type.split, 2, 3, 2), + (ModuleStoreEnum.Type.split, 2, 4, 2), ) @ddt.unpack def test_calculate_course_xblocks_data_queries(self, store_type, children_per_block, depth, expected_mongo_calls): 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 425f2c89a1..39b30bd417 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, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 4, 4), (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/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index f3760cb987..8b082bef0b 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -743,7 +743,7 @@ class TestCohortsAndPartitionGroups(ModuleStoreTestCase): self.partition_id, self.group1_id, ) - with pytest.raises(IntegrityError): + with pytest.raises(IntegrityError), self.allow_transaction_exception(): self._link_cohort_partition_group( self.first_cohort, self.partition_id, diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 2c66336bce..4e00ac0b23 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -396,7 +396,15 @@ class SequenceApiTestViews(MasqueradeMixin, BaseCoursewareTests): def test_hidden_after_due(self, is_past_due, masquerade_config, expected_hidden, expected_banner): """Validate the metadata when hide-after-due is set for a sequence""" due = datetime.now() + timedelta(days=-1 if is_past_due else 1) - sequence = ItemFactory(parent=self.chapter, category='sequential', hide_after_due=True, due=due) + sequence = ItemFactory( + parent_location=self.chapter.location, + # ^ It is very important that we use parent_location=self.chapter.location (and not parent=self.chapter), as + # chapter is a class attribute and passing it by value will update its .children=[] which will then leak + # into other tests and cause errors if the children no longer exist. + category='sequential', + hide_after_due=True, + due=due, + ) CourseEnrollment.enroll(self.user, self.course.id) diff --git a/openedx/core/djangoapps/user_api/tests/test_models.py b/openedx/core/djangoapps/user_api/tests/test_models.py index 6a5d64bd24..5634aa00b9 100644 --- a/openedx/core/djangoapps/user_api/tests/test_models.py +++ b/openedx/core/djangoapps/user_api/tests/test_models.py @@ -23,7 +23,8 @@ class UserPreferenceModelTest(ModuleStoreTestCase): def test_duplicate_user_key(self): user = UserFactory.create() UserPreferenceFactory.create(user=user, key="testkey", value="first") - pytest.raises(IntegrityError, UserPreferenceFactory.create) + with self.allow_transaction_exception(): + pytest.raises(IntegrityError, UserPreferenceFactory.create) def test_arbitrary_values(self): user = UserFactory.create() diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 36dd785966..e8499de9fb 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -205,8 +205,8 @@ class TestCourseHomePage(CourseHomePageTestCase): # lint-amnesty, pylint: disab # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(72, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(4): + with self.assertNumQueries(73, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(3): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index a3abea4b3d..a246486e2d 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -49,7 +49,7 @@ class TestCourseUpdatesPage(BaseCourseUpdatesTestCase): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(50, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): - with check_mongo_calls(4): + with self.assertNumQueries(51, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with check_mongo_calls(3): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/tests/completion_integration/test_models.py b/openedx/tests/completion_integration/test_models.py index 8871272b3c..74c61de254 100644 --- a/openedx/tests/completion_integration/test_models.py +++ b/openedx/tests/completion_integration/test_models.py @@ -62,8 +62,8 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): self.set_up_completion() def test_changed_value(self): - with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 2 * OTHER): - # OTHER = user exists, completion exists + with self.assertNumQueries(SELECT + UPDATE + 2 * SAVEPOINT + 4 * OTHER): + # OTHER = user exists, completion exists, 2x look up course in splitmodulestorecourseindex completion, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, block_key=self.block_key, @@ -88,7 +88,7 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): def test_new_user(self): newuser = UserFactory() - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=newuser, block_key=self.block_key, @@ -99,7 +99,7 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): def test_new_block(self): newblock = UsageKey.from_string('block-v1:edx+test+run+type@video+block@puppers') - with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): + with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT + 2 * OTHER): _, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, block_key=newblock,