Merge lp:~free.ekanayaka/storm/schema-advance into lp:storm

[Free Ekanayaka]

Add a Schema.advance method that applies to the given store a certain patch version. This is useful for applying patches with the same number across a set of schemas.

[lp:~free.ekanayaka/storm/schema-advance]

470. By Free Ekanayaka on 2014-12-12

Add PatchPackage class

471. By Free Ekanayaka on 2014-12-12

Add dummy patch module

472. By Free Ekanayaka on 2014-12-12

More backward compatibility

[Björn Tillenius (bjornt)]

Thanks for the changes. The new directory structure looks like a reasonable compromise. It makes it fairly easy to write patches for a single store only and it gives a good overview of which stores get patched.

It looks good in general, but I'm putting it in Needs Fixing, since some tests need to be enabled and rewritten.

review: Needs Fixing

[Free Ekanayaka (free.ekanayaka)]

Thanks Bjorn, should be all good.

[lp:~free.ekanayaka/storm/schema-advance]

473. By Free Ekanayaka on 2014-12-18

Address review comments

[Björn Tillenius (bjornt)]

review: Approve

=== modified file 'storm/schema/patch.py'
--- storm/schema/patch.py	2010-08-18 08:16:52 +0000
+++ storm/schema/patch.py	2014-12-18 10:58:55 +0000
@@ -37,6 +37,7 @@
 import sys
 import os
 import re
+import types
 
 from storm.locals import StormError, Int
 
@@ -78,32 +79,25 @@
     """Apply to a L{Store} the database patches from a given Python package.
 
     @param store: The L{Store} to apply the patches to.
-    @param package: The Python package containing the patches. Each patch is
-        represented by a file inside the module, whose filename must match
-        the format 'patch_N.py', where N is an integer number.
+    @param patch_set: The L{PatchSet} containing the patches to apply.
     @param committer: Optionally an object implementing 'commit()' and
         'rollback()' methods, to be used to commit or rollback the changes
         after applying a patch. If C{None} is given, the C{store} itself is
         used.
     """
 
-    def __init__(self, store, package, committer=None):
+    def __init__(self, store, patch_set, committer=None):
         self._store = store
-        self._package = package
+        if isinstance(patch_set, types.ModuleType):
+            # Up to version 0.20.0 the second positional parameter used to
+            # be a raw module containing the patches. We wrap it with PatchSet
+            # for keeping backward-compatibility.
+            patch_set = PatchSet(patch_set)
+        self._patch_set = patch_set
         if committer is None:
             committer = store
         self._committer = committer
 
-    def _module(self, version):
-        """Import the Python module of the patch file with the given version.
-
-        @param: The version of the module patch to import.
-        @return: The imported module.
-        """
-        module_name = "patch_%d" % (version,)
-        return __import__(self._package.__name__ + "." + module_name,
-                          None, None, [''])
-
     def apply(self, version):
         """Execute the patch with the given version.
 
@@ -116,7 +110,7 @@
         self._store.add(patch)
         module = None
         try:
-            module = self._module(version)
+            module = self._patch_set.get_patch_module(version)
             module.apply(self._store)
         except StormError:
             self._committer.rollback()
@@ -136,10 +130,8 @@
         @raises UnknownPatchError: If the patch table has versions for which
             no patch file actually exists.
         """
-        unknown_patches = self.get_unknown_patch_versions()
-        if unknown_patches:
-            raise UnknownPatchError(self._store, unknown_patches)
-        for version in self._get_unapplied_versions():
+        self.check_unknown()
+        for version in self.get_unapplied_versions():
             self.apply(version)
 
     def mark_applied(self, version):
@@ -149,12 +141,12 @@
 
     def mark_applied_all(self):
         """Mark all unapplied patches as applied."""
-        for version in self._get_unapplied_versions():
+        for version in self.get_unapplied_versions():
             self.mark_applied(version)
 
     def has_pending_patches(self):
         """Return C{True} if there are unapplied patches, C{False} if not."""
-        for version in self._get_unapplied_versions():
+        for version in self.get_unapplied_versions():
             return True
         return False
 
