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

Proposed by Colin Watson on 2019-06-25
Status: Merged
Merged at revision: 509
Proposed branch: lp:~cjwatson/storm/refactor-exception-wrapping
Merge into: lp:storm
Diff against target: 423 lines (+228/-31)
6 files modified
NEWS (+4/-0)
storm/database.py (+132/-1)
storm/databases/postgres.py (+28/-7)
storm/databases/sqlite.py (+15/-9)
storm/exceptions.py (+46/-14)
tests/database.py (+3/-0)
To merge this branch: bzr merge lp:~cjwatson/storm/refactor-exception-wrapping
Reviewer Review Type Date Requested Status
Simon Poirier 2019-06-25 Approve on 2019-07-02
Review via email: mp+369319@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
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) wrote :

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) wrote :

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

review: Approve
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.

Colin Watson (cjwatson) :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2019-06-05 11:41:07 +0000
3+++ NEWS 2019-07-02 15:55:56 +0000
4@@ -8,6 +8,10 @@
5 package, which can apply patches "in parallel" against a set of stores. See
6 the module docstring for more information.
7 - Added tox testing support.
8+- Re-raise DB-API exceptions wrapped in exception types that inherit from
9+ both StormError and the appropriate DB-API exception type, rather than
10+ injecting virtual base classes. This preserves existing exception handling
11+ in applications while also being a viable approach in Python 3.
12
13 Bug fixes
14 ---------
15
16=== modified file 'storm/database.py'
17--- storm/database.py 2019-06-05 11:41:07 +0000
18+++ storm/database.py 2019-07-02 15:55:56 +0000
19@@ -27,6 +27,12 @@
20
21 from __future__ import print_function
22
23+try:
24+ from collections.abc import Callable
25+except ImportError:
26+ from collections import Callable
27+from functools import wraps
28+
29 from storm.expr import Expr, State, compile
30 # Circular import: imported at the end of the module.
31 # from storm.tracer import trace
32@@ -34,7 +40,7 @@
33 from storm.xid import Xid
34 from storm.exceptions import (
35 ClosedError, ConnectionBlockedError, DatabaseError, DisconnectionError,
36- Error, ProgrammingError)
37+ Error, ProgrammingError, wrap_exceptions)
38 from storm.uri import URI
39 import storm
40
41@@ -163,6 +169,78 @@
42 return row
43
44
45+class CursorWrapper(object):
46+ """A DB-API cursor, wrapping exceptions as StormError instances."""
47+
48+ def __init__(self, cursor, database):
49+ super(CursorWrapper, self).__setattr__('_cursor', cursor)
50+ super(CursorWrapper, self).__setattr__('_database', database)
51+
52+ def __getattr__(self, name):
53+ attr = getattr(self._cursor, name)
54+ if isinstance(attr, Callable):
55+ @wraps(attr)
56+ def wrapper(*args, **kwargs):
57+ with wrap_exceptions(self._database):
58+ return attr(*args, **kwargs)
59+
60+ return wrapper
61+ else:
62+ return attr
63+
64+ def __setattr__(self, name, value):
65+ return setattr(self._cursor, name, value)
66+
67+ def __iter__(self):
68+ with wrap_exceptions(self._database):
69+ for item in self._cursor:
70+ yield item
71+
72+ def __enter__(self):
73+ return self
74+
75+ def __exit__(self, type_, value, tb):
76+ with wrap_exceptions(self._database):
77+ self.close()
78+
79+
80+class ConnectionWrapper(object):
81+ """A DB-API connection, wrapping exceptions as StormError instances."""
82+
83+ def __init__(self, connection, database):
84+ self.__dict__['_connection'] = connection
85+ self.__dict__['_database'] = database
86+
87+ def __getattr__(self, name):
88+ attr = getattr(self._connection, name)
89+ if isinstance(attr, Callable):
90+ @wraps(attr)
91+ def wrapper(*args, **kwargs):
92+ with wrap_exceptions(self._database):
93+ return attr(*args, **kwargs)
94+
95+ return wrapper
96+ else:
97+ return attr
98+
99+ def __setattr__(self, name, value):
100+ return setattr(self._connection, name, value)
101+
102+ def __enter__(self):
103+ return self
104+
105+ def __exit__(self, type_, value, tb):
106+ with wrap_exceptions(self._database):
107+ if type_ is None and value is None and tb is None:
108+ self.commit()
109+ else:
110+ self.rollback()
111+
112+ def cursor(self):
113+ with wrap_exceptions(self._database):
114+ return CursorWrapper(self._connection.cursor(), self._database)
115+
116+
117 class Connection(object):
118 """A connection to a database.
119
120@@ -483,6 +561,7 @@
121
122 def __init__(self, uri=None):
123 self._uri = uri
124+ self._exception_types = {}
125
126 def get_uri(self):
127 """Return the URI object this database was created with."""
128@@ -512,6 +591,58 @@
129 """
130 raise NotImplementedError
131
132+ @property
133+ def _exception_module(self):
134+ """The module where appropriate DB-API exception types are defined.
135+
136+ Subclasses should set this if they support re-raising DB-API
137+ exceptions as StormError instances.
138+ """
139+ return None
140+
141+ def _make_combined_exception_type(self, wrapper_type, dbapi_type):
142+ """Make a combined exception based on both DB-API and Storm.
143+
144+ Storm historically defined its own exception types as ABCs and
145+ registered the DB-API exception types as virtual subclasses.
146+ However, this doesn't work properly in Python 3
147+ (https://bugs.python.org/issue12029). Instead, we create and cache
148+ subclass-specific exception types that inherit from both StormError
149+ and the DB-API exception type, allowing code that catches either
150+ StormError (or subclasses) or the specific DB-API exceptions to keep
151+ working.
152+
153+ @type wrapper_type: L{type}
154+ @param wrapper_type: The type of the wrapper exception to create; a
155+ subclass of L{StormError}.
156+ @type dbapi_type: L{type}
157+ @param dbapi_type: The type of the DB-API exception.
158+
159+ @return: The combined exception type.
160+ """
161+ if wrapper_type not in self._exception_types:
162+ self._exception_types[wrapper_type] = type(
163+ dbapi_type.__name__, (dbapi_type, wrapper_type), {})
164+ return self._exception_types[wrapper_type]
165+
166+ def _wrap_exception(self, wrapper_type, exception):
167+ """Wrap a DB-API exception as a StormError instance.
168+
169+ This constructs a wrapper exception with the same C{args} as the
170+ DB-API exception. Subclasses may override this to set additional
171+ attributes on the wrapper exception.
172+
173+ @type wrapper_type: L{type}
174+ @param wrapper_type: The type of the wrapper exception to create; a
175+ subclass of L{StormError}.
176+ @type exception: L{Exception}
177+ @param exception: The DB-API exception to wrap.
178+
179+ @return: The wrapped exception; an instance of L{StormError}.
180+ """
181+ return self._make_combined_exception_type(
182+ wrapper_type, exception.__class__)(*exception.args)
183+
184
185 def convert_param_marks(statement, from_param_mark, to_param_mark):
186 # TODO: Add support for $foo$bar$foo$ literals.
187
188=== modified file 'storm/databases/postgres.py'
189--- storm/databases/postgres.py 2019-06-05 11:41:07 +0000
190+++ storm/databases/postgres.py 2019-07-02 15:55:56 +0000
191@@ -50,14 +50,13 @@
192 from storm.variables import (
193 Variable, ListVariable, JSONVariable as BaseJSONVariable)
194 from storm.properties import SimpleProperty
195-from storm.database import Database, Connection, Result
196+from storm.database import Database, Connection, ConnectionWrapper, Result
197 from storm.exceptions import (
198- install_exceptions, DatabaseError, DatabaseModuleError, InterfaceError,
199- OperationalError, ProgrammingError, TimeoutError, Error)
200+ DatabaseError, DatabaseModuleError, InterfaceError,
201+ OperationalError, ProgrammingError, TimeoutError, Error, wrap_exceptions)
202 from storm.tracer import TimeoutTracer
203
204
205-install_exceptions(psycopg2)
206 compile = compile.create_child()
207
208
209@@ -366,6 +365,8 @@
210
211 connection_factory = PostgresConnection
212
213+ _exception_module = psycopg2
214+
215 # An integer representing the server version. If the server does
216 # not support the server_version_num variable, this will be set to
217 # 0. In practice, this means the variable will be 0 or greater
218@@ -397,15 +398,31 @@
219 "'autocommit', 'serializable', 'read-committed'" %
220 (isolation,))
221
222- def raw_connect(self):
223- raw_connection = psycopg2.connect(self._dsn)
224+ _psycopg_error_attributes = ("pgerror", "pgcode", "cursor", "diag")
225+
226+ def _make_combined_exception_type(self, wrapper_type, dbapi_type):
227+ combined_type = super(Postgres, self)._make_combined_exception_type(
228+ wrapper_type, dbapi_type)
229+ for name in self._psycopg_error_attributes:
230+ setattr(combined_type, name, lambda err: getattr(err, "_" + name))
231+ return combined_type
232+
233+ def _wrap_exception(self, wrapper_type, exception):
234+ wrapped = super(Postgres, self)._wrap_exception(
235+ wrapper_type, exception)
236+ for name in self._psycopg_error_attributes:
237+ setattr(wrapped, "_" + name, getattr(exception, name))
238+ return wrapped
239+
240+ def _raw_connect(self):
241+ raw_connection = ConnectionWrapper(psycopg2.connect(self._dsn), self)
242
243 if self._version is None:
244 cursor = raw_connection.cursor()
245
246 try:
247 cursor.execute("SHOW server_version_num")
248- except psycopg2.ProgrammingError:
249+ except ProgrammingError:
250 self._version = 0
251 else:
252 self._version = int(cursor.fetchone()[0])
253@@ -415,6 +432,10 @@
254 raw_connection.set_isolation_level(self._isolation)
255 return raw_connection
256
257+ def raw_connect(self):
258+ with wrap_exceptions(self):
259+ return self._raw_connect()
260+
261
262 create_from_uri = Postgres
263
264
265=== modified file 'storm/databases/sqlite.py'
266--- storm/databases/sqlite.py 2019-06-07 12:17:35 +0000
267+++ storm/databases/sqlite.py 2019-07-02 15:55:56 +0000
268@@ -35,16 +35,14 @@
269 sqlite = dummy
270
271 from storm.variables import Variable, RawStrVariable
272-from storm.database import Database, Connection, Result
273-from storm.exceptions import install_exceptions, DatabaseModuleError
274+from storm.database import Database, Connection, ConnectionWrapper, Result
275+from storm.exceptions import (
276+ DatabaseModuleError, OperationalError, wrap_exceptions)
277 from storm.expr import (
278 Insert, Select, SELECT, Undef, SQLRaw, Union, Except, Intersect,
279 compile, compile_insert, compile_select)
280
281
282-install_exceptions(sqlite)
283-
284-
285 compile = compile.create_child()
286
287 @compile.when(Select)
288@@ -160,7 +158,7 @@
289 while True:
290 try:
291 return Connection.raw_execute(self, statement, params)
292- except sqlite.OperationalError as e:
293+ except OperationalError as e:
294 if str(e) != "database is locked":
295 raise
296 elif now() - started < self._database._timeout:
297@@ -180,6 +178,8 @@
298
299 connection_factory = SQLiteConnection
300
301+ _exception_module = sqlite
302+
303 def __init__(self, uri):
304 super(SQLite, self).__init__(uri)
305 if sqlite is dummy:
306@@ -190,10 +190,12 @@
307 self._journal_mode = uri.options.get("journal_mode")
308 self._foreign_keys = uri.options.get("foreign_keys")
309
310- def raw_connect(self):
311+ def _raw_connect(self):
312 # See the story at the end to understand why we set isolation_level.
313- raw_connection = sqlite.connect(self._filename, timeout=self._timeout,
314- isolation_level=None)
315+ raw_connection = ConnectionWrapper(
316+ sqlite.connect(
317+ self._filename, timeout=self._timeout, isolation_level=None),
318+ self)
319 if self._synchronous is not None:
320 raw_connection.execute("PRAGMA synchronous = %s" %
321 (self._synchronous,))
322@@ -208,6 +210,10 @@
323
324 return raw_connection
325
326+ def raw_connect(self):
327+ with wrap_exceptions(self):
328+ return self._raw_connect()
329+
330
331 create_from_uri = SQLite
332
333
334=== modified file 'storm/exceptions.py'
335--- storm/exceptions.py 2019-06-07 16:35:06 +0000
336+++ storm/exceptions.py 2019-07-02 15:55:56 +0000
337@@ -20,12 +20,14 @@
338 #
339 from __future__ import print_function
340
341-from abc import ABCMeta
342-import types
343+from contextlib import contextmanager
344+import sys
345+
346+import six
347
348
349 class StormError(Exception):
350- __metaclass__ = ABCMeta
351+ pass
352
353
354 class CompileError(StormError):
355@@ -140,14 +142,44 @@
356 """Raised when an attempt is made to use a blocked connection."""
357
358
359-def install_exceptions(module):
360- for exception in (Error, Warning, DatabaseError, InternalError,
361- OperationalError, ProgrammingError, IntegrityError,
362- DataError, NotSupportedError, InterfaceError):
363- module_exception = getattr(module, exception.__name__, None)
364- if (module_exception is not None and
365- isinstance(module_exception, (type, types.ClassType))):
366- # XXX This may need to be revisited when porting to Python 3 if
367- # virtual subclasses are still ignored for exception handling
368- # (https://bugs.python.org/issue12029).
369- exception.register(module_exception)
370+# More generic exceptions must come later. For convenience, use the order
371+# of definition above and then reverse it.
372+_wrapped_exception_types = tuple(reversed((
373+ Error,
374+ Warning,
375+ InterfaceError,
376+ DatabaseError,
377+ InternalError,
378+ OperationalError,
379+ ProgrammingError,
380+ IntegrityError,
381+ DataError,
382+ NotSupportedError,
383+ )))
384+
385+
386+@contextmanager
387+def wrap_exceptions(database):
388+ """Context manager that re-raises DB exceptions as StormError instances."""
389+ try:
390+ yield
391+ except Exception as e:
392+ module = database._exception_module
393+ if module is None:
394+ # This backend does not support re-raising database exceptions.
395+ raise
396+ for wrapper_type in _wrapped_exception_types:
397+ dbapi_type = getattr(module, wrapper_type.__name__, None)
398+ if (dbapi_type is not None and
399+ isinstance(dbapi_type, six.class_types) and
400+ isinstance(e, dbapi_type)):
401+ wrapped = database._wrap_exception(wrapper_type, e)
402+ tb = sys.exc_info()[2]
403+ # As close to "raise wrapped.with_traceback(tb) from e" as
404+ # we can manage, but without causing syntax errors on
405+ # various versions of Python.
406+ if six.PY2:
407+ six.reraise(wrapped, None, tb)
408+ else:
409+ six.raise_from(wrapped.with_traceback(tb), e)
410+ raise
411
412=== modified file 'tests/database.py'
413--- tests/database.py 2019-06-05 11:41:07 +0000
414+++ tests/database.py 2019-07-02 15:55:56 +0000
415@@ -96,6 +96,9 @@
416
417 class FakeConnection(object):
418
419+ def __init__(self):
420+ self._database = Database()
421+
422 def _check_disconnect(self, _function, *args, **kwargs):
423 return _function(*args, **kwargs)
424

Subscribers

People subscribed via source and target branches

to status/vote changes: