From e2ba1122c4780fbd641c2a3cab2fb12dc8511c91 Mon Sep 17 00:00:00 2001 From: Slater Date: Wed, 30 Oct 2013 13:00:10 -0400 Subject: [PATCH] Proper abstraction of Module store base class implemented Read/Write inheritance split up into separate base classes --- .../xmodule/xmodule/modulestore/__init__.py | 132 +++++++++++------- .../lib/xmodule/xmodule/modulestore/mixed.py | 11 +- .../xmodule/xmodule/modulestore/mongo/base.py | 11 +- .../xmodule/modulestore/split_mongo/split.py | 14 +- .../modulestore/tests/test_abstraction.py | 16 +++ .../modulestore/tests/test_modulestore.py | 7 +- .../xmodule/modulestore/tests/test_mongo.py | 4 +- common/lib/xmodule/xmodule/modulestore/xml.py | 11 +- 8 files changed, 139 insertions(+), 67 deletions(-) create mode 100644 common/lib/xmodule/xmodule/modulestore/tests/test_abstraction.py diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index a8068bd065..d7f8a93067 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/common/lib/xmodule/xmodule/modulestore/__init__.py @@ -8,11 +8,14 @@ import re from collections import namedtuple +from abc import ABCMeta, abstractmethod + from .exceptions import InvalidLocationError, InsufficientSpecificationError from xmodule.errortracker import make_error_tracker log = logging.getLogger('mitx.' + 'modulestore') +SPLIT_MONGO_MODULESTORE_TYPE = 'split' MONGO_MODULESTORE_TYPE = 'mongo' XML_MODULESTORE_TYPE = 'xml' @@ -254,17 +257,22 @@ class Location(_LocationBase): return self._replace(**kwargs) -class ModuleStore(object): +class ModuleStoreRead(object): """ An abstract interface for a database backend that stores XModuleDescriptor - instances + instances and extends read-only functionality """ + + __metaclass__ = ABCMeta + + @abstractmethod def has_item(self, course_id, location): """ Returns True if location exists in this ModuleStore. """ - raise NotImplementedError + pass + @abstractmethod def get_item(self, location, depth=0): """ Returns an XModuleDescriptor instance for the item at location. @@ -282,15 +290,17 @@ class ModuleStore(object): in the request. The depth is counted in the number of calls to get_children() to cache. None indicates to cache all descendents """ - raise NotImplementedError + pass + @abstractmethod def get_instance(self, course_id, location, depth=0): """ Get an instance of this location, with policy for course_id applied. TODO (vshnayder): this may want to live outside the modulestore eventually """ - raise NotImplementedError + pass + @abstractmethod def get_item_errors(self, location): """ Return a list of (msg, exception-or-None) errors that the modulestore @@ -301,8 +311,9 @@ class ModuleStore(object): Raises the same exceptions as get_item if the location isn't found or isn't fully specified. """ - raise NotImplementedError + pass + @abstractmethod def get_items(self, location, course_id=None, depth=0): """ Returns a list of XModuleDescriptor instances for the items @@ -316,8 +327,58 @@ class ModuleStore(object): in the request. The depth is counted in the number of calls to get_children() to cache. None indicates to cache all descendents """ - raise NotImplementedError + pass + @abstractmethod + def get_courses(self): + ''' + Returns a list containing the top level XModuleDescriptors of the courses + in this modulestore. + ''' + pass + + @abstractmethod + def get_course(self, course_id): + ''' + Look for a specific course id. Returns the course descriptor, or None if not found. + ''' + pass + + @abstractmethod + def get_parent_locations(self, location, course_id): + '''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. + ''' + pass + + @abstractmethod + def get_errored_courses(self): + """ + Return a dictionary of course_dir -> [(msg, exception_str)], for each + course_dir where course loading failed. + """ + pass + + @abstractmethod + def get_modulestore_type(self, course_id): + """ + Returns a type which identifies which modulestore is servicing the given + course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses + """ + pass + + +class ModuleStoreWrite(ModuleStoreRead): + """ + An abstract interface for a database backend that stores XModuleDescriptor + instances and extends both read and write functionality + """ + + __metaclass__ = ABCMeta + + @abstractmethod def update_item(self, location, data, allow_not_found=False): """ Set the data in the item specified by the location to @@ -326,8 +387,9 @@ class ModuleStore(object): location: Something that can be passed to Location data: A nested dictionary of problem data """ - raise NotImplementedError + pass + @abstractmethod def update_children(self, location, children): """ Set the children for the item specified by the location to @@ -336,8 +398,9 @@ class ModuleStore(object): location: Something that can be passed to Location children: A list of child item identifiers """ - raise NotImplementedError + pass + @abstractmethod def update_metadata(self, location, metadata): """ Set the metadata for the item specified by the location to @@ -346,58 +409,24 @@ class ModuleStore(object): location: Something that can be passed to Location metadata: A nested dictionary of module metadata """ - raise NotImplementedError + pass + @abstractmethod def delete_item(self, location): """ Delete an item from this modulestore location: Something that can be passed to Location """ - raise NotImplementedError - - def get_courses(self): - ''' - Returns a list containing the top level XModuleDescriptors of the courses - in this modulestore. - ''' - course_filter = Location("i4x", category="course") - return self.get_items(course_filter) - - def get_course(self, course_id): - ''' - Look for a specific course id. Returns the course descriptor, or None if not found. - ''' - raise NotImplementedError - - def get_parent_locations(self, location, course_id): - '''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. - ''' - raise NotImplementedError - - def get_errored_courses(self): - """ - Return a dictionary of course_dir -> [(msg, exception_str)], for each - course_dir where course loading failed. - """ - raise NotImplementedError - - def get_modulestore_type(self, course_id): - """ - Returns a type which identifies which modulestore is servicing the given - course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses - """ - raise NotImplementedError + pass -class ModuleStoreBase(ModuleStore): +class ModuleStoreReadBase(ModuleStoreRead): ''' Implement interface functionality that can be shared. ''' # pylint: disable=W0613 + def __init__( self, doc_store_config=None, # ignore if passed up @@ -456,3 +485,10 @@ class ModuleStoreBase(ModuleStore): if c.id == course_id: return c return None + + +class ModuleStoreWriteBase(ModuleStoreReadBase, ModuleStoreWrite): + ''' + Implement interface functionality that can be shared. + ''' + pass diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 2883c104d9..46da67cb65 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -6,14 +6,14 @@ In this way, courses can be served up both - say - XMLModuleStore or MongoModule IMPORTANT: This modulestore only supports READONLY applications, e.g. LMS """ -from . import ModuleStoreBase +from . import ModuleStoreWriteBase from xmodule.modulestore.django import create_modulestore_instance import logging log = logging.getLogger(__name__) -class MixedModuleStore(ModuleStoreBase): +class MixedModuleStore(ModuleStoreWriteBase): """ ModuleStore that can be backed by either XML or Mongo """ @@ -138,8 +138,11 @@ class MixedModuleStore(ModuleStoreBase): def get_modulestore_type(self, course_id): """ - Returns a type which identifies which modulestore is servicing the given - course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses + Returns a type which identifies which modulestore is servicing the given course_id. + The return can be one of: + "xml" (for XML based courses), + "mongo" for old-style MongoDB backed courses, + "split" for new-style split MongoDB backed courses. """ return self._get_modulestore_for_courseid(course_id).get_modulestore_type(course_id) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 086cbb6bb7..2f6fabfce0 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -31,7 +31,7 @@ from xblock.runtime import DbModel from xblock.exceptions import InvalidScopeError from xblock.fields import Scope, ScopeIds -from xmodule.modulestore import ModuleStoreBase, Location, MONGO_MODULESTORE_TYPE +from xmodule.modulestore import ModuleStoreWriteBase, Location, MONGO_MODULESTORE_TYPE from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import own_metadata, InheritanceMixin, inherit_metadata, InheritanceKeyValueStore import re @@ -251,7 +251,7 @@ def metadata_cache_key(location): return u"{0.org}/{0.course}".format(location) -class MongoModuleStore(ModuleStoreBase): +class MongoModuleStore(ModuleStoreWriteBase): """ A Mongodb backed ModuleStore """ @@ -850,8 +850,11 @@ class MongoModuleStore(ModuleStoreBase): def get_modulestore_type(self, course_id): """ - Returns a type which identifies which modulestore is servicing the given - course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses + Returns an enumeration-like type reflecting the type of this modulestore + The return can be one of: + "xml" (for XML based courses), + "mongo" for old-style MongoDB backed courses, + "split" for new-style split MongoDB backed courses. """ return MONGO_MODULESTORE_TYPE diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 68aa883441..6152416596 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -13,7 +13,7 @@ from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor from xmodule.modulestore.locator import BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError -from xmodule.modulestore import inheritance, ModuleStoreBase, Location +from xmodule.modulestore import inheritance, ModuleStoreWriteBase, Location, SPLIT_MONGO_MODULESTORE_TYPE from ..exceptions import ItemNotFoundError from .definition_lazy_loader import DefinitionLazyLoader @@ -44,7 +44,7 @@ log = logging.getLogger(__name__) #============================================================================== -class SplitMongoModuleStore(ModuleStoreBase): +class SplitMongoModuleStore(ModuleStoreWriteBase): """ A Mongodb backed ModuleStore supporting versions, inheritance, and sharing. @@ -1455,3 +1455,13 @@ class SplitMongoModuleStore(ModuleStoreBase): if 'category' in fields: del fields['category'] return fields + + def get_modulestore_type(self, course_id): + """ + Returns an enumeration-like type reflecting the type of this modulestore + The return can be one of: + "xml" (for XML based courses), + "mongo" for old-style MongoDB backed courses, + "split" for new-style split MongoDB backed courses. + """ + return SPLIT_MONGO_MODULESTORE_TYPE diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_abstraction.py b/common/lib/xmodule/xmodule/modulestore/tests/test_abstraction.py new file mode 100644 index 0000000000..5aac079ae9 --- /dev/null +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_abstraction.py @@ -0,0 +1,16 @@ +""" +Simple test to ensure that modulestore base classes remain abstract +""" +from unittest import TestCase + +from xmodule.modulestore import ModuleStoreRead, ModuleStoreWrite + + +class AbstractionTest(TestCase): + """ + Tests that the ModuleStore objects are properly abstracted + """ + + def test_cant_instantiate_abstract_class(self): + self.assertRaises(TypeError, ModuleStoreRead) # Cannot be instantiated due to explicit abstraction + self.assertRaises(TypeError, ModuleStoreWrite) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py index 36d4a3b8de..b737dd653b 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_modulestore.py @@ -3,10 +3,11 @@ from nose.tools import assert_equals, assert_raises # pylint: disable=E0611 from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.search import path_to_location - def check_path_to_location(modulestore): - '''Make sure that path_to_location works: should be passed a modulestore - with the toy and simple courses loaded.''' + """ + Make sure that path_to_location works: should be passed a modulestore + with the toy and simple courses loaded. + """ should_work = ( ("i4x://edX/toy/video/Welcome", ("edX/toy/2012_Fall", "Overview", "Welcome", None)), diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py index eb7efa0431..1050aba01f 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mongo.py @@ -13,7 +13,7 @@ from xblock.runtime import KeyValueStore from xblock.exceptions import InvalidScopeError from xmodule.tests import DATA_DIR -from xmodule.modulestore import Location +from xmodule.modulestore import Location, MONGO_MODULESTORE_TYPE from xmodule.modulestore.mongo import MongoModuleStore, MongoKeyValueStore from xmodule.modulestore.draft import DraftModuleStore from xmodule.modulestore.xml_importer import import_from_xml, perform_xlint @@ -122,7 +122,7 @@ class TestMongoModuleStore(object): {'host': HOST, 'db': DB, 'collection': COLLECTION}, FS_ROOT, RENDER_TEMPLATE, default_class=DEFAULT_CLASS ) - assert_equals(store.get_modulestore_type('foo/bar/baz'), 'mongo') + assert_equals(store.get_modulestore_type('foo/bar/baz'), MONGO_MODULESTORE_TYPE) def test_get_courses(self): '''Make sure the course objects loaded properly''' diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 1e7074a847..5512af2143 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -24,7 +24,7 @@ from xblock.core import XBlock from xblock.fields import ScopeIds from xblock.field_data import DictFieldData -from . import ModuleStoreBase, Location, XML_MODULESTORE_TYPE +from . import ModuleStoreReadBase, Location, XML_MODULESTORE_TYPE from .exceptions import ItemNotFoundError from .inheritance import compute_inherited_metadata @@ -292,7 +292,7 @@ class ParentTracker(object): return list(self._parents[child]) -class XMLModuleStore(ModuleStoreBase): +class XMLModuleStore(ModuleStoreReadBase): """ An XML backed ModuleStore """ @@ -651,7 +651,10 @@ class XMLModuleStore(ModuleStoreBase): def get_modulestore_type(self, course_id): """ - Returns a type which identifies which modulestore is servicing the given - course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses + Returns an enumeration-like type reflecting the type of this modulestore + The return can be one of: + "xml" (for XML based courses), + "mongo" for old-style MongoDB backed courses, + "split" for new-style split MongoDB backed courses. """ return XML_MODULESTORE_TYPE