From: Mike Bayer Date: Mon, 7 Feb 2011 21:12:24 +0000 (-0500) Subject: - Adjusted flush accounting step to occur before X-Git-Tag: rel_0_7b1~29 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8f381f202e569eda0d5de13f584304440492222c;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Adjusted flush accounting step to occur before the commit in the case of autocommit=True. This allows autocommit=True to work appropriately with expire_on_commit=True, and also allows post-flush session hooks to operate in the same transactional context as when autocommit=False. [ticket:2041] --- diff --git a/CHANGES b/CHANGES index 0e4e0c8ca8..7a2a2f8847 100644 --- a/CHANGES +++ b/CHANGES @@ -39,9 +39,12 @@ CHANGES - Flushing of Orphans that have no parent is allowed [ticket:1912] - - Session constructor emits a warning when autoflush=True - or expire_on_commit=True when autocommit=True. - [ticket:2041] + - Adjusted flush accounting step to occur before + the commit in the case of autocommit=True. This allows + autocommit=True to work appropriately with + expire_on_commit=True, and also allows post-flush session + hooks to operate in the same transactional context + as when autocommit=False. [ticket:2041] - Warnings generated when collection members, scalar referents not part of the flush diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index b0593ec690..935eed9754 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -205,10 +205,12 @@ class SessionTransaction(object): for s in set(self._new).union(self.session._new): self.session._expunge_state(s) + if s.key: + del s.key for s in set(self._deleted).union(self.session._deleted): if s.deleted: - # assert s in self._deleted + #assert s in self._deleted del s.deleted self.session._update_impl(s) @@ -508,14 +510,6 @@ class Session(object): self.twophase = twophase self._query_cls = query_cls - if autocommit: - if expire_on_commit and _enable_transaction_accounting: - util.warn("expire_on_commit=False is recommended with autocommit=True, " - "else excessive SELECT statements may be emitted.") - if autoflush: - util.warn("autoflush=False is recommended with autocommit=True, " - "else premature/excessive amounts of transaction commits may occur.") - if extension: for ext in util.to_list(extension): SessionExtension._adapt_listener(self, ext) @@ -1541,22 +1535,24 @@ class Session(object): flush_context.execute() self.dispatch.after_flush(self, flush_context) + + flush_context.finalize_flush_changes() + + # useful assertions: + #if not objects: + # assert not self.identity_map._modified + #else: + # assert self.identity_map._modified == \ + # self.identity_map._modified.difference(objects) + + self.dispatch.after_flush_postexec(self, flush_context) + transaction.commit() + except: transaction.rollback(_capture_exception=True) raise - flush_context.finalize_flush_changes() - - # useful assertions: - #if not objects: - # assert not self.identity_map._modified - #else: - # assert self.identity_map._modified == \ - # self.identity_map._modified.difference(objects) - #self.identity_map._modified.clear() - - self.dispatch.after_flush_postexec(self, flush_context) def is_modified(self, instance, include_collections=True, passive=False): """Return ``True`` if instance has modified attributes. diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 3c467909a7..0ab1c17e3d 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -509,8 +509,8 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest): eq_( canary, [ 'after_attach', 'before_flush', 'after_begin', - 'after_flush', 'before_commit', 'after_commit', - 'after_flush_postexec', ] + 'after_flush', 'after_flush_postexec', + 'before_commit', 'after_commit',] ) @testing.resolve_artifact_names @@ -976,9 +976,9 @@ class SessionExtensionTest(_fixtures.FixtureTest): 'before_flush', 'after_begin', 'after_flush', + 'after_flush_postexec', 'before_commit', 'after_commit', - 'after_flush_postexec', ] log = [] sess = create_session(autocommit=False, extension=MyExt()) diff --git a/test/orm/test_session.py b/test/orm/test_session.py index 8d51dac118..c3f6dcd71a 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -250,21 +250,6 @@ class SessionTest(_fixtures.FixtureTest): eq_(bind.connect().execute("select count(1) from users").scalar(), 1) sess.close() - def test_autocommit_warnings(self): - assert_raises_message( - sa.exc.SAWarning, - "expire_on_commit=False is recommended with autocommit=True, " - "else excessive SELECT statements may be emitted.", - Session, autocommit=True, autoflush=False - ) - - assert_raises_message( - sa.exc.SAWarning, - "autoflush=False is recommended with autocommit=True, " - "else premature/excessive amounts of transaction commits may occur.", - Session, autocommit=True, expire_on_commit=False - ) - @testing.resolve_artifact_names def test_make_transient(self): mapper(User, users) diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py index a89c5f4b24..2b1167b53b 100644 --- a/test/orm/test_transaction.py +++ b/test/orm/test_transaction.py @@ -2,7 +2,7 @@ from test.lib.testing import eq_, assert_raises, assert_raises_message from sqlalchemy import * from sqlalchemy.orm import attributes -from sqlalchemy import exc as sa_exc +from sqlalchemy import exc as sa_exc, event from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import * from test.lib.util import gc_collect @@ -493,6 +493,82 @@ class AutoCommitTest(TransactionTest): assert u1 in sess assert sess.query(User).filter_by(name='ed').one() is u1 + def test_accounting_commit_fails_add(self): + sess = create_session(autocommit=True) + + fail = False + def fail_fn(*arg, **kw): + if fail: + raise Exception("commit fails") + + event.listen(sess, "after_flush_postexec", fail_fn) + u1 = User(name='ed') + sess.add(u1) + + fail = True + assert_raises( + Exception, + sess.flush + ) + fail = False + + assert u1 not in sess + u1new = User(id=2, name='fred') + sess.add(u1new) + sess.add(u1) + sess.flush() + assert u1 in sess + eq_( + sess.query(User.name).order_by(User.name).all(), + [('ed', ), ('fred',)] + ) + + def test_accounting_commit_fails_delete(self): + sess = create_session(autocommit=True) + + fail = False + def fail_fn(*arg, **kw): + if fail: + raise Exception("commit fails") + + event.listen(sess, "after_flush_postexec", fail_fn) + u1 = User(name='ed') + sess.add(u1) + sess.flush() + + sess.delete(u1) + fail = True + assert_raises( + Exception, + sess.flush + ) + fail = False + + assert u1 in sess + assert u1 not in sess.deleted + sess.delete(u1) + sess.flush() + assert u1 not in sess + eq_( + sess.query(User.name).order_by(User.name).all(), + [] + ) + + def test_accounting_no_select_needed(self): + """test that flush accounting works on non-expired instances + when autocommit=True/expire_on_commit=True.""" + sess = create_session(autocommit=True, expire_on_commit=True) + + u1 = User(id=1, name='ed') + sess.add(u1) + sess.flush() + + u1.id = 3 + u1.name = 'fred' + self.assert_sql_count(testing.db, sess.flush, 1) + assert 'id' not in u1.__dict__ + eq_(u1.id, 3) + class NaturalPKRollbackTest(_base.MappedTest): @classmethod def define_tables(cls, metadata):