Warn for object replaced in identity map during flush

A warning is emitted for a condition in which the :class:`.Session` may
implicitly swap an object out of the identity map for another one with the
same primary key, detaching the old one, which can be an observed result of
load operations which occur within the :meth:`.SessionEvents.after_flush`
hook.  The warning is intended to notify the user that some special
condition has caused this to happen and that the previous object may not be
in the expected state.

Fixes: #4890
Change-Id: Ide11c6b9f21ca67ff5a96266c521d0c56fd6af8d

diff --git a/doc/build/changelog/unreleased_13/4890.rst b/doc/build/changelog/unreleased_13/4890.rst
new file mode 100644 (file)
index 0000000..26928d6
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4890.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4890
+
+    A warning is emitted for a condition in which the :class:`.Session` may
+    implicitly swap an object out of the identity map for another one with the
+    same primary key, detaching the old one, which can be an observed result of
+    load operations which occur within the :meth:`.SessionEvents.after_flush`
+    hook.  The warning is intended to notify the user that some special
+    condition has caused this to happen and that the previous object may not be
+    in the expected state.

diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py
index 6cc0e2eba14de9bbe905028968c1c1e505440b80..778ad3378032998e232495dd2e855d9c1bebb276 100644 (file)
--- a/lib/sqlalchemy/orm/events.py
+++ b/lib/sqlalchemy/orm/events.py
@@ -1463,6 +1463,15 @@ class SessionEvents(event.Events):
         'dirty', and 'deleted' lists still show pre-flush state as well
         as the history settings on instance attributes.
 
+        .. warning:: This event runs after the :class:`.Session` has emitted
+           SQL to modify the database, but **before** it has altered its
+           internal state to reflect those changes, including that newly
+           inserted objects are placed into the identity map.  ORM operations
+           emitted within this event such as loads of related items
+           may produce new identity map entries that will immediately
+           be replaced, sometimes causing confusing results.  SQLAlchemy will
+           emit a warning for this condition as of version 1.3.9.
+
         :param session: The target :class:`.Session`.
         :param flush_context: Internal :class:`.UOWTransaction` object
          which handles the details of the flush.

diff --git a/lib/sqlalchemy/orm/identity.py b/lib/sqlalchemy/orm/identity.py
index 2da5d66b3f47c9ab7bb3bb73b30f15ca361dea5e..842b7915aec1c0fe8bf52bfdadf6855849934c39 100644 (file)
--- a/lib/sqlalchemy/orm/identity.py
+++ b/lib/sqlalchemy/orm/identity.py
@@ -125,10 +125,13 @@ class WeakInstanceDict(IdentityMap):
                 if existing is not state:
                     self._manage_removed_state(existing)
                 else:
-                    return
+                    return None
+        else:
+            existing = None
 
         self._dict[state.key] = state
         self._manage_incoming_state(state)
+        return existing
 
     def add(self, state):
         key = state.key
@@ -297,9 +300,12 @@ class StrongInstanceDict(IdentityMap):
                 self._manage_removed_state(existing)
             else:
                 return
+        else:
+            existing = None
 
         self._dict[state.key] = state.obj()
         self._manage_incoming_state(state)
