From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 21 Oct 2020 17:58:22 +0000 (-0400)
Subject: Don't populate expired attrs w/ evaluator
X-Git-Tag: rel_1_4_0b1~19
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e3dc20ff27fa75e571fb2631e64737ea8f25f7c5;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Don't populate expired attrs w/ evaluator

Fixed bug in :meth:`_orm.Query.update` where objects in the
:class:`_orm.Session` that were already expired would be unnecessarily
SELECTed individually when they were refreshed by the "evaluate"
synchronize strategy.

For 1.4 there was also a similar issue with fetch that would actually
get the wrong data back, as the new value would be loaded, then
applied with the evaluator.

Fixes: #5664
Change-Id: I6e6aff88462654fcefa9fce2b45f5446c418deee
---

diff --git a/doc/build/changelog/unreleased_13/5664.rst b/doc/build/changelog/unreleased_13/5664.rst
new file mode 100644
index 0000000000..9c7969709f
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/5664.rst
@@ -0,0 +1,8 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 5664
+
+    Fixed bug in :meth:`_orm.Query.update` where objects in the
+    :class:`_orm.Session` that were already expired would be unnecessarily
+    SELECTed individually when they were refreshed by the "evaluate"
+    synchronize strategy.
diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 4a0b2d07d9..1794cc2ce2 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -2209,6 +2209,8 @@ class BulkORMUpdate(UpdateDMLState, BulkUDCompileState):
             # only evaluate unmodified attributes
             to_evaluate = state.unmodified.intersection(evaluated_keys)
             for key in to_evaluate:
+                if key not in dict_:
+                    continue
                 dict_[key] = update_options._value_evaluators[key](obj)
 
             state.manager.dispatch.refresh(state, None, to_evaluate)
@@ -2264,7 +2266,8 @@ class BulkORMUpdate(UpdateDMLState, BulkUDCompileState):
 
             to_evaluate = state.unmodified.intersection(evaluated_keys)
             for key in to_evaluate:
-                dict_[key] = update_options._value_evaluators[key](obj)
+                if key in dict_:
+                    dict_[key] = update_options._value_evaluators[key](obj)
             state.manager.dispatch.refresh(state, None, to_evaluate)
 
             state._commit(dict_, list(to_evaluate))
diff --git a/test/orm/test_update_delete.py b/test/orm/test_update_delete.py
index 01eb7279bd..00bc344bce 100644
--- a/test/orm/test_update_delete.py
+++ b/test/orm/test_update_delete.py
@@ -276,6 +276,111 @@ class UpdateDeleteTest(fixtures.MappedTest):
         )
         eq_(jill.ufoo, "moonbeam")
 
+    def test_evaluate_dont_refresh_expired_objects(self):
+        User = self.classes.User
+
+        sess = Session()
+
+        john, jack, jill, jane = sess.query(User).order_by(User.id).all()
+
+        sess.expire(john)
+        sess.expire(jill)
+        sess.expire(jane, ["name"])
+
+        with self.sql_execution_asserter() as asserter:
+            # using 1.x style for easier backport
+            sess.query(User).update(
+                {"age": User.age + 10}, synchronize_session="evaluate"
+            )
+
+        asserter.assert_(
+            CompiledSQL(
+                "UPDATE users SET age_int=(users.age_int + :age_int_1)",
+                [{"age_int_1": 10}],
+            ),
+        )
+
+        with self.sql_execution_asserter() as asserter:
+            eq_(john.age, 35)  # needs refresh
+            eq_(jack.age, 57)  # no SQL needed
+            eq_(jill.age, 39)  # needs refresh
+            eq_(jane.age, 47)  # no SQL needed
+
+        asserter.assert_(
+            # refresh john
+            CompiledSQL(
+                "SELECT users.age_int AS users_age_int, "
+                "users.id AS users_id, users.name AS users_name FROM users "
+                "WHERE users.id = :param_1",
+                [{"param_1": 1}],
+            ),
+            # refresh jill
+            CompiledSQL(
+                "SELECT users.age_int AS users_age_int, "
+                "users.id AS users_id, users.name AS users_name FROM users "
+                "WHERE users.id = :param_1",
+                [{"param_1": 3}],
+            ),
+        )
+
+    def test_fetch_dont_refresh_expired_objects(self):
+        User = self.classes.User
+
+        sess = Session()
+
+        john, jack, jill, jane = sess.query(User).order_by(User.id).all()
+
+        sess.expire(john)
+        sess.expire(jill)
+        sess.expire(jane, ["name"])
+
+        with self.sql_execution_asserter() as asserter:
+            # using 1.x style for easier backport
+            sess.query(User).update(
+                {"age": User.age + 10}, synchronize_session="fetch"
+            )
+
+        if testing.db.dialect.full_returning:
+            asserter.assert_(
+                CompiledSQL(
+                    "UPDATE users SET age_int=(users.age_int + %(age_int_1)s) "
+                    "RETURNING users.id",
+                    [{"age_int_1": 10}],
+                    dialect="postgresql",
+                ),
+            )
+        else:
+            asserter.assert_(
+                CompiledSQL("SELECT users.id FROM users"),
+                CompiledSQL(
+                    "UPDATE users SET age_int=(users.age_int + :age_int_1)",
+                    [{"age_int_1": 10}],
+                ),
+            )
+
+        with self.sql_execution_asserter() as asserter:
+            eq_(john.age, 35)  # needs refresh
+            eq_(jack.age, 57)  # no SQL needed
+            eq_(jill.age, 39)  # needs refresh
+            eq_(jane.age, 47)  # no SQL needed
+
+        asserter.assert_(
+            # refresh john
+            CompiledSQL(
+                "SELECT users.age_int AS users_age_int, "
+                "users.id AS users_id, users.name AS users_name FROM users "
+                "WHERE users.id = :param_1",
+                [{"param_1": 1}],
+            ),
+            # refresh jill
+            CompiledSQL(
+                "SELECT users.age_int AS users_age_int, "
+                "users.id AS users_id, users.name AS users_name FROM users "
+                "WHERE users.id = :param_1",
+                [{"param_1": 3}],
+            ),
+        )
+
     def test_delete(self):
         User = self.classes.User