From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Sun, 4 Dec 2011 19:25:00 +0000 (-0500)
Subject: - [bug] Fixed bug whereby transaction.rollback()
X-Git-Tag: rel_0_7_4~30
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=632043bc8a72651f497396eb17e6f2b19bf98608;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

- [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 6d4870ab70..af6e748900 100644
--- 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 e350b5bedc..a99814d404 100644
--- 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 82fe55422a..b545aca523 100644
--- 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()