From 0ae434cc092617b970fee3968084c378d609c4f2 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 31 Jul 2012 11:24:39 -0400 Subject: [PATCH] Move path_to_location out of mongo.py * also bugfix for load_definition in html_module * a bit of refactoring of Location checking code in mongo.py --- common/lib/xmodule/xmodule/html_module.py | 2 +- .../xmodule/xmodule/modulestore/__init__.py | 47 +++---- .../lib/xmodule/xmodule/modulestore/mongo.py | 120 +++--------------- .../lib/xmodule/xmodule/modulestore/search.py | 96 ++++++++++++++ .../xmodule/modulestore/tests/test_mongo.py | 15 +-- common/lib/xmodule/xmodule/xml_module.py | 8 +- lms/djangoapps/courseware/views.py | 3 +- 7 files changed, 155 insertions(+), 136 deletions(-) create mode 100644 common/lib/xmodule/xmodule/modulestore/search.py diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index 22d3bf163f..996d3c4ead 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -54,7 +54,7 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor): # snippets that will be included in the middle of pages. @classmethod - def definition_loader(cls, xml_object, system): + def load_definition(cls, xml_object, system, location): '''Load a descriptor from the specified xml_object: If there is a filename attribute, load it as a string, and diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 279782b61a..a0167d781b 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -5,7 +5,7 @@ that are stored in a database an accessible using their Location as an identifie import re from collections import namedtuple -from .exceptions import InvalidLocationError +from .exceptions import InvalidLocationError, InsufficientSpecificationError import logging log = logging.getLogger('mitx.' + 'modulestore') @@ -38,15 +38,15 @@ class Location(_LocationBase): ''' __slots__ = () - @classmethod - def clean(cls, value): + @staticmethod + def clean(value): """ Return value, made into a form legal for locations """ return re.sub('_+', '_', INVALID_CHARS.sub('_', value)) - @classmethod - def is_valid(cls, value): + @staticmethod + def is_valid(value): ''' Check if the value is a valid location, in any acceptable format. ''' @@ -56,6 +56,21 @@ class Location(_LocationBase): return False return True + @staticmethod + def ensure_fully_specified(location): + '''Make sure location is valid, and fully specified. Raises + InvalidLocationError or InsufficientSpecificationError if not. + + returns a Location object corresponding to location. + ''' + loc = Location(location) + for key, val in loc.dict().iteritems(): + if key != 'revision' and val is None: + raise InsufficientSpecificationError(location) + return loc + + + def __new__(_cls, loc_or_tag=None, org=None, course=None, category=None, name=None, revision=None): """ @@ -254,25 +269,11 @@ class ModuleStore(object): ''' raise NotImplementedError - def path_to_location(self, location, course=None, chapter=None, section=None): - ''' - Try to find a course/chapter/section[/position] path to this location. + def get_parent_locations(self, location): + '''Find all locations that are the parents of this location. Needed + for path_to_location(). - raise ItemNotFoundError if the location doesn't exist. - - If course, chapter, section are not None, restrict search to paths with those - components as specified. - - raise NoPathToItem if the location exists, but isn't accessible via - a path that matches the course/chapter/section restrictions. - - In general, a location may be accessible via many paths. This method may - return any valid path. - - Return a tuple (course, chapter, section, position). - - If the section a sequence, position should be the position of this location - in that sequence. Otherwise, position should be None. + returns an iterable of things that can be passed to Location. ''' raise NotImplementedError diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index df4e20f3a7..061e2aafe9 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -9,11 +9,10 @@ from importlib import import_module from xmodule.errorhandlers import strict_error_handler from xmodule.x_module import XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem -from xmodule.course_module import CourseDescriptor from mitxmako.shortcuts import render_to_string from . import ModuleStore, Location -from .exceptions import (ItemNotFoundError, InsufficientSpecificationError, +from .exceptions import (ItemNotFoundError, NoPathToItem, DuplicateItemError) # TODO (cpennington): This code currently operates under the assumption that @@ -172,12 +171,17 @@ class MongoModuleStore(ModuleStore): return self.get_items(course_filter) def _find_one(self, location): - '''Look for a given location in the collection. - If revision isn't specified, returns the latest.''' - return self.collection.find_one( + '''Look for a given location in the collection. If revision is not + specified, returns the latest. If the item is not present, raise + ItemNotFoundError. + ''' + item = self.collection.find_one( location_to_query(location), sort=[('revision', pymongo.ASCENDING)], ) + if item is None: + raise ItemNotFoundError(location) + return item def get_item(self, location, depth=0): """ @@ -197,14 +201,8 @@ class MongoModuleStore(ModuleStore): calls to get_children() to cache. None indicates to cache all descendents. """ - - for key, val in Location(location).dict().iteritems(): - if key != 'revision' and val is None: - raise InsufficientSpecificationError(location) - + location = Location.ensure_fully_specified(location) item = self._find_one(location) - if item is None: - raise ItemNotFoundError(location) return self._load_items([item], depth)[0] def get_items(self, location, depth=0): @@ -282,96 +280,20 @@ class MongoModuleStore(ModuleStore): ) def get_parent_locations(self, location): - '''Find all locations that are the parents of this location. - Mostly intended for use in path_to_location, but exposed for testing - and possible other usefulness. + '''Find all locations that are the parents of this location. Needed + for path_to_location(). - returns an iterable of things that can be passed to Location. + If there is no data at location in this modulestore, raise + ItemNotFoundError. + + returns an iterable of things that can be passed to Location. This may + be empty if there are no parents. ''' - location = Location(location) + location = Location.ensure_fully_specified(location) + # Check that it's actually in this modulestore. + item = self._find_one(location) + # now get the parents items = self.collection.find({'definition.children': str(location)}, {'_id': True}) return [i['_id'] for i in items] - def path_to_location(self, location, course_name=None): - ''' - Try to find a course_id/chapter/section[/position] path to this location. - The courseware insists that the first level in the course is chapter, - but any kind of module can be a "section". - - location: something that can be passed to Location - course_name: [optional]. If not None, restrict search to paths - in that course. - - raise ItemNotFoundError if the location doesn't exist. - - raise NoPathToItem if the location exists, but isn't accessible via - a chapter/section path in the course(s) being searched. - - Return a tuple (course_id, chapter, section, position) suitable for the - courseware index view. - - A location may be accessible via many paths. This method may - return any valid path. - - If the section is a sequence, position will be the position - of this location in that sequence. Otherwise, position will - be None. TODO (vshnayder): Not true yet. - ''' - # Check that location is present at all - if self._find_one(location) is None: - raise ItemNotFoundError(location) - - def flatten(xs): - '''Convert lisp-style (a, (b, (c, ()))) lists into a python list. - Not a general flatten function. ''' - p = [] - while xs != (): - p.append(xs[0]) - xs = xs[1] - return p - - def find_path_to_course(location, course_name=None): - '''Find a path up the location graph to a node with the - specified category. If no path exists, return None. If a - path exists, return it as a list with target location - first, and the starting location last. - ''' - # Standard DFS - - # To keep track of where we came from, the work queue has - # tuples (location, path-so-far). To avoid lots of - # copying, the path-so-far is stored as a lisp-style - # list--nested hd::tl tuples, and flattened at the end. - queue = [(location, ())] - while len(queue) > 0: - (loc, path) = queue.pop() # Takes from the end - loc = Location(loc) - # print 'Processing loc={0}, path={1}'.format(loc, path) - if loc.category == "course": - if course_name is None or course_name == loc.name: - # Found it! - path = (loc, path) - return flatten(path) - - # otherwise, add parent locations at the end - newpath = (loc, path) - parents = self.get_parent_locations(loc) - queue.extend(zip(parents, repeat(newpath))) - - # If we're here, there is no path - return None - - path = find_path_to_course(location, course_name) - if path is None: - raise(NoPathToItem(location)) - - n = len(path) - course_id = CourseDescriptor.location_to_id(path[0]) - chapter = path[1].name if n > 1 else None - section = path[2].name if n > 2 else None - - # TODO (vshnayder): not handling position at all yet... - position = None - - return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py new file mode 100644 index 0000000000..a383b3f8ec --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -0,0 +1,96 @@ +from itertools import repeat + +from xmodule.course_module import CourseDescriptor + +from .exceptions import (ItemNotFoundError, NoPathToItem) +from . import ModuleStore, Location + + +def path_to_location(modulestore, location, course_name=None): + ''' + Try to find a course_id/chapter/section[/position] path to location in + modulestore. The courseware insists that the first level in the course is + chapter, but any kind of module can be a "section". + + location: something that can be passed to Location + course_name: [optional]. If not None, restrict search to paths + in that course. + + raise ItemNotFoundError if the location doesn't exist. + + raise NoPathToItem if the location exists, but isn't accessible via + a chapter/section path in the course(s) being searched. + + Return a tuple (course_id, chapter, section, position) suitable for the + courseware index view. + + A location may be accessible via many paths. This method may + return any valid path. + + If the section is a sequence, position will be the position + of this location in that sequence. Otherwise, position will + be None. TODO (vshnayder): Not true yet. + ''' + + def flatten(xs): + '''Convert lisp-style (a, (b, (c, ()))) list into a python list. + Not a general flatten function. ''' + p = [] + while xs != (): + p.append(xs[0]) + xs = xs[1] + return p + + def find_path_to_course(location, course_name=None): + '''Find a path up the location graph to a node with the + specified category. + + If no path exists, return None. + + If a path exists, return it as a list with target location first, and + the starting location last. + ''' + # Standard DFS + + # To keep track of where we came from, the work queue has + # tuples (location, path-so-far). To avoid lots of + # copying, the path-so-far is stored as a lisp-style + # list--nested hd::tl tuples, and flattened at the end. + queue = [(location, ())] + while len(queue) > 0: + (loc, path) = queue.pop() # Takes from the end + loc = Location(loc) + + # get_parent_locations should raise ItemNotFoundError if location + # isn't found so we don't have to do it explicitly. Call this + # first to make sure the location is there (even if it's a course, and + # we would otherwise immediately exit). + parents = modulestore.get_parent_locations(loc) + + # print 'Processing loc={0}, path={1}'.format(loc, path) + if loc.category == "course": + if course_name is None or course_name == loc.name: + # Found it! + path = (loc, path) + return flatten(path) + + # otherwise, add parent locations at the end + newpath = (loc, path) + queue.extend(zip(parents, repeat(newpath))) + + # If we're here, there is no path + return None + + path = find_path_to_course(location, course_name) + if path is None: + raise(NoPathToItem(location)) + + n = len(path) + course_id = CourseDescriptor.location_to_id(path[0]) + chapter = path[1].name if n > 1 else None + section = path[2].name if n > 2 else None + + # TODO (vshnayder): not handling position at all yet... + position = None + + return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index cb2bc6e20c..24f0441ee0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -8,6 +8,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem from xmodule.modulestore.mongo import MongoModuleStore from xmodule.modulestore.xml_importer import import_from_xml +from xmodule.modulestore.search import path_to_location # from ~/mitx_all/mitx/common/lib/xmodule/xmodule/modulestore/tests/ # to ~/mitx_all/mitx/common/test @@ -28,7 +29,7 @@ DEFAULT_CLASS = 'xmodule.raw_module.RawDescriptor' class TestMongoModuleStore(object): - + '''Tests!''' @classmethod def setupClass(cls): cls.connection = pymongo.connection.Connection(HOST, PORT) @@ -67,7 +68,7 @@ class TestMongoModuleStore(object): def test_init(self): '''Make sure the db loads, and print all the locations in the db. - Call this directly from failing tests to see what's loaded''' + Call this directly from failing tests to see what is loaded''' ids = list(self.connection[DB][COLLECTION].find({}, {'_id': True})) pprint([Location(i['_id']).url() for i in ids]) @@ -93,8 +94,6 @@ class TestMongoModuleStore(object): self.store.get_item("i4x://edX/toy/video/Welcome"), None) - - def test_find_one(self): assert_not_equals( self.store._find_one(Location("i4x://edX/toy/course/2012_Fall")), @@ -117,13 +116,13 @@ class TestMongoModuleStore(object): ("edX/toy/2012_Fall", "Overview", "Toy_Videos", None)), ) for location, expected in should_work: - assert_equals(self.store.path_to_location(location), expected) + assert_equals(path_to_location(self.store, location), expected) not_found = ( - "i4x://edX/toy/video/WelcomeX", + "i4x://edX/toy/video/WelcomeX", "i4x://edX/toy/course/NotHome" ) for location in not_found: - assert_raises(ItemNotFoundError, self.store.path_to_location, location) + assert_raises(ItemNotFoundError, path_to_location, self.store, location) # Since our test files are valid, there shouldn't be any # elements with no path to them. But we can look for them in @@ -132,5 +131,5 @@ class TestMongoModuleStore(object): "i4x://edX/simple/video/Lost_Video", ) for location in no_path: - assert_raises(NoPathToItem, self.store.path_to_location, location, "toy") + assert_raises(NoPathToItem, path_to_location, self.store, location, "toy") diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index 188df21130..de62bc5d38 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -163,7 +163,7 @@ class XmlDescriptor(XModuleDescriptor): return etree.parse(file_object).getroot() @classmethod - def definition_loader(cls, xml_object, system, location): + def load_definition(cls, xml_object, system, location): '''Load a descriptor definition from the specified xml_object. Subclasses should not need to override this except in special cases (e.g. html module)''' @@ -225,7 +225,7 @@ class XmlDescriptor(XModuleDescriptor): slug = xml_object.get('url_name', xml_object.get('slug')) location = Location('i4x', org, course, xml_object.tag, slug) - def metadata_loader(): + def load_metadata(): metadata = {} for attr in cls.metadata_attributes: val = xml_object.get(attr) @@ -237,8 +237,8 @@ class XmlDescriptor(XModuleDescriptor): metadata[attr_map.metadata_key] = attr_map.to_metadata(val) return metadata - definition = cls.definition_loader(xml_object, system, location) - metadata = metadata_loader() + definition = cls.load_definition(xml_object, system, location) + metadata = load_metadata() # VS[compat] -- just have the url_name lookup once translation is done slug = xml_object.get('url_name', xml_object.get('slug')) return cls( diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 7281ab01ad..a52f715efd 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -20,6 +20,7 @@ from module_render import toc_for_course, get_module, get_section from models import StudentModuleCache from student.models import UserProfile from xmodule.modulestore import Location +from xmodule.modulestore.search import path_to_location from xmodule.modulestore.exceptions import InvalidLocationError, ItemNotFoundError, NoPathToItem from xmodule.modulestore.django import modulestore from xmodule.course_module import CourseDescriptor @@ -233,7 +234,7 @@ def jump_to(request, location): # Complain if there's not data for this location try: - (course_id, chapter, section, position) = modulestore().path_to_location(location) + (course_id, chapter, section, position) = path_to_location(modulestore(), location) except ItemNotFoundError: raise Http404("No data at this location: {0}".format(location)) except NoPathToItem: