11 files changed, 172 insertions, 65 deletions

diff --git a/src/mailman/commands/docs/conf.rst b/src/mailman/commands/docs/conf.rst
index 7b8529ac3..b3fef9db7 100644
--- a/src/mailman/commands/docs/conf.rst
+++ b/src/mailman/commands/docs/conf.rst
@@ -22,7 +22,7 @@ To get a list of all key-value pairs of any section, you need to call the
 command without any options.
 
     >>> command.process(FakeArgs)
-    [logging.archiver] path: mailman.log
+    [logging.dbmigration] path: mailman.log
     ...
     [passwords] password_length: 8
     ...
@@ -43,12 +43,14 @@ key, along with the names of the corresponding sections.
     >>> FakeArgs.section = None
     >>> FakeArgs.key = 'path'
     >>> command.process(FakeArgs)
+    [logging.dbmigration] path: mailman.log
     [logging.archiver] path: mailman.log
     [logging.locks] path: mailman.log
     [logging.mischief] path: mailman.log
     [logging.config] path: mailman.log
     [logging.error] path: mailman.log
     [logging.smtp] path: smtp.log
+    [logging.database] path: mailman.log
     [logging.http] path: mailman.log
     [logging.root] path: mailman.log
     [logging.fromusenet] path: mailman.log
diff --git a/src/mailman/config/alembic.cfg b/src/mailman/config/alembic.cfg
index 1858eb94a..78c047379 100644
--- a/src/mailman/config/alembic.cfg
+++ b/src/mailman/config/alembic.cfg
@@ -19,39 +19,3 @@ script_location = mailman.database:alembic
 # a source .py file to be detected as revisions in the
 # versions/ directory
 # sourceless = false
-
-
-# Logging configuration
-[loggers]
-keys = root,sqlalchemy,alembic
-
-[handlers]
-keys = console
-
-[formatters]
-keys = generic
-
-[logger_root]
-level = WARN
-handlers = console
-qualname = console
-
-[logger_sqlalchemy]
-level = WARN
-handlers = console
-qualname = sqlalchemy.engine
-
-[logger_alembic]
-level = WARN
-handlers = console
-qualname = alembic
-
-[handler_console]
-class = StreamHandler
-args = (sys.stderr,)
-level = WARN
-formatter = generic
-
-[formatter_generic]
-format = %(filename)s - $(funcName)s - %(levelname)-5.5s [%(name)s] %(message)s
-datefmt = %H:%M:%S
diff --git a/src/mailman/config/schema.cfg b/src/mailman/config/schema.cfg
index 6ab6e939b..3316f54e0 100644
--- a/src/mailman/config/schema.cfg
+++ b/src/mailman/config/schema.cfg
@@ -204,11 +204,6 @@ class: mailman.database.sqlite.SQLiteDatabase
 url: sqlite:///$DATA_DIR/mailman.db
 debug: no
 
-# Where can we find the Alembic migration scripts?  The `python:` schema means
-# that this is a module path relative to the Mailman 3 source installation.
-# alembic_scripts: mailman.database:alembic
-
-
 [logging.template]
 # This defines various log settings.  The options available are:
 #
@@ -242,6 +237,8 @@ debug: no
 # - smtp-failure    --  Unsuccessful SMTP activity
 # - subscribe       --  Information about leaves/joins
 # - vette           --  Message vetting information
+# - database        --  Database activity
+# - dbmigration     --  Database migrations
 format: %(asctime)s (%(process)d) %(message)s
 datefmt: %b %d %H:%M:%S %Y
 propagate: no
@@ -306,6 +303,12 @@ failure: $msgid delivery to $recip failed with code $smtpcode, $smtpmsg
 
 [logging.vette]
 
+[logging.database]
+level: warn
+
+[logging.dbmigration]
+level: warn
+
 
 [webservice]
 # The hostname at which admin web service resources are exposed.
diff --git a/src/mailman/core/logging.py b/src/mailman/core/logging.py
index 43030436a..591f3c996 100644
--- a/src/mailman/core/logging.py
+++ b/src/mailman/core/logging.py
@@ -126,6 +126,10 @@ def initialize(propagate=None):
             continue
         if sub_name == 'locks':
             log = logging.getLogger('flufl.lock')
