- [bug] Fixed bug whereby transaction.rollback()

would throw an error on an invalidated
connection if the transaction were a
two-phase or savepoint transaction.
For plain transactions, rollback() is a no-op
if the connection is invalidated, so while
it wasn't 100% clear if it should be a no-op,
at least now the interface is consistent.
[ticket:2317]

1 files changed, 15 insertions, 9 deletions

diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index e350b5bed..a99814d40 100644
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -1031,6 +1031,13 @@ class Connection(Connectable):
         return getattr(self.__connection, 'is_valid', False)
 
     @property
+    def _still_open_and_connection_is_valid(self):
+        return \
+            not self.closed and \
+            not self.invalidated and \
+            getattr(self.__connection, 'is_valid', False)
+
+    @property
     def info(self):
         """A collection of per-DB-API connection instance properties."""
 
@@ -1204,8 +1211,7 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.rollback(self)
 
-        if not self.closed and not self.invalidated and \
-                        self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             if self._echo:
                 self.engine.logger.info("ROLLBACK")
             try:
@@ -1237,7 +1243,7 @@ class Connection(Connectable):
         if name is None:
             self.__savepoint_seq += 1
             name = 'sa_savepoint_%s' % self.__savepoint_seq
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             self.engine.dialect.do_savepoint(self, name)
             return name
 
@@ -1245,7 +1251,7 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.rollback_savepoint(self, name, context)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             self.engine.dialect.do_rollback_to_savepoint(self, name)
         self.__transaction = context
 
@@ -1253,7 +1259,7 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.release_savepoint(self, name, context)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             self.engine.dialect.do_release_savepoint(self, name)
         self.__transaction = context
 
@@ -1261,14 +1267,14 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.begin_twophase(self, xid)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             self.engine.dialect.do_begin_twophase(self, xid)
 
     def _prepare_twophase_impl(self, xid):
         if self._has_events:
             self.engine.dispatch.prepare_twophase(self, xid)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_prepare_twophase(self, xid)
 
@@ -1276,7 +1282,7 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.rollback_twophase(self, xid, is_prepared)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_rollback_twophase(self, xid, is_prepared)
         self.__transaction = None
@@ -1285,7 +1291,7 @@ class Connection(Connectable):
         if self._has_events:
             self.engine.dispatch.commit_twophase(self, xid, is_prepared)
 
-        if self._connection_is_valid:
+        if self._still_open_and_connection_is_valid:
             assert isinstance(self.__transaction, TwoPhaseTransaction)
             self.engine.dialect.do_commit_twophase(self, xid, is_prepared)
         self.__transaction = None