@@ -164,7 +156,7 @@
         database, but don't appear in the schema's patches module.
         """
         applied = self._get_applied_patches()
-        known_patches = self._get_patch_versions()
+        known_patches = self._patch_set.get_patch_versions()
         unknown_patches = set()
 
         for patch in applied:
@@ -172,10 +164,20 @@
                 unknown_patches.add(patch)
         return unknown_patches
 
-    def _get_unapplied_versions(self):
+    def check_unknown(self):
+        """Look for patches that we don't know about.
+
+        @raises UnknownPatchError: If the store has applied patch versions
+            this schema doesn't know about.
+        """
+        unknown_patches = self.get_unknown_patch_versions()
+        if unknown_patches:
+            raise UnknownPatchError(self._store, unknown_patches)
+
+    def get_unapplied_versions(self):
         """Return the versions of all unapplied patches."""
         applied = self._get_applied_patches()
-        for version in self._get_patch_versions():
+        for version in self._patch_set.get_patch_versions():
             if version not in applied:
                 yield version
 
@@ -186,12 +188,89 @@
             applied.add(patch.version)
         return applied
 
-    def _get_patch_versions(self):
+
+class PatchSet(object):
+    """A collection of patch modules.
+
+    Each patch module lives in a regular Python module file, contained in a
+    sub-directory named against the patch version. For example, given
+    a directory tree like:
+
+      mypackage/
+          __init__.py
+          patch_1/
+              __init__.py
+              foo.py
+
+    the following code will return a patch module object for foo.py:
+
+      >>> import mypackage
+      >>> patch_set = PackagePackage(mypackage, sub_level="foo")
+      >>> patch_module = patch_set.get_patch_module(1)
+      >>> print patch_module.__name__
+      'mypackage.patch_1.foo'
+
+    Different sub-levels can be used to apply different patches to different
+    stores (see L{Sharding}).
+
+    Alternatively if no sub-level is provided, the structure will be flat:
+
+      mypackage/
+          __init__.py
+          patch_1.py
+
+      >>> import mypackage
+      >>> patch_set = PackagePackage(mypackage)
+      >>> patch_module = patch_set.get_patch_module(1)
+      >>> print patch_module.__name__
+      'mypackage.patch_1'
+
+    This simpler structure can be used if you have just one store to patch
+    or you don't care to co-ordinate the patches across your stores.
+    """
+
+    def __init__(self, package, sub_level=None):
+        self._package = package
+        self._sub_level = sub_level
+
+    def get_patch_versions(self):
         """Return the versions of all available patches."""
-        format = re.compile(r"^patch_(\d+).py$")
+        pattern = r"^patch_(\d+)"
+        if not self._sub_level:
+            pattern += ".py"
+        pattern += "$"
+        format = re.compile(pattern)
 
-        filenames = os.listdir(os.path.dirname(self._package.__file__))
+        patch_directory = self._get_patch_directory()
+        filenames = os.listdir(patch_directory)
         matches = [(format.match(fn), fn) for fn in filenames]
         matches = sorted(filter(lambda x: x[0], matches),
-                         key=lambda x: int(x[1][6:-3]))
+                         key=lambda x: int(x[0].group(1)))
         return [int(match.group(1)) for match, filename in matches]
+
+    def get_patch_module(self, version):
+        """Import the Python module of the patch file with the given version.
+
+        @param: The version of the module patch to import.
+        @return: The imported module.
+        """
+        name = "patch_%d" % version
+        levels = [self._package.__name__, name]
+        if self._sub_level:
+            directory = self._get_patch_directory()
+            path = os.path.join(directory, name, self._sub_level + ".py")
+            if not os.path.exists(path):
+                return _EmptyPatchModule()
+            levels.append(self._sub_level)
+        return __import__(".".join(levels), None, None, [''])
+
+    def _get_patch_directory(self):
+        """Get the path to the directory of the patch package."""
+        return os.path.dirname(self._package.__file__)
+
+
+class _EmptyPatchModule(object):
+    """Fake module object with a no-op C{apply} function."""
+
+    def apply(self, store):
+        pass

=== modified file 'storm/schema/schema.py'
--- storm/schema/schema.py	2013-09-11 08:00:51 +0000
+++ storm/schema/schema.py	2014-12-18 10:58:55 +0000
@@ -34,7 +34,8 @@
 >>> drops = ['DROP TABLE person']
 >>> deletes = ['DELETE FROM person']
 >>> import patch_module
->>> schema = Schema(creates, drops, deletes, patch_module)
+>>> patch_set = PatchSet(patch_module)
+>>> schema = Schema(creates, drops, deletes, patch_set)
 >>> schema.create(store)
 
 
@@ -42,8 +43,24 @@
 upgrade the schema over time.
 """
 
