Don't run pending_to_persistent for non-new objects

Fixed fairly simple but critical issue where the
:meth:`.SessionEvents.pending_to_persistent` event would be invoked for
objects not just when they move from pending to persistent, but when they
were also already persistent and just being updated, thus causing the event
to be invoked for all objects on every update.

Fixes: #4489
Change-Id: Ibe147020aa62f7d605cb1029b7f3b776f42e6b43

diff --git a/doc/build/changelog/unreleased_12/4489.rst b/doc/build/changelog/unreleased_12/4489.rst
new file mode 100644 (file)
index 0000000..6f50c4b
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4489.rst
@@ -0,0 +1,9 @@
+.. change::
+   :tags: bug, orm
+   :tickets: 4489
+
+   Fixed fairly simple but critical issue where the
+   :meth:`.SessionEvents.pending_to_persistent` event would be invoked for
+   objects not just when they move from pending to persistent, but when they
+   were also already persistent and just being updated, thus causing the event
+   to be invoked for all objects on every update.

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 6d4198a4ee830f02bc65b0d627c636375b7135e1..0b1a3b1010227667123284ee822ea11012765c91 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1827,7 +1827,14 @@ class Session(_SessionClassMethods):
             states, self, to_transient=to_transient
         )
 
-    def _register_newly_persistent(self, states):
+    def _register_persistent(self, states):
+        """Register all persistent objects from a flush.
+
+        This is used both for pending objects moving to the persistent
+        state as well as already persistent objects.
+
+        """
+
         pending_to_persistent = self.dispatch.pending_to_persistent or None
         for state in states:
             mapper = _state_mapper(state)
@@ -1871,6 +1878,9 @@ class Session(_SessionClassMethods):
                     )
                     state.key = instance_key
 
+                # there can be an existing state in the identity map
+                # that is replaced when the primary keys of two instances
+                # are swapped; see test/orm/test_naturalpks.py -> test_reverse
                 self.identity_map.replace(state)
                 state._orphaned_outside_of_session = False
 
@@ -1881,7 +1891,7 @@ class Session(_SessionClassMethods):
         self._register_altered(states)
 
         if pending_to_persistent is not None:
-            for state in states:
+            for state in states.intersection(self._new):
                 pending_to_persistent(self, state.obj())
 
         # remove from new last, might be the last strong ref

diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py
index 3b91c8990d06e771e0e5ed36673eaf8adea022b9..1f2221ea930cd7fe46c14fc6f15b7627becd3c21 100644 (file)
--- a/lib/sqlalchemy/orm/unitofwork.py
+++ b/lib/sqlalchemy/orm/unitofwork.py
@@ -440,7 +440,7 @@ class UOWTransaction(object):
         if isdel:
             self.session._remove_newly_deleted(isdel)
         if other:
-            self.session._register_newly_persistent(other)
+            self.session._register_persistent(other)
 
 
 class IterateMappersMixin(object):

diff --git a/test/orm/test_events.py b/test/orm/test_events.py
index fc550eb833a6b1afb5516347fc359dc3795db6d1..d6d087d753416a1dbcf1b09535cba1c78d0d20bf 100644 (file)
--- a/test/orm/test_events.py
+++ b/test/orm/test_events.py
@@ -2004,6 +2004,15 @@ class SessionLifecycleEventsTest(_RemoveListeners, _fixtures.FixtureTest):
             [call.pending_to_persistent(sess, u1), call.flag_checked(u1)],
         )
 
+        u1.name = 'u2'
+        sess.flush()
+
+        # event was not called again
+        eq_(
+            listener.mock_calls,
+            [call.pending_to_persistent(sess, u1), call.flag_checked(u1)],
+        )
+
     def test_pending_to_persistent_del(self):
         sess, User, start_events = self._fixture()