Merge lp:~cjwatson/storm/refactor-exception-wrapping into lp:storm

[Colin Watson]

Wrap DB-API exceptions rather than injecting virtual base classes.

Re-raise DB-API exceptions wrapped in exception types that inherit from both StormError and the appropriate DB-API exception type, rather than injecting virtual base classes. This preserves existing exception handling in applications while also being a viable approach in Python 3.

Once I managed to get the right wrappers in place, this ended up being surprisingly simple, which makes me feel good about it (my first draft had many more explicit calls to wrap_exceptions and so felt much more fragile). The main difficulty was in creating just the right wrapper exception types to maintain API compatibility without making the code painfully repetitive, and I finally got that to fall into place today.

I'm running the full Launchpad test suite over this at the moment, though it looks clean so far and I've already found and fixed some problems via some of its most Storm-sensitive tests.

[lp:~cjwatson/storm/refactor-exception-wrapping]

509. By Colin Watson on 2019-06-26

Make wrapper exceptions look slightly more like the DB-API originals.

510. By Colin Watson on 2019-06-26

Improve comment.

[Colin Watson (cjwatson)]

I've done a full successful Launchpad test run with this patch. The only change I had to make to Launchpad was to adjust LaunchpadDatabase.__init__, which is just because it's subclassing Postgres while (for unrelated reasons) not running the superclass constructor, so as a consequence it has to keep up with changes to the superclass constructor, which I think is fine.

[Simon Poirier (simpoir)]

+1
I ran the full landscape test suite against this without any apparent regression.

review: Approve

[lp:~cjwatson/storm/refactor-exception-wrapping]

511. By Colin Watson on 2019-07-02

Use superclass __setattr__ rather than changing __dict__ directly.

512. By Colin Watson on 2019-07-02

Use more neutral language.

=== modified file 'NEWS'
--- NEWS	2019-06-05 11:41:07 +0000
+++ NEWS	2019-07-02 15:55:56 +0000
@@ -8,6 +8,10 @@
   package, which can apply patches "in parallel" against a set of stores. See
   the module docstring for more information.
 - Added tox testing support.
+- Re-raise DB-API exceptions wrapped in exception types that inherit from
+  both StormError and the appropriate DB-API exception type, rather than
+  injecting virtual base classes. This preserves existing exception handling
+  in applications while also being a viable approach in Python 3.
 
 Bug fixes
 ---------

=== modified file 'storm/database.py'
--- storm/database.py	2019-06-05 11:41:07 +0000
+++ storm/database.py	2019-07-02 15:55:56 +0000
@@ -27,6 +27,12 @@
 
 from __future__ import print_function
 
+try:
+    from collections.abc import Callable
+except ImportError:
+    from collections import Callable
+from functools import wraps
+
 from storm.expr import Expr, State, compile
 # Circular import: imported at the end of the module.
 # from storm.tracer import trace
@@ -34,7 +40,7 @@
 from storm.xid import Xid
 from storm.exceptions import (
     ClosedError, ConnectionBlockedError, DatabaseError, DisconnectionError,
-    Error, ProgrammingError)
+    Error, ProgrammingError, wrap_exceptions)
 from storm.uri import URI
 import storm
 
@@ -163,6 +169,78 @@
         return row
 
 
+class CursorWrapper(object):
+    """A DB-API cursor, wrapping exceptions as StormError instances."""
+
+    def __init__(self, cursor, database):
+        super(CursorWrapper, self).__setattr__('_cursor', cursor)
+        super(CursorWrapper, self).__setattr__('_database', database)
+
+    def __getattr__(self, name):
+        attr = getattr(self._cursor, name)
+        if isinstance(attr, Callable):
+            @wraps(attr)
+            def wrapper(*args, **kwargs):
+                with wrap_exceptions(self._database):
+                    return attr(*args, **kwargs)
+
+            return wrapper
+        else:
+            return attr
+
+    def __setattr__(self, name, value):
+        return setattr(self._cursor, name, value)
+
+    def __iter__(self):
+        with wrap_exceptions(self._database):
+            for item in self._cursor:
+                yield item
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, type_, value, tb):
+        with wrap_exceptions(self._database):
+            self.close()
+
+
+class ConnectionWrapper(object):
+    """A DB-API connection, wrapping exceptions as StormError instances."""
+
+    def __init__(self, connection, database):
+        self.__dict__['_connection'] = connection
+        self.__dict__['_database'] = database
+
+    def __getattr__(self, name):
+        attr = getattr(self._connection, name)
+        if isinstance(attr, Callable):
+            @wraps(attr)
+            def wrapper(*args, **kwargs):
+                with wrap_exceptions(self._database):
+                    return attr(*args, **kwargs)
+
+            return wrapper
+        else:
+            return attr
+
+    def __setattr__(self, name, value):
+        return setattr(self._connection, name, value)
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, type_, value, tb):
+        with wrap_exceptions(self._database):
+            if type_ is None and value is None and tb is None:
+                self.commit()
+            else:
+                self.rollback()
+
+    def cursor(self):
+        with wrap_exceptions(self._database):
+            return CursorWrapper(self._connection.cursor(), self._database)
+
+
 class Connection(object):
     """A connection to a database.
 
@@ -483,6 +561,7 @@
 
     def __init__(self, uri=None):
         self._uri = uri
+        self._exception_types = {}
 
     def get_uri(self):
         """Return the URI object this database was created with."""
@@ -512,6 +591,58 @@
         """
         raise NotImplementedError
 
+    @property
+    def _exception_module(self):
+        """The module where appropriate DB-API exception types are defined.
+
+        Subclasses should set this if they support re-raising DB-API
+        exceptions as StormError instances.
+        """
+        return None
+
+    def _make_combined_exception_type(self, wrapper_type, dbapi_type):
+        """Make a combined exception based on both DB-API and Storm.
+
+        Storm historically defined its own exception types as ABCs and
+        registered the DB-API exception types as virtual subclasses.
+        However, this doesn't work properly in Python 3
+        (https://bugs.python.org/issue12029).  Instead, we create and cache
+        subclass-specific exception types that inherit from both StormError
+        and the DB-API exception type, allowing code that catches either
+        StormError (or subclasses) or the specific DB-API exceptions to keep
+        working.
+
+        @type wrapper_type: L{type}
+        @param wrapper_type: The type of the wrapper exception to create; a
+            subclass of L{StormError}.
+        @type dbapi_type: L{type}
+        @param dbapi_type: The type of the DB-API exception.
+
+        @return: The combined exception type.
+        """
+        if wrapper_type not in self._exception_types:
+            self._exception_types[wrapper_type] = type(
+                dbapi_type.__name__, (dbapi_type, wrapper_type), {})
+        return self._exception_types[wrapper_type]
+
+    def _wrap_exception(self, wrapper_type, exception):
+        """Wrap a DB-API exception as a StormError instance.
+
+        This constructs a wrapper exception with the same C{args} as the
+        DB-API exception.  Subclasses may override this to set additional
+        attributes on the wrapper exception.
+
+        @type wrapper_type: L{type}
+        @param wrapper_type: The type of the wrapper exception to create; a
+            subclass of L{StormError}.
+        @type exception: L{Exception}
+        @param exception: The DB-API exception to wrap.
+
+        @return: The wrapped exception; an instance of L{StormError}.
+        """
+        return self._make_combined_exception_type(
+            wrapper_type, exception.__class__)(*exception.args)
+
 
 def convert_param_marks(statement, from_param_mark, to_param_mark):
     # TODO: Add support for $foo$bar$foo$ literals.

=== modified file 'storm/databases/postgres.py'
--- storm/databases/postgres.py	2019-06-05 11:41:07 +0000
+++ storm/databases/postgres.py	2019-07-02 15:55:56 +0000
@@ -50,14 +50,13 @@
 from storm.variables import (
     Variable, ListVariable, JSONVariable as BaseJSONVariable)
 from storm.properties import SimpleProperty
-from storm.database import Database, Connection, Result
+from storm.database import Database, Connection, ConnectionWrapper, Result
 from storm.exceptions import (
-    install_exceptions, DatabaseError, DatabaseModuleError, InterfaceError,
-    OperationalError, ProgrammingError, TimeoutError, Error)
+    DatabaseError, DatabaseModuleError, InterfaceError,
+    OperationalError, ProgrammingError, TimeoutError, Error, wrap_exceptions)
 from storm.tracer import TimeoutTracer
 
 
-install_exceptions(psycopg2)
 compile = compile.create_child()
 
 
@@ -366,6 +365,8 @@
 
     connection_factory = PostgresConnection
 
+    _exception_module = psycopg2
+
     # An integer representing the server version.  If the server does
     # not support the server_version_num variable, this will be set to
     # 0.  In practice, this means the variable will be 0 or greater
@@ -397,15 +398,31 @@
                 "'autocommit', 'serializable', 'read-committed'" %
                 (isolation,))
 
-    def raw_connect(self):
-        raw_connection = psycopg2.connect(self._dsn)
+    _psycopg_error_attributes = ("pgerror", "pgcode", "cursor", "diag")
+
+    def _make_combined_exception_type(self, wrapper_type, dbapi_type):
+        combined_type = super(Postgres, self)._make_combined_exception_type(
+            wrapper_type, dbapi_type)
+        for name in self._psycopg_error_attributes:
+            setattr(combined_type, name, lambda err: getattr(err, "_" + name))
+        return combined_type
+
+    def _wrap_exception(self, wrapper_type, exception):
+        wrapped = super(Postgres, self)._wrap_exception(
+            wrapper_type, exception)
+        for name in self._psycopg_error_attributes:
+            setattr(wrapped, "_" + name, getattr(exception, name))
+        return wrapped
+
+    def _raw_connect(self):
+        raw_connection = ConnectionWrapper(psycopg2.connect(self._dsn), self)
 
         if self._version is None:
             cursor = raw_connection.cursor()
 
             try:
                 cursor.execute("SHOW server_version_num")
-            except psycopg2.ProgrammingError:
+            except ProgrammingError:
                 self._version = 0
             else:
                 self._version = int(cursor.fetchone()[0])
@@ -415,6 +432,10 @@
         raw_connection.set_isolation_level(self._isolation)
         return raw_connection
 
+    def raw_connect(self):
+        with wrap_exceptions(self):
+            return self._raw_connect()
+
 
 create_from_uri = Postgres
 

=== modified file 'storm/databases/sqlite.py'
--- storm/databases/sqlite.py	2019-06-07 12:17:35 +0000
+++ storm/databases/sqlite.py	2019-07-02 15:55:56 +0000
@@ -35,16 +35,14 @@
         sqlite = dummy
 
 from storm.variables import Variable, RawStrVariable
-from storm.database import Database, Connection, Result
-from storm.exceptions import install_exceptions, DatabaseModuleError
+from storm.database import Database, Connection, ConnectionWrapper, Result
+from storm.exceptions import (
+    DatabaseModuleError, OperationalError, wrap_exceptions)
 from storm.expr import (
     Insert, Select, SELECT, Undef, SQLRaw, Union, Except, Intersect,
     compile, compile_insert, compile_select)
 
 
-install_exceptions(sqlite)
-
-
 compile = compile.create_child()
 
 @compile.when(Select)
@@ -160,7 +158,7 @@
         while True:
             try:
                 return Connection.raw_execute(self, statement, params)
-            except sqlite.OperationalError as e:
+            except OperationalError as e:
                 if str(e) != "database is locked":
                     raise
                 elif now() - started < self._database._timeout:
@@ -180,6 +178,8 @@
 
     connection_factory = SQLiteConnection
 