+import types
+
 from storm.locals import StormError
-from storm.schema.patch import PatchApplier
+from storm.schema.patch import PatchApplier, PatchSet
+
+
+class SchemaMissingError(Exception):
+    """Raised when a L{Store} has no schema at all."""
+
+
+class UnappliedPatchesError(Exception):
+    """Raised when a L{Store} has unapplied schema patches.
+
+    @ivar unapplied_versions: A list containing all unapplied patch versions.
+    """
+
+    def __init__(self, unapplied_versions):
+        self.unapplied_versions = unapplied_versions
 
 
 class Schema(object):
@@ -52,7 +69,7 @@
     @param creates: A list of C{CREATE TABLE} statements.
     @param drops: A list of C{DROP TABLE} statements.
     @param deletes: A list of C{DELETE FROM} statements.
-    @param patch_package: The Python package containing patch modules to apply.
+    @param patch_set: The L{PatchSet} containing patch modules to apply.
     @param committer: Optionally a committer to pass to the L{PatchApplier}.
 
     @see: L{PatchApplier}.
@@ -61,11 +78,16 @@
     _drop_patch = "DROP TABLE IF EXISTS patch"
     _autocommit = True
 
-    def __init__(self, creates, drops, deletes, patch_package, committer=None):
+    def __init__(self, creates, drops, deletes, patch_set, committer=None):
         self._creates = creates
         self._drops = drops
         self._deletes = deletes
-        self._patch_package = patch_package
+        if isinstance(patch_set, types.ModuleType):
+            # Up to version 0.20.0 the fourth positional parameter used to
+            # be a raw module containing the patches. We wrap it with PatchSet
+            # for keeping backward-compatibility.
+            patch_set = PatchSet(patch_set)
+        self._patch_set = patch_set
         self._committer = committer
 
     def _execute_statements(self, store, statements):
@@ -90,10 +112,38 @@
         """
         self._autocommit = flag
 
+    def check(self, store):
+        """Check that the given L{Store} is compliant with this L{Schema}.
+
+        @param store: The L{Store} to check.
+
+        @raises SchemaMissingError: If there is no schema at all.
+        @raises UnappliedPatchesError: If there are unapplied schema patches.
+        @raises UnknownPatchError: If the store has patches the schema doesn't.
+        """
+        try:
+            store.execute("SELECT * FROM patch WHERE version=0")
+        except StormError:
+            # No schema at all. Create it from the ground.
+            store.rollback()
+            raise SchemaMissingError()
+
+        patch_applier = self._build_patch_applier(store)
+        patch_applier.check_unknown()
+        unapplied_versions = list(patch_applier.get_unapplied_versions())
+        if unapplied_versions:
+            raise UnappliedPatchesError(unapplied_versions)
+
     def create(self, store):
-        """Run C{CREATE TABLE} SQL statements with C{store}."""
+        """Run C{CREATE TABLE} SQL statements with C{store}.
+
+        @raises SchemaAlreadyCreatedError: If the schema for this store was
+            already created.
+        """
         self._execute_statements(store, [self._create_patch])
         self._execute_statements(store, self._creates)
+        patch_applier = self._build_patch_applier(store)
+        patch_applier.mark_applied_all()
 
     def drop(self, store):
         """Run C{DROP TABLE} SQL statements with C{store}."""
@@ -110,20 +160,39 @@
         If a schema isn't present a new one will be created.  Unapplied
         patches will be applied to an existing schema.
         """
