[Sqlalchemy-commits] sqlalchemy: - [bug] Fixed issue where modified session state
Brought to you by:
zzzeek
From: <co...@sq...> - 2012-01-28 01:33:13
|
details: http://hg.sqlalchemy.org/sqlalchemy/sqlalchemy/rev/5b231b7d7348 changeset: 8050:5b231b7d7348 user: Mike Bayer <mi...@zz...> date: Fri Jan 27 20:32:52 2012 -0500 description: - [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 diffstat: CHANGES | 10 ++++++ lib/sqlalchemy/orm/session.py | 22 ++++++++++++-- test/lib/testing.py | 6 +++- test/orm/test_session.py | 63 ++++++++++++++++++++++++++++++++++++++---- 4 files changed, 89 insertions(+), 12 deletions(-) diffs (195 lines): diff -r d29b88aeb85e -r 5b231b7d7348 CHANGES --- a/CHANGES Thu Jan 26 12:15:43 2012 -0500 +++ b/CHANGES Fri Jan 27 20:32:52 2012 -0500 @@ -6,6 +6,16 @@ 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 -r d29b88aeb85e -r 5b231b7d7348 lib/sqlalchemy/orm/session.py --- a/lib/sqlalchemy/orm/session.py Thu Jan 26 12:15:43 2012 -0500 +++ b/lib/sqlalchemy/orm/session.py Fri Jan 27 20:32:52 2012 -0500 @@ -343,6 +343,16 @@ 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 @@ 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 @@ 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 -r d29b88aeb85e -r 5b231b7d7348 test/lib/testing.py --- a/test/lib/testing.py Thu Jan 26 12:15:43 2012 -0500 +++ b/test/lib/testing.py Fri Jan 27 20:32:52 2012 -0500 @@ -358,14 +358,18 @@ 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 -r d29b88aeb85e -r 5b231b7d7348 test/orm/test_session.py --- a/test/orm/test_session.py Thu Jan 26 12:15:43 2012 -0500 +++ b/test/orm/test_session.py Fri Jan 27 20:32:52 2012 -0500 @@ -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 @@ 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 @@ 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 @@ 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 |