From 4aa43ecbd78e5a7dd3d983ca46a377af4e01877e Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 4 Oct 2019 11:12:27 -0400 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_13/4890.rst | 11 +++++ lib/sqlalchemy/orm/events.py | 9 ++++ lib/sqlalchemy/orm/identity.py | 8 +++- lib/sqlalchemy/orm/persistence.py | 1 - lib/sqlalchemy/orm/session.py | 13 +++++- test/aaa_profiling/test_zoomark_orm.py | 4 ++ test/orm/test_naturalpks.py | 3 ++ test/orm/test_session.py | 48 ++++++++++++++++++++++ 8 files changed, 94 insertions(+), 3 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/4890.rst diff --git a/doc/build/changelog/unreleased_13/4890.rst b/doc/build/changelog/unreleased_13/4890.rst new file mode 100644 index 0000000000..26928d6210 --- /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 6cc0e2eba1..778ad33780 100644 --- 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 2da5d66b3f..842b7915ae 100644 --- 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 68052dfdd1..58c8bcc065 100644 --- 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 c345c42813..3aa392ecfa 100644 --- 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 74b559ae95..62b154436c 100644 --- 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 9a25a618dc..57f9506edb 100644 --- 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 ad7ff2d35a..4bc8e76915 100644 --- 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 -- 2.47.2