-        class NoopCommitter(object):
-            commit = lambda _: None
-            rollback = lambda _: None
-
-        committer = self._committer if self._autocommit else NoopCommitter()
-        patch_applier = PatchApplier(store, self._patch_package, committer)
+        patch_applier = self._build_patch_applier(store)
         try:
-            store.execute("SELECT * FROM patch WHERE version=0")
-        except StormError:
+            self.check(store)
+        except SchemaMissingError:
             # No schema at all. Create it from the ground.
-            store.rollback()
             self.create(store)
-            patch_applier.mark_applied_all()
-            if self._autocommit:
-                store.commit()
-        else:
-            patch_applier.apply_all()
+        except UnappliedPatchesError, error:
+            patch_applier.check_unknown()
+            for version in error.unapplied_versions:
+                self.advance(store, version)
+
+    def advance(self, store, version):
+        """Advance the schema of C{store} by applying the next unapplied patch.
+
+        @return: The version of patch that has been applied or C{None} if
+            no patch was applied (i.e. the schema is fully upgraded).
+        """
+        patch_applier = self._build_patch_applier(store)
+        patch_applier.apply(version)
+
+    def _build_patch_applier(self, store):
+        """Build a L{PatchApplier} to use for the given C{store}."""
+        committer = self._committer
+        if not self._autocommit:
+            committer = _NoopCommitter()
+        return PatchApplier(store, self._patch_set, committer)
+
+
+class _NoopCommitter(object):
+    """Dummy committer that does nothing."""
+
+    def commit(self):
+        pass
+
+    def rollback(self):
+        pass

=== modified file 'storm/zope/testing.py'
--- storm/zope/testing.py	2012-08-27 11:57:29 +0000
+++ storm/zope/testing.py	2014-12-18 10:58:55 +0000
@@ -196,7 +196,8 @@
         """
         Return the modification time of the C{schema}'s patch directory.
         """
-        schema_stat = os.stat(os.path.dirname(schema._patch_package.__file__))
+        patch_directory = os.path.dirname(schema._patch_set._package.__file__)
+        schema_stat = os.stat(patch_directory)
         return int(schema_stat.st_mtime)
 
     def _get_schema_stamp_mtime(self, name):

=== modified file 'tests/schema/patch.py'
--- tests/schema/patch.py	2010-08-17 23:57:51 +0000
+++ tests/schema/patch.py	2014-12-18 10:58:55 +0000
@@ -24,7 +24,7 @@
 
 from storm.locals import StormError, Store, create_database
 from storm.schema.patch import (
-    Patch, PatchApplier, UnknownPatchError, BadPatchError)
+    Patch, PatchApplier, UnknownPatchError, BadPatchError, PatchSet)
 from tests.mocker import MockerTestCase
 
 
@@ -99,10 +99,10 @@
         self.committed += 1
 
 
-class PatchTest(MockerTestCase):
+class PatchApplierTest(MockerTestCase):
 
     def setUp(self):
-        super(PatchTest, self).setUp()
+        super(PatchApplierTest, self).setUp()
 
         self.patchdir = self.makeDir()
         self.pkgdir = os.path.join(self.patchdir, "mypackage")
@@ -133,6 +133,7 @@
 
         import mypackage
         self.mypackage = mypackage
+        self.patch_set = PatchSet(mypackage)
 
         # Create another connection just to keep track of the state of the
         # whole transaction manager.  See the assertion functions below.
@@ -152,11 +153,11 @@
                 self.another_store.rollback()
 
         self.committer = Committer()