+        return existing
 
     def add(self, state):
         if state.key in self:

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 68052dfdd1f54abca0530d5f66f50939a8db9043..58c8bcc0656b9086f9ed580579bea712e49dcd2f 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1084,7 +1084,6 @@ def _emit_insert_statements(
             multiparams = [rec[2] for rec in records]
 
             c = cached_connections[connection].execute(statement, multiparams)
-
             if bookkeeping:
                 for (
                     (

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index c345c42813f802d2f17f4f71059ab792355a1439..3aa392ecfa29d802f0f2cb30d8151c8445afec89 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1935,7 +1935,18 @@ class Session(_SessionClassMethods):
                 # 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)
+                old = self.identity_map.replace(state)
+                if (
+                    old is not None
+                    and mapper._identity_key_from_state(old) == instance_key
+                    and old.obj() is not None
+                ):
+                    util.warn(
+                        "Identity map already had an identity for %s, "
+                        "replacing it with newly flushed object.   Are there "
+                        "load operations occurring inside of an event handler "
+                        "within the flush?" % (instance_key,)
+                    )
                 state._orphaned_outside_of_session = False
 
         statelib.InstanceState._commit_all_states(

diff --git a/test/aaa_profiling/test_zoomark_orm.py b/test/aaa_profiling/test_zoomark_orm.py
index 74b559ae95bb597661b48322b4973fc20fe83d9e..62b154436c4b65356305e57e2d57cffb0b3352a9 100644 (file)
--- a/test/aaa_profiling/test_zoomark_orm.py
+++ b/test/aaa_profiling/test_zoomark_orm.py
@@ -142,6 +142,10 @@ class ZooMarkTest(replay_fixture.ReplayFixtureTest):
         )
         self.session.add(lp)
 
+        # ensure we flush the tables in a specific order as the replayable
+        # session assumes this
+        self.session.flush()
+
         # Animals
 
         leopard = Animal(Species="Leopard", Lifespan=73.5)

diff --git a/test/orm/test_naturalpks.py b/test/orm/test_naturalpks.py
index 9a25a618dc9de5ef395535300b800a90c4875d5f..57f9506edb4c1b58e0fbbda3229b68066905e661 100644 (file)
--- a/test/orm/test_naturalpks.py
+++ b/test/orm/test_naturalpks.py
@@ -813,6 +813,9 @@ class ReversePKsTest(fixtures.MappedTest):
         session.add(a_editable)
         session.commit()
 
+        # see also much more recent issue #4890 where we add a warning
+        # for almost this same case
+
         # do the switch in both directions -
         # one or the other should raise the error
         # based on platform dictionary ordering

diff --git a/test/orm/test_session.py b/test/orm/test_session.py
index ad7ff2d35ae38144cbee6b0e77e308c941a92cc7..4bc8e7691578e8a91ab71d37cd3f8f95d5befa56 100644 (file)
--- a/test/orm/test_session.py
+++ b/test/orm/test_session.py
@@ -1,6 +1,7 @@
 import sqlalchemy as sa
 from sqlalchemy import event
 from sqlalchemy import ForeignKey
+from sqlalchemy import inspect
 from sqlalchemy import Integer
 from sqlalchemy import Sequence
 from sqlalchemy import String
@@ -651,6 +652,53 @@ class SessionStateTest(_fixtures.FixtureTest):
                 sa.orm.attributes.instance_state(u2),
             )
 
+    def test_internal_identity_conflict_warning_weak(self):
+        self._test_internal_identity_conflict_warning(True)
+
+    def test_internal_identity_conflict_warning_strong(self):
+        self._test_internal_identity_conflict_warning(False)
+
+    def _test_internal_identity_conflict_warning(self, weak_identity_map):
+        # test for issue #4890
+        # see also test_naturalpks::ReversePKsTest::test_reverse
+        users, User = self.tables.users, self.classes.User
+        addresses, Address = self.tables.addresses, self.classes.Address
+
+        mapper(
+            User,
+            users,
+            properties={"addresses": relationship(Address, backref="user")},
+        )
+        mapper(Address, addresses)
+
+        with testing.expect_deprecated():
+            session = Session(weak_identity_map=weak_identity_map)
+
+        @event.listens_for(session, "after_flush")
+        def load_collections(session, flush_context):
+            for target in set(session.new).union(session.dirty):
+                if isinstance(target, User):
+                    target.addresses
+
+        u1 = User(name="u1")
+        a1 = Address(email_address="e1", user=u1)
+        session.add_all([u1, a1])
+        session.flush()
+
+        session.expire_all()
+
+        # create new Address via backref, so that u1.addresses remains
+        # expired and a2 is in pending mutations
+        a2 = Address(email_address="e2", user=u1)
+        assert "addresses" not in inspect(u1).dict
+        assert a2 in inspect(u1)._pending_mutations["addresses"].added_items
+
+        with assertions.expect_warnings(
+            r"Identity map already had an identity "
+            r"for \(.*Address.*\), replacing"
+        ):
+            session.flush()
+
     def test_pickled_update(self):
         users, User = self.tables.users, pickleable.User