- [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]

diff --git a/CHANGES b/CHANGES
index 6d4870ab7025ac46c46deb2927e1fba3668aad6b..af6e748900f880e3b64fdbfa48af0a3bfed2d8e4 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -140,6 +140,17 @@ CHANGES
      expected behavior, which changed as a result
      of [ticket:2261].  [ticket:2319]
 
+- engine
+  - [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]
+
 - schema
   - [feature] Added new support for remote "schemas":
     - MetaData() accepts "schema" and "quote_schema"

diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py
index e350b5bedc967e7f00a19703364c07668fc0fa29..a99814d404a4b3c29bf3ea619b8d6e3b9bb29515 100644 (file)
--- a/lib/sqlalchemy/engine/base.py
+++ b/lib/sqlalchemy/engine/base.py
@@ -1030,6 +1030,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

diff --git a/test/engine/test_reconnect.py b/test/engine/test_reconnect.py
index 82fe55422abdcba7f1b38ee50cc04836d35de87f..b545aca523bfa9ebdb07b6875fb5dd1cb2c13f0f 100644 (file)
--- a/test/engine/test_reconnect.py
+++ b/test/engine/test_reconnect.py
@@ -280,6 +280,27 @@ class RealReconnectTest(fixtures.TestBase):
             conn.execute, select([1])
         )
 
+    def test_rollback_on_invalid_plain(self):
+        conn = engine.connect()
+        trans = conn.begin()
+        conn.invalidate()
+        trans.rollback()
+
+    @testing.requires.two_phase_transactions
+    def test_rollback_on_invalid_twophase(self):
+        conn = engine.connect()
+        trans = conn.begin_twophase()
+        conn.invalidate()
+        trans.rollback()
+
+    @testing.requires.savepoints
+    def test_rollback_on_invalid_savepoint(self):
+        conn = engine.connect()
+        trans = conn.begin()
+        trans2 = conn.begin_nested()
+        conn.invalidate()
+        trans2.rollback()
+
     def test_invalidate_twice(self):
         conn = engine.connect()
         conn.invalidate()