-        self.patch_applier = PatchApplier(self.store, self.mypackage,
+        self.patch_applier = PatchApplier(self.store, self.patch_set,
                                           self.committer)
 
     def tearDown(self):
-        super(PatchTest, self).tearDown()
+        super(PatchApplierTest, self).tearDown()
         self.committer.rollback()
         sys.path.remove(self.patchdir)
         for name in list(sys.modules):
@@ -202,6 +203,21 @@
 
         self.assert_transaction_committed()
 
+    def test_apply_with_patch_directory(self):
+        """
+        If the given L{PatchSet} uses sub-level patches, then the
+        L{PatchApplier.apply} method will look at the per-patch directory and
+        apply the relevant sub-level patch.
+        """
+        path = os.path.join(self.pkgdir, "patch_99")
+        self.makeDir(path=path)
+        self.makeFile(content="", path=os.path.join(path, "__init__.py"))
+        self.makeFile(content=patch_test_0, path=os.path.join(path, "foo.py"))
+        self.patch_set._sub_level = "foo"
+        self.add_module("patch_99/foo.py", patch_test_0)
+        self.patch_applier.apply(99)
+        self.assertTrue(self.store.get(Patch, (99)))
+
     def test_apply_all(self):
         """
         L{PatchApplier.apply_all} executes all unapplied patches.
@@ -239,10 +255,10 @@
         """
         self.add_module("patch_666.py", patch_explosion)
         self.add_module("patch_667.py", patch_after_explosion)
-        self.assertEquals(list(self.patch_applier._get_unapplied_versions()),
+        self.assertEquals(list(self.patch_applier.get_unapplied_versions()),
                           [42, 380, 666, 667])
         self.assertRaises(StormError, self.patch_applier.apply_all)
-        self.assertEquals(list(self.patch_applier._get_unapplied_versions()),
+        self.assertEquals(list(self.patch_applier.get_unapplied_versions()),
                           [666, 667])
 
     def test_mark_applied(self):
@@ -381,3 +397,66 @@
             self.assertTrue("# Comment" in formatted)
         else:
             self.fail("BadPatchError not raised")
+
+
+class PatchSetTest(MockerTestCase):
+
+    def setUp(self):
+        super(PatchSetTest, self).setUp()
+        self.sys_dir = self.makeDir()
+        self.package_dir = os.path.join(self.sys_dir, "mypackage")
+        os.makedirs(self.package_dir)
+
+        self.makeFile(
+            content="", dirname=self.package_dir, basename="__init__.py")
+
+        sys.path.append(self.sys_dir)
+        import mypackage
+        self.patch_package = PatchSet(mypackage, sub_level="foo")
+
+    def tearDown(self):
+        super(PatchSetTest, self).tearDown()
+        for name in list(sys.modules):
+            if name == "mypackage" or name.startswith("mypackage."):
+                del sys.modules[name]
+
+    def test_get_patch_versions(self):
+        """
+        The C{get_patch_versions} method returns the available patch versions,
+        by looking at directories named like "patch_N".
+        """
+        patch_1_dir = os.path.join(self.package_dir, "patch_1")
+        os.makedirs(patch_1_dir)
+        self.assertEqual([1], self.patch_package.get_patch_versions())
+
+    def test_get_patch_versions_ignores_non_patch_directories(self):
+        """
+        The C{get_patch_versions} method ignores files or directories not
+        matching the required name pattern.
+        """
+        random_dir = os.path.join(self.package_dir, "random")
+        os.makedirs(random_dir)
+        self.assertEqual([], self.patch_package.get_patch_versions())
+
+    def test_get_patch_module(self):
+        """
+        The C{get_patch_module} method returns the Python module for the patch
+        with the given version.
+        """
+        patch_1_dir = os.path.join(self.package_dir, "patch_1")
+        os.makedirs(patch_1_dir)
+        self.makeFile(content="", dirname=patch_1_dir, basename="__init__.py")
+        self.makeFile(content="", dirname=patch_1_dir, basename="foo.py")
+        patch_module = self.patch_package.get_patch_module(1)
+        self.assertEqual("mypackage.patch_1.foo", patch_module.__name__)
+
+    def test_get_patch_module_no_sub_level(self):
+        """
+        The C{get_patch_module} method returns a dummy patch module if no
+        sub-level file exists in the patch directory for the given version.
+        """
+        patch_1_dir = os.path.join(self.package_dir, "patch_1")
+        os.makedirs(patch_1_dir)
+        patch_module = self.patch_package.get_patch_module(1)
+        store = object()
+        self.assertIsNone(patch_module.apply(store))

=== modified file 'tests/schema/schema.py'
--- tests/schema/schema.py	2012-09-28 12:31:28 +0000
+++ tests/schema/schema.py	2014-12-18 10:58:55 +0000
@@ -22,7 +22,8 @@
 import sys
 
 from storm.locals import StormError, Store, create_database
-from storm.schema import Schema
+from storm.schema.schema import (
+    Schema, SchemaMissingError, UnappliedPatchesError)
 from tests.mocker import MockerTestCase
 
 
@@ -96,6 +97,29 @@
 
         return Package(package_dir, name)
 
+    def test_check_with_missing_schema(self):
+        """
+        L{Schema.check} raises an exception if the given store is
+        completely pristine and no schema has been applied yet.
+        """
+        rollbacks = []
+        self.store.rollback = lambda: rollbacks.append(True)
+        self.assertRaises(SchemaMissingError, self.schema.check, self.store)
+        self.assertEqual([True], rollbacks)
+
+    def test_check_with_unapplied_patches(self):
+        """
+        L{Schema.check} raises an exception if the given store has unapplied
+        schema patches.
+        """
+        self.schema.create(self.store)
+        contents = """
+def apply(store):
+    pass
+"""
+        self.package.create_module("patch_1.py", contents)
+        self.assertRaises(UnappliedPatchesError, self.schema.check, self.store)
+
     def test_create(self):
         """
         L{Schema.create} can be used to create the tables of a L{Store}.
@@ -104,6 +128,9 @@
                           self.store.execute, "SELECT * FROM person")
         self.schema.create(self.store)
         self.assertEquals(list(self.store.execute("SELECT * FROM person")), [])
+        # By default changes are committed
+        store2 = Store(self.database)
+        self.assertEquals(list(store2.execute("SELECT * FROM person")), [])
 
     def test_create_with_autocommit_off(self):
         """
@@ -190,3 +217,24 @@
             "INSERT INTO person (id, name, phone) VALUES (1, 'Jane', '123')")
         self.assertEquals(list(self.store.execute("SELECT * FROM person")),
                           [(1, u"Jane", u"123")])
+
+    def test_advance(self):
+        """
+        L{Schema.advance} executes the given patch version.
+        """
+        self.schema.create(self.store)
+        contents1 = """
+def apply(store):
+    store.execute('ALTER TABLE person ADD COLUMN phone TEXT')
+"""
+        contents2 = """
+def apply(store):
+    store.execute('ALTER TABLE person ADD COLUMN address TEXT')
+"""
+        self.package.create_module("patch_1.py", contents1)
+        self.package.create_module("patch_2.py", contents2)
+        self.schema.advance(self.store, 1)
+        self.store.execute(
+            "INSERT INTO person (id, name, phone) VALUES (1, 'Jane', '123')")
+        self.assertEquals(list(self.store.execute("SELECT * FROM person")),
+                          [(1, u"Jane", u"123")])

=== modified file 'tests/zope/testing.py'
--- tests/zope/testing.py	2012-08-27 11:57:29 +0000
+++ tests/zope/testing.py	2014-12-18 10:58:55 +0000
@@ -27,6 +27,7 @@
 from storm.locals import create_database, Store, Unicode, Int
 from storm.exceptions import IntegrityError
 from storm.testing import CaptureTracer
+from storm.schema.patch import PatchSet
 
 if has_transaction and has_zope_component and has_testresources:
     from zope.component import provideUtility, getUtility
@@ -62,7 +63,7 @@
         drop = ["DROP TABLE test"]
         delete = ["DELETE FROM test"]
         uri = "sqlite:///%s" % self.makeFile()
-        schema = ZSchema(create, drop, delete, patch_package)
+        schema = ZSchema(create, drop, delete, PatchSet(patch_package))
         self.databases = [{"name": "test", "uri": uri, "schema": schema}]
         self.resource = ZStormResourceManager(self.databases)
         self.store = Store(create_database(uri))