- [bug] Fixed issue where modified session state

established after a failed flush would be committed
as part of the subsequent transaction that
begins automatically after manual call
to rollback().   The state of the session is
checked within rollback(), and if new state
is present, a warning is emitted and
restore_snapshot() is called a second time,
discarding those changes. [ticket:2389]

- repaired testing.assert_warnings to also verify
that any warnings were emitted

diff --git a/CHANGES b/CHANGES
index ba7327399779eb492b751adbd4d35097abdd7888..1859b91459be0378ba4198497bd14812a8937c03 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -6,6 +6,16 @@ CHANGES
 0.7.5
 =====
 - orm
+  - [bug] Fixed issue where modified session state
+    established after a failed flush would be committed
+    as part of the subsequent transaction that 
+    begins automatically after manual call 
+    to rollback().   The state of the session is
+    checked within rollback(), and if new state
+    is present, a warning is emitted and 
+    restore_snapshot() is called a second time,
+    discarding those changes. [ticket:2389]
+
   - [feature] Added "class_registry" argument to
     declarative_base().  Allows two or more declarative
     bases to share the same registry of class names.

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index eb03a5b819b3fb2ca1e35b7229ec0adbfc134237..4299290d0d5fa17dbe9b6bfc63330d1b50aa714d 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -343,6 +343,16 @@ class SessionTransaction(object):
 
         sess = self.session
 
+        if self.session._enable_transaction_accounting and \
+            not sess._is_clean():
+            # if items were added, deleted, or mutated
+            # here, we need to re-restore the snapshot
+            util.warn(
+                    "Session's state has been changed on "
+                    "a non-active transaction - this state "
+                    "will be discarded.")
+            self._restore_snapshot()
+
         self.close()
         if self._parent and _capture_exception:
             self._parent._rollback_exception = sys.exc_info()[1]
@@ -575,7 +585,7 @@ class Session(object):
         if self.transaction is not None:
             if subtransactions or nested:
                 self.transaction = self.transaction._begin(
-                    nested=nested)
+                                        nested=nested)
             else:
                 raise sa_exc.InvalidRequestError(
                     "A transaction is already begun.  Use subtransactions=True "
@@ -1542,16 +1552,20 @@ class Session(object):
         if self._flushing:
             raise sa_exc.InvalidRequestError("Session is already flushing")
 
+        if self._is_clean():
+            return
         try:
             self._flushing = True
             self._flush(objects)
         finally:
             self._flushing = False
 
+    def _is_clean(self):
+        return not self.identity_map.check_modified() and \
+                not self._deleted and \
+                not self._new
+
     def _flush(self, objects=None):
-        if (not self.identity_map.check_modified() and
-            not self._deleted and not self._new):
-            return
 
         dirty = self._dirty_states
         if not dirty and not self._deleted and not self._new:

diff --git a/test/lib/testing.py b/test/lib/testing.py
index 1d00d04d88929ce64c9ccf0abc1cdafd1c2f2092..a84c5a7ae89a5a6565a8541cb28eef0eae8760dd 100644 (file)
--- a/test/lib/testing.py
+++ b/test/lib/testing.py
@@ -358,14 +358,18 @@ def emits_warning_on(db, *warnings):
 def assert_warnings(fn, warnings):
     """Assert that each of the given warnings are emitted by fn."""
 
+    canary = []
     orig_warn = util.warn
     def capture_warnings(*args, **kw):
         orig_warn(*args, **kw)
         popwarn = warnings.pop(0)
+        canary.append(popwarn)
         eq_(args[0], popwarn)
     util.warn = util.langhelpers.warn = capture_warnings
 
-    return emits_warning()(fn)()
+    result = emits_warning()(fn)()
+    assert canary, "No warning was emitted"
+    return result
 
 def uses_deprecated(*messages):
     """Mark a test as immune from fatal deprecation warnings.

diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index a90e15497b7db80372c0c77fff96186788f77601..d1544ba99a42763b9082c3e20a6b153cff597827 100644 (file)
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -1,5 +1,5 @@
 from test.lib.testing import eq_, assert_raises, \
-    assert_raises_message
+    assert_raises_message, assert_warnings
 from test.lib.util import gc_collect
 from test.lib import pickleable
 from sqlalchemy.util import pickle
@@ -712,7 +712,7 @@ class SessionTest(_fixtures.FixtureTest):
 
         eq_(len(sess.query(User).all()), 1)
 
-    def test_error_on_using_inactive_session(self):
+    def test_error_on_using_inactive_session_commands(self):
         users, User = self.tables.users, self.classes.User
 
         mapper(User, users)
@@ -730,17 +730,67 @@ class SessionTest(_fixtures.FixtureTest):
                               sess.begin, subtransactions=True)
         sess.close()
 
-    def test_preserve_flush_error(self):
+    def _inactive_flushed_session_fixture(self):
         users, User = self.tables.users, self.classes.User
 
         mapper(User, users)
         sess = Session()
+        u1 = User(id=1, name='u1')
+        sess.add(u1)
+        sess.commit()
 
-        sess.add(User(id=5))
+        sess.add(User(id=1, name='u2'))
         assert_raises(
-            sa.exc.DBAPIError,
-            sess.commit
+            orm_exc.FlushError, sess.flush
         )
+        return sess, u1
+
+    def test_warning_on_using_inactive_session_new(self):
+        User = self.classes.User
+
+        sess, u1 = self._inactive_flushed_session_fixture()
+        u2 = User(name='u2')
+        sess.add(u2)
+        def go():
+            sess.rollback()
+        assert_warnings(go, 
+            ["Session's state has been changed on a "
+            "non-active transaction - this state "
+            "will be discarded."],
+        )
+        assert u2 not in sess
+        assert u1 in sess
+
+    def test_warning_on_using_inactive_session_dirty(self):
+        sess, u1 = self._inactive_flushed_session_fixture()
+        u1.name = 'newname'
+        def go():
+            sess.rollback()
+        assert_warnings(go, 
+            ["Session's state has been changed on a "
+            "non-active transaction - this state "
+            "will be discarded."],
+        )
+        assert u1 in sess
+        assert u1 not in sess.dirty
+
+    def test_warning_on_using_inactive_session_delete(self):
+        sess, u1 = self._inactive_flushed_session_fixture()
+        sess.delete(u1)
+        def go():
+            sess.rollback()
+        assert_warnings(go, 
+            ["Session's state has been changed on a "
+            "non-active transaction - this state "
+            "will be discarded."],
+        )
+        assert u1 in sess
+        assert u1 not in sess.deleted
+
+    def test_preserve_flush_error(self):
+        User = self.classes.User
+
+        sess, u1 = self._inactive_flushed_session_fixture()
 
         for i in range(5):
             assert_raises_message(sa.exc.InvalidRequestError,
@@ -755,7 +805,6 @@ class SessionTest(_fixtures.FixtureTest):
         sess.add(User(id=5, name='some name'))
         sess.commit()
 
-
     def test_no_autocommit_with_explicit_commit(self):
         User, users = self.classes.User, self.tables.users