Merge Aurélien Bompard's latest merge branch, with some cleaning up by Barry.

6 files changed, 191 insertions, 22 deletions

diff --git a/src/mailman/database/alembic/__init__.py b/src/mailman/database/alembic/__init__.py
index ffd3af6df..9ac7f1311 100644
--- a/src/mailman/database/alembic/__init__.py
+++ b/src/mailman/database/alembic/__init__.py
@@ -29,4 +29,4 @@ from alembic.config import Config
 from mailman.utilities.modules import expand_path
 
 
-alembic_cfg = Config(expand_path('python:mailman.config.alembic'))
+alembic_cfg = Config(expand_path("python:mailman.config.schema"))
diff --git a/src/mailman/database/alembic/versions/429e08420177_initial.py b/src/mailman/database/alembic/versions/51b7f92bd06c_initial.py
index 14f22079b..3feb24fff 100644
--- a/src/mailman/database/alembic/versions/429e08420177_initial.py
+++ b/src/mailman/database/alembic/versions/51b7f92bd06c_initial.py
@@ -22,29 +22,45 @@ in the database.  As a consequence, if the database version is reported
 as None, it means the database needs to be created from scratch with
 SQLAlchemy itself.
 
-It also removes the `version` table left over from Storm (if it exists).
+It also removes schema items left over from Storm.
 
-Revision ID: 429e08420177
+Revision ID: 51b7f92bd06c
 Revises: None
-Create Date: 2014-10-02 10:18:17.333354
+Create Date: 2014-10-10 09:53:35.624472
 """
 
 from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 __all__ = [
+    'downgrade',
+    'upgrade',
     ]
 
-# Revision identifiers, used by Alembic.
-revision = '429e08420177'
-down_revision = None
 
 from alembic import op
+import sqlalchemy as sa
+
+
+# Revision identifiers, used by Alembic.
+revision = '51b7f92bd06c'
+down_revision = None
 
 
 def upgrade():
     op.drop_table('version')
+    if op.get_bind().dialect.name != 'sqlite':
+        # SQLite does not support dropping columns.
+        op.drop_column('mailinglist', 'acceptable_aliases_id')
+    op.create_index(op.f('ix_user__user_id'), 'user',
+                    ['_user_id'], unique=False)
+    op.drop_index('ix_user_user_id', table_name='user')
 
 
 def downgrade():
-    pass
+    op.create_table('version')
+    op.create_index('ix_user_user_id', 'user', ['_user_id'], unique=False)
+    op.drop_index(op.f('ix_user__user_id'), table_name='user')
+    op.add_column(
+        'mailinglist',
+        sa.Column('acceptable_aliases_id', sa.INTEGER(), nullable=True))
diff --git a/src/mailman/database/base.py b/src/mailman/database/base.py
index 2f69ed6ad..e360dcedf 100644
--- a/src/mailman/database/base.py
+++ b/src/mailman/database/base.py
@@ -25,13 +25,11 @@ __all__ = [
 
 import logging
 
-from alembic import command
 from sqlalchemy import create_engine
 from sqlalchemy.orm import sessionmaker
 from zope.interface import implementer
 
 from mailman.config import config
-from mailman.database.alembic import alembic_cfg
 from mailman.interfaces.database import IDatabase
 from mailman.utilities.string import expand
 
@@ -91,14 +89,6 @@ class SABaseDatabase:
         """
         pass
 
-    def stamp(self, debug=False):
-        """Stamp the database with the latest Alembic version."""
-        # Newly created databases don't need migrations from Alembic, since
-        # create_all() ceates the latest schema.  This patches the database
-        # with the latest Alembic version to add an entry in the
-        # alembic_version table.
-        command.stamp(alembic_cfg, 'head')
-
     def initialize(self, debug=None):
         """See `IDatabase`."""
         # Calculate the engine url.
diff --git a/src/mailman/database/factory.py b/src/mailman/database/factory.py
index 9cbe1088f..64174449d 100644
--- a/src/mailman/database/factory.py
+++ b/src/mailman/database/factory.py
@@ -28,8 +28,8 @@ __all__ = [
 
 import os
 import types
+import alembic.command
 
-from alembic import command
 from alembic.migration import MigrationContext
 from alembic.script import ScriptDirectory
 from flufl.lock import Lock
@@ -84,6 +84,8 @@ class SchemaManager:
         last_version = self._database.store.query(Version.c.version).filter(
             Version.c.component == 'schema'
             ).order_by(Version.c.version.desc()).first()
+        # Don't leave open transactions or they will block any schema change.
+        self._database.commit()
         return last_version
 
     def setup_database(self):
@@ -99,7 +101,8 @@ class SchemaManager:
             if storm_version is None:
                 # Initial database creation.
                 Model.metadata.create_all(self._database.engine)
-                command.stamp(alembic_cfg, 'head')
+                self._database.commit()
+                alembic.command.stamp(alembic_cfg, 'head')
             else:
                 # The database was previously managed by Storm.
                 if storm_version.version < LAST_STORM_SCHEMA_VERSION:
@@ -107,9 +110,9 @@ class SchemaManager:
                         'Upgrades skipping beta versions is not supported.')
                 # Run migrations to remove the Storm-specific table and upgrade
                 # to SQLAlchemy and Alembic.
-                command.upgrade(alembic_cfg, 'head')
+                alembic.command.upgrade(alembic_cfg, 'head')
         elif current_rev != head_rev:
-            command.upgrade(alembic_cfg, 'head')
+            alembic.command.upgrade(alembic_cfg, 'head')
         return head_rev
 
 
