From abbfa95e4c715df81acff725c0048f1eb6437b4d Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Mon, 4 Aug 2014 17:45:12 -0400 Subject: [PATCH] LMS-11168 Support for removing versions and branch in Split, Mixed, and SQL. Make default_store thread-safe. --- .../contentstore/tests/test_contentstore.py | 92 ++++++--- cms/djangoapps/contentstore/views/assets.py | 2 +- cms/djangoapps/contentstore/views/course.py | 46 +++-- cms/djangoapps/contentstore/views/tasks.py | 32 ++- cms/envs/bok_choy.auth.json | 1 - common/djangoapps/cache_toolbox/core.py | 18 +- common/djangoapps/user_api/middleware.py | 4 +- common/djangoapps/xmodule_django/models.py | 40 +++- .../xmodule/xmodule/contentstore/content.py | 3 +- .../lib/xmodule/xmodule/contentstore/mongo.py | 6 +- .../xmodule/xmodule/modulestore/__init__.py | 44 +++-- .../lib/xmodule/xmodule/modulestore/mixed.py | 186 +++++++++++++----- .../modulestore/modulestore_settings.py | 5 +- .../xmodule/xmodule/modulestore/mongo/base.py | 34 ++-- .../xmodule/modulestore/mongo/draft.py | 10 +- .../xmodule/modulestore/split_migrator.py | 39 ++-- .../split_mongo/caching_descriptor_system.py | 11 +- .../xmodule/modulestore/split_mongo/split.py | 62 +++--- .../modulestore/split_mongo/split_draft.py | 59 +++--- .../split_mongo/split_mongo_kvs.py | 53 ++--- .../tests/test_mixed_modulestore.py | 80 +++++++- .../tests/test_modulestore_settings.py | 4 +- common/lib/xmodule/xmodule/modulestore/xml.py | 19 +- .../xmodule/modulestore/xml_importer.py | 4 +- lms/envs/bok_choy.auth.json | 1 - lms/envs/common.py | 1 - 26 files changed, 580 insertions(+), 276 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 659bd9eca5..c8fac4fed9 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -28,7 +28,7 @@ from xmodule.exceptions import NotFoundError, InvalidVersionError from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation, CourseLocator from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.xml_exporter import export_to_xml @@ -47,6 +47,7 @@ from student.roles import CourseCreatorRole, CourseInstructorRole from opaque_keys import InvalidKeyError from contentstore.tests.utils import get_url from course_action_state.models import CourseRerunState, CourseRerunUIStateManager +from course_action_state.managers import CourseActionStateItemNotFoundError TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) @@ -1115,8 +1116,7 @@ class ContentStoreTest(ContentStoreTestCase): def test_create_course_with_bad_organization(self): """Test new course creation - error path for bad organization name""" self.course_data['org'] = 'University of California, Berkeley' - self.assert_course_creation_failed( - r"(?s)Unable to create course 'Robot Super Course'.*: Invalid characters in u'University of California, Berkeley'") + self.assert_course_creation_failed(r"(?s)Unable to create course 'Robot Super Course'.*") def test_create_course_with_course_creation_disabled_staff(self): """Test new course creation -- course creation disabled, but staff access.""" @@ -1572,31 +1572,35 @@ class RerunCourseTest(ContentStoreTestCase): 'display_name': 'Robot Super Course', 'run': '2013_Spring' } - self.destination_course_key = _get_course_id(self.destination_course_data) - def post_rerun_request(self, source_course_key, response_code=200): + def post_rerun_request( + self, source_course_key, destination_course_data=None, response_code=200, expect_error=False + ): """Create and send an ajax post for the rerun request""" # create data to post rerun_course_data = {'source_course_key': unicode(source_course_key)} - rerun_course_data.update(self.destination_course_data) + if not destination_course_data: + destination_course_data = self.destination_course_data + rerun_course_data.update(destination_course_data) + destination_course_key = _get_course_id(destination_course_data) # post the request - course_url = get_url('course_handler', self.destination_course_key, 'course_key_string') + course_url = get_url('course_handler', destination_course_key, 'course_key_string') response = self.client.ajax_post(course_url, rerun_course_data) # verify response self.assertEqual(response.status_code, response_code) - if response_code == 200: - self.assertNotIn('ErrMsg', parse_json(response)) + if not expect_error: + json_resp = parse_json(response) + self.assertNotIn('ErrMsg', json_resp) + destination_course_key = CourseKey.from_string(json_resp['destination_course_key']) - def create_course_listing_html(self, course_key): - """Creates html fragment that is created for the given course_key in the course listing section""" - return ' 0: - raise InvalidLocationError( - "There are already courses with the given org and course id: {}".format([ - course['_id'] for course in courses - ])) + raise DuplicateCourseError(course_id, courses[0]['_id']) location = course_id.make_usage_key('course', course_id.run) course = self.create_xmodule( @@ -1253,7 +1261,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): """ return ModuleStoreEnum.Type.mongo - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Return an array of all of the locations for orphans in the course. """ @@ -1274,7 +1282,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): item_locs -= all_reachable return [course_key.make_usage_key_from_deprecated_string(item_loc) for item_loc in item_locs] - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index d853516c05..e9cb77e977 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -47,7 +47,7 @@ class DraftModuleStore(MongoModuleStore): This module also includes functionality to promote DRAFT modules (and their children) to published modules. """ - def get_item(self, usage_key, depth=0, revision=None): + def get_item(self, usage_key, depth=0, revision=None, **kwargs): """ Returns an XModuleDescriptor instance for the item at usage_key. @@ -155,7 +155,7 @@ class DraftModuleStore(MongoModuleStore): course_query = self._course_key_to_son(course_key) self.collection.remove(course_query, multi=True) - def clone_course(self, source_course_id, dest_course_id, user_id, fields=None): + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): """ Only called if cloning within this store or if env doesn't set up mixed. * copy the courseware @@ -439,7 +439,7 @@ class DraftModuleStore(MongoModuleStore): # convert the subtree using the original item as the root self._breadth_first(convert_item, [location]) - def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False): + def update_item(self, xblock, user_id, allow_not_found=False, force=False, isPublish=False, **kwargs): """ See superclass doc. In addition to the superclass's behavior, this method converts the unit to draft if it's not @@ -616,7 +616,7 @@ class DraftModuleStore(MongoModuleStore): else: return False - def publish(self, location, user_id): + def publish(self, location, user_id, **kwargs): """ Publish the subtree rooted at location to the live course and remove the drafts. Such publishing may cause the deletion of previously published but subsequently deleted @@ -690,7 +690,7 @@ class DraftModuleStore(MongoModuleStore): self.collection.remove({'_id': {'$in': to_be_deleted}}) return self.get_item(as_published(location)) - def unpublish(self, location, user_id): + def unpublish(self, location, user_id, **kwargs): """ Turn the published version into a draft, removing the published version. diff --git a/common/lib/xmodule/xmodule/modulestore/split_migrator.py b/common/lib/xmodule/xmodule/modulestore/split_migrator.py index 5d5712939b..8fff1be717 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_migrator.py +++ b/common/lib/xmodule/xmodule/modulestore/split_migrator.py @@ -25,7 +25,9 @@ class SplitMigrator(object): self.split_modulestore = split_modulestore self.source_modulestore = source_modulestore - def migrate_mongo_course(self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None): + def migrate_mongo_course( + self, source_course_key, user_id, new_org=None, new_course=None, new_run=None, fields=None, **kwargs + ): """ 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 @@ -43,7 +45,7 @@ class SplitMigrator(object): # 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.source_modulestore.get_course(source_course_key) + original_course = self.source_modulestore.get_course(source_course_key, **kwargs) if new_org is None: new_org = source_course_key.org @@ -60,17 +62,20 @@ class SplitMigrator(object): new_org, new_course, new_run, user_id, fields=new_fields, master_branch=ModuleStoreEnum.BranchName.published, + **kwargs ) 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) + self._copy_published_modules_to_course( + new_course, original_course.location, source_course_key, user_id, **kwargs + ) # 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) + self._add_draft_modules_to_course(new_course.location, source_course_key, user_id, **kwargs) return new_course.id - def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id): + def _copy_published_modules_to_course(self, new_course, old_course_loc, source_course_key, user_id, **kwargs): """ Copy all of the modules from the 'direct' version of the course to the new split course. """ @@ -79,7 +84,7 @@ 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.source_modulestore.get_items( - source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only + source_course_key, revision=ModuleStoreEnum.RevisionOption.published_only, **kwargs ): # don't copy the course again. if module.location != old_course_loc: @@ -95,7 +100,8 @@ class SplitMigrator(object): fields=self._get_fields_translate_references( module, course_version_locator, new_course.location.block_id ), - continue_version=True + continue_version=True, + **kwargs ) # after done w/ published items, add version for DRAFT pointing to the published structure index_info = self.split_modulestore.get_course_index_info(course_version_locator) @@ -107,7 +113,7 @@ 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_usage_key, source_course_key, user_id): + def _add_draft_modules_to_course(self, published_course_usage_key, source_course_key, user_id, **kwargs): """ update each draft. Create any which don't exist in published and attach to their parents. """ @@ -117,11 +123,13 @@ class SplitMigrator(object): # 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.source_modulestore.get_items(source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only): + for module in self.source_modulestore.get_items( + source_course_key, revision=ModuleStoreEnum.RevisionOption.draft_only, **kwargs + ): 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) + split_module = self.split_modulestore.get_item(new_locator, **kwargs) # need to remove any no-longer-explicitly-set values and add/update any now set values. for name, field in split_module.fields.iteritems(): if field.is_set_on(split_module) and not module.fields[name].is_set_on(module): @@ -131,7 +139,7 @@ class SplitMigrator(object): ).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, **kwargs) else: # only a draft version (aka, 'private'). _new_module = self.split_modulestore.create_item( @@ -140,22 +148,23 @@ class SplitMigrator(object): block_id=new_locator.block_id, fields=self._get_fields_translate_references( module, new_draft_course_loc, published_course_usage_key.block_id - ) + ), + **kwargs ) awaiting_adoption[module.location] = new_locator for draft_location, new_locator in awaiting_adoption.iteritems(): parent_loc = self.source_modulestore.get_parent_location( - draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred + draft_location, revision=ModuleStoreEnum.RevisionOption.draft_preferred, **kwargs ) 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) + old_parent = self.source_modulestore.get_item(parent_loc, **kwargs) 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) + new_parent = self.split_modulestore.get_item(split_parent_loc, **kwargs) # 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 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 611423d7c5..5aedf025cf 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 @@ -53,7 +53,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): self.default_class = default_class self.local_modules = {} - def _load_item(self, block_id, course_entry_override=None): + def _load_item(self, block_id, course_entry_override=None, **kwargs): if isinstance(block_id, BlockUsageLocator): if isinstance(block_id.block_id, LocalId): try: @@ -77,7 +77,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): raise ItemNotFoundError(block_id) class_ = self.load_block_type(json_data.get('category')) - return self.xblock_from_json(class_, block_id, json_data, course_entry_override) + return self.xblock_from_json(class_, block_id, json_data, course_entry_override, **kwargs) # xblock's runtime does not always pass enough contextual information to figure out # which named container (course x branch) or which parent is requesting an item. Because split allows @@ -90,7 +90,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): # low; thus, the course_entry is most likely correct. If the thread is looking at > 1 named container # pointing to the same structure, the access is likely to be chunky enough that the last known container # is the intended one when not given a course_entry_override; thus, the caching of the last branch/course id. - def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None): + def xblock_from_json(self, class_, block_id, json_data, course_entry_override=None, **kwargs): if course_entry_override is None: course_entry_override = self.course_entry else: @@ -126,6 +126,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem): definition, converted_fields, json_data.get('_inherited_settings'), + **kwargs ) field_data = KvsFieldData(kvs) @@ -151,8 +152,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem): edit_info = json_data.get('edit_info', {}) module.edited_by = edit_info.get('edited_by') module.edited_on = edit_info.get('edited_on') - module.published_by = None # TODO - module.published_date = None # TODO + module.published_by = None # TODO - addressed with LMS-11184 + module.published_date = None # TODO - addressed with LMS-11184 module.previous_version = edit_info.get('previous_version') module.update_version = edit_info.get('update_version') module.source_version = edit_info.get('source_version', None) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 7adffe89d6..8f461deeec 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -63,7 +63,7 @@ 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, - LocalId, Locator + LocalId, ) from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError, \ DuplicateCourseError @@ -77,7 +77,6 @@ from .caching_descriptor_system import CachingDescriptorSystem from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.split_mongo import encode_key_for_mongo, decode_key_from_mongo -import types from _collections import defaultdict @@ -111,7 +110,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): """ SCHEMA_VERSION = 1 - reference_type = Locator # a list of field names to store in course index search_targets. Note, this will # only record one value per key. If branches disagree, the last one set wins. # It won't recompute the value on operations such as update_course_index (e.g., to revert to a prev @@ -214,7 +212,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): system.module_data.update(new_module_data) return system.module_data - def _load_items(self, course_entry, block_ids, depth=0, lazy=True): + def _load_items(self, course_entry, block_ids, depth=0, lazy=True, **kwargs): ''' Load & cache the given blocks from the course. Prefetch down to the given depth. Load the definitions into each block if lazy is False; @@ -248,7 +246,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): branch=course_entry.get('branch'), ) self.cache_items(system, block_ids, course_key, depth, lazy) - return [system.load_item(block_id, course_entry) for block_id in block_ids] + return [system.load_item(block_id, course_entry, **kwargs) for block_id in block_ids] def _get_cache(self, course_version_guid): """ @@ -333,7 +331,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } return envelope - def get_courses(self, branch, qualifiers=None): + def get_courses(self, branch, qualifiers=None, **kwargs): ''' Returns a list of course descriptors matching any given qualifiers. @@ -373,12 +371,21 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): 'structure': entry, } root = entry['root'] - course_list = self._load_items(envelope, [root], 0, lazy=True) + course_list = self._load_items(envelope, [root], 0, lazy=True, **kwargs) if not isinstance(course_list[0], ErrorDescriptor): result.append(course_list[0]) return result - def get_course(self, course_id, depth=0): + def make_course_key(self, org, course, run): + """ + Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore + that matches the supplied `org`, `course`, and `run`. + + This key may represent a course that doesn't exist in this modulestore. + """ + return CourseLocator(org, course, run) + + def get_course(self, course_id, depth=0, **kwargs): ''' Gets the course descriptor for the course identified by the locator ''' @@ -388,10 +395,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): course_entry = self._lookup_course(course_id) root = course_entry['structure']['root'] - result = self._load_items(course_entry, [root], 0, lazy=True) + result = self._load_items(course_entry, [root], 0, lazy=True, **kwargs) return result[0] - def has_course(self, course_id, ignore_case=False): + def has_course(self, course_id, ignore_case=False, **kwargs): ''' Does this course exist in this modulestore. This method does not verify that the branch &/or version in the course_id exists. Use get_course_index_info to check that. @@ -423,7 +430,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): return self._get_block_from_structure(course_structure, usage_key.block_id) is not None - def get_item(self, usage_key, depth=0): + def get_item(self, usage_key, depth=0, **kwargs): """ depth (int): An argument that some module stores may use to prefetch descendants of the queried modules for more efficient results later @@ -437,7 +444,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): raise ItemNotFoundError(usage_key) course = self._lookup_course(usage_key) - items = self._load_items(course, [usage_key.block_id], depth, lazy=True) + items = self._load_items(course, [usage_key.block_id], depth, lazy=True, **kwargs) if len(items) == 0: raise ItemNotFoundError(usage_key) elif len(items) > 1: @@ -490,7 +497,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block_id = kwargs.pop('name') block = course['structure']['blocks'].get(block_id) if _block_matches_all(block): - return self._load_items(course, [block_id], lazy=True) + return self._load_items(course, [block_id], lazy=True, **kwargs) else: return [] # don't expect caller to know that children are in fields @@ -501,7 +508,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): items.append(block_id) if len(items) > 0: - return self._load_items(course, items, 0, lazy=True) + return self._load_items(course, items, 0, lazy=True, **kwargs) else: return [] @@ -523,7 +530,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): block_id=decode_key_from_mongo(parent_id), ) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Return an array of all of the orphans in the course. """ @@ -821,10 +828,9 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): the new version_guid from the locator in the returned object! """ # split handles all the fields in one dict not separated by scope - fields = kwargs.get('fields', {}) + fields = fields or {} fields.update(kwargs.pop('metadata', {}) or {}) fields.update(kwargs.pop('definition_data', {}) or {}) - kwargs['fields'] = fields # find course_index entry if applicable and structures entry index_entry = self._get_index_if_valid(course_key, force, continue_version) @@ -946,18 +952,20 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # don't need to update the index b/c create_item did it for this version return xblock - def clone_course(self, source_course_id, dest_course_id, user_id, fields=None): + def clone_course(self, source_course_id, dest_course_id, user_id, fields=None, **kwargs): """ See :meth: `.ModuleStoreWrite.clone_course` for documentation. In split, other than copying the assets, this is cheap as it merely creates a new version of the existing course. """ - super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields) + super(SplitMongoModuleStore, self).clone_course(source_course_id, dest_course_id, user_id, fields, **kwargs) source_index = self.get_course_index_info(source_course_id) + if source_index is None: + raise ItemNotFoundError("Cannot find a course at {0}. Aborting".format(source_course_id)) return self.create_course( dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id, fields=fields, - versions_dict=source_index['versions'], search_targets=source_index['search_targets'] + versions_dict=source_index['versions'], search_targets=source_index['search_targets'], **kwargs ) def create_course( @@ -1093,10 +1101,10 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self._update_search_targets(index_entry, fields) self.db_connection.insert_course_index(index_entry) # expensive hack to persist default field values set in __init__ method (e.g., wiki_slug) - course = self.get_course(locator) - return self.update_item(course, user_id) + course = self.get_course(locator, **kwargs) + return self.update_item(course, user_id, **kwargs) - def update_item(self, descriptor, user_id, allow_not_found=False, force=False): + def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): """ Save the descriptor's fields. it doesn't descend the course dag to save the children. Return the new descriptor (updated location). @@ -1167,12 +1175,12 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): # fetch and return the new item--fetching is unnecessary but a good qc step new_locator = descriptor.location.map_into_course(course_key) - return self.get_item(new_locator) + return self.get_item(new_locator, **kwargs) else: # nothing changed, just return the one sent in return descriptor - def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None): + def create_xblock(self, runtime, category, fields=None, block_id=None, definition_id=None, parent_xblock=None, **kwargs): """ This method instantiates the correct subclass of XModuleDescriptor based on the contents of json_data. It does not persist it and can create one which @@ -1199,7 +1207,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if field_name in fields: json_data['_inherited_settings'][field_name] = fields[field_name] - new_block = runtime.xblock_from_json(xblock_class, block_id, json_data) + new_block = runtime.xblock_from_json(xblock_class, block_id, json_data, **kwargs) if parent_xblock is not None: parent_xblock.children.append(new_block.scope_ids.usage_id) # decache pending children field settings @@ -1946,7 +1954,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): for entry in entries ] - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py index 85344f6478..0edef5349b 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_draft.py @@ -48,16 +48,22 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS item = super(DraftVersioningModuleStore, self).create_course( org, course, run, user_id, master_branch=master_branch, **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item - def get_courses(self): + def get_courses(self, **kwargs): """ - Returns all the courses on the Draft branch (which is a superset of the courses on the Published branch). + Returns all the courses on the Draft or Published branch depending on the branch setting. """ - return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft) + branch_setting = self.get_branch_setting() + if branch_setting == ModuleStoreEnum.Branch.draft_preferred: + return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.draft, **kwargs) + elif branch_setting == ModuleStoreEnum.Branch.published_only: + return super(DraftVersioningModuleStore, self).get_courses(ModuleStoreEnum.BranchName.published, **kwargs) + else: + raise InsufficientSpecificationError() - def _auto_publish_no_children(self, location, category, user_id): + def _auto_publish_no_children(self, location, category, user_id, **kwargs): """ Publishes item if the category is DIRECT_ONLY. This assumes another method has checked that location points to the head of the branch and ignores the version. If you call this in any @@ -66,16 +72,17 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS """ if location.branch == ModuleStoreEnum.BranchName.draft and category in DIRECT_ONLY_CATEGORIES: # version_agnostic b/c of above assumption in docstring - self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL) + self.publish(location.version_agnostic(), user_id, blacklist=EXCLUDE_ALL, **kwargs) - def update_item(self, descriptor, user_id, allow_not_found=False, force=False): + def update_item(self, descriptor, user_id, allow_not_found=False, force=False, **kwargs): item = super(DraftVersioningModuleStore, self).update_item( descriptor, user_id, allow_not_found=allow_not_found, - force=force + force=force, + **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item def create_item( @@ -88,7 +95,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS definition_locator=definition_locator, fields=fields, force=force, continue_version=continue_version, **kwargs ) - self._auto_publish_no_children(item.location, item.location.category, user_id) + self._auto_publish_no_children(item.location, item.location.category, user_id, **kwargs) return item def create_child( @@ -99,7 +106,7 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS user_id, parent_usage_key, block_type, block_id=block_id, fields=fields, continue_version=continue_version, **kwargs ) - self._auto_publish_no_children(parent_usage_key, item.location.category, user_id) + self._auto_publish_no_children(parent_usage_key, item.location.category, user_id, **kwargs) return item def delete_item(self, location, user_id, revision=None, **kwargs): @@ -134,8 +141,8 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS for branch in branches_to_delete: branched_location = location.for_branch(branch) parent_loc = self.get_parent_location(branched_location) - SplitMongoModuleStore.delete_item(self, branched_location, user_id, **kwargs) - self._auto_publish_no_children(parent_loc, parent_loc.category, user_id) + SplitMongoModuleStore.delete_item(self, branched_location, user_id) + self._auto_publish_no_children(parent_loc, parent_loc.category, user_id, **kwargs) def _map_revision_to_branch(self, key, revision=None): """ @@ -157,12 +164,12 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS usage_key = self._map_revision_to_branch(usage_key, revision=revision) return super(DraftVersioningModuleStore, self).has_item(usage_key) - def get_item(self, usage_key, depth=0, revision=None): + def get_item(self, usage_key, depth=0, revision=None, **kwargs): """ Returns the item identified by usage_key and revision. """ usage_key = self._map_revision_to_branch(usage_key, revision=revision) - return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth) + return super(DraftVersioningModuleStore, self).get_item(usage_key, depth=depth, **kwargs) def get_items(self, course_locator, settings=None, content=None, revision=None, **kwargs): """ @@ -200,14 +207,18 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS :param xblock: the block to check :return: True if the draft and published versions differ """ - # TODO for better performance: lookup the courses and get the block entry, don't create the instances - draft = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.draft)) - try: - published = self.get_item(xblock.location.for_branch(ModuleStoreEnum.BranchName.published)) - except ItemNotFoundError: + def get_block(branch_name): + course_structure = self._lookup_course(xblock.location.for_branch(branch_name))['structure'] + return self._get_block_from_structure(course_structure, xblock.location.block_id) + + draft_block = get_block(ModuleStoreEnum.BranchName.draft) + published_block = get_block(ModuleStoreEnum.BranchName.published) + if not published_block: return True - return draft.update_version != published.source_version + # check if the draft has changed since the published was created + return draft_block['edit_info']['update_version'] != published_block['edit_info']['source_version'] + def publish(self, location, user_id, blacklist=None, **kwargs): """ @@ -224,15 +235,15 @@ class DraftVersioningModuleStore(ModuleStoreDraftAndPublished, SplitMongoModuleS [location], blacklist=blacklist ) - return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published)) + return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.published), **kwargs) - def unpublish(self, location, user_id): + def unpublish(self, location, user_id, **kwargs): """ Deletes the published version of the item. Returns the newly unpublished item. """ self.delete_item(location, user_id, revision=ModuleStoreEnum.RevisionOption.published_only) - return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft)) + return self.get_item(location.for_branch(ModuleStoreEnum.BranchName.draft), **kwargs) def revert_to_published(self, location, user_id): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py index d3fca2d8b0..dae355972e 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split_mongo_kvs.py @@ -15,45 +15,52 @@ class SplitMongoKVS(InheritanceKeyValueStore): known to the MongoModuleStore (data, children, and metadata) """ - def __init__(self, definition, fields, inherited_settings): + def __init__(self, definition, initial_values, inherited_settings, **kwargs): """ :param definition: either a lazyloader or definition id for the definition - :param fields: a dictionary of the locally set fields + :param initial_values: a dictionary of the locally set values :param inherited_settings: the json value of each inheritable field from above this. Note, local fields may override and disagree w/ this b/c this says what the value should be if the field is undefined. """ # deepcopy so that manipulations of fields does not pollute the source - super(SplitMongoKVS, self).__init__(copy.deepcopy(fields), inherited_settings) + super(SplitMongoKVS, self).__init__(copy.deepcopy(initial_values), inherited_settings) self._definition = definition # either a DefinitionLazyLoader or the db id of the definition. # if the db id, then the definition is presumed to be loaded into _fields + # a decorator function for field values (to be called when a field is accessed) + self.field_decorator = kwargs.get('field_decorator', lambda x: x) + def get(self, key): - # simplest case, field is directly set + # load the field, if needed + if key.field_name not in self._fields: + # parent undefined in editing runtime (I think) + if key.scope == Scope.parent: + # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None + return None + if key.scope == Scope.children: + # didn't find children in _fields; so, see if there's a default + raise KeyError() + elif key.scope == Scope.settings: + # get default which may be the inherited value + raise KeyError() + elif key.scope == Scope.content: + if isinstance(self._definition, DefinitionLazyLoader): + self._load_definition() + else: + raise KeyError() + else: + raise InvalidScopeError(key) + if key.field_name in self._fields: - return self._fields[key.field_name] + field_value = self._fields[key.field_name] - # parent undefined in editing runtime (I think) - if key.scope == Scope.parent: - # see STUD-624. Right now copies MongoKeyValueStore.get's behavior of returning None - return None - if key.scope == Scope.children: - # didn't find children in _fields; so, see if there's a default - raise KeyError() - elif key.scope == Scope.settings: - # get default which may be the inherited value - raise KeyError() - elif key.scope == Scope.content: - if isinstance(self._definition, DefinitionLazyLoader): - self._load_definition() - if key.field_name in self._fields: - return self._fields[key.field_name] + # return the "decorated" field value + return self.field_decorator(field_value) - raise KeyError() - else: - raise InvalidScopeError(key) + return None def set(self, key, value): # handle any special cases 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 07166e1739..2a0f79c655 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,6 +1,7 @@ import pymongo from uuid import uuid4 import ddt +import itertools from importlib import import_module from collections import namedtuple import unittest @@ -8,7 +9,6 @@ import datetime from pytz import UTC from xmodule.tests import DATA_DIR -from opaque_keys.edx.locations import Location from xmodule.modulestore import ModuleStoreEnum, PublishState from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.exceptions import InvalidVersionError @@ -21,6 +21,7 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from django.conf import settings from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.search import path_to_location +from xmodule.modulestore.exceptions import DuplicateCourseError if not settings.configured: settings.configure() from xmodule.modulestore.mixed import MixedModuleStore @@ -192,6 +193,10 @@ class TestMixedModuleStore(unittest.TestCase): """ return self.course_locations[string].course_key + def _initialize_mixed(self): + self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options) + self.addCleanup(self.store.close_all_connections) + def initdb(self, default): """ Initialize the database and create one test course in it @@ -203,8 +208,7 @@ class TestMixedModuleStore(unittest.TestCase): if index > 0: store_configs[index], store_configs[0] = store_configs[0], store_configs[index] break - self.store = MixedModuleStore(None, create_modulestore_instance=create_modulestore_instance, **self.options) - self.addCleanup(self.store.close_all_connections) + self._initialize_mixed() # convert to CourseKeys self.course_locations = { @@ -216,12 +220,9 @@ class TestMixedModuleStore(unittest.TestCase): 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 } - 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.fake_location = self.course_locations[self.MONGO_COURSEID].course_key.make_usage_key('vertical', 'fake') + self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace( category='chapter', name='Overview' ) @@ -248,6 +249,23 @@ class TestMixedModuleStore(unittest.TestCase): SlashSeparatedCourseKey('foo', 'bar', '2012_Fall')), mongo_ms_type ) + @ddt.data(*itertools.product( + (ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split), + (True, False) + )) + @ddt.unpack + def test_duplicate_course_error(self, default_ms, reset_mixed_mappings): + """ + Make sure we get back the store type we expect for given mappings + """ + self._initialize_mixed() + with self.store.default_store(default_ms): + self.store.create_course('org_x', 'course_y', 'run_z', self.user_id) + if reset_mixed_mappings: + self.store.mappings = {} + with self.assertRaises(DuplicateCourseError): + self.store.create_course('org_x', 'course_y', 'run_z', self.user_id) + # split has one lookup for the course and then one for the course items @ddt.data(('draft', 1, 0), ('split', 2, 0)) @ddt.unpack @@ -512,7 +530,7 @@ class TestMixedModuleStore(unittest.TestCase): with check_mongo_calls(mongo_store, max_find, max_send): self.store.delete_item(private_leaf.location, self.user_id) - @ddt.data(('draft', 3, 0), ('split', 6, 0)) + @ddt.data(('draft', 2, 0), ('split', 3, 0)) @ddt.unpack def test_get_courses(self, default_ms, max_find, max_send): self.initdb(default_ms) @@ -1189,6 +1207,48 @@ class TestMixedModuleStore(unittest.TestCase): # there should be no published problems with the old name assertNumProblems(problem_original_name, 0) + def verify_default_store(self, store_type): + # verify default_store property + self.assertEquals(self.store.default_modulestore.get_modulestore_type(), store_type) + + # verify internal helper method + store = self.store._get_modulestore_for_courseid() + self.assertEquals(store.get_modulestore_type(), store_type) + + # verify store used for creating a course + try: + course = self.store.create_course("org", "course{}".format(uuid4().hex[:3]), "run", self.user_id) + self.assertEquals(course.system.modulestore.get_modulestore_type(), store_type) + except NotImplementedError: + self.assertEquals(store_type, ModuleStoreEnum.Type.xml) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.xml) + def test_default_store(self, default_ms): + """ + Test the default store context manager + """ + # initialize the mixed modulestore + self._initialize_mixed() + + with self.store.default_store(default_ms): + self.verify_default_store(default_ms) + + def test_nested_default_store(self): + """ + Test the default store context manager, nested within one another + """ + # initialize the mixed modulestore + self._initialize_mixed() + + with self.store.default_store(ModuleStoreEnum.Type.mongo): + self.verify_default_store(ModuleStoreEnum.Type.mongo) + with self.store.default_store(ModuleStoreEnum.Type.split): + self.verify_default_store(ModuleStoreEnum.Type.split) + with self.store.default_store(ModuleStoreEnum.Type.xml): + self.verify_default_store(ModuleStoreEnum.Type.xml) + self.verify_default_store(ModuleStoreEnum.Type.split) + self.verify_default_store(ModuleStoreEnum.Type.mongo) + #============================================================================================================= # General utils for not using django settings diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py index 70dcf2af4e..166bf22327 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore_settings.py @@ -45,7 +45,6 @@ class ModuleStoreSettingsMigration(TestCase): "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore", "OPTIONS": { "mappings": {}, - "reference_type": "Location", "stores": { "an_old_mongo_store": { "DOC_STORE_CONFIG": {}, @@ -82,7 +81,6 @@ class ModuleStoreSettingsMigration(TestCase): 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { 'mappings': {}, - 'reference_type': 'Location', 'stores': [ { 'NAME': 'split', @@ -146,7 +144,7 @@ class ModuleStoreSettingsMigration(TestCase): Tests whether the split module store is configured in the given setting. """ stores = self._get_mixed_stores(mixed_setting) - split_settings = [store for store in stores if 'DraftVersioningModuleStore' in store['ENGINE']] + split_settings = [store for store in stores if store['ENGINE'].endswith('.DraftVersioningModuleStore')] if len(split_settings): # there should only be one setting for split self.assertEquals(len(split_settings), 1) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 5c71518364..72c9d727b5 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -24,6 +24,7 @@ from xmodule.modulestore import ModuleStoreEnum, ModuleStoreReadBase from xmodule.tabs import CourseTabList from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locations import SlashSeparatedCourseKey, Location +from opaque_keys.edx.locator import CourseLocator from xblock.field_data import DictFieldData from xblock.runtime import DictKeyValueStore, IdGenerator @@ -403,7 +404,6 @@ class XMLModuleStore(ModuleStoreReadBase): self.default_class = class_ self.parent_trackers = defaultdict(ParentTracker) - self.reference_type = Location # All field data will be stored in an inheriting field data. self.field_data = inheriting_field_data(kvs=DictKeyValueStore()) @@ -700,7 +700,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return usage_key in self.modules[usage_key.course_key] - def get_item(self, usage_key, depth=0): + def get_item(self, usage_key, depth=0, **kwargs): """ Returns an XBlock instance for the item for this UsageKey. @@ -766,7 +766,16 @@ class XMLModuleStore(ModuleStoreReadBase): return items - def get_courses(self, depth=0): + def make_course_key(self, org, course, run): + """ + Return a valid :class:`~opaque_keys.edx.keys.CourseKey` for this modulestore + that matches the supplied `org`, `course`, and `run`. + + This key may represent a course that doesn't exist in this modulestore. + """ + return CourseLocator(org, course, run, deprecated=True) + + def get_courses(self, depth=0, **kwargs): """ Returns a list of course descriptors. If there were errors on loading, some of these may be ErrorDescriptors instead. @@ -780,7 +789,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return dict((k, self.errored_courses[k].errors) for k in self.errored_courses) - def get_orphans(self, course_key): + def get_orphans(self, course_key, **kwargs): """ Get all of the xblocks in the given course which have no parents and are not of types which are usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't @@ -806,7 +815,7 @@ class XMLModuleStore(ModuleStoreReadBase): """ return ModuleStoreEnum.Type.xml - def get_courses_for_wiki(self, wiki_slug): + def get_courses_for_wiki(self, wiki_slug, **kwargs): """ Return the list of courses which use this wiki_slug :param wiki_slug: the course wiki root slug diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index 84527ce8ba..66a3bf4dc0 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -17,7 +17,7 @@ from .store_utilities import rewrite_nonportable_content_links import xblock from xmodule.tabs import CourseTabList from xmodule.modulestore.django import ASSET_IGNORE_REGEX -from xmodule.modulestore.exceptions import InvalidLocationError +from xmodule.modulestore.exceptions import DuplicateCourseError from xmodule.modulestore.mongo.base import MongoRevisionKey from xmodule.modulestore import ModuleStoreEnum @@ -174,7 +174,7 @@ def import_from_xml( if create_new_course_if_not_present and not store.has_course(dest_course_id, ignore_case=True): try: store.create_course(dest_course_id.org, dest_course_id.course, dest_course_id.run, user_id) - except InvalidLocationError: + except DuplicateCourseError: # course w/ same org and course exists log.debug( "Skipping import of course with id, {0}," diff --git a/lms/envs/bok_choy.auth.json b/lms/envs/bok_choy.auth.json index a0429f4159..a094274a5b 100644 --- a/lms/envs/bok_choy.auth.json +++ b/lms/envs/bok_choy.auth.json @@ -51,7 +51,6 @@ "ENGINE": "xmodule.modulestore.mixed.MixedModuleStore", "OPTIONS": { "mappings": {}, - "reference_type": "Location", "stores": [ { "NAME": "draft", diff --git a/lms/envs/common.py b/lms/envs/common.py index b2b9632968..a667fe246e 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -504,7 +504,6 @@ MODULESTORE = { 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { 'mappings': {}, - 'reference_type': 'Location', 'stores': [ { 'NAME': 'draft',