From 51ab4430fb34e9e4dbaa1d4457339c059fdaf9b1 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 21 Oct 2020 13:58:22 -0400 Subject: [PATCH] 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 (cherry picked from commit e3dc20ff27fa75e571fb2631e64737ea8f25f7c5) --- doc/build/changelog/unreleased_13/5664.rst | 8 ++ lib/sqlalchemy/orm/persistence.py | 2 + test/orm/test_update_delete.py | 109 +++++++++++++++++++++ 3 files changed, 119 insertions(+) create mode 100644 doc/build/changelog/unreleased_13/5664.rst 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 7ec1e8c53a..93460da493 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -1957,6 +1957,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) state.manager.dispatch.refresh(state, None, to_evaluate) diff --git a/test/orm/test_update_delete.py b/test/orm/test_update_delete.py index 46bb870621..b715991dab 100644 --- a/test/orm/test_update_delete.py +++ b/test/orm/test_update_delete.py @@ -22,6 +22,7 @@ from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ from sqlalchemy.testing import mock +from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table @@ -229,6 +230,114 @@ 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" + ) + + asserter.assert_( + CompiledSQL("SELECT users.id AS 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) # refreshes in 1.3 + eq_(jill.age, 39) # needs refresh + eq_(jane.age, 47) # refreshes in 1.3 + + 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 jack.age in 1.3 only + CompiledSQL( + "SELECT users.age_int AS users_age_int FROM users " + "WHERE users.id = :param_1", + [{"param_1": 2}], + ), + # 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}], + ), + # refresh jane, seems to be a full refresh in 1.3. + CompiledSQL( + "SELECT users.age_int AS users_age_int, " + "users.name AS users_name FROM users " + "WHERE users.id = :param_1", + [{"param_1": 4}], + ), + ) + def test_delete(self): User = self.classes.User -- 2.47.3