Clean up pass.

5 files changed, 42 insertions, 61 deletions

diff --git a/src/mailman/database/base.py b/src/mailman/database/base.py
index f379b3124..8c426f8cf 100644
--- a/src/mailman/database/base.py
+++ b/src/mailman/database/base.py
@@ -19,28 +19,22 @@ from __future__ import absolute_import, print_function, unicode_literals
 
 __metaclass__ = type
 __all__ = [
-    'StormBaseDatabase',
+    'SABaseDatabase',
     ]
 
 
-import os
-import sys
 import logging
 
-from lazr.config import as_boolean
-from pkg_resources import resource_listdir, resource_string
 from sqlalchemy import create_engine
 from sqlalchemy.orm import sessionmaker
-from sqlalchemy.orm.session import Session
 from zope.interface import implementer
 
 from mailman.config import config
 from mailman.interfaces.database import IDatabase
-from mailman.model.version import Version
 from mailman.utilities.string import expand
 
-log = logging.getLogger('mailman.config')
 
+log = logging.getLogger('mailman.config')
 NL = '\n'
 
 
@@ -53,17 +47,15 @@ class SABaseDatabase:
     """
     # Tag used to distinguish the database being used.  Override this in base
     # classes.
-
     TAG = ''
 
     def __init__(self):
         self.url = None
         self.store = None
-        self.transaction = None
 
     def begin(self):
         """See `IDatabase`."""
-        # SA does this for us.
+        # SQLAlchemy does this for us.
         pass
 
     def commit(self):
@@ -102,9 +94,13 @@ class SABaseDatabase:
         """
         pass
 
+    # XXX Abhilash removed teh _prepare() method.  Is that because SA takes
+    # care of this for us?  If so, then the comment below must be updated.
+    # For reference, the SQLite bug is marked "won't fix".
+
     def initialize(self, debug=None):
-        """See `IDatabase`"""
-        # Calculate the engine url
+        """See `IDatabase`."""
+        # Calculate the engine url.
         url = expand(config.database.url, config.paths)
         log.debug('Database url: %s', url)
         # XXX By design of SQLite, database file creation does not honor
@@ -127,6 +123,9 @@ class SABaseDatabase:
         self.store = session()
         self.store.commit()
 
+    # XXX We should probably rename load_migrations() and perhaps get rid of
+    # load_sql().  The latter is never called any more.
+
     def load_migrations(self, until=None):
         """Load schema migrations.
 
diff --git a/src/mailman/database/factory.py b/src/mailman/database/factory.py
index 64fcc242c..450672e5b 100644
--- a/src/mailman/database/factory.py
+++ b/src/mailman/database/factory.py
@@ -62,6 +62,7 @@ class DatabaseFactory:
 
 def _reset(self):
     """See `IDatabase`."""
+    # Avoid a circular import at module level.
     from mailman.database.model import Model
     self.store.rollback()
     self._pre_reset(self.store)
diff --git a/src/mailman/database/model.py b/src/mailman/database/model.py
index d86ebb80e..f8a15162c 100644
--- a/src/mailman/database/model.py
+++ b/src/mailman/database/model.py
@@ -26,23 +26,31 @@ __all__ = [
 
 
 import contextlib
-from operator import attrgetter
 
 from sqlalchemy.ext.declarative import declarative_base
 
 from mailman.config import config
 
-class ModelMeta(object):
-    """Do more magic on table classes."""
 
+class ModelMeta:
+    """The custom metaclass for all model base classes.
+
+    This is used in the test suite to quickly reset the database after each
+    test.  It works by iterating over all the tables, deleting each.  The test
+    suite will then recreate the tables before each test.
+    """
     @staticmethod
     def _reset(db):
-        meta = Model.metadata
-        engine = config.db.engine
-        with contextlib.closing(engine.connect()) as con:
-            trans = con.begin()
-            for table in reversed(meta.sorted_tables):
-                con.execute(table.delete())
-            trans.commit()
+        with contextlib.closing(config.db.engine.connect()) as connection:
+            transaction = connection.begin()
+            try:
+                for table in reversed(Model.metadata.sorted_tables):
+                    connection.execute(table.delete())
+            except:
+                transaction.abort()
+                raise
+            else:
+                transaction.commit()
+
 
 Model = declarative_base(cls=ModelMeta)
diff --git a/src/mailman/database/sqlite.py b/src/mailman/database/sqlite.py
index 0594d9091..b70e474cc 100644
--- a/src/mailman/database/sqlite.py
+++ b/src/mailman/database/sqlite.py
@@ -56,7 +56,7 @@ class SQLiteDatabase(SABaseDatabase):
         assert parts.scheme == 'sqlite', (
             'Database url mismatch (expected sqlite prefix): {0}'.format(url))
         path = os.path.normpath(parts.path)
-        fd = os.open(path, os.O_WRONLY |  os.O_NONBLOCK | os.O_CREAT, 0666)
+        fd = os.open(path, os.O_WRONLY |  os.O_NONBLOCK | os.O_CREAT, 0o666)
         # Ignore errors
         if fd > 0:
             os.close(fd)
@@ -72,7 +72,7 @@ def _cleanup(self, tempdir):
 def make_temporary(database):
     """Adapts by monkey patching an existing SQLite IDatabase."""
     tempdir = tempfile.mkdtemp()
-    url = 'sqlite:///'   + os.path.join(tempdir, 'mailman.db')
+    url = 'sqlite:///' + os.path.join(tempdir, 'mailman.db')
     with configuration('database', url=url):
         database.initialize()
     database._cleanup = types.MethodType(
diff --git a/src/mailman/database/types.py b/src/mailman/database/types.py
index a6f0b32ca..380ce37dc 100644
--- a/src/mailman/database/types.py
+++ b/src/mailman/database/types.py
@@ -29,30 +29,28 @@ __all__ = [
 import uuid
 
 from sqlalchemy import Integer
-from sqlalchemy.types import TypeDecorator, BINARY, CHAR
 from sqlalchemy.dialects import postgresql
+from sqlalchemy.types import TypeDecorator, BINARY, CHAR
 
 
 
 class Enum(TypeDecorator):
-    """
-    Stores an integer-based Enum as an integer in the database, and converts it
-    on-the-fly.
-    """
+    """Handle Python 3.4 style enums.
 
