From ca0dd8390fa89b4f913ad4497564e28296837d86 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Mon, 24 Feb 2014 11:36:34 -0500 Subject: [PATCH] Mixed no longer translates --- .../contentstore/tests/test_export_git.py | 5 +- cms/djangoapps/contentstore/views/item.py | 7 +- cms/envs/bok_choy.py | 2 +- .../xmodule/xmodule/modulestore/__init__.py | 4 +- .../xmodule/xmodule/modulestore/locator.py | 49 +- .../lib/xmodule/xmodule/modulestore/mixed.py | 446 +++++------------- .../xmodule/xmodule/modulestore/mongo/base.py | 3 +- .../xmodule/modulestore/mongo/draft.py | 4 +- .../xmodule/modulestore/split_mongo/split.py | 4 +- .../xmodule/modulestore/tests/django_utils.py | 6 +- .../modulestore/tests/test_locators.py | 5 +- .../tests/test_mixed_modulestore.py | 139 +++--- lms/envs/acceptance.py | 1 - lms/envs/cms/dev.py | 1 - 14 files changed, 249 insertions(+), 427 deletions(-) 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/item.py b/cms/djangoapps/contentstore/views/item.py index a0e1bfb164..6c15a7d400 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -202,7 +202,12 @@ 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. should not convert to draft if not + # already draft + if getattr(component, 'is_draft', False): + modulestore('draft').update_item(component, None) + else: + modulestore('direct').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. 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/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index d752b6dcae..b67480ac75 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -479,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. diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 01c7d4155c..b76ca4861a 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,16 @@ class CourseLocator(Locator): """ Return a string representing this location. """ + result = u"" if self.package_id: - result = unicode(self.package_id) + result += 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 '' + if self.version_guid: + if self.package_id: + result += u"/" + result += u"{prefix}{guid}".format(prefix=VERSION_PREFIX, guid=self.version_guid) + return result def url(self): """ @@ -432,22 +438,29 @@ 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. + + :raises: ValueError if the block locator has no package_id :param block_locator: """ - 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 + + :param block_locator: + """ + 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 e95dcee5aa..663552b98c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -5,58 +5,41 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule """ -import re -from importlib import import_module import logging -from xblock.fields import Reference, ReferenceList, String from . import ModuleStoreWriteBase from xmodule.modulestore.django import create_modulestore_instance, loc_mapper from xmodule.modulestore import Location, SPLIT_MONGO_MODULESTORE_TYPE -from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator -from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError -from xmodule.modulestore.parsers import ALLOWED_ID_CHARS +from xmodule.modulestore.locator import CourseLocator +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 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 a class object such as Locator or Location or the fully - qualified dot-path to that class def to indicate what type of reference the 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.reference_type = Location - elif isinstance(reference_type, basestring): - module_path, _, class_name = reference_type.rpartition('.') - class_ = getattr(import_module(module_path), class_name) - self.reference_type = class_ - else: - self.reference_type = reference_type 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() @@ -69,8 +52,8 @@ class MixedModuleStore(ModuleStoreWriteBase): store['OPTIONS'], i18n_service=i18n_service, ) - # it would be better to have a notion of read-only rather than hardcode - # key name + # 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) @@ -85,148 +68,6 @@ class MixedModuleStore(ModuleStoreWriteBase): mapping = self.mappings.get(course_id, 'default') return self.modulestores[mapping] - # TODO move the location converters to a helper class which returns a converter object w/ 2 - # methods: convert_xblock and convert_reference. Then have mixed get the converter and use it. - 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 == None, 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 reference is None: - return None - if issubclass(store.reference_type, self.reference_type): - 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 reference is None: - return None - if issubclass(store.reference_type, self.reference_type): - 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 - """ - xblock.location = adaptor(store, course_id, xblock.location) - 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.scope_ids.usage_id - ) - 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.scope_ids.usage_id.__class__, xblock.scope_ids.usage_id - ) - 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 issubclass(self.reference_type, reference_type): - 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 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 NotImplementedError(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 has_item(self, course_id, reference): """ Does the course include the xblock who's id is reference? @@ -235,21 +76,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): """ @@ -262,44 +101,15 @@ 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')) - if not issubclass(self.reference_type, store.reference_type): - if store.reference_type == Location: - 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 not isinstance(location, Location): - location = Location(location) - try: - location.ensure_fully_specified() - location = loc_mapper().translate_location( - course_id, location, location.revision == None, 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 == None - ) - 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): """ @@ -312,35 +122,39 @@ class MixedModuleStore(ModuleStoreWriteBase): Returns a list containing the top level XModuleDescriptors of the courses in this modulestore. ''' - courses = [] - for key in self.modulestores: - store = self.modulestores[key] - store_courses = store.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 - # TODO obviously this filtering only applies to filebased stores - if key != 'default': - for course in store_courses: - course_id = self._get_course_id_from_course_location(course.location) - # make sure that the courseId is mapped to the store in question - if key == self.mappings.get(course_id, 'default'): - courses.append( - self._outgoing_reference_adaptor(store, course_id, course.location) - ) - else: - # if we're the 'default' store provider, then we surface all courses hosted in - # that store provider - store_courses = [ - self._outgoing_reference_adaptor( - store, self._get_course_id_from_course_location(course.location), course.location - ) - for course in store_courses - ] - courses = courses + store_courses + courses = {} + # order the modulestores and ensure no dupes: an awkward bit of hardcoding to ensure precedence + # xml is in here because mappings trump discovery + stores = [self.modulestores['default'], self.modulestores['xml']] + for key, store in self.modulestores.iteritems(): + # awkward hardcoding of knowledge that 'draft' is a dupe of 'direct' + if key != 'draft' and store not in stores: + stores.append(store) - return courses + has_locators = False + 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): + 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 + elif isinstance(course.location, CourseLocator): + has_locators = True + courses[course_location] = course + else: + courses[course_location] = course + + return courses.values() def get_course(self, course_id): """ @@ -350,46 +164,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 issubclass(store.reference_type, self.reference_type): - 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): """ @@ -409,8 +196,7 @@ class MixedModuleStore(ModuleStoreWriteBase): """ course_id = getattr(course_location, 'course_id', getattr(course_location, 'package_id', None)) 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): """ @@ -426,13 +212,16 @@ class MixedModuleStore(ModuleStoreWriteBase): """ Get the course_id from the block or from asking its store. Expensive. """ - if block.course_id is not None: - return block.course_id + try: + if block.course_id is not None: + return block.course_id + except AssertionError: # will occur if no xmodule set + pass try: course = store._get_course_for_item(block.scope_ids.usage_id) if course: return course.scope_ids.usage_id.course_id - except: # sorry, that method just raises vanilla Exception + except: # sorry, that method just raises vanilla Exception if it doesn't find course pass def _infer_course_id_try(self, location): @@ -442,10 +231,27 @@ class MixedModuleStore(ModuleStoreWriteBase): 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 @@ -455,16 +261,19 @@ class MixedModuleStore(ModuleStoreWriteBase): for store in self.modulestores.itervalues(): if isinstance(location, store.reference_type): try: - block = store.get_item(location) - course_id = self._get_course_id_from_block(block, store) + 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] - if block.course_id is not None: - return block.course_id + try: + if block.course_id is not None: + return block.course_id + except AssertionError: + pass except ItemNotFoundError: pass # if we get here, it must be in a Locator based store, but we won't be able to find @@ -481,19 +290,20 @@ class MixedModuleStore(ModuleStoreWriteBase): NOTE: unlike the other mixed modulestore methods, this does not adapt its argument to the persistence store but requires its caller to know what the persistence store - wants for args. It does not translate any references on the way in; so, don't - pass children or other reference fields here. - It does, however, adapt the xblock on the way out to the app's - reference_type + wants for args. :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) + raise NotImplementedError(u"Cannot create a course on store %s" % store_name) if store.get_modulestore_type(course_location.course_id) == SPLIT_MONGO_MODULESTORE_TYPE: org = kwargs.pop('org', course_location.org) pretty_id = kwargs.pop('pretty_id', None) + fields = kwargs.get('fields', {}) + fields.update(kwargs.pop('metadata', {})) + fields.update(kwargs.pop('definition_data', {})) + kwargs['fields'] = fields # TODO rename id_root to package_id for consistency. It's too confusing id_root = kwargs.pop('id_root', u"{0.org}.{0.course}.{0.name}".format(course_location)) course = store.create_course( @@ -509,7 +319,7 @@ class MixedModuleStore(ModuleStoreWriteBase): course = store.create_course(course_location, **kwargs) loc_mapper().translate_location(course_location.course_id, course_location) - return self._outgoing_xblock_adaptor(store, course_location.course_id, course) + return course def create_item(self, course_or_parent_loc, category, user_id=None, **kwargs): """ @@ -520,22 +330,17 @@ class MixedModuleStore(ModuleStoreWriteBase): Adds an entry to the loc map using the kwarg location if provided (must be a Location if provided) or block_id and category if provided. - :param course_or_parent_loc: will be translated appropriately to the course's store. - Can be a course_id (org/course/run), CourseLocator, Location, or BlockUsageLocator. + :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 - if self.reference_type == Location: - if hasattr(course_or_parent_loc, 'tag'): - course_id = self._infer_course_id_try(course_or_parent_loc) - else: - course_id = course_or_parent_loc - else: - course_id = course_or_parent_loc.package_id + course_id = self._infer_course_id_try(course_or_parent_loc) + store = self._get_modulestore_for_courseid(course_id) location = kwargs.pop('location', None) # invoke its create_item - if store.reference_type == Location: + if isinstance(store, MongoModuleStore): # convert parent loc if it's legit block_id = kwargs.pop('block_id', uuid4().hex) if isinstance(course_or_parent_loc, basestring): @@ -546,7 +351,7 @@ class MixedModuleStore(ModuleStoreWriteBase): locn_dict['name'] = block_id location = Location(locn_dict) else: - parent_loc = self._incoming_reference_adaptor(store, course_id, course_or_parent_loc) + 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) @@ -559,7 +364,7 @@ class MixedModuleStore(ModuleStoreWriteBase): parent = store.get_item(parent_loc) parent.children.append(location.url()) store.update_item(parent) - else: + elif isinstance(store, SplitMongoModuleStore): if isinstance(course_or_parent_loc, basestring): # course_id old_course_id = course_or_parent_loc course_or_parent_loc = loc_mapper().translate_location_to_course_locator( @@ -570,13 +375,15 @@ class MixedModuleStore(ModuleStoreWriteBase): course_or_parent_loc, get_course=True ) old_course_id = old_course_loc.course_id - else: # it's a Location - old_course_id = course_id - course_or_parent_loc = self._location_to_locator(course_id, course_or_parent_loc) + else: + 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) if location is None: locn_dict = Location.parse_course_id(old_course_id) @@ -587,6 +394,9 @@ class MixedModuleStore(ModuleStoreWriteBase): loc_mapper().translate_location( old_course_id, location, passed_block_id=xblock.location.block_id ) + 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): @@ -594,37 +404,21 @@ class MixedModuleStore(ModuleStoreWriteBase): 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.reference_type == Location: - course_id = xblock.course_id - if course_id is None: - 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) - else: - locator = xblock.scope_ids.usage_id - 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) - - # 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) + return store.update_item(xblock, user_id) def delete_item(self, location, user_id=None): """ Delete the given item from persistence. """ - if self.reference_type == Location: - course_id = self._infer_course_id_try(location) - if course_id is None: - raise ItemNotFoundError(u"Cannot find modulestore for %s", location) - else: - course_id = location.package_id + 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) - - decoded_ref = self._incoming_reference_adaptor(store, course_id, location) - return store.delete_item(decoded_ref, user_id=user_id) + return store.delete_item(location, user_id=user_id) def close_all_connections(self): """ @@ -636,3 +430,17 @@ class MixedModuleStore(ModuleStoreWriteBase): 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 NotImplementedError(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) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 4dcf3b6739..b97b1e1686 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -633,8 +633,7 @@ class MongoModuleStore(ModuleStoreWriteBase): raise ValueError(u"Course roots must be of category 'course': {}".format(unicode(location))) return self.create_and_save_xmodule(location, definition_data, metadata, runtime) - def create_xmodule(self, location, definition_data=None, metadata=None, system=None, - fields={}): + 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. diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/draft.py b/common/lib/xmodule/xmodule/modulestore/mongo/draft.py index 99a10c6e4a..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,7 +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) + 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): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 9ac0533eb9..f48a68632f 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -556,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'], diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index ef30e96add..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': 'xmodule.modulestore.Location', 'stores': { 'default': mongo_config['default'], 'xml': xml_config['default'] @@ -219,14 +218,17 @@ 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() - store.db.connection.close() + 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() 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 81ebd70a33..39073e4952 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1,24 +1,24 @@ import pymongo from uuid import uuid4 -import copy import ddt -from mock import patch +from mock import patch, Mock +from importlib import import_module from xmodule.tests import DATA_DIR from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE, SPLIT_MONGO_MODULESTORE_TYPE, \ XML_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.locator import BlockUsageLocator +from xmodule.modulestore.locator import BlockUsageLocator, CourseLocator from xmodule.modulestore.tests.test_location_mapper import LocMapperSetupSansDjango, loc_mapper - -# FIXME remove settings +# Mixed modulestore depends on django, so we'll manually configure some django settings +# before importing the module from django.conf import settings if not settings.configured: settings.configure() - from xmodule.modulestore.mixed import MixedModuleStore + @ddt.ddt class TestMixedModuleStore(LocMapperSetupSansDjango): """ @@ -32,7 +32,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): FS_ROOT = DATA_DIR DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' RENDER_TEMPLATE = lambda t_n, d, ctx = None, nsp = 'main': '' - REFERENCE_TYPE = 'xmodule.modulestore.Location' IMPORT_COURSEID = 'MITx/999/2013_Spring' XML_COURSEID1 = 'edX/toy/2012_Fall' @@ -54,7 +53,6 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): XML_COURSEID2: 'xml', IMPORT_COURSEID: 'default' }, - 'reference_type': REFERENCE_TYPE, 'stores': { 'xml': { 'ENGINE': 'xmodule.modulestore.xml.XMLModuleStore', @@ -101,10 +99,13 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.connection.drop_database(self.DB) self.addCleanup(self.connection.drop_database, self.DB) self.addCleanup(self.connection.close) - super(TestMixedModuleStore, self).setUp() - patcher = patch('xmodule.modulestore.mixed.loc_mapper', return_value=LocMapperSetupSansDjango.loc_store) + 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') @@ -115,28 +116,26 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): NOTE: course_location and item_location must be Location regardless of the app reference type in order to cause the right mapping to be created. """ - if default == 'split': - course = self.store.create_course(course_location, store_name=default) - chapter = self.store.create_item( - # don't use course_location as it may not be the repr - course.location, item_location.category, location=item_location, block_id=item_location.name - ) - else: - course = self.store.create_course( - course_location, store_name=default, metadata={'display_name': course_location.name} - ) - chapter = self.store.create_item(course_location, item_location.category, location=item_location) - if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator': + course = self.store.create_course( + course_location, store_name=default, metadata={'display_name': course_location.name} + ) + chapter = self.store.create_item( + # don't use course_location as it may not be the repr + course.location, item_location.category, location=item_location, block_id=item_location.name + ) + if isinstance(course.location, CourseLocator): # add_entry is false b/c this is a test that the right thing happened w/o # wanting any additional side effects lookup_map = loc_mapper().translate_location( course_location.course_id, course_location, add_entry_if_missing=False ) self.assertEqual(lookup_map, course.location) + self.course_locations[self.IMPORT_COURSEID] = course.location.version_agnostic() lookup_map = loc_mapper().translate_location( course_location.course_id, item_location, add_entry_if_missing=False ) self.assertEqual(lookup_map, chapter.location) + self.import_chapter_location = chapter.location.version_agnostic() else: self.assertEqual(course.location, course_location) self.assertEqual(chapter.location, item_location) @@ -154,8 +153,10 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): """ Generate the locations for the given ids """ - org, course, run = course_id.split('/') - return Location('i4x', org, course, 'course', run) + course_dict = Location.parse_course_id(course_id) + course_dict['tag'] = 'i4x' + course_dict['category'] = 'course' + return Location(course_dict) self.course_locations = { course_id: generate_location(course_id) @@ -171,18 +172,8 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): # grab old style location b4 possibly converted import_location = self.course_locations[self.IMPORT_COURSEID] # get Locators and set up the loc mapper if app is Locator based - if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator': + if default == 'split': self.fake_location = loc_mapper().translate_location('foo/bar/2012_Fall', self.fake_location) - self.import_chapter_location = loc_mapper().translate_location( - self.IMPORT_COURSEID, self.import_chapter_location - ) - self.xml_chapter_location = loc_mapper().translate_location( - self.XML_COURSEID1, self.xml_chapter_location - ) - self.course_locations = { - course_id: loc_mapper().translate_location(course_id, locn) - for course_id, locn in self.course_locations.iteritems() - } self._create_course(default, import_location, self.import_chapter_location) @@ -206,8 +197,11 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.assertTrue(self.store.has_item(course_id, course_locn)) # try negative cases - self.assertFalse(self.store.has_item(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID])) - self.assertFalse(self.store.has_item(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1])) + 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.IMPORT_COURSEID, self.fake_location)) @ddt.data('direct', 'split') def test_get_item(self, default_ms): @@ -223,9 +217,12 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): # try negative cases with self.assertRaises(ItemNotFoundError): - self.store.get_instance(self.XML_COURSEID1, self.course_locations[self.IMPORT_COURSEID]) + self.store.get_instance( + self.XML_COURSEID1, + self.course_locations[self.XML_COURSEID1].replace(name='not_findable', category='problem') + ) with self.assertRaises(ItemNotFoundError): - self.store.get_instance(self.IMPORT_COURSEID, self.course_locations[self.XML_COURSEID1]) + self.store.get_instance(self.IMPORT_COURSEID, self.fake_location) @ddt.data('direct', 'split') def test_get_items(self, default_ms): @@ -246,11 +243,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): Update should fail for r/o dbs and succeed for r/w ones """ self.initdb(default_ms) - # try a r/o db - if self.REFERENCE_TYPE == 'xmodule.modulestore.locator.CourseLocator': - course_id = self.course_locations[self.XML_COURSEID1] - else: - course_id = self.XML_COURSEID1 + 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") @@ -288,10 +281,14 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): @ddt.data('direct', 'split') def test_get_courses(self, default_ms): self.initdb(default_ms) - # we should have 3 total courses aggregated + # we should have 3 total courses across all stores courses = self.store.get_courses() self.assertEqual(len(courses), 3) - course_ids = [course.location for course in courses] + course_ids = [ + course.location.version_agnostic() + if hasattr(course.location, 'version_agnostic') else course.location + for course in courses + ] self.assertIn(self.course_locations[self.IMPORT_COURSEID], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID1], course_ids) self.assertIn(self.course_locations[self.XML_COURSEID2], course_ids) @@ -300,6 +297,7 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): """ Test that the xml modulestore only loaded the courses from the maps. """ + self.initdb('direct') courses = self.store.modulestores['xml'].get_courses() self.assertEqual(len(courses), 2) course_ids = [course.location.course_id for course in courses] @@ -339,33 +337,28 @@ class TestMixedModuleStore(LocMapperSetupSansDjango): self.assertEqual(len(parents), 1) self.assertEqual(parents[0], self.course_locations[self.XML_COURSEID1]) -@ddt.ddt -class TestMixedUseLocator(TestMixedModuleStore): - """ - Tests a mixed ms which uses Locators instead of Locations - """ - REFERENCE_TYPE = 'xmodule.modulestore.locator.CourseLocator' +#============================================================================================================= +# General utils for not using django settings +#============================================================================================================= - def setUp(self): - self.options = copy.copy(self.OPTIONS) - self.options['reference_type'] = self.REFERENCE_TYPE - super(TestMixedUseLocator, self).setUp() - -@ddt.ddt -class TestMixedMSInit(TestMixedModuleStore): +def load_function(path): """ - Test initializing w/o a reference_type - """ - REFERENCE_TYPE = None - def setUp(self): - self.options = copy.copy(self.OPTIONS) - del self.options['reference_type'] - super(TestMixedMSInit, self).setUp() + Load a function by name. - @ddt.data('direct', 'split') - def test_use_locations(self, default_ms): - """ - Test that use_locations defaulted correctly - """ - self.initdb(default_ms) - self.assertEqual(self.store.reference_type, Location) + 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) + + +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 + ) diff --git a/lms/envs/acceptance.py b/lms/envs/acceptance.py index eeafd783ea..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': 'xmodule.modulestore.Location', 'stores': { 'default': { 'ENGINE': 'xmodule.modulestore.mongo.MongoModuleStore', 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': {