From 639658913d024b9f048c1d64f96d294d0eef0e40 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Tue, 8 Jul 2014 15:24:08 -0400 Subject: [PATCH] Refactor split migrator LMS-2936 Also, a bunch of ancillary cleanups. Conflicts: common/lib/xmodule/xmodule/modulestore/tests/test_publish.py Conflicts: cms/djangoapps/contentstore/management/commands/migrate_to_split.py cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py common/lib/xmodule/xmodule/modulestore/__init__.py common/lib/xmodule/xmodule/modulestore/mixed.py common/lib/xmodule/xmodule/modulestore/mongo/draft.py common/lib/xmodule/xmodule/modulestore/split_migrator.py common/lib/xmodule/xmodule/modulestore/split_mongo/split.py common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py common/lib/xmodule/xmodule/modulestore/tests/test_split_w_old_mongo.py --- .../management/commands/migrate_to_split.py | 33 +++-- .../commands/rollback_split_course.py | 51 ------- .../commands/tests/test_migrate_to_split.py | 2 +- .../tests/test_rollback_split_course.py | 110 -------------- .../contentstore/tests/test_clone_course.py | 4 - .../contentstore/tests/test_crud.py | 38 +---- cms/djangoapps/contentstore/tests/utils.py | 80 ++++++++--- cms/djangoapps/contentstore/views/course.py | 2 +- cms/envs/test.py | 2 +- .../external_auth/tests/test_ssl.py | 3 - .../lib/xmodule/xmodule/contentstore/mongo.py | 7 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 34 ++--- .../xmodule/xmodule/modulestore/mongo/base.py | 10 +- .../xmodule/modulestore/mongo/draft.py | 16 +-- .../xmodule/modulestore/split_migrator.py | 136 ++++++++++-------- .../modulestore/split_mongo/__init__.py | 23 ++- .../split_mongo/caching_descriptor_system.py | 10 +- .../xmodule/modulestore/split_mongo/split.py | 114 +++++++++------ .../xmodule/modulestore/tests/django_utils.py | 5 +- .../tests/test_mixed_modulestore.py | 23 +-- .../xmodule/modulestore/tests/test_orphan.py | 2 +- .../xmodule/modulestore/tests/test_publish.py | 29 ++-- .../modulestore/tests/test_split_migrator.py | 56 +++----- .../tests/test_split_modulestore.py | 6 +- .../tests/test_split_w_old_mongo.py | 33 +++-- 25 files changed, 351 insertions(+), 478 deletions(-) delete mode 100644 cms/djangoapps/contentstore/management/commands/rollback_split_course.py delete mode 100644 cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py diff --git a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py index 9d7b0953b2..bb164fc6d8 100644 --- a/cms/djangoapps/contentstore/management/commands/migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/migrate_to_split.py @@ -1,5 +1,3 @@ -# pylint: disable=protected-access - """ Django management command to migrate a course from the old Mongo modulestore to the new split-Mongo modulestore. @@ -8,7 +6,6 @@ from django.core.management.base import BaseCommand, CommandError from django.contrib.auth.models import User from xmodule.modulestore.django import modulestore from xmodule.modulestore.split_migrator import SplitMigrator -from xmodule.modulestore.django import loc_mapper from opaque_keys.edx.keys import CourseKey from opaque_keys import InvalidKeyError from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -26,20 +23,21 @@ def user_from_str(identifier): user_id = int(identifier) except ValueError: return User.objects.get(email=identifier) - else: - return User.objects.get(id=user_id) + + return User.objects.get(id=user_id) class Command(BaseCommand): - "Migrate a course from old-Mongo to split-Mongo" + """ + Migrate a course from old-Mongo to split-Mongo. It reuses the old course id except where overridden. + """ - help = "Migrate a course from old-Mongo to split-Mongo" - args = "course_key email " + help = "Migrate a course from old-Mongo to split-Mongo. The new org, course, and run will default to the old one unless overridden" + args = "course_key email " def parse_args(self, *args): """ - Return a 4-tuple of (course_key, user, org, offering). - If the user didn't specify an org & offering, those will be None. + Return a 5-tuple of passed in values for (course_key, user, org, course, run). """ if len(args) < 2: raise CommandError( @@ -57,21 +55,22 @@ class Command(BaseCommand): except User.DoesNotExist: raise CommandError("No user found identified by {}".format(args[1])) + org = course = run = None try: org = args[2] - offering = args[3] + course = args[3] + run = args[4] except IndexError: - org = offering = None + pass - return course_key, user, org, offering + return course_key, user, org, course, run def handle(self, *args, **options): - course_key, user, org, offering = self.parse_args(*args) + course_key, user, org, course, run = self.parse_args(*args) migrator = SplitMigrator( - draft_modulestore=modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo), + source_modulestore=modulestore(), split_modulestore=modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split), - loc_mapper=loc_mapper(), ) - migrator.migrate_mongo_course(course_key, user, org, offering) + migrator.migrate_mongo_course(course_key, user, org, course, run) diff --git a/cms/djangoapps/contentstore/management/commands/rollback_split_course.py b/cms/djangoapps/contentstore/management/commands/rollback_split_course.py deleted file mode 100644 index 46273c0589..0000000000 --- a/cms/djangoapps/contentstore/management/commands/rollback_split_course.py +++ /dev/null @@ -1,51 +0,0 @@ -""" -Django management command to rollback a migration to split. The way to do this -is to delete the course from the split mongo datastore. -""" -from django.core.management.base import BaseCommand, CommandError -from xmodule.modulestore.django import modulestore, loc_mapper -from xmodule.modulestore.exceptions import ItemNotFoundError -from opaque_keys.edx.locator import CourseLocator - - -class Command(BaseCommand): - "Rollback a course that was migrated to the split Mongo datastore" - - help = "Rollback a course that was migrated to the split Mongo datastore" - args = "org offering" - - def handle(self, *args, **options): - if len(args) < 2: - raise CommandError( - "rollback_split_course requires 2 arguments (org offering)" - ) - - try: - locator = CourseLocator(org=args[0], offering=args[1]) - except ValueError: - raise CommandError("Invalid org or offering string {}, {}".format(*args)) - - location = loc_mapper().translate_locator_to_location(locator, get_course=True) - if not location: - raise CommandError( - "This course does not exist in the old Mongo store. " - "This command is designed to rollback a course, not delete " - "it entirely." - ) - old_mongo_course = modulestore('direct').get_item(location) - if not old_mongo_course: - raise CommandError( - "This course does not exist in the old Mongo store. " - "This command is designed to rollback a course, not delete " - "it entirely." - ) - - try: - modulestore('split').delete_course(locator) - except ItemNotFoundError: - raise CommandError("No course found with locator {}".format(locator)) - - print( - 'Course rolled back successfully. To delete this course entirely, ' - 'call the "delete_course" management command.' - ) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py index cbc47f6ee7..4b3b284519 100644 --- a/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py +++ b/cms/djangoapps/contentstore/management/commands/tests/test_migrate_to_split.py @@ -84,6 +84,6 @@ class TestMigrateToSplit(ModuleStoreTestCase): str(self.user.id), "org.dept+name.run", ) - locator = CourseLocator(org="org.dept", offering="name.run", branch=ModuleStoreEnum.RevisionOption.published_only) + locator = CourseLocator(org="org.dept", course="name", run="run", branch=ModuleStoreEnum.RevisionOption.published_only) course_from_split = modulestore('split').get_course(locator) self.assertIsNotNone(course_from_split) diff --git a/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py b/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py deleted file mode 100644 index 4083f4a4f8..0000000000 --- a/cms/djangoapps/contentstore/management/commands/tests/test_rollback_split_course.py +++ /dev/null @@ -1,110 +0,0 @@ -""" -Unittests for deleting a split mongo course -""" -import unittest -from StringIO import StringIO -from mock import patch - -from django.contrib.auth.models import User -from django.core.management import CommandError, call_command -from contentstore.management.commands.rollback_split_course import Command -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.persistent_factories import PersistentCourseFactory -from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.django import modulestore, loc_mapper -from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.split_migrator import SplitMigrator -from xmodule.modulestore import ModuleStoreEnum -# pylint: disable=E1101 -# pylint: disable=W0212 - - -@unittest.skip("Not fixing split mongo until we land opaque-keys 0.9") -class TestArgParsing(unittest.TestCase): - """ - Tests for parsing arguments for the `rollback_split_course` management command - """ - def setUp(self): - self.command = Command() - - def test_no_args(self): - errstring = "rollback_split_course requires at least one argument" - with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle() - - def test_invalid_locator(self): - errstring = "Invalid locator string !?!" - with self.assertRaisesRegexp(CommandError, errstring): - self.command.handle("!?!") - - -@unittest.skip("Not fixing split mongo until we land opaque-keys 0.9") -class TestRollbackSplitCourseNoOldMongo(ModuleStoreTestCase): - """ - Unit tests for rolling back a split-mongo course from command line, - where the course doesn't exist in the old mongo store - """ - - def setUp(self): - super(TestRollbackSplitCourseNoOldMongo, self).setUp() - self.course = PersistentCourseFactory() - - def test_no_old_course(self): - locator = self.course.location - errstring = "course does not exist in the old Mongo store" - with self.assertRaisesRegexp(CommandError, errstring): - Command().handle(str(locator)) - - -@unittest.skip("Not fixing split mongo until we land opaque-keys 0.9") -class TestRollbackSplitCourseNoSplitMongo(ModuleStoreTestCase): - """ - Unit tests for rolling back a split-mongo course from command line, - where the course doesn't exist in the split mongo store - """ - - def setUp(self): - super(TestRollbackSplitCourseNoSplitMongo, self).setUp() - self.old_course = CourseFactory() - - def test_nonexistent_locator(self): - locator = loc_mapper().translate_location(self.old_course.location) - errstring = "No course found with locator" - with self.assertRaisesRegexp(CommandError, errstring): - Command().handle(str(locator)) - - -@unittest.skip("Not fixing split mongo until we land opaque-keys 0.9") -class TestRollbackSplitCourse(ModuleStoreTestCase): - """ - Unit tests for rolling back a split-mongo course from command line - """ - def setUp(self): - super(TestRollbackSplitCourse, self).setUp() - self.old_course = CourseFactory() - uname = 'testuser' - email = 'test+courses@edx.org' - password = 'foo' - self.user = User.objects.create_user(uname, email, password) - - # migrate old course to split - migrator = SplitMigrator( - draft_modulestore=modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.mongo), - split_modulestore=modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split), - loc_mapper=loc_mapper(), - ) - migrator.migrate_mongo_course(self.old_course.location, self.user) - self.course = modulestore('split').get_course(self.old_course.id) - - @patch("sys.stdout", new_callable=StringIO) - def test_happy_path(self, mock_stdout): - course_id = self.course.id - call_command( - "rollback_split_course", - str(course_id), - ) - with self.assertRaises(ItemNotFoundError): - modulestore('split').get_course(course_id) - - self.assertIn("Course rolled back successfully", mock_stdout.getvalue()) - diff --git a/cms/djangoapps/contentstore/tests/test_clone_course.py b/cms/djangoapps/contentstore/tests/test_clone_course.py index b0eaf6f927..71f148820a 100644 --- a/cms/djangoapps/contentstore/tests/test_clone_course.py +++ b/cms/djangoapps/contentstore/tests/test_clone_course.py @@ -1,8 +1,6 @@ """ Unit tests for cloning a course between the same and different module stores. """ -from django.utils.unittest.case import skipIf -from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.locator import CourseLocator from xmodule.modulestore import ModuleStoreEnum from contentstore.tests.utils import CourseTestCase @@ -12,8 +10,6 @@ class CloneCourseTest(CourseTestCase): """ Unit tests for cloning a course """ - # TODO Don is fixing this on his branch of split migrator - @skipIf(True, "Don is still working on split migrator") def test_clone_course(self): """Tests cloning of a course as follows: XML -> Mongo (+ data) -> Mongo -> Split -> Split""" # 1. import and populate test toy course diff --git a/cms/djangoapps/contentstore/tests/test_crud.py b/cms/djangoapps/contentstore/tests/test_crud.py index cb1b8f0bfe..e5d608eb89 100644 --- a/cms/djangoapps/contentstore/tests/test_crud.py +++ b/cms/djangoapps/contentstore/tests/test_crud.py @@ -4,8 +4,7 @@ from xmodule import templates from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests import persistent_factories from xmodule.course_module import CourseDescriptor -from xmodule.modulestore.django import modulestore, clear_existing_modulestores, _MIXED_MODULESTORE, \ - loc_mapper, _loc_singleton +from xmodule.modulestore.django import modulestore, clear_existing_modulestores from xmodule.seq_module import SequenceDescriptor from xmodule.capa_module import CapaDescriptor from opaque_keys.edx.locator import BlockUsageLocator, LocalId @@ -225,38 +224,3 @@ class TemplateTests(unittest.TestCase): version_history = self.split_store.get_block_generations(second_problem.location) self.assertNotEqual(version_history.locator.version_guid, first_problem.location.version_guid) - - -class SplitAndLocMapperTests(unittest.TestCase): - """ - Test injection of loc_mapper into Split - """ - def test_split_inject_loc_mapper(self): - """ - Test loc_mapper created before split - """ - # ensure modulestore is not instantiated - self.assertIsNone(_MIXED_MODULESTORE) - - # instantiate location mapper before split - mapper = loc_mapper() - - # instantiate mixed modulestore and thus split - split_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split) - - # split must inject the same location mapper object since the mapper existed before it did - self.assertEqual(split_store.loc_mapper, mapper) - - def test_loc_inject_into_split(self): - """ - Test split created before loc_mapper - """ - # ensure loc_mapper is not instantiated - self.assertIsNone(_loc_singleton) - - # instantiate split before location mapper - split_store = modulestore()._get_modulestore_by_type(ModuleStoreEnum.Type.split) - - # split must have instantiated loc_mapper - mapper = loc_mapper() - self.assertEqual(split_store.loc_mapper, mapper) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index f1a20fab87..bd029fe99b 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -4,15 +4,12 @@ Utilities for contentstore tests ''' import json -import re from django.test.client import Client from django.contrib.auth.models import User from xmodule.contentstore.django import contentstore -from xmodule.contentstore.content import StaticContent -from xmodule.modulestore import PublishState, ModuleStoreEnum -from xmodule.modulestore.django import modulestore +from xmodule.modulestore import PublishState, ModuleStoreEnum, mongo from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -20,6 +17,9 @@ from xmodule.modulestore.xml_importer import import_from_xml from student.models import Registration from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation from contentstore.utils import reverse_url +from xmodule.modulestore.mongo.draft import DraftModuleStore +from xblock.fields import Scope +from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore def parse_json(response): @@ -249,14 +249,24 @@ class CourseTestCase(ModuleStoreTestCase): for course1_item in course1_items: course2_item_location = course1_item.location.map_into_course(course2_id) if course1_item.location.category == 'course': - course2_item_location = course2_item_location.replace(name=course2_item_location.run) + # mongo uses the run as the name, split uses 'course' + store = self.store._get_modulestore_for_courseid(course2_id) # pylint: disable=protected-access + new_name = 'course' if isinstance(store, SplitMongoModuleStore) else course2_item_location.run + course2_item_location = course2_item_location.replace(name=new_name) course2_item = self.store.get_item(course2_item_location) - # compare published state - self.assertEqual( - self.store.compute_publish_state(course1_item), - self.store.compute_publish_state(course2_item) - ) + try: + # compare published state + self.assertEqual( + self.store.compute_publish_state(course1_item), + self.store.compute_publish_state(course2_item) + ) + except AssertionError: + # old mongo calls things draft if draft exists even if it's != published; so, do more work + self.assertEqual( + self.compute_real_state(course1_item), + self.compute_real_state(course2_item) + ) # compare data self.assertEqual(hasattr(course1_item, 'data'), hasattr(course2_item, 'data')) @@ -274,17 +284,18 @@ class CourseTestCase(ModuleStoreTestCase): expected_children.append( course1_item_child.map_into_course(course2_id) ) - self.assertEqual(expected_children, course2_item.children) + # also process course2_children just in case they have version guids + course2_children = [child.version_agnostic() for child in course2_item.children] + self.assertEqual(expected_children, course2_children) # compare assets - content_store = contentstore() + content_store = self.store.contentstore course1_assets, count_course1_assets = content_store.get_all_content_for_course(course1_id) _, count_course2_assets = content_store.get_all_content_for_course(course2_id) self.assertEqual(count_course1_assets, count_course2_assets) for asset in course1_assets: - asset_id = asset.get('content_son', asset['_id']) - asset_key = StaticContent.compute_location(course1_id, asset_id['name']) - self.assertAssetsEqual(asset_key, course1_id, course2_id) + asset_son = asset.get('content_son', asset['_id']) + self.assertAssetsEqual(asset_son, course1_id, course2_id) def check_verticals(self, items): """ Test getting the editing HTML for each vertical. """ @@ -294,20 +305,47 @@ class CourseTestCase(ModuleStoreTestCase): resp = self.client.get_html(get_url('unit_handler', descriptor.location)) self.assertEqual(resp.status_code, 200) - def assertAssetsEqual(self, asset_key, course1_id, course2_id): + def assertAssetsEqual(self, asset_son, course1_id, course2_id): """Verifies the asset of the given key has the same attributes in both given courses.""" content_store = contentstore() - course1_asset_attrs = content_store.get_attrs(asset_key.map_into_course(course1_id)) - course2_asset_attrs = content_store.get_attrs(asset_key.map_into_course(course2_id)) + category = asset_son.block_type if hasattr(asset_son, 'block_type') else asset_son['category'] + filename = asset_son.block_id if hasattr(asset_son, 'block_id') else asset_son['name'] + course1_asset_attrs = content_store.get_attrs(course1_id.make_asset_key(category, filename)) + course2_asset_attrs = content_store.get_attrs(course2_id.make_asset_key(category, filename)) self.assertEqual(len(course1_asset_attrs), len(course2_asset_attrs)) for key, value in course1_asset_attrs.iteritems(): - if key == '_id': - self.assertEqual(value['name'], course2_asset_attrs[key]['name']) - elif key == 'filename' or key == 'uploadDate' or key == 'content_son' or key == 'thumbnail_location': + if key in ['_id', 'filename', 'uploadDate', 'content_son', 'thumbnail_location']: pass else: self.assertEqual(value, course2_asset_attrs[key]) + def compute_real_state(self, item): + """ + In draft mongo, compute_published_state can return draft when the draft == published, but in split, + it'll return public in that case + """ + supposed_state = self.store.compute_publish_state(item) + if supposed_state == PublishState.draft and isinstance(item.runtime.modulestore, DraftModuleStore): + # see if the draft differs from the published + published = self.store.get_item(item.location, revision=ModuleStoreEnum.RevisionOption.published_only) + if item.get_explicitly_set_fields_by_scope() != published.get_explicitly_set_fields_by_scope(): + return supposed_state + if item.get_explicitly_set_fields_by_scope(Scope.settings) != published.get_explicitly_set_fields_by_scope(Scope.settings): + return supposed_state + if item.has_children and item.children != published.children: + return supposed_state + return PublishState.public + elif supposed_state == PublishState.public and item.location.category in mongo.base.DIRECT_ONLY_CATEGORIES: + if not all([ + self.store.has_item(child_loc, revision=ModuleStoreEnum.RevisionOption.draft_only) + for child_loc in item.children + ]): + return PublishState.draft + else: + return supposed_state + else: + return supposed_state + def get_url(handler_name, key_value, key_name='usage_key_string', kwargs=None): """ diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index f5f266a1d4..b4cd5cf7d8 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -109,7 +109,7 @@ def course_handler(request, course_key_string=None): index entry. PUT json: update this course (index entry not xblock) such as repointing head, changing display name, org, - offering. Return same json as above. + course, run. Return same json as above. DELETE json: delete this branch from this course (leaving off /branch/draft would imply delete the course) """ diff --git a/cms/envs/test.py b/cms/envs/test.py index 480a5cd641..46239478b6 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -63,7 +63,7 @@ STATICFILES_DIRS += [ MODULESTORE['default']['OPTIONS']['stores'].append( { 'NAME': 'split', - 'ENGINE': 'xmodule.modulestore.split_mongo.SplitMongoModuleStore', + 'ENGINE': 'xmodule.modulestore.split_mongo.split.SplitMongoModuleStore', 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, 'OPTIONS': { 'render_template': 'edxmako.shortcuts.render_to_string', diff --git a/common/djangoapps/external_auth/tests/test_ssl.py b/common/djangoapps/external_auth/tests/test_ssl.py index 614a753459..70bd50b2f1 100644 --- a/common/djangoapps/external_auth/tests/test_ssl.py +++ b/common/djangoapps/external_auth/tests/test_ssl.py @@ -10,7 +10,6 @@ from django.contrib.auth import SESSION_KEY from django.contrib.auth.models import AnonymousUser, User from django.contrib.sessions.middleware import SessionMiddleware from django.core.urlresolvers import reverse -from django.test import TestCase from django.test.client import Client from django.test.client import RequestFactory from django.test.utils import override_settings @@ -23,8 +22,6 @@ from opaque_keys import InvalidKeyError from student.models import CourseEnrollment from student.roles import CourseStaffRole from student.tests.factories import UserFactory -from xmodule.modulestore.django import loc_mapper -from xmodule.modulestore.exceptions import InsufficientSpecificationError from xmodule.modulestore.tests.django_utils import (ModuleStoreTestCase, mixed_store_config) from xmodule.modulestore.tests.factories import CourseFactory diff --git a/common/lib/xmodule/xmodule/contentstore/mongo.py b/common/lib/xmodule/xmodule/contentstore/mongo.py index d3f9bdb288..fddb4f51ad 100644 --- a/common/lib/xmodule/xmodule/contentstore/mongo.py +++ b/common/lib/xmodule/xmodule/contentstore/mongo.py @@ -12,8 +12,7 @@ from fs.osfs import OSFS import os import json from bson.son import SON -from opaque_keys.edx.locator import AssetLocator -from opaque_keys.edx.locations import AssetLocation +from opaque_keys.edx.keys import AssetKey class MongoContentStore(ContentStore): @@ -74,7 +73,7 @@ class MongoContentStore(ContentStore): return content def delete(self, location_or_id): - if isinstance(location_or_id, AssetLocator): + if isinstance(location_or_id, AssetKey): location_or_id, _ = self.asset_db_key(location_or_id) # Deletes of non-existent files are considered successful self.fs.delete(location_or_id) @@ -272,7 +271,7 @@ class MongoContentStore(ContentStore): # don't convert from string until fs access source_content = self.fs.get(asset_key) if isinstance(asset_key, basestring): - asset_key = AssetLocation.from_string(asset_key) + asset_key = AssetKey.from_string(asset_key) __, asset_key = self.asset_db_key(asset_key) asset_key['org'] = dest_course_key.org asset_key['course'] = dest_course_key.course diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 78ef997f61..a82c29e9df 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -11,8 +11,8 @@ from contextlib import contextmanager from opaque_keys import InvalidKeyError from . import ModuleStoreWriteBase -from xmodule.modulestore import PublishState, ModuleStoreEnum, split_migrator -from xmodule.modulestore.django import create_modulestore_instance, loc_mapper +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import create_modulestore_instance from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator from xmodule.modulestore.exceptions import ItemNotFoundError from opaque_keys.edx.keys import CourseKey, UsageKey @@ -20,6 +20,7 @@ from xmodule.modulestore.mongo.base import MongoModuleStore from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore from opaque_keys.edx.locations import SlashSeparatedCourseKey import itertools +from xmodule.modulestore.split_migrator import SplitMigrator log = logging.getLogger(__name__) @@ -66,8 +67,6 @@ class MixedModuleStore(ModuleStoreWriteBase): store_settings.get('OPTIONS', {}), i18n_service=i18n_service, ) - if key == 'split': - store.loc_mapper = loc_mapper() # replace all named pointers to the store into actual pointers for course_key, store_name in self.mappings.iteritems(): if store_name == key: @@ -83,8 +82,8 @@ class MixedModuleStore(ModuleStoreWriteBase): """ if hasattr(course_id, 'version_agnostic'): course_id = course_id.version_agnostic() - if hasattr(course_id, 'branch_agnostic'): - course_id = course_id.branch_agnostic() + if hasattr(course_id, 'branch'): + course_id = course_id.replace(branch=None) return course_id def _get_modulestore_for_courseid(self, course_id=None): @@ -185,26 +184,14 @@ class MixedModuleStore(ModuleStoreWriteBase): # check if the course is not None - possible if the mappings file is outdated # TODO - log an error if the course is None, but move it to an initialization method to keep it less noisy if course is not None: - courses[course_id] = store.get_course(course_id) + courses[course_id] = course - has_locators = any(issubclass(CourseLocator, store.reference_type) for store in self.modulestores) for store in self.modulestores: # filter out ones which were fetched from earlier stores but locations may not be == for course in store.get_courses(): course_id = self._clean_course_id_for_mapping(course.id) if course_id not in courses: - if has_locators and isinstance(course_id, CourseKey): - - # see if a locator version of course is in the result - try: - course_locator = loc_mapper().translate_location_to_course_locator(course_id) - if course_locator in courses: - continue - except ItemNotFoundError: - # if there's no existing mapping, then the course can't have been in split - pass - # course is indeed unique. save it in result courses[course_id] = course @@ -325,12 +312,9 @@ class MixedModuleStore(ModuleStoreWriteBase): super(MixedModuleStore, self).clone_course(source_course_id, dest_course_id, user_id) if dest_modulestore.get_modulestore_type() == ModuleStoreEnum.Type.split: - if not hasattr(self, 'split_migrator'): - self.split_migrator = split_migrator.SplitMigrator( - dest_modulestore, source_modulestore, loc_mapper() - ) - self.split_migrator.migrate_mongo_course( - source_course_id, user_id, dest_course_id.org, dest_course_id.offering + split_migrator = SplitMigrator(dest_modulestore, source_modulestore) + split_migrator.migrate_mongo_course( + source_course_id, user_id, dest_course_id.org, dest_course_id.course, dest_course_id.run ) def create_item(self, course_or_parent_loc, category, user_id, **kwargs): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index d3043f2d77..281f48dfea 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -478,7 +478,8 @@ class MongoModuleStore(ModuleStoreWriteBase): existing_children = results_by_url[location_url].get('definition', {}).get('children', []) additional_children = result.get('definition', {}).get('children', []) total_children = existing_children + additional_children - results_by_url[location_url].setdefault('definition', {})['children'] = total_children + # use set to get rid of duplicates. We don't care about order; so, it shouldn't matter. + results_by_url[location_url].setdefault('definition', {})['children'] = set(total_children) else: results_by_url[location_url] = result if location.category == 'course': @@ -520,8 +521,8 @@ class MongoModuleStore(ModuleStoreWriteBase): 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 course_id in self.request_cache.data.get('metadata_inheritance', {}): - return self.request_cache.data['metadata_inheritance'][course_id] + if self.request_cache is not None and unicode(course_id) in self.request_cache.data.get('metadata_inheritance', {}): + return self.request_cache.data['metadata_inheritance'][unicode(course_id)] # then look in any caching subsystem (e.g. memcached) if self.metadata_inheritance_cache_subsystem is not None: @@ -548,7 +549,7 @@ class MongoModuleStore(ModuleStoreWriteBase): # defined if 'metadata_inheritance' not in self.request_cache.data: self.request_cache.data['metadata_inheritance'] = {} - self.request_cache.data['metadata_inheritance'][course_id] = tree + self.request_cache.data['metadata_inheritance'][unicode(course_id)] = tree return tree @@ -560,6 +561,7 @@ class MongoModuleStore(ModuleStoreWriteBase): 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 course_id not in self.ignore_write_events_on_courses: # below is done for side effects when runtime is None cached_metadata = self._get_cached_metadata_inheritance_tree(course_id, force_refresh=True) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 35e9926885..9e26c211dc 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -371,7 +371,11 @@ class DraftModuleStore(MongoModuleStore): DuplicateItemError: if the source or any of its descendants already has a draft copy """ # delegating to internal b/c we don't want any public user to use the kwargs on the internal - return self._convert_to_draft(location, user_id) + self._convert_to_draft(location, user_id) + + # return the new draft item (does another fetch) + # get_item will wrap_draft so don't call it here (otherwise, it would override the is_draft attribute) + return self.get_item(location) def _convert_to_draft(self, location, user_id, delete_published=False, ignore_if_draft=False): """ @@ -427,10 +431,6 @@ class DraftModuleStore(MongoModuleStore): # convert the subtree using the original item as the root self._breadth_first(convert_item, [location]) - # return the new draft item (does another fetch) - # get_item will wrap_draft so don't call it here (otherwise, it would override the is_draft attribute) - return self.get_item(location) - def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False): """ See superclass doc. @@ -551,6 +551,8 @@ 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): """ @@ -579,8 +581,6 @@ class DraftModuleStore(MongoModuleStore): _internal([root_usage.to_deprecated_son() for root_usage in root_usages]) self.collection.remove({'_id': {'$in': to_be_deleted}}, safe=self.collection.safe) - # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(root_usages[0].course_key) def has_changes(self, location): """ @@ -682,7 +682,7 @@ class DraftModuleStore(MongoModuleStore): to remove things from the published version """ self._verify_branch_setting(ModuleStoreEnum.Branch.draft_preferred) - return self._convert_to_draft(location, user_id, delete_published=True) + self._convert_to_draft(location, user_id, delete_published=True) def revert_to_published(self, location, user_id=None): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index f58f6d96d2..4b485d1bda 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -6,8 +6,13 @@ Exists at the top level of modulestore b/c it needs to know about and access eac In general, it's strategy is to treat the other modulestores as read-only and to never directly manipulate storage but use existing api's. ''' +import logging + from xblock.fields import Reference, ReferenceList, ReferenceValueDict from xmodule.modulestore import ModuleStoreEnum +from opaque_keys.edx.locator import CourseLocator + +log = logging.getLogger(__name__) class SplitMigrator(object): @@ -15,51 +20,53 @@ class SplitMigrator(object): Copies courses from old mongo to split mongo and sets up location mapping so any references to the old name will be able to find the new elements. """ - def __init__(self, split_modulestore, draft_modulestore, loc_mapper): + def __init__(self, split_modulestore, source_modulestore): super(SplitMigrator, self).__init__() self.split_modulestore = split_modulestore - self.draft_modulestore = draft_modulestore - self.loc_mapper = loc_mapper + self.source_modulestore = source_modulestore - def migrate_mongo_course(self, course_key, user, new_org=None, new_offering=None): + def migrate_mongo_course(self, source_course_key, user_id, new_org=None, new_course=None, new_run=None): """ Create a new course in split_mongo representing the published and draft versions of the course from the original mongo store. And return the new CourseLocator If the new course already exists, this raises DuplicateItemError - :param course_location: a Location whose category is 'course' and points to the course - :param user: the user whose action is causing this migration - :param new_org: (optional) the Locator.org for the new course. Defaults to - whatever translate_location_to_locator returns - :param new_offering: (optional) the Locator.offering for the new course. Defaults to - whatever translate_location_to_locator returns + :param source_course_key: which course to migrate + :param user_id: the user whose action is causing this migration + :param new_org, new_course, new_run: (optional) identifiers for the new course. Defaults to + the source_course_key's values. """ - new_course_locator = self.loc_mapper.create_map_entry(course_key, new_org, new_offering) # the only difference in data between the old and split_mongo xblocks are the locations; # so, any field which holds a location must change to a Locator; otherwise, the persistence # layer and kvs's know how to store it. # locations are in location, children, conditionals, course.tab # create the course: set fields to explicitly_set for each scope, id_root = new_course_locator, master_branch = 'production' - original_course = self.draft_modulestore.get_course(course_key) - new_course_root_locator = self.loc_mapper.translate_location(original_course.location) + original_course = self.source_modulestore.get_course(source_course_key) + + if new_org is None: + new_org = source_course_key.org + if new_course is None: + new_course = source_course_key.course + if new_run is None: + new_run = source_course_key.run + new_course_key = CourseLocator(new_org, new_course, new_run, branch=ModuleStoreEnum.BranchName.published) new_course = self.split_modulestore.create_course( - new_course_root_locator.org, - new_course_root_locator.course, - new_course_root_locator.run, - user.id, - fields=self._get_json_fields_translate_references(original_course, course_key, True), - root_block_id=new_course_root_locator.block_id, - master_branch=new_course_root_locator.branch + new_org, new_course, new_run, user_id, + fields=self._get_json_fields_translate_references(original_course, new_course_key, None), + master_branch=ModuleStoreEnum.BranchName.published, ) - self._copy_published_modules_to_course(new_course, original_course.location, course_key, user) - self._add_draft_modules_to_course(new_course.id, course_key, user) + with self.split_modulestore.bulk_write_operations(new_course.id): + self._copy_published_modules_to_course(new_course, original_course.location, source_course_key, user_id) + # create a new version for the drafts + with self.split_modulestore.bulk_write_operations(new_course.id): + self._add_draft_modules_to_course(new_course.location, source_course_key, user_id) - return new_course_locator + return new_course.id - def _copy_published_modules_to_course(self, new_course, old_course_loc, course_key, user): + def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id): """ Copy all of the modules from the 'direct' version of the course to the new split course. """ @@ -67,21 +74,22 @@ class SplitMigrator(object): # iterate over published course elements. Wildcarding rather than descending b/c some elements are orphaned (e.g., # course about pages, conditionals) - for module in self.draft_modulestore.get_items(course_key, revision=ModuleStoreEnum.RevisionOption.published_only): - # don't copy the course again. No drafts should get here + for module in self.source_modulestore.get_items( + source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only + ): + # don't copy the course again. if module.location != old_course_loc: # create split_xblock using split.create_item - # where block_id is computed by translate_location_to_locator - new_locator = self.loc_mapper.translate_location( - module.location, True, add_entry_if_missing=True - ) # NOTE: the below auto populates the children when it migrates the parent; so, # it doesn't need the parent as the first arg. That is, it translates and populates # the 'children' field as it goes. _new_module = self.split_modulestore.create_item( - course_version_locator, module.category, user.id, - block_id=new_locator.block_id, - fields=self._get_json_fields_translate_references(module, course_key, True), + course_version_locator, module.category, user_id, + block_id=module.location.block_id, + fields=self._get_json_fields_translate_references( + module, course_version_locator, new_course.location.block_id + ), + # TODO remove continue_version when bulk write is impl'd continue_version=True ) # after done w/ published items, add version for DRAFT pointing to the published structure @@ -94,20 +102,18 @@ class SplitMigrator(object): # children which meant some pointers were to non-existent locations in 'direct' self.split_modulestore.internal_clean_children(course_version_locator) - def _add_draft_modules_to_course(self, published_course_key, course_key, user): + def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id): """ update each draft. Create any which don't exist in published and attach to their parents. """ # each true update below will trigger a new version of the structure. We may want to just have one new version # but that's for a later date. - new_draft_course_loc = published_course_key.for_branch(ModuleStoreEnum.BranchName.draft) + new_draft_course_loc = published_course_usage_key.course_key.for_branch(ModuleStoreEnum.BranchName.draft) # to prevent race conditions of grandchilden being added before their parents and thus having no parent to # add to awaiting_adoption = {} - for module in self.draft_modulestore.get_items(course_key, revision=ModuleStoreEnum.RevisionOption.draft_only): - new_locator = self.loc_mapper.translate_location( - module.location, False, add_entry_if_missing=True - ) + for module in self.source_modulestore.get_items(source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only): + new_locator = new_draft_course_loc.make_usage_key(module.category, module.location.block_id) if self.split_modulestore.has_item(new_locator): # was in 'direct' so draft is a new version split_module = self.split_modulestore.get_item(new_locator) @@ -115,27 +121,35 @@ class SplitMigrator(object): for name, field in split_module.fields.iteritems(): if field.is_set_on(split_module) and not module.fields[name].is_set_on(module): field.delete_from(split_module) - for field, value in self._get_fields_translate_references(module, course_key, True).iteritems(): - # draft children will insert themselves and the others are here already; so, don't do it 2x - if field.name != 'children': - field.write_to(split_module, value) + for field, value in self._get_fields_translate_references( + module, new_draft_course_loc, published_course_usage_key.block_id + ).iteritems(): + field.write_to(split_module, value) - _new_module = self.split_modulestore.update_item(split_module, user.id) + _new_module = self.split_modulestore.update_item(split_module, user_id) else: - # only a draft version (aka, 'private'). parent needs updated too. - # create a new course version just in case the current head is also the prod head + # only a draft version (aka, 'private'). _new_module = self.split_modulestore.create_item( - new_draft_course_loc, module.category, user.id, + new_draft_course_loc, module.category, user_id, block_id=new_locator.block_id, - fields=self._get_json_fields_translate_references(module, course_key, True) + fields=self._get_json_fields_translate_references( + module, new_draft_course_loc, published_course_usage_key.block_id + ) ) awaiting_adoption[module.location] = new_locator for draft_location, new_locator in awaiting_adoption.iteritems(): - parent_loc = self.draft_modulestore.get_parent_location(draft_location) - old_parent = self.draft_modulestore.get_item(parent_loc) - new_parent = self.split_modulestore.get_item( - self.loc_mapper.translate_location(old_parent.location, False) + parent_loc = self.source_modulestore.get_parent_location( + draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred ) + if parent_loc is None: + log.warn(u'No parent found in source course for %s', draft_location) + continue + old_parent = self.source_modulestore.get_item(parent_loc) + split_parent_loc = new_draft_course_loc.make_usage_key( + parent_loc.category, + parent_loc.block_id if parent_loc.category != 'course' else published_course_usage_key.block_id + ) + new_parent = self.split_modulestore.get_item(split_parent_loc) # this only occurs if the parent was also awaiting adoption: skip this one, go to next if any(new_locator == child.version_agnostic() for child in new_parent.children): continue @@ -144,16 +158,16 @@ class SplitMigrator(object): for old_child_loc in old_parent.children: if old_child_loc == draft_location: break # moved cursor enough, insert it here - sibling_loc = self.loc_mapper.translate_location(old_child_loc, False) + sibling_loc = new_draft_course_loc.make_usage_key(old_child_loc.category, old_child_loc.block_id) # sibling may move cursor for idx in range(new_parent_cursor, len(new_parent.children)): if new_parent.children[idx].version_agnostic() == sibling_loc: new_parent_cursor = idx + 1 break # skipped sibs enough, pick back up scan new_parent.children.insert(new_parent_cursor, new_locator) - new_parent = self.split_modulestore.update_item(new_parent, user.id) + new_parent = self.split_modulestore.update_item(new_parent, user_id) - def _get_json_fields_translate_references(self, xblock, old_course_id, published): + def _get_json_fields_translate_references(self, xblock, new_course_key, course_block_id): """ Return the json repr for explicitly set fields but convert all references to their Locators """ @@ -161,7 +175,10 @@ class SplitMigrator(object): """ Convert the location and add to loc mapper """ - return self.loc_mapper.translate_location(location, published, add_entry_if_missing=True) + return new_course_key.make_usage_key( + location.category, + location.block_id if location.category != 'course' else course_block_id + ) result = {} for field_name, field in xblock.fields.iteritems(): @@ -183,7 +200,7 @@ class SplitMigrator(object): return result - def _get_fields_translate_references(self, xblock, old_course_id, published): + def _get_fields_translate_references(self, xblock, new_course_key, course_block_id): """ Return a dictionary of field: value pairs for explicitly set fields but convert all references to their BlockUsageLocators @@ -192,7 +209,10 @@ class SplitMigrator(object): """ Convert the location and add to loc mapper """ - return self.loc_mapper.translate_location(location, published, add_entry_if_missing=True) + return new_course_key.make_usage_key( + location.category, + location.block_id if location.category != 'course' else course_block_id + ) result = {} for field_name, field in xblock.fields.iteritems(): diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py index 65a4d723dd..fe4d0db0c4 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/__init__.py @@ -1 +1,22 @@ -from split import SplitMongoModuleStore +""" +General utilities +""" +import urllib + + +def encode_key_for_mongo(fieldname): + """ + Fieldnames in mongo cannot have periods nor dollar signs. So encode them. + :param fieldname: an atomic field name. Note, don't pass structured paths as it will flatten them + """ + for char in [".", "$"]: + fieldname = fieldname.replace(char, '%{:02x}'.format(ord(char))) + return fieldname + + +def decode_key_from_mongo(fieldname): + """ + The inverse of encode_key_for_mongo + :param fieldname: with period and dollar escaped + """ + return urllib.unquote(fieldname) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py index b1d1b60396..759fa0264e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/caching_descriptor_system.py @@ -1,14 +1,14 @@ import sys import logging -from xmodule.mako_module import MakoDescriptorSystem +from xblock.runtime import KvsFieldData +from xblock.fields import ScopeIds from opaque_keys.edx.locator import BlockUsageLocator, LocalId, CourseLocator +from xmodule.mako_module import MakoDescriptorSystem from xmodule.error_module import ErrorDescriptor from xmodule.errortracker import exc_info_to_str -from xblock.runtime import KvsFieldData +from xmodule.modulestore.split_mongo import encode_key_for_mongo from ..exceptions import ItemNotFoundError from .split_mongo_kvs import SplitMongoKVS -from xblock.fields import ScopeIds -from xmodule.modulestore.loc_mapper_store import LocMapperStore log = logging.getLogger(__name__) @@ -47,7 +47,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): modulestore.inherit_settings( course_entry['structure'].get('blocks', {}), course_entry['structure'].get('blocks', {}).get( - LocMapperStore.encode_key_for_mongo(course_entry['structure'].get('root')) + encode_key_for_mongo(course_entry['structure'].get('root')) ) ) self.default_class = default_class diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 0b76fbf3cd..a0e1f0799a 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -6,7 +6,7 @@ Representation: ** '_id': a unique id which cannot change, ** 'org': the org's id. Only used for searching not identity, ** 'course': the course's catalog number - ** 'run': the course's run id or whatever user decides, + ** 'run': the course's run id, ** 'edited_by': user_id of user who created the original entry, ** 'edited_on': the datetime of the original creation, ** 'versions': versions_dict: {branch_id: structure_id, ...} @@ -53,7 +53,10 @@ from importlib import import_module from path import path import copy from pytz import UTC +from bson.objectid import ObjectId +from xblock.core import XBlock +from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict from xmodule.errortracker import null_error_tracker from opaque_keys.edx.locator import ( BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, @@ -61,19 +64,14 @@ from opaque_keys.edx.locator import ( ) from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \ DuplicateCourseError -from xmodule.modulestore import ( - inheritance, ModuleStoreWriteBase, ModuleStoreEnum -) +from xmodule.modulestore import inheritance, ModuleStoreWriteBase, ModuleStoreEnum, PublishState from ..exceptions import ItemNotFoundError from .definition_lazy_loader import DefinitionLazyLoader from .caching_descriptor_system import CachingDescriptorSystem -from xblock.fields import Scope, Reference, ReferenceList, ReferenceValueDict -from bson.objectid import ObjectId from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection -from xblock.core import XBlock -from xmodule.modulestore.loc_mapper_store import LocMapperStore from xmodule.error_module import ErrorDescriptor +from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo log = logging.getLogger(__name__) @@ -110,7 +108,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): def __init__(self, contentstore, doc_store_config, fs_root, render_template, default_class=None, error_tracker=null_error_tracker, - loc_mapper=None, i18n_service=None, **kwargs): """ @@ -118,8 +115,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ super(SplitMongoModuleStore, self).__init__(contentstore, **kwargs) - self.loc_mapper = loc_mapper + self.branch_setting_func = kwargs.pop('branch_setting_func', lambda: ModuleStoreEnum.Branch.published_only) self.db_connection = MongoConnection(**doc_store_config) self.db = self.db_connection.database @@ -267,7 +264,16 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param course_locator: any subclass of CourseLocator ''' - if course_locator.org and course_locator.course and course_locator.run and course_locator.branch: + if course_locator.org and course_locator.course and course_locator.run: + if course_locator.branch is None: + # default it based on branch_setting + # NAATODO move this to your mixin + if self.branch_setting_func() == ModuleStoreEnum.Branch.draft_preferred: + course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.draft) + elif self.branch_setting_func() == ModuleStoreEnum.Branch.published_only: + course_locator = course_locator.for_branch(ModuleStoreEnum.BranchName.published) + else: + raise InsufficientSpecificationError(course_locator) # use the course id index = self.db_connection.get_course_index(course_locator) if index is None: @@ -491,9 +497,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if parent_id is None: return None return BlockUsageLocator.make_relative( - locator, - block_type=course['structure']['blocks'][parent_id].get('category'), - block_id=LocMapperStore.decode_key_from_mongo(parent_id), + locator, + block_type=course['structure']['blocks'][parent_id].get('category'), + block_id=decode_key_from_mongo(parent_id), ) def get_orphans(self, course_key): @@ -502,13 +508,13 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")] course = self._lookup_course(course_key) - items = {LocMapperStore.decode_key_from_mongo(block_id) for block_id in course['structure']['blocks'].keys()} + items = {decode_key_from_mongo(block_id) for block_id in course['structure']['blocks'].keys()} items.remove(course['structure']['root']) blocks = course['structure']['blocks'] for block_id, block_data in blocks.iteritems(): items.difference_update(block_data.get('fields', {}).get('children', [])) if block_data['category'] in detached_categories: - items.discard(LocMapperStore.decode_key_from_mongo(block_id)) + items.discard(decode_key_from_mongo(block_id)) return [ BlockUsageLocator(course_key=course_key, block_type=blocks[block_id]['category'], block_id=block_id) for block_id in items @@ -816,7 +822,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # generate usage id if block_id is not None: - if LocMapperStore.encode_key_for_mongo(block_id) in new_structure['blocks']: + if encode_key_for_mongo(block_id) in new_structure['blocks']: raise DuplicateItemError(block_id, self, 'structures') else: new_block_id = block_id @@ -841,7 +847,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # if given parent, add new block as child and update parent's version parent = None if isinstance(course_or_parent_locator, BlockUsageLocator) and course_or_parent_locator.block_id is not None: - encoded_block_id = LocMapperStore.encode_key_for_mongo(course_or_parent_locator.block_id) + encoded_block_id = encode_key_for_mongo(course_or_parent_locator.block_id) parent = new_structure['blocks'][encoded_block_id] parent['fields'].setdefault('children', []).append(new_block_id) if not continue_version or parent['edit_info']['update_version'] != structure['_id']: @@ -886,13 +892,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id) source_index = self.get_course_index_info(source_course_id) return self.create_course( - dest_course_id.org, dest_course_id.offering, user_id, fields=None, # override start_date? + dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=None, # override start_date? versions_dict=source_index['versions'] ) def create_course( self, org, course, run, user_id, fields=None, - master_branch=ModuleStoreEnum.BranchName.draft, versions_dict=None, root_category='course', + master_branch=ModuleStoreEnum.BranchName.draft, + versions_dict=None, root_category='course', root_block_id='course', **kwargs ): """ @@ -984,7 +991,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if definition_fields or block_fields: draft_structure = self._version_structure(draft_structure, user_id) new_id = draft_structure['_id'] - encoded_block_id = LocMapperStore.encode_key_for_mongo(draft_structure['root']) + encoded_block_id = encode_key_for_mongo(draft_structure['root']) root_block = draft_structure['blocks'][encoded_block_id] if block_fields is not None: root_block['fields'].update(self._serialize_fields(root_category, block_fields)) @@ -1179,12 +1186,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block_id = getattr(xblock.scope_ids.usage_id.block_id, 'block_id', None) if block_id is None: block_id = self._generate_block_id(structure_blocks, xblock.category) - encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id) + encoded_block_id = encode_key_for_mongo(block_id) new_usage_id = xblock.scope_ids.usage_id.replace(block_id=block_id) xblock.scope_ids = xblock.scope_ids._replace(usage_id=new_usage_id) # pylint: disable=protected-access else: is_new = False - encoded_block_id = LocMapperStore.encode_key_for_mongo(xblock.location.block_id) + encoded_block_id = encode_key_for_mongo(xblock.location.block_id) children = [] if xblock.has_children: @@ -1370,7 +1377,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ Remove the subtree rooted at block_id """ - encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id) + encoded_block_id = encode_key_for_mongo(block_id) for child in new_blocks[encoded_block_id]['fields'].get('children', []): remove_subtree(child) del new_blocks[encoded_block_id] @@ -1445,7 +1452,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): for child in block_fields.get('children', []): try: - child = LocMapperStore.encode_key_for_mongo(child) + child = encode_key_for_mongo(child) self.inherit_settings(block_map, block_map[child], inheriting_settings) except KeyError: # here's where we need logic for looking up in other structures when we allow cross pointers @@ -1460,7 +1467,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): (0 => this usage only, 1 => this usage and its children, etc...) A depth of None returns all descendants """ - encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id) + encoded_block_id = encode_key_for_mongo(block_id) if encoded_block_id not in block_map: return descendent_map @@ -1509,7 +1516,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if 'fields' in block and 'children' in block['fields']: block['fields']["children"] = [ block_id for block_id in block['fields']["children"] - if LocMapperStore.encode_key_for_mongo(block_id) in original_structure['blocks'] + if encode_key_for_mongo(block_id) in original_structure['blocks'] ] self.db_connection.update_structure(original_structure) # clear cache again b/c inheritance may be wrong over orphans @@ -1532,10 +1539,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ # if this was taken from cache, then its fields are already converted if isinstance(block_id, BlockUsageLocator): - return block_id + return block_id.map_into_course(course_key) try: return course_key.make_usage_key( - blocks[LocMapperStore.encode_key_for_mongo(block_id)]['category'], block_id + blocks[encode_key_for_mongo(block_id)]['category'], block_id ) except KeyError: return course_key.make_usage_key('unknown', block_id) @@ -1569,7 +1576,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): :param continue_version: if True, assumes this operation requires a head version and will not create a new version but instead continue an existing transaction on this version. This flag cannot be True if force is True. """ - if locator.org is None or locator.course is None or locator. run is None or locator.branch is None: + if locator.org is None or locator.course is None or locator.run is None or locator.branch is None: if continue_version: raise InsufficientSpecificationError( "To continue a version, the locator must point to one ({}).".format(locator) @@ -1647,7 +1654,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): ] elif isinstance(xblock_class.fields[field_name], ReferenceValueDict): for key, subvalue in value.iteritems(): - assert isinstance(subvalue, Location) value[key] = subvalue.block_id # I think these are obsolete conditions; so, I want to confirm that. Thus the warnings @@ -1668,7 +1674,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ new_id = ObjectId() if root_category is not None: - encoded_root = LocMapperStore.encode_key_for_mongo(root_block_id) + encoded_root = encode_key_for_mongo(root_block_id) blocks = { encoded_root: self._new_block( user_id, root_category, block_fields, definition_id, new_id @@ -1726,7 +1732,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Return any newly discovered orphans (as a set) """ orphans = set() - encoded_block_id = LocMapperStore.encode_key_for_mongo(block_id) + encoded_block_id = encode_key_for_mongo(block_id) destination_block = destination_blocks.get(encoded_block_id) new_block = source_blocks[encoded_block_id] if destination_block: @@ -1769,7 +1775,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Delete the orphan and any of its descendants which no longer have parents. """ if self._get_parent_from_structure(orphan, structure) is None: - encoded_block_id = LocMapperStore.encode_key_for_mongo(orphan) + encoded_block_id = encode_key_for_mongo(orphan) for child in structure['blocks'][encoded_block_id]['fields'].get('children', []): self._delete_if_true_orphan(child, structure) del structure['blocks'][encoded_block_id] @@ -1802,14 +1808,14 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Encodes the block id before retrieving it from the structure to ensure it can be a json dict key. """ - return structure['blocks'].get(LocMapperStore.encode_key_for_mongo(block_id)) + return structure['blocks'].get(encode_key_for_mongo(block_id)) def _update_block_in_structure(self, structure, block_id, content): """ Encodes the block id before accessing it in the structure to ensure it can be a json dict key. """ - structure['blocks'][LocMapperStore.encode_key_for_mongo(block_id)] = content + structure['blocks'][encode_key_for_mongo(block_id)] = content def get_courses_for_wiki(self, wiki_slug): """ @@ -1828,20 +1834,42 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ return {ModuleStoreEnum.Type.split: self.db_connection.heartbeat()} + def compute_publish_state(self, xblock): """ Returns whether this xblock is draft, public, or private. Returns: - PublishState.draft - content is in the process of being edited, but still has a previous - version deployed to LMS - PublishState.public - content is locked and deployed to LMS - PublishState.private - content is editable and not deployed to LMS + PublishState.draft - published exists and is different from draft + PublishState.public - published exists and is the same as draft + PublishState.private - no published version exists """ - # TODO implement - raise NotImplementedError() + # TODO figure out what to say if xblock is not from the HEAD of its branch + def get_head(branch): + course_structure = self._lookup_course(xblock.location.course_key.for_branch(branch))['structure'] + return self._get_block_from_structure(course_structure, xblock.location.block_id) - def convert_to_draft(self, location, user_id): + if xblock.location.branch is None: + raise ValueError(u'{} is not in a branch; so, this is nonsensical'.format(xblock.location)) + if xblock.location.branch == ModuleStoreEnum.BranchName.draft: + other = get_head(ModuleStoreEnum.BranchName.published) + elif xblock.location.branch == ModuleStoreEnum.BranchName.published: + other = get_head(ModuleStoreEnum.BranchName.draft) + else: + raise ValueError(u'{} is not in a branch other than draft or published; so, this is nonsensical'.format(xblock.location)) + + if not other: + if xblock.location.branch == ModuleStoreEnum.BranchName.draft: + return PublishState.private + else: + return PublishState.public # a bit nonsensical + elif xblock.update_version != other['edit_info']['update_version']: + return PublishState.draft + else: + return PublishState.public + + +def convert_to_draft(self, location, user_id): """ Create a copy of the source and mark its revision as draft. diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 745784c85d..52fcdc9a5c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -197,6 +197,7 @@ class ModuleStoreTestCase(TestCase): if hasattr(store, 'collection'): connection = store.collection.database.connection store.collection.drop() + connection.drop_database(store.collection.database.name) connection.close() elif hasattr(store, 'close_all_connections'): store.close_all_connections() @@ -247,7 +248,7 @@ class ModuleStoreTestCase(TestCase): """ # Flush the Mongo modulestore - ModuleStoreTestCase.drop_mongo_collections() + self.drop_mongo_collections() # Call superclass implementation super(ModuleStoreTestCase, self)._pre_setup() @@ -256,7 +257,7 @@ class ModuleStoreTestCase(TestCase): """ Flush the ModuleStore after each test. """ - ModuleStoreTestCase.drop_mongo_collections() + self.drop_mongo_collections() # Call superclass implementation super(ModuleStoreTestCase, self)._post_teardown() diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index b404d7457b..7dc251b4e1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,9 +1,10 @@ import pymongo from uuid import uuid4 import ddt -from mock import patch, Mock +from mock import patch from importlib import import_module from collections import namedtuple +import unittest from xmodule.tests import DATA_DIR from opaque_keys.edx.locations import Location @@ -11,20 +12,19 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.exceptions import InvalidVersionError +from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator -from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper # Mixed modulestore depends on django, so we'll manually configure some django settings # before importing the module +# TODO remove this import and the configuration -- xmodule should not depend on django! from django.conf import settings -from opaque_keys.edx.locations import SlashSeparatedCourseKey -from xmodule.modulestore.mongo.base import MongoRevisionKey if not settings.configured: settings.configure() from xmodule.modulestore.mixed import MixedModuleStore @ddt.ddt -class TestMixedModuleStore(LocMapperSetupSansDjango): +class TestMixedModuleStore(unittest.TestCase): """ Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and Location-based dbs) @@ -67,7 +67,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): }, { 'NAME': 'split', - 'ENGINE': 'xmodule.modulestore.split_mongo.SplitMongoModuleStore', + 'ENGINE': 'xmodule.modulestore.split_mongo.split.SplitMongoModuleStore', 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, 'OPTIONS': modulestore_options }, @@ -106,7 +106,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): patcher = patch.multiple( 'xmodule.modulestore.mixed', - loc_mapper=Mock(return_value=LocMapperSetupSansDjango.loc_store), create_modulestore_instance=create_modulestore_instance, ) patcher.start() @@ -221,7 +220,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): course_id: course_key.make_usage_key('course', course_key.run) for course_id, course_key in self.course_locations.iteritems() # pylint: disable=maybe-no-member } - self.fake_location = Location('foo', 'bar', 'slowly', 'vertical', 'baz') + if default == 'split': + self.fake_location = CourseLocator( + 'foo', 'bar', 'slowly', branch=ModuleStoreEnum.BranchName.draft + ).make_usage_key('vertical', 'baz') + else: + self.fake_location = Location('foo', 'bar', 'slowly', 'vertical', 'baz') self.writable_chapter_location = self.course_locations[self.MONGO_COURSEID].replace( category='chapter', name='Overview' ) @@ -229,9 +233,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): category='chapter', name='Overview' ) - # get Locators and set up the loc mapper if app is Locator based - if default == 'split': - self.fake_location = loc_mapper().translate_location(self.fake_location) self._create_course(default, self.course_locations[self.MONGO_COURSEID].course_key) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py index c6cbed8bc1..fb7366bfe7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_orphan.py @@ -29,7 +29,7 @@ class TestOrphan(SplitWMongoCourseBoostrapper): """ Test that old mongo finds the orphans """ - orphans = self.old_mongo.get_orphans(self.old_course_key) + orphans = self.draft_mongo.get_orphans(self.old_course_key) self.assertEqual(len(orphans), 3, "Wrong # {}".format(orphans)) location = self.old_course_key.make_usage_key('chapter', 'OrphanChapter') self.assertIn(location.to_deprecated_string(), orphans) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py index f0226e5f7c..3790aea17b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_publish.py @@ -4,6 +4,7 @@ Test the publish code (mostly testing that publishing doesn't result in orphans) from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBoostrapper from xmodule.modulestore.tests.factories import check_mongo_calls +from xmodule.modulestore import ModuleStoreEnum class TestPublish(SplitWMongoCourseBoostrapper): @@ -15,18 +16,26 @@ class TestPublish(SplitWMongoCourseBoostrapper): Create the course, publish all verticals * some detached items """ - # There should be 12 inserts and 11 updates (max_sends) - # Should be 1 to verify course unique, 11 parent fetches, - # and n per _create_item where n is the size of the course tree non-leaf nodes - # for inheritance computation (which is 7*4 + sum(1..4) = 38) (max_finds) - with check_mongo_calls(self.draft_mongo, 71, 27): - with check_mongo_calls(self.old_mongo, 70, 27): - super(TestPublish, self)._create_course(split=False) + # There are 12 created items and 7 parent updates + # create course: finds: 1 to verify uniqueness, 1 to find parents + # sends: 1 to create course, 1 to create overview + with check_mongo_calls(self.draft_mongo, 5, 2): + super(TestPublish, self)._create_course(split=False) # 2 inserts (course and overview) + # with bulk will delay all inheritance computations which won't be added into the mongo_calls + with self.draft_mongo.bulk_write_operations(self.old_course_key): + # finds: 1 for parent to add child, 1 for parent to update edit info + # sends: 1 for insert, 2 for parent (add child, change edit info) + with check_mongo_calls(self.draft_mongo, 5, 3): self._create_item('chapter', 'Chapter1', {}, {'display_name': 'Chapter 1'}, 'course', 'runid', split=False) + + with check_mongo_calls(self.draft_mongo, 5, 3): self._create_item('chapter', 'Chapter2', {}, {'display_name': 'Chapter 2'}, 'course', 'runid', split=False) + # update info propagation is 2 levels. create looks for draft and then published and then creates + with check_mongo_calls(self.draft_mongo, 16, 8): self._create_item('vertical', 'Vert1', {}, {'display_name': 'Vertical 1'}, 'chapter', 'Chapter1', split=False) self._create_item('vertical', 'Vert2', {}, {'display_name': 'Vertical 2'}, 'chapter', 'Chapter1', split=False) + with check_mongo_calls(self.draft_mongo, 36, 36): self._create_item('html', 'Html1', "