+        elif sub_name == 'database':
+            log = logging.getLogger('sqlalchemy')
+        elif sub_name == 'dbmigration':
+            log = logging.getLogger('alembic')
         else:
             logger_name = 'mailman.' + sub_name
             log = logging.getLogger(logger_name)
diff --git a/src/mailman/database/alembic/env.py b/src/mailman/database/alembic/env.py
index 269dcf835..56f673803 100644
--- a/src/mailman/database/alembic/env.py
+++ b/src/mailman/database/alembic/env.py
@@ -28,7 +28,6 @@ __all__ = [
 
 from alembic import context
 from contextlib import closing
-from logging.config import fileConfig
 from sqlalchemy import create_engine
 
 from mailman.core import initialize
@@ -38,8 +37,6 @@ from mailman.database.model import Model
 from mailman.utilities.string import expand
 
 
-fileConfig(alembic_cfg.config_file_name)
-
 
 def run_migrations_offline():
     """Run migrations in 'offline' mode.
diff --git a/src/mailman/database/base.py b/src/mailman/database/base.py
index dcaedade0..6e86faf04 100644
--- a/src/mailman/database/base.py
+++ b/src/mailman/database/base.py
@@ -31,7 +31,6 @@ 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,15 +90,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 469ed5d18..ea2048143 100644
--- a/src/mailman/database/factory.py
+++ b/src/mailman/database/factory.py
@@ -70,8 +70,7 @@ class SchemaManager:
 
     def __init__(self, database):
         self.database = database
-        self.alembic_cfg = alembic_cfg
-        self.script = ScriptDirectory.from_config(self.alembic_cfg)
+        self.script = ScriptDirectory.from_config(alembic_cfg)
 
     def get_storm_schema_version(self):
         md = MetaData()
@@ -82,8 +81,18 @@ 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 _create(self):
+        # initial DB creation
+        Model.metadata.create_all(self.database.engine)
+        command.stamp(alembic_cfg, "head")
+
+    def _upgrade(self):
+        command.upgrade(alembic_cfg, "head")
+
     def setup_db(self):
         context = MigrationContext.configure(self.database.store.connection())
         current_rev = context.get_current_revision()
@@ -95,8 +104,7 @@ class SchemaManager:
             storm_version = self.get_storm_schema_version()
             if storm_version is None:
                 # initial DB creation
-                Model.metadata.create_all(self.database.engine)
-                command.stamp(self.alembic_cfg, "head")
+                self._create()
             else:
                 # DB from a previous version managed by Storm
                 if storm_version.version < self.LAST_STORM_SCHEMA_VERSION:
@@ -106,9 +114,9 @@ class SchemaManager:
                             "Mailman beta release")
                 # Run migrations to remove the Storm-specific table and
                 # upgrade to SQLAlchemy & Alembic
-                command.upgrade(self.alembic_cfg, "head")
+                self._upgrade()
         elif current_rev != head_rev:
-            command.upgrade(self.alembic_cfg, "head")
+            self._upgrade()
         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..a87bca7be
--- /dev/null
+++ b/src/mailman/database/tests/test_factory.py
@@ -0,0 +1,134 @@
+# 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__ = [
+    ]
+
+
+import unittest
+import types
+
+import alembic.command
+from mock import Mock
+from sqlalchemy import MetaData, Table, Column, Integer, Unicode
+
+from mailman.config import config
+from mailman.testing.layers import ConfigLayer
+from mailman.database.factory import SchemaManager, _reset
+from mailman.database.sqlite import SQLiteDatabase
+from mailman.database.alembic import alembic_cfg
+from mailman.database.model import Model
+
+
+
+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)
+        if "alembic_version" in md.tables:
+            md.tables["alembic_version"].drop(config.db.engine)
+        self.schema_mgr = SchemaManager(config.db)
+
+    def tearDown(self):
+        if "version" in Model.metadata.tables:
+            version = Model.metadata.tables["version"]
+            version.drop(config.db.engine, checkfirst=True)
+            Model.metadata.remove(version)
+        # Restore a virgin DB
+        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_version_table(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()
+
+
+    def test_current_db(self):
+        """The database is already at the latest version"""
+        alembic.command.stamp(alembic_cfg, "head")
+        self.schema_mgr._create = Mock()
+        self.schema_mgr._upgrade = Mock()
+        self.schema_mgr.setup_db()
+        self.assertFalse(self.schema_mgr._create.called)
+        self.assertFalse(self.schema_mgr._upgrade.called)
+
+    def test_initial(self):
+        """No existing database"""
+        self.assertFalse(self._table_exists("mailinglist"))
+        self.assertFalse(self._table_exists("alembic_version"))
+        self.schema_mgr._upgrade = Mock()
+        self.schema_mgr.setup_db()
+        self.assertFalse(self.schema_mgr._upgrade.called)
+        self.assertTrue(self._table_exists("mailinglist"))
+        self.assertTrue(self._table_exists("alembic_version"))
+
+    def test_storm(self):
+        """Existing Storm database"""
+        Model.metadata.create_all(config.db.engine)
+        self._create_storm_version_table(
+                self.schema_mgr.LAST_STORM_SCHEMA_VERSION)
+        self.schema_mgr._create = Mock()
+        self.schema_mgr.setup_db()
+        self.assertFalse(self.schema_mgr._create.called)
+        self.assertTrue(self._table_exists("mailinglist")
+                    and self._table_exists("alembic_version")
+                    and not self._table_exists("version"))
+
+    def test_old_storm(self):
+        """Existing Storm database in an old version"""
+        Model.metadata.create_all(config.db.engine)
+        self._create_storm_version_table("001")
+        self.schema_mgr._create = Mock()
+        self.assertRaises(RuntimeError, self.schema_mgr.setup_db)
+        self.assertFalse(self.schema_mgr._create.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()
+        self.schema_mgr._create = Mock()
+        self.schema_mgr._upgrade = Mock()
+        self.schema_mgr.setup_db()
+        self.assertFalse(self.schema_mgr._create.called)
+        self.assertTrue(self.schema_mgr._upgrade.called)
diff --git a/src/mailman/testing/layers.py b/src/mailman/testing/layers.py
index 4a9841fc2..99852e135 100644
--- a/src/mailman/testing/layers.py
+++ b/src/mailman/testing/layers.py
@@ -47,13 +47,16 @@ import tempfile
 
 from lazr.config import as_boolean
 from pkg_resources import resource_string
+from sqlalchemy import MetaData
 from textwrap import dedent
+from zope import event
 from zope.component import getUtility
 
 from mailman.config import config
 from mailman.core import initialize
 from mailman.core.initialize import INHIBIT_CONFIG_FILE
 from mailman.core.logging import get_handler
+from mailman.database.model import Model
 from mailman.database.transaction import transaction
 from mailman.interfaces.domain import IDomainManager
 from mailman.testing.helpers import (
@@ -98,7 +101,9 @@ class ConfigLayer(MockAndMonkeyLayer):
         # Set up the basic configuration stuff.  Turn off path creation until
         # we've pushed the testing config.
         config.create_paths = False
-        initialize.initialize_1(INHIBIT_CONFIG_FILE)
+        if not event.subscribers:
+            # only if not yet initialized by another layer
+            initialize.initialize_1(INHIBIT_CONFIG_FILE)
         assert cls.var_dir is None, 'Layer already set up'
         # Calculate a temporary VAR_DIR directory so that run-time artifacts
         # of the tests won't tread on the installation's data.  This also
diff --git a/src/mailman/testing/testing.cfg b/src/mailman/testing/testing.cfg
index 39d1d922a..eb9de5513 100644
--- a/src/mailman/testing/testing.cfg
+++ b/src/mailman/testing/testing.cfg
@@ -18,9 +18,9 @@
 # A testing configuration.
 
 # For testing against PostgreSQL.
-# [database]
-# class: mailman.database.postgresql.PostgreSQLDatabase
-# url: postgres://maxking:maxking@localhost/mailman_test
+[database]
+class: mailman.database.postgresql.PostgreSQLDatabase
+url: postgresql://maxking:maxking@localhost/mailman_test
 
 [mailman]
 site_owner: noreply@example.com