From fc0a7614bf6e1c189613cac69a5ecdd5f887567a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 8 Jan 2013 11:21:22 -0500 Subject: [PATCH 1/4] Load courses in dir name order, keep separate ParentTracker per course. --- .../xmodule/xmodule/modulestore/__init__.py | 2 +- .../lib/xmodule/xmodule/modulestore/mongo.py | 2 +- .../lib/xmodule/xmodule/modulestore/search.py | 2 +- common/lib/xmodule/xmodule/modulestore/xml.py | 19 ++++++++++--------- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 5b94add68f..42c516a199 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -345,7 +345,7 @@ class ModuleStore(object): ''' raise NotImplementedError - def get_parent_locations(self, location): + def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location. Needed for path_to_location(). diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index baa4e7870c..cc4d1a7cda 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -309,7 +309,7 @@ class MongoModuleStore(ModuleStoreBase): self._update_single_item(location, {'metadata': metadata}) - def get_parent_locations(self, location): + def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location. Needed for path_to_location(). diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index f9901e8bfe..4a5ece6854 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -64,7 +64,7 @@ def path_to_location(modulestore, course_id, 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) + parents = modulestore.get_parent_locations(loc, course_id) # print 'Processing loc={0}, path={1}'.format(loc, path) if loc.category == "course": diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 6b3ff9bff4..bee3ca4440 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -275,14 +275,15 @@ class XMLModuleStore(ModuleStoreBase): class_ = getattr(import_module(module_path), class_name) self.default_class = class_ - self.parent_tracker = ParentTracker() + # self.parent_tracker = ParentTracker() + self.parent_trackers = defaultdict(ParentTracker) # If we are specifically asked for missing courses, that should # be an error. If we are asked for "all" courses, find the ones # that have a course.xml if course_dirs is None: - course_dirs = [d for d in os.listdir(self.data_dir) if - os.path.exists(self.data_dir / d / "course.xml")] + course_dirs = sorted([d for d in os.listdir(self.data_dir) if + os.path.exists(self.data_dir / d / "course.xml")]) for course_dir in course_dirs: self.try_load_course(course_dir) @@ -307,7 +308,7 @@ class XMLModuleStore(ModuleStoreBase): if course_descriptor is not None: self.courses[course_dir] = course_descriptor self._location_errors[course_descriptor.location] = errorlog - self.parent_tracker.make_known(course_descriptor.location) + self.parent_trackers[course_descriptor.id].make_known(course_descriptor.location) else: # Didn't load course. Instead, save the errors elsewhere. self.errored_courses[course_dir] = errorlog @@ -432,7 +433,7 @@ class XMLModuleStore(ModuleStoreBase): course_dir, policy, tracker, - self.parent_tracker, + self.parent_trackers[course_id], self.load_error_modules, ) @@ -541,7 +542,7 @@ class XMLModuleStore(ModuleStoreBase): """ raise NotImplementedError("XMLModuleStores are read-only") - def get_parent_locations(self, location): + def get_parent_locations(self, location, course_id): '''Find all locations that are the parents of this location. Needed for path_to_location(). @@ -552,7 +553,7 @@ class XMLModuleStore(ModuleStoreBase): be empty if there are no parents. ''' location = Location.ensure_fully_specified(location) - if not self.parent_tracker.is_known(location): - raise ItemNotFoundError(location) + if not self.parent_trackers[course_id].is_known(location): + raise ItemNotFoundError("{0} not in {1}".format(location, course_id)) - return self.parent_tracker.parents(location) + return self.parent_trackers[course_id].parents(location) From 8952eda98d0953c210eb159f24fd0ddce26cfb81 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 8 Jan 2013 13:34:22 -0500 Subject: [PATCH 2/4] XML loaded courses should no longer be able to throw NoPathToItem. The parent trees are built on a per-course basis (so it'll throw ItemNotFoundError if an item exists in a different course from the one you're asking after. NoPathToItem can still happen with MongoDB backed courseware. They store things like the info page as HTML snippets which are orphaned from other content. --- .../xmodule/modulestore/tests/test_modulestore.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py index c1d1d50a53..64816581ce 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py @@ -23,12 +23,3 @@ def check_path_to_location(modulestore): for location in not_found: assert_raises(ItemNotFoundError, path_to_location, modulestore, course_id, 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 - # another course. - no_path = ( - "i4x://edX/simple/video/Lost_Video", - ) - for location in no_path: - assert_raises(NoPathToItem, path_to_location, modulestore, course_id, location) - From 051339afb03252d8166fd816cf5cf81961df1a58 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 8 Jan 2013 13:54:44 -0500 Subject: [PATCH 3/4] Tiny cleanup. --- common/lib/xmodule/xmodule/modulestore/xml.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index bee3ca4440..f967cd0a7f 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -275,7 +275,6 @@ class XMLModuleStore(ModuleStoreBase): class_ = getattr(import_module(module_path), class_name) self.default_class = class_ - # self.parent_tracker = ParentTracker() self.parent_trackers = defaultdict(ParentTracker) # If we are specifically asked for missing courses, that should From 1122cdb2862b3ef13c6a438a8bf4baaa350811f1 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 8 Jan 2013 16:37:31 -0500 Subject: [PATCH 4/4] Added more comments in response to code review. --- common/lib/xmodule/xmodule/modulestore/__init__.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/mongo.py | 4 ++-- common/lib/xmodule/xmodule/modulestore/xml.py | 8 +++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 42c516a199..f86a6e9600 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -346,8 +346,8 @@ class ModuleStore(object): raise NotImplementedError def get_parent_locations(self, location, course_id): - '''Find all locations that are the parents of this location. Needed - for path_to_location(). + '''Find all locations that are the parents of this location in this + course. Needed for path_to_location(). returns an iterable of things that can be passed to Location. ''' diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index cc4d1a7cda..4c7ef3c050 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -310,8 +310,8 @@ class MongoModuleStore(ModuleStoreBase): self._update_single_item(location, {'metadata': metadata}) def get_parent_locations(self, location, course_id): - '''Find all locations that are the parents of this location. Needed - for path_to_location(). + '''Find all locations that are the parents of this location in this + course. Needed for path_to_location(). If there is no data at location in this modulestore, raise ItemNotFoundError. diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index f967cd0a7f..04f3a94d1b 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -279,7 +279,9 @@ class XMLModuleStore(ModuleStoreBase): # If we are specifically asked for missing courses, that should # be an error. If we are asked for "all" courses, find the ones - # that have a course.xml + # that have a course.xml. We sort the dirs in alpha order so we always + # read things in the same order (OS differences in load order have + # bitten us in the past.) if course_dirs is None: course_dirs = sorted([d for d in os.listdir(self.data_dir) if os.path.exists(self.data_dir / d / "course.xml")]) @@ -542,8 +544,8 @@ class XMLModuleStore(ModuleStoreBase): raise NotImplementedError("XMLModuleStores are read-only") def get_parent_locations(self, location, course_id): - '''Find all locations that are the parents of this location. Needed - for path_to_location(). + '''Find all locations that are the parents of this location in this + course. Needed for path_to_location(). If there is no data at location in this modulestore, raise ItemNotFoundError.