Don't load expired objects from evaluator

part 2 of e3dc20ff27fa75e571fb2631e64737ea8f25f7c5, the
pre-evaluate step was also emitting SELECT for expired objects.

Fixes: #5664
Change-Id: I9f5de2a5d480eafeb290ec0c45ce2a82ec93475b
(cherry picked from commit cd7d14f7ce8ef54e66d166b4904e8c21e967a5a2)

diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 93460da4933a651c98dfa9ece2467401bc4d31d3..cffd9bedc5f5b88fbb7bcf0e8587ca6938b10023 100644 (file)
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -1786,7 +1786,9 @@ class BulkEvaluate(BulkUD):
                 pk,
                 identity_token,
             ), obj in query.session.identity_map.items()
-            if issubclass(cls, target_cls) and eval_condition(obj)
+            if issubclass(cls, target_cls)
+            and not attributes.instance_state(obj).expired
+            and eval_condition(obj)
         ]
 
 
@@ -1957,9 +1959,8 @@ class BulkUpdateEvaluate(BulkEvaluate, BulkUpdate):
             # only evaluate unmodified attributes
             to_evaluate = state.unmodified.intersection(evaluated_keys)
             for key in to_evaluate:
-                if key not in dict_:
-                    continue
-                dict_[key] = self.value_evaluators[key](obj)
+                if key in dict_:
+                    dict_[key] = self.value_evaluators[key](obj)
 
             state.manager.dispatch.refresh(state, None, to_evaluate)
 

diff --git a/test/orm/test_evaluator.py b/test/orm/test_evaluator.py
index 5bc05448653bff60a37a1284f638308b4094eb2c..c1e392f2daa9ebfdb35576b0b0f9b83be81ce989 100644 (file)
--- a/test/orm/test_evaluator.py
+++ b/test/orm/test_evaluator.py
@@ -9,9 +9,11 @@ from sqlalchemy import not_
 from sqlalchemy import or_
 from sqlalchemy import String
 from sqlalchemy.orm import evaluator
+from sqlalchemy.orm import exc as orm_exc
 from sqlalchemy.orm import mapper
 from sqlalchemy.orm import relationship
 from sqlalchemy.orm import Session
+from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
 from sqlalchemy.testing import expect_warnings
 from sqlalchemy.testing import fixtures
@@ -224,10 +226,10 @@ class M2OEvaluateTest(fixtures.DeclarativeMappedTest):
             name = Column(String(50), primary_key=True)
             parent = relationship(Parent)
 
-    def test_delete(self):
+    def test_delete_not_expired(self):
         Parent, Child = self.classes("Parent", "Child")
 
-        session = Session()
+        session = Session(expire_on_commit=False)
 
         p = Parent(id=1)
         session.add(p)
@@ -240,3 +242,24 @@ class M2OEvaluateTest(fixtures.DeclarativeMappedTest):
         session.query(Child).filter(Child.parent == p).delete("evaluate")
 
         is_(inspect(c).deleted, True)
+
+    def test_delete_expired(self):
+        Parent, Child = self.classes("Parent", "Child")
+
+        session = Session()
+
+        p = Parent(id=1)
+        session.add(p)
+        session.commit()
+
+        c = Child(name="foo", parent=p)
+        session.add(c)
+        session.commit()
+
+        session.query(Child).filter(Child.parent == p).delete("evaluate")
+
+        # because it's expired
+        is_(inspect(c).deleted, False)
+
+        # but it's gone
+        assert_raises(orm_exc.ObjectDeletedError, lambda: c.name)

diff --git a/test/orm/test_update_delete.py b/test/orm/test_update_delete.py
index b715991dabc6f975ba87b8d71602aa83a322dbe7..fab522b4fa1b79f40e5d99b75807001cce737491 100644 (file)
--- a/test/orm/test_update_delete.py
+++ b/test/orm/test_update_delete.py
@@ -230,7 +230,15 @@ class UpdateDeleteTest(fixtures.MappedTest):
         )
         eq_(jill.ufoo, "moonbeam")
 
