Proper abstraction of Module store base class implemented

Read/Write inheritance split up into separate base classes

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
+        pass
-
-    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
 
 
-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

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
+        Returns a type which identifies which modulestore is servicing the given course_id.
-        course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses
+        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)
 

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
+        Returns an enumeration-like type reflecting the type of this modulestore
-        course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses
+        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
 

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

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)

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)),

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'''

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
+        Returns an enumeration-like type reflecting the type of this modulestore
-        course_id. The return can be either "xml" (for XML based courses) or "mongo" for MongoDB backed courses
+        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