From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Tue, 20 Apr 2021 15:33:02 +0000 (-0400)
Subject: Narrow refresh populate_existing to just refresh_state
X-Git-Tag: rel_1_4_10~7
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=033e673f8771dfb20dd7b67a780c6ef3d3210e37;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Narrow refresh populate_existing to just refresh_state

Fixed additional regression caused by the "eagerloaders on refresh" feature
added in :ticket:`1763` where the refresh operation historically would set
``populate_existing``, which given the new feature now overwrites pending
changes on eagerly loaded objects when autoflush is false. The
populate_existing flag has been turned off for this case and a more
specific method used to ensure the correct attributes refreshed.

Fixes: #6326
Change-Id: I40315e4164eae28972c5839c04580d292bc6cb24
---

diff --git a/doc/build/changelog/unreleased_14/6326.rst b/doc/build/changelog/unreleased_14/6326.rst
new file mode 100644
index 0000000000..13ad425030
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/6326.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: orm, bug, regression
+    :tickets: 6326
+
+    Fixed additional regression caused by the "eagerloaders on refresh" feature
+    added in :ticket:`1763` where the refresh operation historically would set
+    ``populate_existing``, which given the new feature now overwrites pending
+    changes on eagerly loaded objects when autoflush is false. The
+    populate_existing flag has been turned off for this case and a more
+    specific method used to ensure the correct attributes refreshed.
diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 12acfc5b78..ea6d0f1fe6 100644
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -481,7 +481,6 @@ def load_on_pk_identity(
         _, load_options = _set_get_options(
             compile_options,
             load_options,
-            populate_existing=bool(refresh_state),
             version_check=version_check,
             only_load_props=only_load_props,
             refresh_state=refresh_state,
@@ -513,7 +512,6 @@ def load_on_pk_identity(
         q._compile_options, load_options = _set_get_options(
             compile_options,
             load_options,
-            populate_existing=bool(refresh_state),
             version_check=version_check,
             only_load_props=only_load_props,
             refresh_state=refresh_state,
@@ -916,17 +914,23 @@ def _instance_processor(
                 state.session_id = session_id
                 session_identity_map._add_unpresent(state, identitykey)
 
+        effective_populate_existing = populate_existing
+        if refresh_state is state:
+            effective_populate_existing = True
+
         # populate.  this looks at whether this state is new
         # for this load or was existing, and whether or not this
         # row is the first row with this identity.
-        if currentload or populate_existing:
+        if currentload or effective_populate_existing:
             # full population routines.  Objects here are either
             # just created, or we are doing a populate_existing
 
             # be conservative about setting load_path when populate_existing
             # is in effect; want to maintain options from the original
             # load.  see test_expire->test_refresh_maintains_deferred_options
-            if isnew and (propagated_loader_options or not populate_existing):
+            if isnew and (
+                propagated_loader_options or not effective_populate_existing
+            ):
                 state.load_options = propagated_loader_options
                 state.load_path = load_path
 
@@ -938,7 +942,7 @@ def _instance_processor(
                 isnew,
                 load_path,
                 loaded_instance,
-                populate_existing,
+                effective_populate_existing,
                 populators,
             )
 
@@ -966,7 +970,7 @@ def _instance_processor(
                     if state.runid != runid:
                         _warn_for_runid_changed(state)
 
-                if populate_existing or state.modified:
+                if effective_populate_existing or state.modified:
                     if refresh_state and only_load_props:
                         state._commit(dict_, only_load_props)
                     else:
diff --git a/test/orm/test_expire.py b/test/orm/test_expire.py
index 95efae850e..63ed7a3a71 100644
--- a/test/orm/test_expire.py
+++ b/test/orm/test_expire.py
@@ -663,6 +663,38 @@ class ExpireTest(_fixtures.FixtureTest):
         # together when eager load used with Query
         self.assert_sql_count(testing.db, go, 1)
 
+    def test_unexpire_eager_dont_overwrite_related(self):
+        users, Address, addresses, User = (
+            self.tables.users,
+            self.classes.Address,
+            self.tables.addresses,
+            self.classes.User,
+        )
+
+        mapper(
+            User,
+            users,
+            properties={
+                "addresses": relationship(
+                    Address, backref="user", lazy="joined"
+                )
+            },
+        )
+        mapper(Address, addresses)
+
+        sess = fixture_session(autoflush=False)
+        u = sess.query(User).get(7)
+
+        a1 = u.addresses[0]
+        eq_(a1.email_address, "jack@bean.com")
+
+        sess.expire(u)
+        a1.email_address = "foo"
+
+        assert a1 in u.addresses
+        eq_(a1.email_address, "foo")
+        assert a1 in sess.dirty
+
     def test_relationship_changes_preserved(self):
         users, Address, addresses, User = (
             self.tables.users,