diff --git a/src/mailman/database/tests/__init__.py b/src/mailman/database/tests/__init__.py
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/src/mailman/database/tests/__init__.py
diff --git a/src/mailman/database/tests/test_factory.py b/src/mailman/database/tests/test_factory.py
new file mode 100644
index 000000000..d7c4d8503
--- /dev/null
+++ b/src/mailman/database/tests/test_factory.py
@@ -0,0 +1,160 @@
+# Copyright (C) 2013-2014 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test database schema migrations"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'TestSchemaManager',
+    ]
+
+
+import unittest
+import alembic.command
+
+from mock import patch
+from sqlalchemy import MetaData, Table, Column, Integer, Unicode
+from sqlalchemy.exc import ProgrammingError, OperationalError
+from sqlalchemy.schema import Index
+
+from mailman.config import config
+from mailman.database.alembic import alembic_cfg
+from mailman.database.factory import LAST_STORM_SCHEMA_VERSION, SchemaManager
+from mailman.database.model import Model
+from mailman.interfaces.database import DatabaseError
+from mailman.testing.layers import ConfigLayer
+
+
+
+class TestSchemaManager(unittest.TestCase):
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        # Drop the existing database.
+        Model.metadata.drop_all(config.db.engine)
+        md = MetaData()
+        md.reflect(bind=config.db.engine)
+        for tablename in ('alembic_version', 'version'):
+            if tablename in md.tables:
+                md.tables[tablename].drop(config.db.engine)
+        self.schema_mgr = SchemaManager(config.db)
+
+    def tearDown(self):
+        self._drop_storm_database()
+        # Restore a virgin database.
+        Model.metadata.create_all(config.db.engine)
+
+    def _table_exists(self, tablename):
+        md = MetaData()
+        md.reflect(bind=config.db.engine)
+        return tablename in md.tables
+
+    def _create_storm_database(self, revision):
+        version_table = Table(
+            'version', Model.metadata,
+            Column('id', Integer, primary_key=True),
+            Column('component', Unicode),
+            Column('version', Unicode),
+            )
+        version_table.create(config.db.engine)
+        config.db.store.execute(version_table.insert().values(
+            component='schema', version=revision))
+        config.db.commit()
+        # Other Storm specific changes, those SQL statements hopefully work on
+        # all DB engines...
+        config.db.engine.execute(
+            'ALTER TABLE mailinglist ADD COLUMN acceptable_aliases_id INT')
+        Index('ix_user__user_id').drop(bind=config.db.engine)
+        # Don't pollute our main metadata object, create a new one.
+        md = MetaData()
+        user_table = Model.metadata.tables['user'].tometadata(md)
+        Index('ix_user_user_id', user_table.c._user_id).create(
+            bind=config.db.engine)
+        config.db.commit()
+
+    def _drop_storm_database(self):
+        """Remove the leftovers from a Storm DB.
+
+        A drop_all() must be issued afterwards.
+        """
+        if 'version' in Model.metadata.tables:
+            version = Model.metadata.tables['version']
+            version.drop(config.db.engine, checkfirst=True)
+            Model.metadata.remove(version)
+        try:
+            Index('ix_user_user_id').drop(bind=config.db.engine)
+        except (ProgrammingError, OperationalError):
+            # Nonexistent.  PostgreSQL raises a ProgrammingError, while SQLite
+            # raises an OperationalError.
+            pass
+        config.db.commit()
+
+    def test_current_database(self):
+        # The database is already at the latest version.
+        alembic.command.stamp(alembic_cfg, 'head')
+        with patch('alembic.command') as alembic_command:
+            self.schema_mgr.setup_database()
+            self.assertFalse(alembic_command.stamp.called)
+            self.assertFalse(alembic_command.upgrade.called)
+
+    @patch('alembic.command')
+    def test_initial(self, alembic_command):
+        # No existing database.
+        self.assertFalse(self._table_exists('mailinglist'))
+        self.assertFalse(self._table_exists('alembic_version'))
+        self.schema_mgr.setup_database()
+        self.assertFalse(alembic_command.upgrade.called)
+        self.assertTrue(self._table_exists('mailinglist'))
+        self.assertTrue(self._table_exists('alembic_version'))
+
+    @patch('alembic.command.stamp')
+    def test_storm(self, alembic_command_stamp):
+        # Existing Storm database.
+        Model.metadata.create_all(config.db.engine)
+        self._create_storm_database(LAST_STORM_SCHEMA_VERSION)
+        self.schema_mgr.setup_database()
+        self.assertFalse(alembic_command_stamp.called)
+        self.assertTrue(
+            self._table_exists('mailinglist')
+            and self._table_exists('alembic_version')
+            and not self._table_exists('version'))
+
+    @patch('alembic.command')
+    def test_old_storm(self, alembic_command):
+        # Existing Storm database in an old version.
+        Model.metadata.create_all(config.db.engine)
+        self._create_storm_database('001')
+        self.assertRaises(DatabaseError, self.schema_mgr.setup_database)
+        self.assertFalse(alembic_command.stamp.called)
+        self.assertFalse(alembic_command.upgrade.called)
+
+    def test_old_db(self):
+        # The database is in an old revision, must upgrade.
+        alembic.command.stamp(alembic_cfg, 'head')
+        md = MetaData()
+        md.reflect(bind=config.db.engine)
+        config.db.store.execute(md.tables['alembic_version'].delete())
+        config.db.store.execute(md.tables['alembic_version'].insert().values(
+            version_num='dummyrevision'))
+        config.db.commit()
+        with patch('alembic.command') as alembic_command:
+            self.schema_mgr.setup_database()
+            self.assertFalse(alembic_command.stamp.called)
+            self.assertTrue(alembic_command.upgrade.called)