+    Stores an integer-based Enum as an integer in the database, and
+    converts it on-the-fly.
+    """
     impl = Integer
 
-    def __init__(self, *args, **kw):
-        self.enum = kw.pop("enum")
-        TypeDecorator.__init__(self, *args, **kw)
+    def __init__(self, enum, *args, **kw):
+        self.enum = enum
+        super(Enum, self).__init__(*args, **kw)
 
     def process_bind_param(self, value, dialect):
         if value is None:
             return None
-
         return value.value
 
-
     def process_result_value(self, value, dialect):
         if value is None:
             return None
@@ -61,29 +59,12 @@ class Enum(TypeDecorator):
 
 
 class UUID(TypeDecorator):
-    """
-    Stores a UUID in the database natively when it can and falls back to
-    a BINARY(16) or a CHAR(32) when it can't.
-
-    ::
-
-        from sqlalchemy_utils import UUIDType
-        import uuid
-
-        class User(Base):
-            __tablename__ = 'user'
+    """Handle UUIds."""
 
-            # Pass `binary=False` to fallback to CHAR instead of BINARY
-            id = sa.Column(UUIDType(binary=False), primary_key=True)
-    """
     impl = BINARY(16)
-
     python_type = uuid.UUID
 
     def __init__(self, binary=True, native=True):
-        """
-        :param binary: Whether to use a BINARY(16) or CHAR(32) fallback.
-        """
         self.binary = binary
         self.native = native
 
@@ -91,7 +72,6 @@ class UUID(TypeDecorator):
         if dialect.name == 'postgresql' and self.native:
             # Use the native UUID type.
             return dialect.type_descriptor(postgresql.UUID())
-
         else:
             # Fallback to either a BINARY or a CHAR.
             kind = self.impl if self.binary else CHAR(32)
@@ -102,29 +82,22 @@ class UUID(TypeDecorator):
         if value and not isinstance(value, uuid.UUID):
             try:
                 value = uuid.UUID(value)
-
             except (TypeError, ValueError):
                 value = uuid.UUID(bytes=value)
-
         return value
 
     def process_bind_param(self, value, dialect):
         if value is None:
             return value
-
         if not isinstance(value, uuid.UUID):
             value = self._coerce(value)
-
         if self.native and dialect.name == 'postgresql':
             return str(value)
-
         return value.bytes if self.binary else value.hex
 
     def process_result_value(self, value, dialect):
         if value is None:
             return value
-
         if self.native and dialect.name == 'postgresql':
             return uuid.UUID(value)
-
         return uuid.UUID(bytes=value) if self.binary else uuid.UUID(value)