+    _exception_module = sqlite
+
     def __init__(self, uri):
         super(SQLite, self).__init__(uri)
         if sqlite is dummy:
@@ -190,10 +190,12 @@
         self._journal_mode = uri.options.get("journal_mode")
         self._foreign_keys = uri.options.get("foreign_keys")
 
-    def raw_connect(self):
+    def _raw_connect(self):
         # See the story at the end to understand why we set isolation_level.
-        raw_connection = sqlite.connect(self._filename, timeout=self._timeout,
-                                        isolation_level=None)
+        raw_connection = ConnectionWrapper(
+            sqlite.connect(
+                self._filename, timeout=self._timeout, isolation_level=None),
+            self)
         if self._synchronous is not None:
             raw_connection.execute("PRAGMA synchronous = %s" %
                                    (self._synchronous,))
@@ -208,6 +210,10 @@
 
         return raw_connection
 
+    def raw_connect(self):
+        with wrap_exceptions(self):
+            return self._raw_connect()
+
 
 create_from_uri = SQLite
 

=== modified file 'storm/exceptions.py'
--- storm/exceptions.py	2019-06-07 16:35:06 +0000
+++ storm/exceptions.py	2019-07-02 15:55:56 +0000
@@ -20,12 +20,14 @@
 #
 from __future__ import print_function
 
-from abc import ABCMeta
-import types
+from contextlib import contextmanager
+import sys
+
+import six
 
 
 class StormError(Exception):
-    __metaclass__ = ABCMeta
+    pass
 
 
 class CompileError(StormError):
@@ -140,14 +142,44 @@
     """Raised when an attempt is made to use a blocked connection."""
 
 
-def install_exceptions(module):
-    for exception in (Error, Warning, DatabaseError, InternalError,
-                      OperationalError, ProgrammingError, IntegrityError,
-                      DataError, NotSupportedError, InterfaceError):
-        module_exception = getattr(module, exception.__name__, None)
-        if (module_exception is not None and
-            isinstance(module_exception, (type, types.ClassType))):
-            # XXX This may need to be revisited when porting to Python 3 if
-            # virtual subclasses are still ignored for exception handling
-            # (https://bugs.python.org/issue12029).
-            exception.register(module_exception)
+# More generic exceptions must come later.  For convenience, use the order
+# of definition above and then reverse it.
+_wrapped_exception_types = tuple(reversed((
+    Error,
+    Warning,
+    InterfaceError,
+    DatabaseError,
+    InternalError,
+    OperationalError,
+    ProgrammingError,
+    IntegrityError,
+    DataError,
+    NotSupportedError,
+    )))
+
+
+@contextmanager
+def wrap_exceptions(database):
+    """Context manager that re-raises DB exceptions as StormError instances."""
+    try:
+        yield
+    except Exception as e:
+        module = database._exception_module
+        if module is None:
+            # This backend does not support re-raising database exceptions.
+            raise
+        for wrapper_type in _wrapped_exception_types:
+            dbapi_type = getattr(module, wrapper_type.__name__, None)
+            if (dbapi_type is not None and
+                    isinstance(dbapi_type, six.class_types) and
+                    isinstance(e, dbapi_type)):
+                wrapped = database._wrap_exception(wrapper_type, e)
+                tb = sys.exc_info()[2]
+                # As close to "raise wrapped.with_traceback(tb) from e" as
+                # we can manage, but without causing syntax errors on
+                # various versions of Python.
+                if six.PY2:
+                    six.reraise(wrapped, None, tb)
+                else:
+                    six.raise_from(wrapped.with_traceback(tb), e)
+        raise

=== modified file 'tests/database.py'
--- tests/database.py	2019-06-05 11:41:07 +0000
+++ tests/database.py	2019-07-02 15:55:56 +0000
@@ -96,6 +96,9 @@
 
 class FakeConnection(object):
 
+    def __init__(self):
+        self._database = Database()
+
     def _check_disconnect(self, _function, *args, **kwargs):
         return _function(*args, **kwargs)