Mike Bayer avatar Mike Bayer committed cc21ca9

- 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]

Comments (0)

Files changed (5)

   - 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

lib/sqlalchemy/orm/session.py

 
         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)
 
         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)
             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.

test/orm/test_events.py

         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
             'before_flush',
             'after_begin',
             'after_flush',
+            'after_flush_postexec',
             'before_commit',
             'after_commit',
-            'after_flush_postexec',
             ]
         log = []
         sess = create_session(autocommit=False, extension=MyExt())

test/orm/test_session.py

         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)

test/orm/test_transaction.py

 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
         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):
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.