Goodbye

", {'display_name': 'Parented Html'}, 'vertical', 'Vert1', split=False) self._create_item( 'discussion', 'Discussion1', @@ -53,11 +62,11 @@ class TestPublish(SplitWMongoCourseBoostrapper): 'vertical', 'Vert2', split=False ) + with check_mongo_calls(self.draft_mongo, 2, 2): + # 2 finds b/c looking for non-existent parents self._create_item('static_tab', 'staticuno', "

tab

", {'display_name': 'Tab uno'}, None, None, split=False) - self._create_item('about', 'overview', "

overview

", {}, None, None, split=False) self._create_item('course_info', 'updates', "
  1. Sep 22

    test

", {}, None, None, split=False) - def test_publish_draft_delete(self): """ To reproduce a bug (STUD-811) publish a vertical, convert to draft, delete a child, move a child, publish. @@ -93,7 +102,7 @@ class TestPublish(SplitWMongoCourseBoostrapper): self.draft_mongo.update_item(other_vert, self.user_id) # publish self.draft_mongo.publish(vert_location, self.user_id) - item = self.old_mongo.get_item(vert_location, 0) + item = self.draft_mongo.get_item(draft_vert.location, revision=ModuleStoreEnum.RevisionOption.published_only) self.assertNotIn(location, item.children) self.assertIsNone(self.draft_mongo.get_parent_location(location)) with self.assertRaises(ItemNotFoundError): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py index e573acc78e..c92e817533 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_migrator.py @@ -5,13 +5,9 @@ Tests for split_migrator import uuid import random import mock -from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.mongo.base import MongoRevisionKey -from xmodule.modulestore.loc_mapper_store import LocMapperStore -from xmodule.modulestore.split_migrator import SplitMigrator -from xmodule.modulestore.tests import test_location_mapper -from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBoostrapper from xblock.fields import Reference, ReferenceList, ReferenceValueDict +from xmodule.modulestore.split_migrator import SplitMigrator +from xmodule.modulestore.tests.test_split_w_old_mongo import SplitWMongoCourseBoostrapper class TestMigration(SplitWMongoCourseBoostrapper): @@ -21,15 +17,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): def setUp(self): super(TestMigration, self).setUp() - # pylint: disable=W0142 - self.loc_mapper = LocMapperStore(test_location_mapper.TrivialCache(), **self.db_config) - self.split_mongo.loc_mapper = self.loc_mapper - self.migrator = SplitMigrator(self.split_mongo, self.draft_mongo, self.loc_mapper) - - def tearDown(self): - dbref = self.loc_mapper.db - dbref.drop_collection(self.loc_mapper.location_map) - super(TestMigration, self).tearDown() + self.migrator = SplitMigrator(self.split_mongo, self.draft_mongo) def _create_course(self): """ @@ -64,7 +52,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): self.create_random_units(False, both_vert_loc) draft_both = self.draft_mongo.get_item(both_vert_loc) draft_both.display_name = 'Both vertical renamed' - self.draft_mongo.update_item(draft_both, ModuleStoreEnum.UserID.test) + self.draft_mongo.update_item(draft_both, self.user_id) self.create_random_units(True, both_vert_loc) # vertical in draft only (x2) draft_vert_loc = self.old_course_key.make_usage_key('vertical', uuid.uuid4().hex) @@ -86,7 +74,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): live_vert_loc.category, live_vert_loc.name, {}, {'display_name': 'Live vertical end'}, 'chapter', chapter1_name, draft=False, split=False ) - self.create_random_units(True, draft_vert_loc) + self.create_random_units(False, live_vert_loc) # now the other chapter w/ the conditional # create pointers to children (before the children exist) @@ -155,40 +143,28 @@ class TestMigration(SplitWMongoCourseBoostrapper): draft=draft, split=False ) - def compare_courses(self, presplit, published): + def compare_courses(self, presplit, new_course_key, published): # descend via children to do comparison old_root = presplit.get_course(self.old_course_key) - new_root_locator = self.loc_mapper.translate_location_to_course_locator( - old_root.id, published - ) - new_root = self.split_mongo.get_course(new_root_locator) + new_root = self.split_mongo.get_course(new_course_key) self.compare_dags(presplit, old_root, new_root, published) # grab the detached items to compare they should be in both published and draft for category in ['conditional', 'about', 'course_info', 'static_tab']: for conditional in presplit.get_items(self.old_course_key, category=category): - locator = self.loc_mapper.translate_location( - conditional.location, - published, - add_entry_if_missing=False - ) + locator = new_course_key.make_usage_key(category, conditional.location.block_id) self.compare_dags(presplit, conditional, self.split_mongo.get_item(locator), published) def compare_dags(self, presplit, presplit_dag_root, split_dag_root, published): - # check that locations match - self.assertEqual( - presplit_dag_root.location, - self.loc_mapper.translate_locator_to_location(split_dag_root.location).replace( - revision=MongoRevisionKey.published - ) - ) - # compare all fields but children + if split_dag_root.category != 'course': + self.assertEqual(presplit_dag_root.location.block_id, split_dag_root.location.block_id) + # compare all fields but references for name, field in presplit_dag_root.fields.iteritems(): if not isinstance(field, (Reference, ReferenceList, ReferenceValueDict)): self.assertEqual( getattr(presplit_dag_root, name), getattr(split_dag_root, name), - "{}/{}: {} != {}".format( + u"{}/{}: {} != {}".format( split_dag_root.location, name, getattr(presplit_dag_root, name), getattr(split_dag_root, name) ) ) @@ -198,7 +174,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): self.assertEqual( # need get_children to filter out drafts len(presplit_dag_root.get_children()), len(split_dag_root.children), - "{0.category} '{0.display_name}': children {1} != {2}".format( + u"{0.category} '{0.display_name}': children {1} != {2}".format( presplit_dag_root, presplit_dag_root.children, split_dag_root.children ) ) @@ -207,7 +183,7 @@ class TestMigration(SplitWMongoCourseBoostrapper): def test_migrator(self): user = mock.Mock(id=1) - self.migrator.migrate_mongo_course(self.old_course_key, user) + new_course_key = self.migrator.migrate_mongo_course(self.old_course_key, user.id, new_run='new_run') # now compare the migrated to the original course - self.compare_courses(self.old_mongo, True) - self.compare_courses(self.draft_mongo, False) + self.compare_courses(self.draft_mongo, new_course_key, True) # published + self.compare_courses(self.draft_mongo, new_course_key, False) # draft 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 1f024624c5..d25c0c3226 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -12,7 +12,7 @@ import random from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.exceptions import (InsufficientSpecificationError, ItemNotFoundError, VersionConflictError, +from xmodule.modulestore.exceptions import (ItemNotFoundError, VersionConflictError, DuplicateItemError, DuplicateCourseError) from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator, VersionTree, LocalId from xmodule.modulestore.inheritance import InheritanceMixin @@ -45,7 +45,7 @@ class SplitModuleTest(unittest.TestCase): } MODULESTORE = { - 'ENGINE': 'xmodule.modulestore.split_mongo.SplitMongoModuleStore', + 'ENGINE': 'xmodule.modulestore.split_mongo.split.SplitMongoModuleStore', 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, 'OPTIONS': modulestore_options } @@ -667,7 +667,7 @@ class SplitModuleCourseTests(SplitModuleTest): def test_get_course_negative(self): # Now negative testing - with self.assertRaises(InsufficientSpecificationError): + with self.assertRaises(ItemNotFoundError): modulestore().get_course(CourseLocator(org='edu', course='meh', run='blah')) with self.assertRaises(ItemNotFoundError): modulestore().get_course(CourseLocator(org='edu', course='nosuchthing', run="run", branch=BRANCH_NAME_DRAFT)) 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 82d5c0d72e..d990271652 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 @@ -7,8 +7,7 @@ import random from xmodule.modulestore.inheritance import InheritanceMixin from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore -from xmodule.modulestore.mongo import MongoModuleStore, DraftMongoModuleStore -from xmodule.modulestore.mongo.draft import DIRECT_ONLY_CATEGORIES +from xmodule.modulestore.mongo import DraftMongoModuleStore from xmodule.modulestore import ModuleStoreEnum @@ -22,7 +21,6 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): Defines the following attrs on self: * user_id: a random non-registered mock user id * split_mongo: a pointer to the split mongo instance - * old_mongo: a pointer to the old_mongo instance * draft_mongo: a pointer to the old draft instance * split_course_key (CourseLocator): of the new course * old_course_key: the SlashSpecifiedCourseKey for the course @@ -54,7 +52,6 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): ) self.addCleanup(self.split_mongo.db.connection.close) self.addCleanup(self.tear_down_split) - self.old_mongo = MongoModuleStore(None, self.db_config, **self.modulestore_options) self.draft_mongo = DraftMongoModuleStore( None, self.db_config, branch_setting_func=lambda: ModuleStoreEnum.Branch.draft_preferred, **self.modulestore_options ) @@ -78,19 +75,23 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): """ split_db = self.split_mongo.db # old_mongo doesn't give a db attr, but all of the dbs are the same - split_db.drop_collection(self.old_mongo.collection) + split_db.drop_collection(self.draft_mongo.collection) def _create_item(self, category, name, data, metadata, parent_category, parent_name, draft=True, split=True): """ Create the item of the given category and block id in split and old mongo, add it to the optional parent. The parent category is only needed because old mongo requires it for the id. + + Note: if draft = False, it will create the draft and then publish it; so, it will overwrite any + existing draft for both the new item and the parent """ location = self.old_course_key.make_usage_key(category, name) - if not draft or category in DIRECT_ONLY_CATEGORIES: - mongo = self.old_mongo - else: - mongo = self.draft_mongo - mongo.create_and_save_xmodule(location, self.user_id, definition_data=data, metadata=metadata, runtime=self.runtime) + + self.draft_mongo.create_and_save_xmodule( + location, self.user_id, definition_data=data, metadata=metadata, runtime=self.runtime + ) + if not draft: + self.draft_mongo.publish(location, self.user_id) if isinstance(data, basestring): fields = {'data': data} else: @@ -99,13 +100,11 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): if parent_name: # add child to parent in mongo parent_location = self.old_course_key.make_usage_key(parent_category, parent_name) - if not draft or parent_category in DIRECT_ONLY_CATEGORIES: - mongo = self.old_mongo - else: - mongo = self.draft_mongo - parent = mongo.get_item(parent_location) + parent = self.draft_mongo.get_item(parent_location) parent.children.append(location) - mongo.update_item(parent, self.user_id) + self.draft_mongo.update_item(parent, self.user_id) + if not draft: + self.draft_mongo.publish(parent_location, self.user_id) # create pointer for split course_or_parent_locator = BlockUsageLocator( course_key=self.split_course_key, @@ -137,6 +136,6 @@ class SplitWMongoCourseBoostrapper(unittest.TestCase): self.split_mongo.create_course( self.split_course_key.org, self.split_course_key.course, self.split_course_key.run, self.user_id, fields=fields, root_block_id='runid' ) - old_course = self.old_mongo.create_course(self.split_course_key.org, 'test_course', 'runid', self.user_id, fields=fields) + old_course = self.draft_mongo.create_course(self.split_course_key.org, 'test_course', 'runid', self.user_id, fields=fields) self.old_course_key = old_course.id self.runtime = old_course.runtime