-    def test_evaluate_dont_refresh_expired_objects(self):
+    @testing.combinations(
+        (False, False),
+        (False, True),
+        (True, False),
+        (True, True),
+    )
+    def test_evaluate_dont_refresh_expired_objects(
+        self, expire_jane_age, add_filter_criteria
+    ):
         User = self.classes.User
 
         sess = Session()
@@ -239,28 +247,74 @@ class UpdateDeleteTest(fixtures.MappedTest):
 
         sess.expire(john)
         sess.expire(jill)
-        sess.expire(jane, ["name"])
+
+        if expire_jane_age:
+            sess.expire(jane, ["name", "age"])
+        else:
+            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"
-            )
+            if add_filter_criteria:
+                sess.query(User).filter(User.name != None).update(
+                    {"age": User.age + 10}, synchronize_session="evaluate"
+                )
+            else:
+                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}],
-            ),
-        )
+        if add_filter_criteria:
+            if expire_jane_age:
+                asserter.assert_(
+                    # it has to unexpire jane.name, because jane is not fully
+                    # expired and the critiera needs to look at this particular
+                    # key
+                    CompiledSQL(
+                        "SELECT users.age_int AS users_age_int, "
+                        "users.name AS users_name FROM users "
+                        "WHERE users.id = :param_1",
+                        [{"param_1": 4}],
+                    ),
+                    CompiledSQL(
+                        "UPDATE users "
+                        "SET age_int=(users.age_int + :age_int_1) "
+                        "WHERE users.name IS NOT NULL",
+                        [{"age_int_1": 10}],
+                    ),
+                )
+            else:
+                asserter.assert_(
+                    # it has to unexpire jane.name, because jane is not fully
+                    # expired and the critiera needs to look at this particular
+                    # key
+                    CompiledSQL(
+                        "SELECT users.name AS users_name FROM users "
+                        "WHERE users.id = :param_1",
+                        [{"param_1": 4}],
+                    ),
+                    CompiledSQL(
+                        "UPDATE users SET "
+                        "age_int=(users.age_int + :age_int_1) "
+                        "WHERE users.name IS NOT NULL",
+                        [{"age_int_1": 10}],
+                    ),
+                )
+        else:
+            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
+            eq_(jane.age, 47)  # needs refresh
 
-        asserter.assert_(
+        to_assert = [
             # refresh john
             CompiledSQL(
                 "SELECT users.age_int AS users_age_int, "
@@ -275,7 +329,19 @@ class UpdateDeleteTest(fixtures.MappedTest):
                 "WHERE users.id = :param_1",
                 [{"param_1": 3}],
             ),
-        )
+        ]
+
+        if expire_jane_age and not add_filter_criteria:
+            to_assert.append(
+                # refresh jane
+                CompiledSQL(
+                    "SELECT users.age_int AS users_age_int, "
+                    "users.name AS users_name FROM users "
+                    "WHERE users.id = :param_1",
+                    [{"param_1": 4}],
+                )
+            )
+        asserter.assert_(*to_assert)
 
     def test_fetch_dont_refresh_expired_objects(self):
         User = self.classes.User
@@ -290,14 +356,18 @@ class UpdateDeleteTest(fixtures.MappedTest):
 
         with self.sql_execution_asserter() as asserter:
             # using 1.x style for easier backport
-            sess.query(User).update(
+            sess.query(User).filter(User.name != None).update(
                 {"age": User.age + 10}, synchronize_session="fetch"
             )
 
         asserter.assert_(
-            CompiledSQL("SELECT users.id AS users_id FROM users"),
             CompiledSQL(
-                "UPDATE users SET age_int=(users.age_int + :age_int_1)",
+                "SELECT users.id AS users_id FROM users "
+                "WHERE users.name IS NOT NULL"
+            ),
+            CompiledSQL(
+                "UPDATE users SET age_int=(users.age_int + :age_int_1) "
+                "WHERE users.name IS NOT NULL",
                 [{"age_int_1": 10}],
             ),
         )