diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index f41a5fe046..300e338cbe 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1003,7 +1003,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # We had a bug where orphaned draft nodes caused export to fail. This is here to cover that case. vertical.location = mongo.draft.as_draft(vertical.location.replace(name='no_references')) - draft_store.save_xmodule(vertical) + draft_store.update_item(vertical, allow_not_found=True) orphan_vertical = draft_store.get_item(vertical.location) self.assertEqual(orphan_vertical.location.name, 'no_references') @@ -1020,13 +1020,14 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # now create a new/different private (draft only) vertical vertical.location = mongo.draft.as_draft(Location(['i4x', 'edX', 'toy', 'vertical', 'a_private_vertical', None])) - draft_store.save_xmodule(vertical) + draft_store.update_item(vertical, allow_not_found=True) private_vertical = draft_store.get_item(vertical.location) vertical = None # blank out b/c i destructively manipulated its location 2 lines above # add the new private to list of children - sequential = module_store.get_item(Location(['i4x', 'edX', 'toy', - 'sequential', 'vertical_sequential', None])) + sequential = module_store.get_item( + Location('i4x', 'edX', 'toy', 'sequential', 'vertical_sequential', None) + ) private_location_no_draft = private_vertical.location.replace(revision=None) sequential.children.append(private_location_no_draft.url()) module_store.update_item(sequential, self.user.id) diff --git a/cms/djangoapps/contentstore/tests/test_export_git.py b/cms/djangoapps/contentstore/tests/test_export_git.py index 6288a7ef00..3d6fef7239 100644 --- a/cms/djangoapps/contentstore/tests/test_export_git.py +++ b/cms/djangoapps/contentstore/tests/test_export_git.py @@ -17,6 +17,7 @@ from .utils import CourseTestCase import contentstore.git_export_utils as git_export_utils from xmodule.contentstore.django import _CONTENTSTORE from xmodule.modulestore.django import modulestore +from contentstore.utils import get_modulestore TEST_DATA_CONTENTSTORE = copy.deepcopy(settings.CONTENTSTORE) TEST_DATA_CONTENTSTORE['DOC_STORE_CONFIG']['db'] = 'test_xcontent_%s' % uuid4().hex @@ -70,7 +71,7 @@ class TestExportGit(CourseTestCase): Test failed course export response. """ self.course_module.giturl = 'foobar' - modulestore().save_xmodule(self.course_module) + get_modulestore(self.course_module.location).update_item(self.course_module) response = self.client.get('{}?action=push'.format(self.test_url)) self.assertIn('Export Failed:', response.content) @@ -93,7 +94,7 @@ class TestExportGit(CourseTestCase): self.populateCourse() self.course_module.giturl = 'file://{}'.format(bare_repo_dir) - modulestore().save_xmodule(self.course_module) + get_modulestore(self.course_module.location).update_item(self.course_module) response = self.client.get('{}?action=push'.format(self.test_url)) self.assertIn('Export Succeeded', response.content) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 68650e9bc1..551f0f6d57 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -26,7 +26,7 @@ from xblock.runtime import Mixologist from lms.lib.xblock.runtime import unquote_slashes -from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitState +from contentstore.utils import get_lms_link_for_item, compute_unit_state, UnitState, get_modulestore from contentstore.views.helpers import get_parent_xblock from models.settings.course_grading import CourseGradingModel @@ -365,7 +365,7 @@ def component_handler(request, usage_id, handler, suffix=''): location = unquote_slashes(usage_id) - descriptor = modulestore().get_item(location) + descriptor = get_modulestore(location).get_item(location) # Let the module handle the AJAX req = django_to_webob_request(request) @@ -376,6 +376,8 @@ def component_handler(request, usage_id, handler, suffix=''): log.info("XBlock %s attempted to access missing handler %r", descriptor, handler, exc_info=True) raise Http404 - modulestore().save_xmodule(descriptor) + # unintentional update to handle any side effects of handle call; so, request user didn't author + # the change + get_modulestore(location).update_item(descriptor, None) return webob_to_django_response(resp) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 667bf91a0d..a56ea58447 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -202,7 +202,9 @@ def xblock_view_handler(request, package_id, view_name, tag=None, branch=None, v log.debug("unable to render studio_view for %r", component, exc_info=True) fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)})) - store.save_xmodule(component) + # change not authored by requestor but by xblocks. + store.update_item(component, None) + elif view_name == 'student_view' and component.has_children: # For non-leaf xblocks on the unit page, show the special rendering # which links to the new container page. @@ -519,8 +521,8 @@ def orphan_handler(request, tag=None, package_id=None, branch=None, version_guid if request.method == 'DELETE': if request.user.is_staff: items = modulestore().get_orphans(old_location, 'draft') - for item in items: - modulestore('draft').delete_item(item, delete_all_versions=True) + for itemloc in items: + modulestore('draft').delete_item(itemloc, delete_all_versions=True) return JsonResponse({'deleted': items}) else: raise PermissionDenied() diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 2b225b9d8e..e8e3a5092c 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -661,11 +661,11 @@ class TestComponentHandler(TestCase): def setUp(self): self.request_factory = RequestFactory() - patcher = patch('contentstore.views.component.modulestore') - self.modulestore = patcher.start() + patcher = patch('contentstore.views.component.get_modulestore') + self.get_modulestore = patcher.start() self.addCleanup(patcher.stop) - self.descriptor = self.modulestore.return_value.get_item.return_value + self.descriptor = self.get_modulestore.return_value.get_item.return_value self.usage_id = 'dummy_usage_id' diff --git a/cms/envs/bok_choy.py b/cms/envs/bok_choy.py index bc2a61a8ea..2218fed90e 100644 --- a/cms/envs/bok_choy.py +++ b/cms/envs/bok_choy.py @@ -16,7 +16,7 @@ os.environ['SERVICE_VARIANT'] = 'bok_choy' os.environ['CONFIG_ROOT'] = path(__file__).abspath().dirname() #pylint: disable=E1120 from .aws import * # pylint: disable=W0401, W0614 -from xmodule.x_module import prefer_xmodules +from xmodule.modulestore import prefer_xmodules ######################### Testing overrides #################################### diff --git a/cms/envs/common.py b/cms/envs/common.py index a582d43e18..9bba1f986f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -34,7 +34,8 @@ from path import path from lms.lib.xblock.mixin import LmsBlockMixin from cms.lib.xblock.mixin import CmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin -from xmodule.x_module import XModuleMixin, only_xmodules +from xmodule.modulestore import only_xmodules +from xmodule.x_module import XModuleMixin from dealer.git import git ############################ FEATURE CONFIGURATION ############################# @@ -229,7 +230,7 @@ XBLOCK_SELECT_FUNCTION = only_xmodules # either by uncommenting them here, or adding them to your private.py # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that # xblocks can be added via advanced settings -# from xmodule.x_module import prefer_xmodules +# from xmodule.modulestore import prefer_xmodules # XBLOCK_SELECT_FUNCTION = prefer_xmodules ############################ SIGNAL HANDLERS ################################ diff --git a/cms/envs/test.py b/cms/envs/test.py index 8d60aff95c..b48ba2787b 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -16,6 +16,7 @@ from .common import * import os from path import path from warnings import filterwarnings +from xmodule.modulestore import prefer_xmodules # Nose Test Runner TEST_RUNNER = 'django_nose.NoseTestSuiteRunner' @@ -158,7 +159,6 @@ filterwarnings('ignore', message='No request passed to the backend, unable to ra ################################# XBLOCK ###################################### -from xmodule.x_module import prefer_xmodules XBLOCK_SELECT_FUNCTION = prefer_xmodules diff --git a/common/lib/xmodule/xmodule/exceptions.py b/common/lib/xmodule/xmodule/exceptions.py index 48c083cbf1..a84336c469 100644 --- a/common/lib/xmodule/xmodule/exceptions.py +++ b/common/lib/xmodule/xmodule/exceptions.py @@ -31,3 +31,10 @@ class SerializationError(Exception): def __init__(self, location, msg): super(SerializationError, self).__init__(msg) self.location = location + + +class UndefinedContext(Exception): + """ + Tried to access an xmodule field which needs a different context (runtime) to have a value. + """ + pass diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 8663d6d65b..e010948e05 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -7,11 +7,15 @@ import logging import re from collections import namedtuple +import collections from abc import ABCMeta, abstractmethod +from xblock.plugin import default_select from .exceptions import InvalidLocationError, InsufficientSpecificationError from xmodule.errortracker import make_error_tracker +from xblock.runtime import Mixologist +from xblock.core import XBlock log = logging.getLogger('edx.modulestore') @@ -447,7 +451,7 @@ class ModuleStoreWrite(ModuleStoreRead): pass @abstractmethod - def delete_item(self, location, user_id=None, delete_all_versions=False, delete_children=False, force=False): + def delete_item(self, location, user_id=None, **kwargs): """ Delete an item from persistence. Pass the user's unique id which the persistent store should save with the update if it has that ability. @@ -475,7 +479,9 @@ class ModuleStoreReadBase(ModuleStoreRead): metadata_inheritance_cache_subsystem=None, request_cache=None, modulestore_update_signal=None, xblock_mixins=(), xblock_select=None, # temporary parms to enable backward compatibility. remove once all envs migrated - db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None + db=None, collection=None, host=None, port=None, tz_aware=True, user=None, password=None, + # allow lower level init args to pass harmlessly + ** kwargs ): ''' Set up the error-tracking logic. @@ -529,9 +535,75 @@ class ModuleStoreReadBase(ModuleStoreRead): return c return None + def update_item(self, xblock, user_id=None, allow_not_found=False, force=False): + """ + Update the given xblock's persisted repr. Pass the user's unique id which the persistent store + should save with the update if it has that ability. + + :param allow_not_found: whether this method should raise an exception if the given xblock + has not been persisted before. + :param force: fork the structure and don't update the course draftVersion if there's a version + conflict (only applicable to version tracking and conflict detecting persistence stores) + + :raises VersionConflictError: if package_id and version_guid given and the current + version head != version_guid and force is not True. (only applicable to version tracking stores) + """ + raise NotImplementedError + + def delete_item(self, location, user_id=None, delete_all_versions=False, delete_children=False, force=False): + """ + Delete an item from persistence. Pass the user's unique id which the persistent store + should save with the update if it has that ability. + + :param delete_all_versions: removes both the draft and published version of this item from + the course if using draft and old mongo. Split may or may not implement this. + :param force: fork the structure and don't update the course draftVersion if there's a version + conflict (only applicable to version tracking and conflict detecting persistence stores) + + :raises VersionConflictError: if package_id and version_guid given and the current + version head != version_guid and force is not True. (only applicable to version tracking stores) + """ + raise NotImplementedError class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): ''' Implement interface functionality that can be shared. ''' - pass + def __init__(self, **kwargs): + super(ModuleStoreWriteBase, self).__init__(**kwargs) + # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington) + # This is only used by partition_fields_by_scope, which is only needed because + # the split mongo store is used for item creation as well as item persistence + self.mixologist = Mixologist(self.xblock_mixins) + + def partition_fields_by_scope(self, category, fields): + """ + Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock + + :param category: the xblock category + :param fields: the dictionary of {fieldname: value} + """ + if fields is None: + return {} + cls = self.mixologist.mix(XBlock.load_class(category, select=prefer_xmodules)) + result = collections.defaultdict(dict) + for field_name, value in fields.iteritems(): + field = getattr(cls, field_name) + result[field.scope][field_name] = value + return result + + +def only_xmodules(identifier, entry_points): + """Only use entry_points that are supplied by the xmodule package""" + from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule'] + + return default_select(identifier, from_xmodule) + + +def prefer_xmodules(identifier, entry_points): + """Prefer entry_points from the xmodule package""" + from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule'] + if from_xmodule: + return default_select(identifier, from_xmodule) + else: + return default_select(identifier, entry_points) diff --git a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py index 1f2325e811..b67454865e 100644 --- a/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py +++ b/common/lib/xmodule/xmodule/modulestore/loc_mapper_store.py @@ -119,7 +119,8 @@ class LocMapperStore(object): return package_id - def translate_location(self, old_style_course_id, location, published=True, add_entry_if_missing=True): + def translate_location(self, old_style_course_id, location, published=True, + add_entry_if_missing=True, passed_block_id=None): """ Translate the given module location to a Locator. If the mapping has the run id in it, then you should provide old_style_course_id with that run id in it to disambiguate the mapping if there exists more @@ -137,6 +138,8 @@ class LocMapperStore(object): :param add_entry_if_missing: a boolean as to whether to raise ItemNotFoundError or to create an entry if the course or block is not found in the map. + :param passed_block_id: what block_id to assign and save if none is found + (only if add_entry_if_missing) NOTE: unlike old mongo, draft branches contain the whole course; so, it applies to all category of locations including course. @@ -158,7 +161,7 @@ class LocMapperStore(object): self.create_map_entry(course_location) entry = self.location_map.find_one(location_id) else: - raise ItemNotFoundError() + raise ItemNotFoundError(location) elif len(maps) == 1: entry = maps[0] else: @@ -172,7 +175,9 @@ class LocMapperStore(object): block_id = entry['block_map'].get(self.encode_key_for_mongo(location.name)) if block_id is None: if add_entry_if_missing: - block_id = self._add_to_block_map(location, location_id, entry['block_map']) + block_id = self._add_to_block_map( + location, location_id, entry['block_map'], passed_block_id + ) else: raise ItemNotFoundError(location) elif isinstance(block_id, dict): @@ -188,7 +193,7 @@ class LocMapperStore(object): elif add_entry_if_missing: block_id = self._add_to_block_map(location, location_id, entry['block_map']) else: - raise ItemNotFoundError() + raise ItemNotFoundError(location) else: raise InvalidLocationError() @@ -297,7 +302,7 @@ class LocMapperStore(object): maps = self.location_map.find(location_id) maps = list(maps) if len(maps) == 0: - raise ItemNotFoundError() + raise ItemNotFoundError(location) elif len(maps) == 1: entry = maps[0] else: @@ -315,18 +320,19 @@ class LocMapperStore(object): else: return draft_course_locator - def _add_to_block_map(self, location, location_id, block_map): + def _add_to_block_map(self, location, location_id, block_map, block_id=None): '''add the given location to the block_map and persist it''' - if self._block_id_is_guid(location.name): - # This makes the ids more meaningful with a small probability of name collision. - # The downside is that if there's more than one course mapped to from the same org/course root - # the block ids will likely be out of sync and collide from an id perspective. HOWEVER, - # if there are few == org/course roots or their content is unrelated, this will work well. - block_id = self._verify_uniqueness(location.category + location.name[:3], block_map) - else: - # if 2 different category locations had same name, then they'll collide. Make the later - # mapped ones unique - block_id = self._verify_uniqueness(location.name, block_map) + if block_id is None: + if self._block_id_is_guid(location.name): + # This makes the ids more meaningful with a small probability of name collision. + # The downside is that if there's more than one course mapped to from the same org/course root + # the block ids will likely be out of sync and collide from an id perspective. HOWEVER, + # if there are few == org/course roots or their content is unrelated, this will work well. + block_id = self._verify_uniqueness(location.category + location.name[:3], block_map) + else: + # if 2 different category locations had same name, then they'll collide. Make the later + # mapped ones unique + block_id = self._verify_uniqueness(location.name, block_map) encoded_location_name = self.encode_key_for_mongo(location.name) block_map.setdefault(encoded_location_name, {})[location.category] = block_id self.location_map.update(location_id, {'$set': {'block_map': block_map}}) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 01c7d4155c..4c384a49f1 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -51,6 +51,12 @@ class Locator(object): def __eq__(self, other): return self.__dict__ == other.__dict__ + def __hash__(self): + """ + Hash on contents. + """ + return hash(unicode(self)) + def __repr__(self): ''' repr(self) returns something like this: CourseLocator("mit.eecs.6002x") @@ -198,16 +204,14 @@ class CourseLocator(Locator): """ Return a string representing this location. """ + parts = [] if self.package_id: - result = unicode(self.package_id) + parts.append(unicode(self.package_id)) if self.branch: - result += '/' + BRANCH_PREFIX + self.branch - return result - elif self.version_guid: - return u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid) - else: - # raise InsufficientSpecificationError("missing package_id or version_guid") - return '' + parts.append(u"{prefix}{branch}".format(prefix=BRANCH_PREFIX, branch=self.branch)) + if self.version_guid: + parts.append(u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid)) + return u"/".join(parts) def url(self): """ @@ -432,22 +436,25 @@ class BlockUsageLocator(CourseLocator): def version_agnostic(self): """ - Returns a copy of itself. - If both version_guid and package_id are known, use a blank package_id in the copy. - We don't care if the locator's version is not the current head; so, avoid version conflict by reducing info. + Returns a copy of itself without any version info. - :param block_locator: + :raises: ValueError if the block locator has no package_id """ - if self.version_guid: - return BlockUsageLocator(version_guid=self.version_guid, - branch=self.branch, - block_id=self.block_id) - else: - return BlockUsageLocator(package_id=self.package_id, - branch=self.branch, - block_id=self.block_id) + return BlockUsageLocator(package_id=self.package_id, + branch=self.branch, + block_id=self.block_id) + + def course_agnostic(self): + """ + We only care about the locator's version not its course. + Returns a copy of itself without any course info. + + :raises: ValueError if the block locator has no version_guid + """ + return BlockUsageLocator(version_guid=self.version_guid, + block_id=self.block_id) def set_block_id(self, new): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 6005638796..5152ea9231 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -5,47 +5,42 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule """ +import logging + from . import ModuleStoreWriteBase from xmodule.modulestore.django import create_modulestore_instance, loc_mapper -import logging -from xmodule.modulestore import Location -from xblock.fields import Reference, ReferenceList, String -from xmodule.modulestore.locator import CourseLocator, Locator, BlockUsageLocator -from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError -from xmodule.modulestore.parsers import ALLOWED_ID_CHARS -import re +from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE, XML_MODULESTORE_TYPE +from xmodule.modulestore.locator import CourseLocator, Locator +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError +from uuid import uuid4 +from xmodule.modulestore.mongo.base import MongoModuleStore +from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore +from xmodule.exceptions import UndefinedContext log = logging.getLogger(__name__) class MixedModuleStore(ModuleStoreWriteBase): """ - ModuleStore knows how to route requests to the right persistence ms and how to convert any - references in the xblocks to the type required by the app and the persistence layer. + ModuleStore knows how to route requests to the right persistence ms """ - def __init__(self, mappings, stores, reference_type=None, i18n_service=None, **kwargs): + def __init__(self, mappings, stores, i18n_service=None, **kwargs): """ Initialize a MixedModuleStore. Here we look into our passed in kwargs which should be a collection of other modulestore configuration informations - - :param reference_type: either Location or Locator to indicate what type of reference this app - uses. """ super(MixedModuleStore, self).__init__(**kwargs) self.modulestores = {} self.mappings = mappings - # temporary code for transition period - if reference_type is None: - log.warn("reference_type not specified in MixedModuleStore settings. %s", - "Will default temporarily to the to-be-deprecated Location.") - self.use_locations = (reference_type != 'Locator') + if 'default' not in stores: raise Exception('Missing a default modulestore in the MixedModuleStore __init__ method.') - for key, store in stores.items(): + for key, store in stores.iteritems(): is_xml = 'XMLModuleStore' in store['ENGINE'] if is_xml: + # restrict xml to only load courses in mapping store['OPTIONS']['course_ids'] = [ course_id for course_id, store_key in self.mappings.iteritems() @@ -58,137 +53,22 @@ class MixedModuleStore(ModuleStoreWriteBase): store['OPTIONS'], i18n_service=i18n_service, ) + # If and when locations can identify their course, we won't need + # these loc maps. They're needed for figuring out which store owns these locations. + if is_xml: + self.ensure_loc_maps_exist(key) def _get_modulestore_for_courseid(self, course_id): """ For a given course_id, look in the mapping table and see if it has been pinned to a particular modulestore """ + # TODO when this becomes a router capable of handling more than one r/w backend + # we'll need to generalize this to handle mappings from old Locations w/o full + # course_id in much the same way as loc_mapper().translate_location does. mapping = self.mappings.get(course_id, 'default') return self.modulestores[mapping] - def _locator_to_location(self, reference): - """ - Convert the referenced locator to a location casting to and from a string as necessary - """ - stringify = isinstance(reference, basestring) - if stringify: - reference = BlockUsageLocator(url=reference) - location = loc_mapper().translate_locator_to_location(reference) - return location.url() if stringify else location - - def _location_to_locator(self, course_id, reference): - """ - Convert the referenced location to a locator casting to and from a string as necessary - """ - stringify = isinstance(reference, basestring) - if stringify: - reference = Location(reference) - locator = loc_mapper().translate_location(course_id, reference, reference.revision == 'draft', True) - return unicode(locator) if stringify else locator - - def _incoming_reference_adaptor(self, store, course_id, reference): - """ - Convert the reference to the type the persistence layer wants - """ - if issubclass(store.reference_type, Location if self.use_locations else Locator): - return reference - if store.reference_type == Location: - return self._locator_to_location(reference) - return self._location_to_locator(course_id, reference) - - def _outgoing_reference_adaptor(self, store, course_id, reference): - """ - Convert the reference to the type the application wants - """ - if issubclass(store.reference_type, Location if self.use_locations else Locator): - return reference - if store.reference_type == Location: - return self._location_to_locator(course_id, reference) - return self._locator_to_location(reference) - - def _xblock_adaptor_iterator(self, adaptor, string_converter, store, course_id, xblock): - """ - Change all reference fields in this xblock to the type expected by the receiving layer - """ - for field in xblock.fields.itervalues(): - if field.is_set_on(xblock): - if isinstance(field, Reference): - field.write_to( - xblock, - adaptor(store, course_id, field.read_from(xblock)) - ) - elif isinstance(field, ReferenceList): - field.write_to( - xblock, - [ - adaptor(store, course_id, ref) - for ref in field.read_from(xblock) - ] - ) - elif isinstance(field, String): - # replace links within the string - string_converter(field, xblock) - return xblock - - def _incoming_xblock_adaptor(self, store, course_id, xblock): - """ - Change all reference fields in this xblock to the type expected by the persistence layer - """ - string_converter = self._get_string_converter( - course_id, store.reference_type, xblock.location - ) - return self._xblock_adaptor_iterator( - self._incoming_reference_adaptor, string_converter, store, course_id, xblock - ) - - def _outgoing_xblock_adaptor(self, store, course_id, xblock): - """ - Change all reference fields in this xblock to the type expected by the persistence layer - """ - string_converter = self._get_string_converter( - course_id, xblock.location.__class__, xblock.location - ) - return self._xblock_adaptor_iterator( - self._outgoing_reference_adaptor, string_converter, store, course_id, xblock - ) - - CONVERT_RE = re.compile(r"/jump_to_id/({}+)".format(ALLOWED_ID_CHARS)) - - def _get_string_converter(self, course_id, reference_type, from_base_addr): - """ - Return a closure which finds and replaces all embedded links in a string field - with the correct rewritten link for the target type - """ - if self.use_locations and reference_type == Location: - return lambda field, xblock: None - if not self.use_locations and issubclass(reference_type, Locator): - return lambda field, xblock: None - if isinstance(from_base_addr, Location): - def mapper(found_id): - """ - Convert the found id to BlockUsageLocator block_id - """ - location = from_base_addr.replace(category=None, name=found_id) - # NOTE without category, it cannot create a new mapping if there's not one already - return loc_mapper().translate_location(course_id, location).block_id - else: - def mapper(found_id): - """ - Convert the found id to Location block_id - """ - locator = BlockUsageLocator.make_relative(from_base_addr, found_id) - return loc_mapper().translate_locator_to_location(locator).name - - def converter(field, xblock): - """ - Find all of the ids in the block and replace them w/ their mapped values - """ - value = field.read_from(xblock) - self.CONVERT_RE.sub(mapper, value) - field.write_to(xblock, value) - return converter - def has_item(self, course_id, reference): """ Does the course include the xblock who's id is reference? @@ -197,21 +77,19 @@ class MixedModuleStore(ModuleStoreWriteBase): :param reference: a Location or BlockUsageLocator """ store = self._get_modulestore_for_courseid(course_id) - decoded_ref = self._incoming_reference_adaptor(store, course_id, reference) - return store.has_item(course_id, decoded_ref) + return store.has_item(course_id, reference) def get_item(self, location, depth=0): """ This method is explicitly not implemented as we need a course_id to disambiguate We should be able to fix this when the data-model rearchitecting is done """ + # Although we shouldn't have both get_item and get_instance imho raise NotImplementedError def get_instance(self, course_id, location, depth=0): store = self._get_modulestore_for_courseid(course_id) - decoded_ref = self._incoming_reference_adaptor(store, course_id, location) - xblock = store.get_instance(course_id, decoded_ref, depth) - return self._outgoing_xblock_adaptor(store, course_id, xblock) + return store.get_instance(course_id, location, depth) def get_items(self, location, course_id=None, depth=0, qualifiers=None): """ @@ -224,70 +102,55 @@ class MixedModuleStore(ModuleStoreWriteBase): a Locator with at least a package_id and branch but possibly no block_id. depth: An argument that some module stores may use to prefetch - descendents of the queried modules for more efficient results later + descendants of the queried modules for more efficient results later in the request. The depth is counted in the number of calls to - get_children() to cache. None indicates to cache all descendents + get_children() to cache. None indicates to cache all descendants """ if not (course_id or hasattr(location, 'package_id')): raise Exception("Must pass in a course_id when calling get_items()") store = self._get_modulestore_for_courseid(course_id or getattr(location, 'package_id')) - # translate won't work w/ missing fields so work around it - if store.reference_type == Location: - if not self.use_locations: - if getattr(location, 'block_id', False): - location = self._incoming_reference_adaptor(store, course_id, location) - else: - # get the course's location - location = loc_mapper().translate_locator_to_location(location, get_course=True) - # now remove the unknowns - location = location.replace( - category=qualifiers.get('category', None), - name=None - ) - else: - if self.use_locations: - if not isinstance(location, Location): - location = Location(location) - try: - location.ensure_fully_specified() - location = loc_mapper().translate_location( - course_id, location, location.revision == 'published', True - ) - except InsufficientSpecificationError: - # construct the Locator by hand - if location.category is not None and qualifiers.get('category', False): - qualifiers['category'] = location.category - location = loc_mapper().translate_location_to_course_locator( - course_id, location, location.revision == 'published' - ) - xblocks = store.get_items(location, course_id, depth, qualifiers) - xblocks = [self._outgoing_xblock_adaptor(store, course_id, xblock) for xblock in xblocks] - return xblocks + return store.get_items(location, course_id, depth, qualifiers) + + def _get_course_id_from_course_location(self, course_location): + """ + Get the proper course_id based on the type of course_location + """ + return getattr(course_location, 'course_id', None) or getattr(course_location, 'package_id', None) def get_courses(self): ''' Returns a list containing the top level XModuleDescriptors of the courses in this modulestore. ''' - courses = [] - for key in self.modulestores: - store_courses = self.modulestores[key].get_courses() - # If the store has not been labeled as 'default' then we should - # only surface courses that have a mapping entry, for example the XMLModuleStore will - # slurp up anything that is on disk, however, we don't want to surface those to - # consumers *unless* there is an explicit mapping in the configuration - if key != 'default': - for course in store_courses: - # make sure that the courseId is mapped to the store in question - if key == self.mappings.get(course.location.course_id, 'default'): - courses = courses + ([course]) - else: - # if we're the 'default' store provider, then we surface all courses hosted in - # that store provider - courses = courses + (store_courses) + # order the modulestores and ensure no dupes (default may be a dupe of a named store) + # remove 'draft' as we know it's a functional dupe of 'direct' (ugly hardcoding) + stores = set([value for key, value in self.modulestores.iteritems() if key != 'draft']) + stores = sorted(stores, cmp=_compare_stores) - return courses + courses = {} # a dictionary of stringified course locations to course objects + has_locators = any(issubclass(CourseLocator, store.reference_type) for store in stores) + for store in stores: + store_courses = store.get_courses() + # filter out ones which were fetched from earlier stores but locations may not be == + for course in store_courses: + course_location = unicode(course.location) + if course_location not in courses: + if has_locators and isinstance(course.location, Location): + # see if a locator version of course is in the result + try: + # if there's no existing mapping, then the course can't have been in split + course_locator = loc_mapper().translate_location( + course.location.course_id, course.location, add_entry_if_missing=False + ) + if unicode(course_locator) not in courses: + courses[course_location] = course + except ItemNotFoundError: + courses[course_location] = course + else: + courses[course_location] = course + + return courses.values() def get_course(self, course_id): """ @@ -297,44 +160,19 @@ class MixedModuleStore(ModuleStoreWriteBase): :param course_id: must be either a string course_id or a CourseLocator """ store = self._get_modulestore_for_courseid( - course_id.package_id if hasattr(course_id, 'package_id') else course_id) + course_id.package_id if hasattr(course_id, 'package_id') else course_id + ) try: - # translate won't work w/ missing fields so work around it - if store.reference_type == Location: - # takes the course_id: figure out if this is old or new style - if not self.use_locations: - if isinstance(course_id, basestring): - course_id = CourseLocator(package_id=course_id, branch='published') - course_location = loc_mapper().translate_locator_to_location(course_id, get_course=True) - course_id = course_location.course_id - xblock = store.get_course(course_id) - else: - # takes a courseLocator - if isinstance(course_id, CourseLocator): - location = course_id - course_id = None # not an old style course_id; so, don't use it further - elif '/' in course_id: - location = loc_mapper().translate_location_to_course_locator(course_id, None, True) - else: - location = CourseLocator(package_id=course_id, branch='published') - course_id = None # not an old style course_id; so, don't use it further - xblock = store.get_course(location) + return store.get_course(course_id) except ItemNotFoundError: return None - if xblock is not None: - return self._outgoing_xblock_adaptor(store, course_id, xblock) - else: - return None def get_parent_locations(self, location, course_id): """ returns the parent locations for a given location and course_id """ store = self._get_modulestore_for_courseid(course_id) - decoded_ref = self._incoming_reference_adaptor(store, course_id, location) - parents = store.get_parent_locations(decoded_ref, course_id) - return [self._outgoing_reference_adaptor(store, course_id, reference) - for reference in parents] + return store.get_parent_locations(location, course_id) def get_modulestore_type(self, course_id): """ @@ -352,10 +190,9 @@ class MixedModuleStore(ModuleStoreWriteBase): usually orphaned. NOTE: may include xblocks which still have references via xblocks which don't use children to point to their dependents. """ - course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None)) + course_id = self._get_course_id_from_course_location(course_location) store = self._get_modulestore_for_courseid(course_id) - decoded_ref = self._incoming_reference_adaptor(store, course_id, course_location) - return store.get_orphans(decoded_ref, branch) + return store.get_orphans(course_location, branch) def get_errored_courses(self): """ @@ -367,31 +204,242 @@ class MixedModuleStore(ModuleStoreWriteBase): errs.update(store.get_errored_courses()) return errs + def _get_course_id_from_block(self, block, store): + """ + Get the course_id from the block or from asking its store. Expensive. + """ + try: + return block.course_id + except UndefinedContext: + pass + try: + course = store._get_course_for_item(block.scope_ids.usage_id) + if course is not None: + return course.scope_ids.usage_id.course_id + except Exception: # sorry, that method just raises vanilla Exception if it doesn't find course + pass + + def _infer_course_id_try(self, location): + """ + Create, Update, Delete operations don't require a fully-specified course_id, but + there's no complete & sound general way to compute the course_id except via the + proper modulestore. This method attempts several sound but not complete methods. + :param location: an old style Location + """ + if isinstance(location, CourseLocator): + return location.package_id + elif isinstance(location, basestring): + try: + location = Location(location) + except InvalidLocationError: + # try to parse as a course_id + try: + Location.parse_course_id(location) + # it's already a course_id + return location + except ValueError: + # cannot interpret the location + return None + + # location is a Location at this point + if location.category == 'course': # easiest case + return location.course_id + # try finding in loc_mapper + try: + # see if the loc mapper knows the course id (requires double translation) + locator = loc_mapper().translate_location_to_course_locator(None, location) + location = loc_mapper().translate_locator_to_location(locator, get_course=True) + return location.course_id + except ItemNotFoundError: + pass + # expensive query against all location-based modulestores to look for location. + for store in self.modulestores.itervalues(): + if isinstance(location, store.reference_type): + try: + xblock = store.get_item(location) + course_id = self._get_course_id_from_block(xblock, store) + if course_id is not None: + return course_id + except NotImplementedError: + blocks = store.get_items(location) + if len(blocks) == 1: + block = blocks[0] + try: + return block.course_id + except UndefinedContext: + pass + except ItemNotFoundError: + pass + # if we get here, it must be in a Locator based store, but we won't be able to find + # it. + return None + + def create_course(self, course_id, user_id=None, store_name='default', **kwargs): + """ + Creates and returns the course. + + :param org: the org + :param fields: a dict of xblock field name - value pairs for the course module. + :param metadata: the old way of setting fields by knowing which ones are scope.settings v scope.content + :param definition_data: the complement to metadata which is also a subset of fields + :param id_root: the split-mongo course_id starting value (see split.create_course) + :param pretty_id: a field split.create_course uses and may quit using + :returns: course xblock + """ + store = self.modulestores[store_name] + if not hasattr(store, 'create_course'): + raise NotImplementedError(u"Cannot create a course on store %s" % store_name) + if store.get_modulestore_type(course_id) == SPLIT_MONGO_MODULESTORE_TYPE: + id_root = kwargs.get('id_root') + try: + course_dict = Location.parse_course_id(course_id) + org = course_dict['org'] + if id_root is None: + id_root = "{org}.{course}.{name}".format(**course_dict) + except ValueError: + org = None + if id_root is None: + id_root = course_id + org = kwargs.pop('org', org) + pretty_id = kwargs.pop('pretty_id', id_root) + fields = kwargs.pop('fields', {}) + fields.update(kwargs.pop('metadata', {})) + fields.update(kwargs.pop('definition_data', {})) + course = store.create_course(org, pretty_id, user_id, id_root=id_root, fields=fields, **kwargs) + else: # assume mongo + course = store.create_course(course_id, **kwargs) + + return course + + def create_item(self, course_or_parent_loc, category, user_id=None, **kwargs): + """ + Create and return the item. If parent_loc is a specific location v a course id, + it installs the new item as a child of the parent (if the parent_loc is a specific + xblock reference). + + :param course_or_parent_loc: Can be a course_id (org/course/run), CourseLocator, + Location, or BlockUsageLocator but must be what the persistence modulestore expects + """ + # find the store for the course + course_id = self._infer_course_id_try(course_or_parent_loc) + if course_id is None: + raise ItemNotFoundError(u"Cannot find modulestore for %s" % course_or_parent_loc) + + store = self._get_modulestore_for_courseid(course_id) + + location = kwargs.pop('location', None) + # invoke its create_item + if isinstance(store, MongoModuleStore): + block_id = kwargs.pop('block_id', getattr(location, 'name', uuid4().hex)) + # convert parent loc if it's legit + if isinstance(course_or_parent_loc, basestring): + parent_loc = None + if location is None: + loc_dict = Location.parse_course_id(course_id) + loc_dict['name'] = block_id + location = Location(category=category, **loc_dict) + else: + parent_loc = course_or_parent_loc + # must have a legitimate location, compute if appropriate + if location is None: + location = parent_loc.replace(category=category, name=block_id) + # do the actual creation + xblock = store.create_and_save_xmodule(location, **kwargs) + # don't forget to attach to parent + if parent_loc is not None and not 'detached' in xblock._class_tags: + parent = store.get_item(parent_loc) + parent.children.append(location.url()) + store.update_item(parent) + elif isinstance(store, SplitMongoModuleStore): + if isinstance(course_or_parent_loc, basestring): # course_id + course_or_parent_loc = loc_mapper().translate_location_to_course_locator( + # hardcode draft version until we figure out how we're handling branches from app + course_or_parent_loc, None, published=False + ) + elif not isinstance(course_or_parent_loc, CourseLocator): + raise ValueError(u"Cannot create a child of {} in split. Wrong repr.".format(course_or_parent_loc)) + + # split handles all the fields in one dict not separated by scope + fields = kwargs.get('fields', {}) + fields.update(kwargs.pop('metadata', {})) + fields.update(kwargs.pop('definition_data', {})) + kwargs['fields'] = fields + + xblock = store.create_item(course_or_parent_loc, category, user_id, **kwargs) + else: + raise NotImplementedError(u"Cannot create an item on store %s" % store) + + return xblock + def update_item(self, xblock, user_id, allow_not_found=False): """ Update the xblock persisted to be the same as the given for all types of fields (content, children, and metadata) attribute the change to the given user. """ - if self.use_locations: - raise NotImplementedError - - locator = xblock.location - course_id = locator.package_id + course_id = self._infer_course_id_try(xblock.scope_ids.usage_id) + if course_id is None: + raise ItemNotFoundError(u"Cannot find modulestore for %s" % xblock.scope_ids.usage_id) store = self._get_modulestore_for_courseid(course_id) + return store.update_item(xblock, user_id) - # if an xblock, convert its contents to correct addr scheme - xblock = self._incoming_xblock_adaptor(store, course_id, xblock) - xblock = store.update_item(xblock, user_id) - - return self._outgoing_xblock_adaptor(store, course_id, xblock) - - def delete_item(self, location, **kwargs): + def delete_item(self, location, user_id=None, **kwargs): """ - Delete the given item from persistence. + Delete the given item from persistence. kwargs allow modulestore specific parameters. """ - if self.use_locations: - raise NotImplementedError + course_id = self._infer_course_id_try(location) + if course_id is None: + raise ItemNotFoundError(u"Cannot find modulestore for %s" % location) + store = self._get_modulestore_for_courseid(course_id) + return store.delete_item(location, user_id=user_id, **kwargs) - store = self._get_modulestore_for_courseid(location.package_id) - decoded_ref = self._incoming_reference_adaptor(store, location.package_id, location) - return store.delete_item(decoded_ref, **kwargs) + def close_all_connections(self): + """ + Close all db connections + """ + for mstore in self.modulestores.itervalues(): + if hasattr(mstore, 'database'): + mstore.database.connection.close() + elif hasattr(mstore, 'db'): + mstore.db.connection.close() + + def ensure_loc_maps_exist(self, store_name): + """ + Ensure location maps exist for every course in the modulestore whose + name is the given name (mostly used for 'xml'). It creates maps for any + missing ones. + + NOTE: will only work if the given store is Location based. If it's not, + it raises NotImplementedError + """ + store = self.modulestores[store_name] + if store.reference_type != Location: + raise ValueError(u"Cannot create maps from %s" % store.reference_type) + for course in store.get_courses(): + loc_mapper().translate_location(course.location.course_id, course.location) + + +def _compare_stores(left, right): + """ + Order stores via precedence: if a course is found in an earlier store, it shadows the later store. + + xml stores take precedence b/c they only contain hardcoded mappings, then Locator-based ones, + then others. Locators before Locations because if some courses may be in both, + the ones in the Locator-based stores shadow the others. + """ + if left.get_modulestore_type(None) == XML_MODULESTORE_TYPE: + if right.get_modulestore_type(None) == XML_MODULESTORE_TYPE: + return 0 + else: + return -1 + elif right.get_modulestore_type(None) == XML_MODULESTORE_TYPE: + return 1 + + if issubclass(left.reference_type, Locator): + if issubclass(right.reference_type, Locator): + return 0 + else: + return -1 + elif issubclass(right.reference_type, Locator): + return 1 + + return 0 diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 6ee52a8b01..1ef0141f55 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -625,7 +625,22 @@ class MongoModuleStore(ModuleStoreWriteBase): modules = self._load_items(list(items), depth) return modules - def create_xmodule(self, location, definition_data=None, metadata=None, system=None): + def create_course(self, course_id, definition_data=None, metadata=None, runtime=None): + """ + Create a course with the given course_id. + """ + if isinstance(course_id, Location): + location = course_id + if location.category != 'course': + raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location))) + else: + course_dict = Location.parse_course_id(course_id) + course_dict['category'] = 'course' + course_dict['tag'] = 'i4x' + location = Location(course_dict) + return self.create_and_save_xmodule(location, definition_data, metadata, runtime) + + def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}): """ Create the new xmodule but don't save it. Returns the new module. @@ -672,36 +687,18 @@ class MongoModuleStore(ModuleStoreWriteBase): ScopeIds(None, location.category, location, location), dbmodel, ) + for key, value in fields.iteritems(): + setattr(xmodule, key, value) # decache any pending field settings from init xmodule.save() return xmodule - def save_xmodule(self, xmodule): - """ - Save the given xmodule (will either create or update based on whether id already exists). - Pulls out the data definition v metadata v children locally but saves it all. - - :param xmodule: - """ - # Save any changes to the xmodule to the MongoKeyValueStore - xmodule.save() - self.collection.save({ - '_id': namedtuple_to_son(xmodule.location), - 'metadata': own_metadata(xmodule), - 'definition': { - 'data': xmodule.get_explicitly_set_fields_by_scope(Scope.content), - 'children': xmodule.children if xmodule.has_children else [] - } - }) - # recompute (and update) the metadata inheritance tree which is cached - self.refresh_cached_metadata_inheritance_tree(xmodule.location) - self.fire_updated_modulestore_signal(get_course_id_no_run(xmodule.location), xmodule.location) - - def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None): + def create_and_save_xmodule(self, location, definition_data=None, metadata=None, system=None, + fields={}): """ Create the new xmodule and save it. Does not return the new module because if the caller will insert it as a child, it's inherited metadata will completely change. The difference - between this and just doing create_xmodule and save_xmodule is this ensures static_tabs get + between this and just doing create_xmodule and update_item is this ensures static_tabs get pointed to by the course. :param location: a Location--must have a category @@ -711,9 +708,9 @@ class MongoModuleStore(ModuleStoreWriteBase): """ # differs from split mongo in that I believe most of this logic should be above the persistence # layer but added it here to enable quick conversion. I'll need to reconcile these. - new_object = self.create_xmodule(location, definition_data, metadata, system) + new_object = self.create_xmodule(location, definition_data, metadata, system, fields) location = new_object.location - self.save_xmodule(new_object) + self.update_item(new_object, allow_not_found=True) # VS[compat] cdodge: This is a hack because static_tabs also have references from the course module, so # if we add one then we need to also add it to the policy information (i.e. metadata) @@ -728,9 +725,9 @@ class MongoModuleStore(ModuleStoreWriteBase): 'url_slug': new_object.location.name }) course.tabs = existing_tabs - # Save any changes to the course to the MongoKeyValueStore - course.save() - self.update_item(course, '**replace_user**') + self.update_item(course) + + return new_object def fire_updated_modulestore_signal(self, course_id, location): """ @@ -787,7 +784,7 @@ class MongoModuleStore(ModuleStoreWriteBase): if result['n'] == 0: raise ItemNotFoundError(location) - def update_item(self, xblock, user, allow_not_found=False): + def update_item(self, xblock, user=None, allow_not_found=False): """ Update the persisted version of xblock to reflect its current values. @@ -861,7 +858,7 @@ class MongoModuleStore(ModuleStoreWriteBase): location = Location.ensure_fully_specified(location) items = self.collection.find({'definition.children': location.url()}, {'_id': True}) - return [i['_id'] for i in items] + return [Location(i['_id']) for i in items] def get_modulestore_type(self, course_id): """ diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 5956b00513..c8ce76e1ec 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py @@ -92,7 +92,7 @@ class DraftModuleStore(MongoModuleStore): except ItemNotFoundError: return wrap_draft(super(DraftModuleStore, self).get_instance(course_id, location, depth=depth)) - def create_xmodule(self, location, definition_data=None, metadata=None, system=None): + def create_xmodule(self, location, definition_data=None, metadata=None, system=None, fields={}): """ Create the new xmodule but don't save it. Returns the new module with a draft locator @@ -104,22 +104,7 @@ class DraftModuleStore(MongoModuleStore): draft_loc = as_draft(location) if draft_loc.category in DIRECT_ONLY_CATEGORIES: raise InvalidVersionError(location) - return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system) - - def save_xmodule(self, xmodule): - """ - Save the given xmodule (will either create or update based on whether id already exists). - Pulls out the data definition v metadata v children locally but saves it all. - - :param xmodule: - """ - orig_location = xmodule.location - - xmodule.location = as_draft(orig_location) - try: - super(DraftModuleStore, self).save_xmodule(xmodule) - finally: - xmodule.location = orig_location + return super(DraftModuleStore, self).create_xmodule(draft_loc, definition_data, metadata, system, fields) def get_items(self, location, course_id=None, depth=0, qualifiers=None): """ @@ -159,7 +144,7 @@ class DraftModuleStore(MongoModuleStore): if draft_location.category in DIRECT_ONLY_CATEGORIES: raise InvalidVersionError(source_location) if not original: - raise ItemNotFoundError + raise ItemNotFoundError(source_location) original['_id'] = namedtuple_to_son(draft_location) try: self.collection.insert(original) @@ -171,7 +156,7 @@ class DraftModuleStore(MongoModuleStore): return self._load_items([original])[0] - def update_item(self, xblock, user, allow_not_found=False): + def update_item(self, xblock, user=None, allow_not_found=False): """ Save the current values to persisted version of the xblock @@ -187,7 +172,7 @@ class DraftModuleStore(MongoModuleStore): raise xblock.location = draft_loc - super(DraftModuleStore, self).update_item(xblock, user) + super(DraftModuleStore, self).update_item(xblock, user, allow_not_found) # don't allow locations to truly represent themselves as draft outside of this file xblock.location = as_published(xblock.location) diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 2d717edd45..da61e061a1 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -51,14 +51,13 @@ import logging import re from importlib import import_module from path import path -import collections import copy from pytz import UTC from xmodule.errortracker import null_error_tracker -from xmodule.x_module import prefer_xmodules from xmodule.modulestore.locator import ( - BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId, Locator + BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, + LocalId, Locator ) from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE @@ -67,7 +66,6 @@ from ..exceptions import ItemNotFoundError from .definition_lazy_loader import DefinitionLazyLoader from .caching_descriptor_system import CachingDescriptorSystem from xblock.fields import Scope -from xblock.runtime import Mixologist from bson.objectid import ObjectId from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection from xblock.core import XBlock @@ -132,11 +130,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): self.render_template = render_template self.i18n_service = i18n_service - # TODO: Don't have a runtime just to generate the appropriate mixin classes (cpennington) - # This is only used by _partition_fields_by_scope, which is only needed because - # the split mongo store is used for item creation as well as item persistence - self.mixologist = Mixologist(self.xblock_mixins) - def cache_items(self, system, base_block_ids, depth=0, lazy=True): ''' Handles caching of items once inheritance and any other one time @@ -281,7 +274,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): } return envelope - def get_courses(self, branch='published', qualifiers=None): + def get_courses(self, branch='draft', qualifiers=None): ''' Returns a list of course descriptors matching any given qualifiers. @@ -291,7 +284,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): Note, this is to find the current head of the named branch type (e.g., 'draft'). To get specific versions via guid use get_course. - :param branch: the branch for which to return courses. Default value is 'published'. + :param branch: the branch for which to return courses. Default value is 'draft'. :param qualifiers: a optional dict restricting which elements should match ''' if qualifiers is None: @@ -563,8 +556,8 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): The block's history tracks its explicit changes but not the changes in its children. ''' - # version_agnostic means we don't care if the head and version don't align, trust the version - course_struct = self._lookup_course(block_locator.version_agnostic())['structure'] + # course_agnostic means we don't care if the head and version don't align, trust the version + course_struct = self._lookup_course(block_locator.course_agnostic())['structure'] block_id = block_locator.block_id update_version_field = 'blocks.{}.edit_info.update_version'.format(block_id) all_versions_with_block = self.db_connection.find_matching_structures({'original_version': course_struct['original_version'], @@ -759,7 +752,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): index_entry = self._get_index_if_valid(course_or_parent_locator, force, continue_version) structure = self._lookup_course(course_or_parent_locator)['structure'] - partitioned_fields = self._partition_fields_by_scope(category, fields) + partitioned_fields = self.partition_fields_by_scope(category, fields) new_def_data = partitioned_fields.get(Scope.content, {}) # persist the definition if persisted != passed if (definition_locator is None or isinstance(definition_locator.definition_id, LocalId)): @@ -822,14 +815,19 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if index_entry is not None: if not continue_version: self._update_head(index_entry, course_or_parent_locator.branch, new_id) - course_parent = course_or_parent_locator.as_course_locator() + item_loc = BlockUsageLocator( + package_id=course_or_parent_locator.package_id, + branch=course_or_parent_locator.branch, + block_id=new_block_id, + ) else: - course_parent = None + item_loc = BlockUsageLocator( + block_id=new_block_id, + version_guid=new_id, + ) # reconstruct the new_item from the cache - return self.get_item(BlockUsageLocator(package_id=course_parent, - block_id=new_block_id, - version_guid=new_id)) + return self.get_item(item_loc) def create_course( self, org, prettyid, user_id, id_root=None, fields=None, @@ -867,7 +865,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): provide any fields overrides, see above). if not provided, will create a mostly empty course structure with just a category course root xblock. """ - partitioned_fields = self._partition_fields_by_scope(root_category, fields) + partitioned_fields = self.partition_fields_by_scope(root_category, fields) block_fields = partitioned_fields.setdefault(Scope.settings, {}) if Scope.children in partitioned_fields: block_fields.update(partitioned_fields[Scope.children]) @@ -1287,7 +1285,7 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): if index is None: raise ItemNotFoundError(package_id) # this is the only real delete in the system. should it do something else? - log.info("deleting course from split-mongo: %s", package_id) + log.info(u"deleting course from split-mongo: %s", package_id) self.db_connection.delete_course_index(index['_id']) def get_errored_courses(self): @@ -1494,22 +1492,6 @@ class SplitMongoModuleStore(ModuleStoreWriteBase): index_entry['versions'][branch] = new_id self.db_connection.update_course_index(index_entry) - def _partition_fields_by_scope(self, category, fields): - """ - Return dictionary of {scope: {field1: val, ..}..} for the fields of this potential xblock - - :param category: the xblock category - :param fields: the dictionary of {fieldname: value} - """ - if fields is None: - return {} - cls = self.mixologist.mix(XBlock.load_class(category, select=self.xblock_select)) - result = collections.defaultdict(dict) - for field_name, value in fields.iteritems(): - field = getattr(cls, field_name) - result[field.scope][field_name] = value - return result - def _filter_special_fields(self, fields): """ Remove any fields which split or its kvs computes or adds but does not want persisted. diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 71887629c4..96603ee607 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -33,7 +33,6 @@ def mixed_store_config(data_dir, mappings): 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { 'mappings': mappings, - 'reference_type': 'Location', 'stores': { 'default': mongo_config['default'], 'xml': xml_config['default'] @@ -219,13 +218,21 @@ class ModuleStoreTestCase(TestCase): # even if we're using a mixed modulestore store = editable_modulestore() if hasattr(store, 'collection'): + connection = store.collection.database.connection store.collection.drop() + connection.close() + elif hasattr(store, 'close_all_connections'): + store.close_all_connections() + if contentstore().fs_files: db = contentstore().fs_files.database db.connection.drop_database(db) + db.connection.close() + location_mapper = loc_mapper() if location_mapper.db: location_mapper.location_map.drop() + location_mapper.db.connection.close() @classmethod def setUpClass(cls): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 4788c8a1d1..6fe138e05c 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -2,8 +2,7 @@ from factory import Factory, lazy_attribute_sequence, lazy_attribute from factory.containers import CyclicDefinitionError from uuid import uuid4 -from xmodule.modulestore import Location -from xmodule.x_module import prefer_xmodules +from xmodule.modulestore import Location, prefer_xmodules from xblock.core import XBlock @@ -58,7 +57,7 @@ class CourseFactory(XModuleFactory): setattr(new_course, k, v) # Update the data in the mongo datastore - store.save_xmodule(new_course) + store.update_item(new_course) return new_course @@ -159,7 +158,7 @@ class ItemFactory(XModuleFactory): setattr(module, attr, val) module.save() - store.save_xmodule(module) + store.update_item(module) if 'detached' not in module._class_tags: parent.children.append(location.url()) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py index 3031990974..77759d2431 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/persistent_factories.py @@ -1,13 +1,23 @@ -from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor from xmodule.x_module import XModuleDescriptor import factory +from factory.helpers import lazy_attribute -# [dhm] I'm not sure why we're using factory_boy if we're not following its pattern. If anyone -# assumes they can call build, it will completely fail, for example. -# pylint: disable=W0232 -class PersistentCourseFactory(factory.Factory): +class SplitFactory(factory.Factory): + """ + Abstracted superclass which defines modulestore so that there's no dependency on django + if the caller passes modulestore in kwargs + """ + @lazy_attribute + def modulestore(self): + # Delayed import so that we only depend on django if the caller + # hasn't provided their own modulestore + from xmodule.modulestore.django import modulestore + return modulestore('split') + + +class PersistentCourseFactory(SplitFactory): """ Create a new course (not a new version of a course, but a whole new index entry). @@ -23,12 +33,15 @@ class PersistentCourseFactory(factory.Factory): # pylint: disable=W0613 @classmethod - def _create(cls, target_class, org='testX', prettyid='999', user_id='test_user', master_branch='draft', **kwargs): + def _create(cls, target_class, org='testX', prettyid='999', user_id='test_user', + master_branch='draft', id_root=None, **kwargs): + modulestore = kwargs.pop('modulestore') + root_block_id = kwargs.pop('root_block_id', 'course') # Write the data to the mongo datastore - new_course = modulestore('split').create_course( - org, prettyid, user_id, fields=kwargs, id_root=prettyid, - master_branch=master_branch) + new_course = modulestore.create_course( + org, prettyid, user_id, fields=kwargs, id_root=id_root or prettyid, + master_branch=master_branch, root_block_id=root_block_id) return new_course @@ -37,7 +50,7 @@ class PersistentCourseFactory(factory.Factory): raise NotImplementedError() -class ItemFactory(factory.Factory): +class ItemFactory(SplitFactory): FACTORY_FOR = XModuleDescriptor display_name = factory.LazyAttributeSequence(lambda o, n: "{} {}".format(o.category, n)) @@ -45,7 +58,8 @@ class ItemFactory(factory.Factory): # pylint: disable=W0613 @classmethod def _create(cls, target_class, parent_location, category='chapter', - user_id='test_user', definition_locator=None, **kwargs): + user_id='test_user', block_id=None, definition_locator=None, force=False, + continue_version=False, **kwargs): """ passes *kwargs* as the new item's field values: @@ -55,8 +69,10 @@ class ItemFactory(factory.Factory): :param definition_locator (optional): the DescriptorLocator for the definition this uses or branches """ - return modulestore('split').create_item( - parent_location, category, user_id, definition_locator, fields=kwargs + modulestore = kwargs.pop('modulestore') + return modulestore.create_item( + parent_location, category, user_id, definition_locator=definition_locator, + block_id=block_id, force=force, continue_version=continue_version, fields=kwargs ) @classmethod diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py index 0848e5c707..7a878cc9f1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_location_mapper.py @@ -12,11 +12,11 @@ from xmodule.modulestore.loc_mapper_store import LocMapperStore from mock import Mock -class TestLocationMapper(unittest.TestCase): +class LocMapperSetupSansDjango(unittest.TestCase): """ - Test the location to locator mapper + Create and destroy a loc mapper for each test """ - + loc_store = None def setUp(self): modulestore_options = { 'host': 'localhost', @@ -27,14 +27,19 @@ class TestLocationMapper(unittest.TestCase): cache_standin = TrivialCache() self.instrumented_cache = Mock(spec=cache_standin, wraps=cache_standin) # pylint: disable=W0142 - TestLocationMapper.loc_store = LocMapperStore(self.instrumented_cache, **modulestore_options) + LocMapperSetupSansDjango.loc_store = LocMapperStore(self.instrumented_cache, **modulestore_options) def tearDown(self): dbref = TestLocationMapper.loc_store.db dbref.drop_collection(TestLocationMapper.loc_store.location_map) dbref.connection.close() - TestLocationMapper.loc_store = None + self.loc_store = None + +class TestLocationMapper(LocMapperSetupSansDjango): + """ + Test the location to locator mapper + """ def test_create_map(self): org = 'foo_org' course = 'bar_course' @@ -125,7 +130,7 @@ class TestLocationMapper(unittest.TestCase): ) test_problem_locn = Location('i4x', org, course, 'problem', 'abc123') # only one course matches - self.translate_n_check(test_problem_locn, old_style_course_id, new_style_package_id, 'problem2', 'published') + # look for w/ only the Location (works b/c there's only one possible course match). Will force # cache as default translation for this problemid self.translate_n_check(test_problem_locn, None, new_style_package_id, 'problem2', 'published') @@ -389,7 +394,7 @@ def loc_mapper(): """ Mocks the global location mapper. """ - return TestLocationMapper.loc_store + return LocMapperSetupSansDjango.loc_store def render_to_template_mock(*_args): diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 047ee65dbb..cc275f91a2 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -200,13 +200,14 @@ class LocatorTest(TestCase): expected_id = 'mit.eecs.6002x' expected_branch = 'published' expected_block_ref = 'HW3' - testobj = BlockUsageLocator(package_id=testurn) + testobj = BlockUsageLocator(url=testurn) self.check_block_locn_fields(testobj, 'test_block constructor', package_id=expected_id, branch=expected_branch, block=expected_block_ref) self.assertEqual(str(testobj), testurn) self.assertEqual(testobj.url(), 'edx://' + testurn) + testobj = BlockUsageLocator(url=testurn, version_guid=ObjectId()) agnostic = testobj.version_agnostic() self.assertIsNone(agnostic.version_guid) self.check_block_locn_fields(agnostic, 'test_block constructor', @@ -225,7 +226,7 @@ class LocatorTest(TestCase): block='lab2', version_guid=ObjectId(test_id_loc) ) - agnostic = testobj.version_agnostic() + agnostic = testobj.course_agnostic() self.check_block_locn_fields( agnostic, 'error parsing URL with version and block', block='lab2', 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 1b7cbb443b..d2e31cb831 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,286 +1,407 @@ -# pylint: disable=E0611 -from nose.tools import assert_equals, assert_raises, assert_false, \ - assert_true, assert_not_equals, assert_in, assert_not_in -# pylint: enable=E0611 import pymongo from uuid import uuid4 +import ddt +from mock import patch, Mock +from importlib import import_module from xmodule.tests import DATA_DIR -from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, XML_MODULESTORE_TYPE +from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, SPLIT_MONGO_MODULESTORE_TYPE, \ + XML_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.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 from django.conf import settings -import unittest -import copy if not settings.configured: settings.configure() - from xmodule.modulestore.mixed import MixedModuleStore -HOST = 'localhost' -PORT = 27017 -DB = 'test_mongo_%s' % uuid4().hex[:5] -COLLECTION = 'modulestore' -FS_ROOT = DATA_DIR -DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' -RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' +@ddt.ddt +class TestMixedModuleStore(LocMapperSetupSansDjango): + """ + Quasi-superclass which tests Location based apps against both split and mongo dbs (Locator and + Location-based dbs) + """ + HOST = 'localhost' + PORT = 27017 + DB = 'test_mongo_%s' % uuid4().hex[:5] + COLLECTION = 'modulestore' + FS_ROOT = DATA_DIR + DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' + RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' -IMPORT_COURSEID = 'MITx/999/2013_Spring' -XML_COURSEID1 = 'edX/toy/2012_Fall' -XML_COURSEID2 = 'edX/simple/2012_Fall' + MONGO_COURSEID = 'MITx/999/2013_Spring' + XML_COURSEID1 = 'edX/toy/2012_Fall' + XML_COURSEID2 = 'edX/simple/2012_Fall' -OPTIONS = { - 'mappings': { - XML_COURSEID1: 'xml', - XML_COURSEID2: 'xml', - IMPORT_COURSEID: 'default' - }, - 'reference_type': 'Location', - 'stores': { - 'xml': { - 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore', - 'OPTIONS': { - 'data_dir': DATA_DIR, - 'default_class': 'xmodule.hidden_module.HiddenDescriptor', - } + modulestore_options = { + 'default_class': DEFAULT_CLASS, + 'fs_root': DATA_DIR, + 'render_template': RENDER_TEMPLATE, + } + DOC_STORE_CONFIG = { + 'host': HOST, + 'db': DB, + 'collection': COLLECTION, + } + OPTIONS = { + 'mappings': { + XML_COURSEID1: 'xml', + XML_COURSEID2: 'xml', + MONGO_COURSEID: 'default' }, - 'default': { - 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', - 'DOC_STORE_CONFIG': { - 'host': HOST, - 'db': DB, - 'collection': COLLECTION, + 'stores': { + 'xml': { + 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore', + 'OPTIONS': { + 'data_dir': DATA_DIR, + 'default_class': 'xmodule.hidden_module.HiddenDescriptor', + } }, - 'OPTIONS': { - 'default_class': DEFAULT_CLASS, - 'fs_root': DATA_DIR, - 'render_template': RENDER_TEMPLATE, + 'direct': { + 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': modulestore_options + }, + 'draft': { + 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': modulestore_options + }, + 'split': { + 'ENGINE': 'xmodule.modulestore.split_mongo.SplitMongoModuleStore', + 'DOC_STORE_CONFIG': DOC_STORE_CONFIG, + 'OPTIONS': modulestore_options } } } -} + def _compareIgnoreVersion(self, loc1, loc2, msg=None): + """ + AssertEqual replacement for CourseLocator + """ + if not (loc1.package_id == loc2.package_id and loc1.branch == loc2.branch and loc1.block_id == loc2.block_id): + self.fail(self._formatMessage(msg, u"{} != {}".format(unicode(loc1), unicode(loc2)))) -class TestMixedModuleStore(object): - '''Tests!''' - @classmethod - def setupClass(cls): + def setUp(self): """ Set up the database for testing """ - cls.connection = pymongo.MongoClient( - host=HOST, - port=PORT, + self.options = getattr(self, 'options', self.OPTIONS) + self.connection = pymongo.MongoClient( + host=self.HOST, + port=self.PORT, tz_aware=True, ) - cls.connection.drop_database(DB) - cls.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz') - import_course_dict = Location.parse_course_id(IMPORT_COURSEID) - cls.import_org = import_course_dict['org'] - cls.import_course = import_course_dict['course'] - cls.import_run = import_course_dict['name'] - # NOTE: Creating a single db for all the tests to save time. This - # is ok only as long as none of the tests modify the db. - # If (when!) that changes, need to either reload the db, or load - # once and copy over to a tmp db for each test. - cls.store = cls.initdb() + self.connection.drop_database(self.DB) + self.addCleanup(self.connection.drop_database, self.DB) + self.addCleanup(self.connection.close) + super(TestMixedModuleStore, self).setUp() - @classmethod - def teardownClass(cls): - """ - Clear out database after test has completed - """ - cls.destroy_db(cls.connection) - - @staticmethod - def initdb(): - """ - Initialize the database and import one test course into it - """ - # connect to the db - _options = {} - _options.update(OPTIONS) - store = MixedModuleStore(**_options) - - import_from_xml( - store._get_modulestore_for_courseid(IMPORT_COURSEID), - DATA_DIR, - ['toy'], - target_location_namespace=Location( - 'i4x', - TestMixedModuleStore.import_org, - TestMixedModuleStore.import_course, - 'course', - TestMixedModuleStore.import_run - ) + patcher = patch.multiple( + 'xmodule.modulestore.mixed', + loc_mapper=Mock(return_value=LocMapperSetupSansDjango.loc_store), + create_modulestore_instance=create_modulestore_instance, ) + patcher.start() + self.addCleanup(patcher.stop) + self.addTypeEqualityFunc(BlockUsageLocator, '_compareIgnoreVersion') + # define attrs which get set in initdb to quell pylint + self.import_chapter_location = self.store = self.fake_location = self.xml_chapter_location = None + self.course_locations = [] - return store - - @staticmethod - def destroy_db(connection): + # pylint: disable=invalid-name + def _create_course(self, default, course_id): """ - Destroy the test db. + Create a course w/ one item in the persistence store using the given course & item location. """ - connection.drop_database(DB) + course = self.store.create_course(course_id, store_name=default) + category = self.import_chapter_location.category + block_id = self.import_chapter_location.name + chapter = self.store.create_item( + # don't use course_location as it may not be the repr + course.location, category, location=self.import_chapter_location, block_id=block_id + ) + if isinstance(course.location, CourseLocator): + self.course_locations[self.MONGO_COURSEID] = course.location.version_agnostic() + self.import_chapter_location = chapter.location.version_agnostic() + else: + self.assertEqual(course.location.course_id, course_id) + self.assertEqual(chapter.location, self.import_chapter_location) - def setUp(self): - # make a copy for convenience - self.connection = TestMixedModuleStore.connection + def initdb(self, default): + """ + Initialize the database and create one test course in it + """ + # set the default modulestore + self.options['stores']['default'] = self.options['stores'][default] + self.store = MixedModuleStore(**self.options) + self.addCleanup(self.store.close_all_connections) - def tearDown(self): - pass + self.course_locations = { + course_id: generate_location(course_id) + for course_id in [self.MONGO_COURSEID, self.XML_COURSEID1, self.XML_COURSEID2] + } + self.fake_location = Location('i4x', 'foo', 'bar', 'vertical', 'baz') + self.import_chapter_location = self.course_locations[self.MONGO_COURSEID].replace( + category='chapter', name='Overview' + ) + self.xml_chapter_location = self.course_locations[self.XML_COURSEID1].replace( + 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('foo/bar/2012_Fall', self.fake_location) - def test_get_modulestore_type(self): + self._create_course(default, self.MONGO_COURSEID) + + @ddt.data('direct', 'split') + def test_get_modulestore_type(self, default_ms): """ Make sure we get back the store type we expect for given mappings """ - assert_equals(self.store.get_modulestore_type(XML_COURSEID1), XML_MODULESTORE_TYPE) - assert_equals(self.store.get_modulestore_type(XML_COURSEID2), XML_MODULESTORE_TYPE) - assert_equals(self.store.get_modulestore_type(IMPORT_COURSEID), MONGO_MODULESTORE_TYPE) + self.initdb(default_ms) + self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID1), XML_MODULESTORE_TYPE) + self.assertEqual(self.store.get_modulestore_type(self.XML_COURSEID2), XML_MODULESTORE_TYPE) + mongo_ms_type = MONGO_MODULESTORE_TYPE if default_ms == 'direct' else SPLIT_MONGO_MODULESTORE_TYPE + self.assertEqual(self.store.get_modulestore_type(self.MONGO_COURSEID), mongo_ms_type) # try an unknown mapping, it should be the 'default' store - assert_equals(self.store.get_modulestore_type('foo/bar/2012_Fall'), MONGO_MODULESTORE_TYPE) + self.assertEqual(self.store.get_modulestore_type('foo/bar/2012_Fall'), mongo_ms_type) - def test_has_item(self): - assert_true(self.store.has_item( - IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run]) - )) - assert_true(self.store.has_item( - XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall']) - )) + @ddt.data('direct', 'split') + def test_has_item(self, default_ms): + self.initdb(default_ms) + for course_id, course_locn in self.course_locations.iteritems(): + self.assertTrue(self.store.has_item(course_id, course_locn)) # try negative cases - assert_false(self.store.has_item( - XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run]) - )) - assert_false(self.store.has_item( - IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall']) + self.assertFalse(self.store.has_item( + self.XML_COURSEID1, + self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') )) + self.assertFalse(self.store.has_item(self.MONGO_COURSEID, self.fake_location)) - def test_get_item(self): - with assert_raises(NotImplementedError): + @ddt.data('direct', 'split') + def test_get_item(self, default_ms): + self.initdb(default_ms) + with self.assertRaises(NotImplementedError): self.store.get_item(self.fake_location) - def test_get_instance(self): - module = self.store.get_instance( - IMPORT_COURSEID, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run]) - ) - assert_not_equals(module, None) - - module = self.store.get_instance( - XML_COURSEID1, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall']) - ) - assert_not_equals(module, None) + @ddt.data('direct', 'split') + def test_get_instance(self, default_ms): + self.initdb(default_ms) + for course_id, course_locn in self.course_locations.iteritems(): + self.assertIsNotNone(self.store.get_instance(course_id, course_locn)) # try negative cases - with assert_raises(ItemNotFoundError): + with self.assertRaises(ItemNotFoundError): self.store.get_instance( - XML_COURSEID1, Location(['i4x', self.import_org, self.import_course, 'course', self.import_run]) + self.XML_COURSEID1, + self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') ) + with self.assertRaises(ItemNotFoundError): + self.store.get_instance(self.MONGO_COURSEID, self.fake_location) - with assert_raises(ItemNotFoundError): - self.store.get_instance( - IMPORT_COURSEID, Location(['i4x', 'edX', 'toy', 'course', '2012_Fall']) - ) + @ddt.data('direct', 'split') + def test_get_items(self, default_ms): + self.initdb(default_ms) + for course_id, course_locn in self.course_locations.iteritems(): + if hasattr(course_locn, 'as_course_locator'): + locn = course_locn.as_course_locator() + else: + locn = course_locn.replace(org=None, course=None, name=None) + # NOTE: use get_course if you just want the course. get_items is expensive + modules = self.store.get_items(locn, course_id, qualifiers={'category': 'course'}) + self.assertEqual(len(modules), 1) + self.assertEqual(modules[0].location, course_locn) - def test_get_items(self): - # NOTE: use get_course if you just want the course. get_items only allows wildcarding of category and name - modules = self.store.get_items(Location('i4x', None, None, 'course', None), IMPORT_COURSEID) - assert_equals(len(modules), 1) - assert_equals(modules[0].location.course, self.import_course) + @ddt.data('direct', 'split') + def test_update_item(self, default_ms): + """ + Update should fail for r/o dbs and succeed for r/w ones + """ + self.initdb(default_ms) + course_id = self.XML_COURSEID1 + course = self.store.get_course(course_id) + # if following raised, then the test is really a noop, change it + self.assertFalse(course.show_calculator, "Default changed making test meaningless") + course.show_calculator = True + with self.assertRaises(NotImplementedError): + self.store.update_item(course, None) + # now do it for a r/w db + # get_course api's are inconsistent: one takes Locators the other an old style course id + if hasattr(self.course_locations[self.MONGO_COURSEID], 'as_course_locator'): + locn = self.course_locations[self.MONGO_COURSEID] + else: + locn = self.MONGO_COURSEID + course = self.store.get_course(locn) + # if following raised, then the test is really a noop, change it + self.assertFalse(course.show_calculator, "Default changed making test meaningless") + course.show_calculator = True + self.store.update_item(course, None) + course = self.store.get_course(locn) + self.assertTrue(course.show_calculator) - modules = self.store.get_items(Location('i4x', None, None, 'course', None), XML_COURSEID1) - assert_equals(len(modules), 1) - assert_equals(modules[0].location.course, 'toy') + @ddt.data('direct', 'split') + def test_delete_item(self, default_ms): + """ + Delete should reject on r/o db and work on r/w one + """ + self.initdb(default_ms) + # r/o try deleting the course + with self.assertRaises(NotImplementedError): + self.store.delete_item(self.xml_chapter_location) + self.store.delete_item(self.import_chapter_location, '**replace_user**') + # verify it's gone + with self.assertRaises(ItemNotFoundError): + self.store.get_instance(self.MONGO_COURSEID, self.import_chapter_location) - modules = self.store.get_items(Location('i4x', 'edX', 'simple', 'course', None), XML_COURSEID2) - assert_equals(len(modules), 1) - assert_equals(modules[0].location.course, 'simple') - - def test_update_item(self): - # FIXME update - with assert_raises(NotImplementedError): - self.store.update_item(self.fake_location, '**replace_user**') - - def test_delete_item(self): - with assert_raises(NotImplementedError): - self.store.delete_item(self.fake_location) - - def test_get_courses(self): - # we should have 3 total courses aggregated + @ddt.data('direct', 'split') + def test_get_courses(self, default_ms): + self.initdb(default_ms) + # we should have 3 total courses across all stores courses = self.store.get_courses() - assert_equals(len(courses), 3) - course_ids = [] - for course in courses: - course_ids.append(course.location.course_id) - assert_true(IMPORT_COURSEID in course_ids) - assert_true(XML_COURSEID1 in course_ids) - assert_true(XML_COURSEID2 in course_ids) + course_ids = [ + course.location.version_agnostic() + if hasattr(course.location, 'version_agnostic') else course.location + for course in courses + ] + self.assertEqual(len(courses), 3, "Not 3 courses: {}".format(course_ids)) + self.assertIn(self.course_locations[self.MONGO_COURSEID], course_ids) + self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) + self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) def test_xml_get_courses(self): """ Test that the xml modulestore only loaded the courses from the maps. """ + self.initdb('direct') courses = self.store.modulestores['xml'].get_courses() - assert_equals(len(courses), 2) + self.assertEqual(len(courses), 2) course_ids = [course.location.course_id for course in courses] - assert_in(XML_COURSEID1, course_ids) - assert_in(XML_COURSEID2, course_ids) + self.assertIn(self.XML_COURSEID1, course_ids) + self.assertIn(self.XML_COURSEID2, course_ids) # this course is in the directory from which we loaded courses but not in the map - assert_not_in("edX/toy/TT_2012_Fall", course_ids) + self.assertNotIn("edX/toy/TT_2012_Fall", course_ids) - def test_get_course(self): - module = self.store.get_course(IMPORT_COURSEID) - assert_equals(module.location.course, self.import_course) + def test_xml_no_write(self): + """ + Test that the xml modulestore doesn't allow write ops. + """ + self.initdb('direct') + with self.assertRaises(NotImplementedError): + self.store.create_course("org/course/run", store_name='xml') - module = self.store.get_course(XML_COURSEID1) - assert_equals(module.location.course, 'toy') + @ddt.data('direct', 'split') + def test_get_course(self, default_ms): + self.initdb(default_ms) + for course_locn in self.course_locations.itervalues(): + if hasattr(course_locn, 'as_course_locator'): + locn = course_locn.as_course_locator() + else: + locn = course_locn.course_id + # NOTE: use get_course if you just want the course. get_items is expensive + course = self.store.get_course(locn) + self.assertIsNotNone(course) + self.assertEqual(course.location, course_locn) - module = self.store.get_course(XML_COURSEID2) - assert_equals(module.location.course, 'simple') - - # pylint: disable=E1101 - def test_get_parent_locations(self): + @ddt.data('direct', 'split') + def test_get_parent_locations(self, default_ms): + self.initdb(default_ms) parents = self.store.get_parent_locations( - Location(['i4x', self.import_org, self.import_course, 'chapter', 'Overview']), - IMPORT_COURSEID + self.import_chapter_location, + self.MONGO_COURSEID ) - assert_equals(len(parents), 1) - assert_equals(Location(parents[0]).org, self.import_org) - assert_equals(Location(parents[0]).course, self.import_course) - assert_equals(Location(parents[0]).name, self.import_run) + self.assertEqual(len(parents), 1) + self.assertEqual(parents[0], self.course_locations[self.MONGO_COURSEID]) parents = self.store.get_parent_locations( - Location(['i4x', 'edX', 'toy', 'chapter', 'Overview']), - XML_COURSEID1 + self.xml_chapter_location, + self.XML_COURSEID1 ) - assert_equals(len(parents), 1) - assert_equals(Location(parents[0]).org, 'edX') - assert_equals(Location(parents[0]).course, 'toy') - assert_equals(Location(parents[0]).name, '2012_Fall') + self.assertEqual(len(parents), 1) + self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1]) -class TestMixedMSInit(unittest.TestCase): - """ - Test initializing w/o a reference_type - """ - def setUp(self): - unittest.TestCase.setUp(self) - options = copy.copy(OPTIONS) - del options['reference_type'] - self.connection = pymongo.MongoClient( - host=HOST, - port=PORT, - tz_aware=True, + @ddt.data('direct', 'split') + def test_get_orphans(self, default_ms): + self.initdb(default_ms) + # create an orphan + if default_ms == 'split': + course_id = self.course_locations[self.MONGO_COURSEID].as_course_locator() + branch = course_id.branch + else: + course_id = self.MONGO_COURSEID + branch = None + orphan = self.store.create_item(course_id, 'problem', block_id='orphan') + found_orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID], branch) + if default_ms == 'split': + self.assertEqual(found_orphans, [orphan.location.version_agnostic()]) + else: + self.assertEqual(found_orphans, [unicode(orphan.location)]) + + @ddt.data('split') + def test_create_item_from_course_id(self, default_ms): + """ + Test code paths missed by the above: + * passing an old-style course_id which has a loc map to split's create_item + """ + self.initdb(default_ms) + # create loc_map entry + loc_mapper().translate_location(self.MONGO_COURSEID, generate_location(self.MONGO_COURSEID)) + orphan = self.store.create_item(self.MONGO_COURSEID, 'problem', block_id='orphan') + self.assertEqual( + orphan.location.version_agnostic().as_course_locator(), + self.course_locations[self.MONGO_COURSEID].as_course_locator() ) - self.store = MixedModuleStore(**options) - def test_use_locations(self): + @ddt.data('direct') + def test_create_item_from_parent_location(self, default_ms): """ - Test that use_locations defaulted correctly + Test a code path missed by the above: passing an old-style location as parent but no + new location for the child """ - self.assertTrue(self.store.use_locations) + self.initdb(default_ms) + self.store.create_item(self.course_locations[self.MONGO_COURSEID], 'problem', block_id='orphan') + orphans = self.store.get_orphans(self.course_locations[self.MONGO_COURSEID], None) + self.assertEqual(len(orphans), 0, "unexpected orphans: {}".format(orphans)) +#============================================================================================================= +# General utils for not using django settings +#============================================================================================================= + + +def load_function(path): + """ + Load a function by name. + + path is a string of the form "path.to.module.function" + returns the imported python object `function` from `path.to.module` + """ + module_path, _, name = path.rpartition('.') + return getattr(import_module(module_path), name) + + +# pylint: disable=unused-argument +def create_modulestore_instance(engine, doc_store_config, options, i18n_service=None): + """ + This will return a new instance of a modulestore given an engine and options + """ + class_ = load_function(engine) + + return class_( + doc_store_config=doc_store_config, + **options + ) + + +def generate_location(course_id): + """ + Generate the locations for the given ids + """ + course_dict = Location.parse_course_id(course_id) + course_dict['tag'] = 'i4x' + course_dict['category'] = 'course' + return Location(course_dict) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index f492c321d2..33b6eaa2a4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -56,6 +56,7 @@ class TestMongoModuleStore(object): def teardownClass(cls): if cls.connection: cls.connection.drop_database(DB) + cls.connection.close() @staticmethod def initdb(): 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 653b2a2f70..d2121bffa1 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -163,8 +163,6 @@ class SplitModuleCourseTests(SplitModuleTest): "children") _verify_published_course(modulestore().get_courses(branch='published')) - # default for branch is 'published'. - _verify_published_course(modulestore().get_courses()) def test_search_qualifiers(self): # query w/ search criteria diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 0f0aa80145..0a185ded87 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -11,10 +11,10 @@ from mock import Mock, patch from django.utils.timezone import UTC from xmodule.xml_module import is_pointer_tag -from xmodule.modulestore import Location +from xmodule.modulestore import Location, only_xmodules from xmodule.modulestore.xml import ImportSystem, XMLModuleStore, LocationReader from xmodule.modulestore.inheritance import compute_inherited_metadata -from xmodule.x_module import XModuleMixin, only_xmodules +from xmodule.x_module import XModuleMixin from xmodule.fields import Date from xmodule.tests import DATA_DIR from xmodule.modulestore.inheritance import InheritanceMixin diff --git a/common/lib/xmodule/xmodule/tests/xml/factories.py b/common/lib/xmodule/xmodule/tests/xml/factories.py index a66fdba2e1..e498645c2d 100644 --- a/common/lib/xmodule/xmodule/tests/xml/factories.py +++ b/common/lib/xmodule/xmodule/tests/xml/factories.py @@ -9,7 +9,7 @@ from factory import Factory, lazy_attribute, post_generation, Sequence from lxml import etree from xmodule.modulestore.inheritance import InheritanceMixin -from xmodule.x_module import only_xmodules +from xmodule.modulestore import only_xmodules class XmlImportData(object): diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 6d9ada7314..38d895b93e 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -26,6 +26,7 @@ from xmodule.errortracker import exc_info_to_str from xmodule.modulestore import Location from xmodule.modulestore.exceptions import ItemNotFoundError, InsufficientSpecificationError, InvalidLocationError from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.exceptions import UndefinedContext log = logging.getLogger(__name__) @@ -580,22 +581,6 @@ class ResourceTemplates(object): return template -def prefer_xmodules(identifier, entry_points): - """Prefer entry_points from the xmodule package""" - from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule'] - if from_xmodule: - return default_select(identifier, from_xmodule) - else: - return default_select(identifier, entry_points) - - -def only_xmodules(identifier, entry_points): - """Only use entry_points that are supplied by the xmodule package""" - from_xmodule = [entry_point for entry_point in entry_points if entry_point.dist.key == 'xmodule'] - - return default_select(identifier, from_xmodule) - - @XBlock.needs("i18n") class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): """ @@ -811,7 +796,8 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): Returns the XModule corresponding to this descriptor. Expects that the system already supports all of the attributes needed by xmodules """ - assert self.xmodule_runtime is not None + if self.xmodule_runtime is None: + raise UndefinedContext() assert self.xmodule_runtime.error_descriptor_class is not None if self.xmodule_runtime.xmodule_instance is None: try: diff --git a/lms/djangoapps/branding/tests.py b/lms/djangoapps/branding/tests.py index 357ddb8e37..d7880a9781 100644 --- a/lms/djangoapps/branding/tests.py +++ b/lms/djangoapps/branding/tests.py @@ -31,7 +31,7 @@ class AnonymousIndexPageTest(ModuleStoreTestCase): self.course = CourseFactory.create() self.course.days_early_for_beta = 5 self.course.enrollment_start = datetime.datetime.now(UTC) + datetime.timedelta(days=3) - self.store.save_xmodule(self.course) + self.store.update_item(self.course) @override_settings(FEATURES=FEATURES_WITH_STARTDATE) def test_none_user_index_access_with_startdate_fails(self): diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index 5f059782ad..c5cacabc3b 100644 --- a/lms/envs/acceptance.py +++ b/lms/envs/acceptance.py @@ -45,7 +45,6 @@ MODULESTORE = { 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { 'mappings': {}, - 'reference_type': 'Location', 'stores': { 'default': { 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index ae8f5bacf0..afeb08d1d5 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -4,7 +4,7 @@ Settings for bok choy tests import os from path import path -from xmodule.x_module import prefer_xmodules +from xmodule.modulestore import prefer_xmodules CONFIG_ROOT = path(__file__).abspath().dirname() #pylint: disable=E1120 diff --git a/lms/envs/cms/dev.py b/lms/envs/cms/dev.py index 57b362ce52..440e74a5c9 100644 --- a/lms/envs/cms/dev.py +++ b/lms/envs/cms/dev.py @@ -32,7 +32,6 @@ MODULESTORE = { 'default': { 'ENGINE': 'xmodule.modulestore.mixed.MixedModuleStore', 'OPTIONS': { - 'reference_type': 'Location', 'mappings': {}, 'stores': { 'default': { diff --git a/lms/envs/common.py b/lms/envs/common.py index 623c03098b..9be8790f7b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -34,7 +34,8 @@ from .discussionsettings import * from lms.lib.xblock.mixin import LmsBlockMixin from xmodule.modulestore.inheritance import InheritanceMixin -from xmodule.x_module import XModuleMixin, only_xmodules +from xmodule.x_module import XModuleMixin +from xmodule.modulestore import only_xmodules ################################### FEATURES ################################### # The display name of the platform to be used in templates/emails/etc. @@ -440,7 +441,7 @@ XBLOCK_SELECT_FUNCTION = only_xmodules # Use the following lines to allow any xblock in the LMS, # either by uncommenting them here, or adding them to your private.py -# from xmodule.x_module import prefer_xmodules +# from xmodule.modulestore import prefer_xmodules # XBLOCK_SELECT_FUNCTION = prefer_xmodules #################### Python sandbox ############################################