From: Mike Bayer Date: Sat, 11 Mar 2017 15:52:43 +0000 (-0500) Subject: Emit after_rollback() event before snapshot removal X-Git-Tag: rel_1_2_0b1~155 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4ece86eb41274ba059211807e23273178c3c3384;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Emit after_rollback() event before snapshot removal The state of the :class:`.Session` is now present when the :meth:`.SessionEvents.after_rollback` event is emitted, that is, the attribute state of objects prior to their being expired. This is now consistent with the behavior of the :meth:`.SessionEvents.after_commit` event which also emits before the attribute state of objects is expired. Change-Id: I9c572656ec5a9bfaeab817e9c95107c75aca1b51 Fixes: #3934 --- diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst index abf7035cad..b99e9ef80e 100644 --- a/doc/build/changelog/changelog_12.rst +++ b/doc/build/changelog/changelog_12.rst @@ -56,3 +56,18 @@ .. seealso:: :ref:`change_2694` + + .. change:: 3934 + :tags: bug, orm + :tickets: 3934 + + The state of the :class:`.Session` is now present when the + :meth:`.SessionEvents.after_rollback` event is emitted, that is, the + attribute state of objects prior to their being expired. This is now + consistent with the behavior of the + :meth:`.SessionEvents.after_commit` event which also emits before the + attribute state of objects is expired. + + .. seealso:: + + :ref:`change_3934` diff --git a/doc/build/changelog/migration_12.rst b/doc/build/changelog/migration_12.rst index c63d585fb0..2eb8659b9c 100644 --- a/doc/build/changelog/migration_12.rst +++ b/doc/build/changelog/migration_12.rst @@ -64,6 +64,44 @@ Where the value of the parameter "x_1" is ``'total/%score'``. Key Behavioral Changes - ORM ============================ +The after_rollback() Session event now emits before the expiration of objects +----------------------------------------------------------------------------- + +The :meth:`.Session.after_rollback` event now has access to the attribute +state of objects before their state has been expired (e.g. the "snapshot +removal"). This allows the event to be consistent with the behavior +of the :meth:`.Session.after_commit` event which also emits before the +"snapshot" has been removed:: + + sess = Session() + + user = sess.query(User).filter_by(name='x').first() + + @event.listens_for(sess, "after_rollback") + def after_rollback(session): + # 'user.name' is now present, assuming it was already + # loaded. previously this would raise upon trying + # to emit a lazy load. + print("user name: %s" % user.name) + + @event.listens_for(sess, "after_commit") + def after_commit(session): + # 'user.name' is present, assuming it was already + # loaded. this is the existing behavior. + print("user name: %s" % user.name) + + if should_rollback: + sess.rollback() + else: + sess.commit() + +Note that the :class:`.Session` will still disallow SQL from being emitted +within this event; meaning that unloaded attributes will still not be +able to load within the scope of the event. + + +:ticket:`3934` + Key Behavioral Changes - Core ============================= @@ -90,23 +128,23 @@ within the :meth:`.Inspector.get_foreign_keys` method will now be "name normalized", that is, expressed as lower case for a case insensitive name, rather than the raw UPPERCASE format that Oracle uses:: - >>> insp.get_indexes("addresses") - [{'unique': False, 'column_names': [u'user_id'], - 'name': u'address_idx', 'dialect_options': {}}] + >>> insp.get_indexes("addresses") + [{'unique': False, 'column_names': [u'user_id'], + 'name': u'address_idx', 'dialect_options': {}}] - >>> insp.get_pk_constraint("addresses") - {'name': u'pk_cons', 'constrained_columns': [u'id']} + >>> insp.get_pk_constraint("addresses") + {'name': u'pk_cons', 'constrained_columns': [u'id']} - >>> insp.get_foreign_keys("addresses") - [{'referred_table': u'users', 'referred_columns': [u'id'], - 'referred_schema': None, 'name': u'user_id_fk', - 'constrained_columns': [u'user_id']}] + >>> insp.get_foreign_keys("addresses") + [{'referred_table': u'users', 'referred_columns': [u'id'], + 'referred_schema': None, 'name': u'user_id_fk', + 'constrained_columns': [u'user_id']}] Previously, the foreign keys result would look like:: - [{'referred_table': u'users', 'referred_columns': [u'id'], - 'referred_schema': None, 'name': 'USER_ID_FK', - 'constrained_columns': [u'user_id']}] + [{'referred_table': u'users', 'referred_columns': [u'id'], + 'referred_schema': None, 'name': 'USER_ID_FK', + 'constrained_columns': [u'user_id']}] Where the above could create problems particularly with Alembic autogenerate. diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 081920487d..2d3cb1b089 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -277,9 +277,9 @@ class SessionTransaction(object): ) elif not deactive_ok: raise sa_exc.InvalidRequestError( - "This Session's transaction has been rolled back " - "by a nested rollback() call. To begin a new " - "transaction, issue Session.rollback() first." + "This session is in 'inactive' state, due to the " + "SQL transaction being rolled back; no further " + "SQL can be emitted within this transaction." ) elif self._state is CLOSED: raise sa_exc.ResourceClosedError(closed_msg) @@ -487,10 +487,17 @@ class SessionTransaction(object): for transaction in self._iterate_self_and_parents(): if transaction._parent is None or transaction.nested: try: - transaction._rollback_impl() + for t in set(transaction._connections.values()): + t[1].rollback() + + transaction._state = DEACTIVE + self.session.dispatch.after_rollback(self.session) except: rollback_err = sys.exc_info() - transaction._state = DEACTIVE + finally: + if self.session._enable_transaction_accounting: + transaction._restore_snapshot( + dirty_only=transaction.nested) boundary = transaction break else: @@ -521,15 +528,6 @@ class SessionTransaction(object): return self._parent - def _rollback_impl(self): - try: - for t in set(self._connections.values()): - t[1].rollback() - finally: - if self.session._enable_transaction_accounting: - self._restore_snapshot(dirty_only=self.nested) - - self.session.dispatch.after_rollback(self.session) def close(self, invalidate=False): self.session.transaction = self._parent diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 80633c90e7..f3cce7da35 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -1617,6 +1617,50 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest): ] ) + def test_snapshot_still_present_after_commit(self): + users, User = self.tables.users, self.classes.User + + mapper(User, users) + + sess = Session() + + u1 = User(name='u1') + + sess.add(u1) + sess.commit() + + u1 = sess.query(User).first() + + @event.listens_for(sess, "after_commit") + def assert_state(session): + assert 'name' in u1.__dict__ + eq_(u1.name, 'u1') + + sess.commit() + assert 'name' not in u1.__dict__ + + def test_snapshot_still_present_after_rollback(self): + users, User = self.tables.users, self.classes.User + + mapper(User, users) + + sess = Session() + + u1 = User(name='u1') + + sess.add(u1) + sess.commit() + + u1 = sess.query(User).first() + + @event.listens_for(sess, "after_rollback") + def assert_state(session): + assert 'name' in u1.__dict__ + eq_(u1.name, 'u1') + + sess.rollback() + assert 'name' not in u1.__dict__ + class SessionLifecycleEventsTest(_RemoveListeners, _fixtures.FixtureTest): run_inserts = None diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py index c4f6a1eaf0..374b6a9e53 100644 --- a/test/orm/test_transaction.py +++ b/test/orm/test_transaction.py @@ -434,12 +434,12 @@ class SessionTransactionTest(FixtureTest): sess.add(User(name='u1')) sess.flush() sess.rollback() - assert_raises_message(sa_exc.InvalidRequestError, - "This Session's transaction has been " - r"rolled back by a nested rollback\(\) " - "call. To begin a new transaction, " - r"issue Session.rollback\(\) first.", - sess.begin, subtransactions=True) + assert_raises_message( + sa_exc.InvalidRequestError, + "This session is in 'inactive' state, due to the SQL transaction " + "being rolled back; no further SQL can be emitted within this " + "transaction.", + sess.begin, subtransactions=True) sess.close() def test_no_sql_during_commit(self): @@ -465,6 +465,19 @@ class SessionTransactionTest(FixtureTest): "SQL can be emitted within this transaction.", sess.execute, "select 1") + def test_no_sql_during_rollback(self): + sess = create_session(bind=testing.db, autocommit=False) + + @event.listens_for(sess, "after_rollback") + def go(session): + session.execute("select 1") + assert_raises_message( + sa_exc.InvalidRequestError, + "This session is in 'inactive' state, due to the SQL transaction " + "being rolled back; no further SQL can be emitted within this " + "transaction.", + sess.rollback) + def test_no_prepare_wo_twophase(self): sess = create_session(bind=testing.db, autocommit=False) @@ -491,9 +504,9 @@ class SessionTransactionTest(FixtureTest): trans2.rollback() assert_raises_message( sa_exc.InvalidRequestError, - r"This Session's transaction has been rolled back by a nested " - r"rollback\(\) call. To begin a new transaction, issue " - r"Session.rollback\(\) first.", + "This session is in 'inactive' state, due to the SQL transaction " + "being rolled back; no further SQL can be emitted within this " + "transaction.", trans.commit ) @@ -622,11 +635,6 @@ class SessionTransactionTest(FixtureTest): orm_exc.FlushError, sess.flush ) - assert_warnings(go, - ["Session's state has been changed on a " - "non-active transaction - this state " - "will be discarded."], - ) assert u3 not in sess def test_preserve_flush_error(self):