From: Mike Bayer Date: Mon, 17 Oct 2022 15:53:07 +0000 (-0400) Subject: simplify unmapped col eval fallback X-Git-Tag: rel_2_0_0b2~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8db3744f9f787a0e5f5464b6abd4e45ec787221d;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git simplify unmapped col eval fallback Removed the warning that emits when using ORM-enabled update/delete regarding evaluation of columns by name, first added in :ticket:`4073`; this warning actually covers up a scenario that otherwise could populate the wrong Python value for an ORM mapped attribute depending on what the actual column is, so this deprecated case is removed. In 2.0, ORM enabled update/delete uses "auto" for "synchronize_session", which should do the right thing automatically for any given UPDATE expression. Fixes: #8656 Change-Id: Idb8b4a86d3caed89f69cde1607886face103cf6a --- diff --git a/doc/build/changelog/unreleased_20/8656.rst b/doc/build/changelog/unreleased_20/8656.rst new file mode 100644 index 0000000000..3b6407d3ec --- /dev/null +++ b/doc/build/changelog/unreleased_20/8656.rst @@ -0,0 +1,11 @@ +.. change:: + :tags: bug, orm + :tickets: 8656 + + Removed the warning that emits when using ORM-enabled update/delete + regarding evaluation of columns by name, first added in :ticket:`4073`; + this warning actually covers up a scenario that otherwise could populate + the wrong Python value for an ORM mapped attribute depending on what the + actual column is, so this deprecated case is removed. In 2.0, ORM enabled + update/delete uses "auto" for "synchronize_session", which should do the + right thing automatically for any given UPDATE expression. diff --git a/lib/sqlalchemy/orm/evaluator.py b/lib/sqlalchemy/orm/evaluator.py index 3c0e62ef53..bf6bff0370 100644 --- a/lib/sqlalchemy/orm/evaluator.py +++ b/lib/sqlalchemy/orm/evaluator.py @@ -14,7 +14,6 @@ from .base import LoaderCallableStatus from .base import PassiveFlag from .. import exc from .. import inspect -from .. import util from ..sql import and_ from ..sql import operators from ..sql.sqltypes import Integer @@ -73,61 +72,56 @@ class EvaluatorCompiler: return lambda obj: True def visit_column(self, clause): - if "parentmapper" in clause._annotations: + try: parentmapper = clause._annotations["parentmapper"] - if self.target_cls and not issubclass( - self.target_cls, parentmapper.class_ - ): - raise UnevaluatableError( - "Can't evaluate criteria against " - f"alternate class {parentmapper.class_}" - ) - - try: - key = parentmapper._columntoproperty[clause].key - except orm_exc.UnmappedColumnError as err: - raise UnevaluatableError( - f"Cannot evaluate expression: {err}" - ) from err - - impl = parentmapper.class_manager[key].impl - - if impl is not None: - - def get_corresponding_attr(obj): - if obj is None: - return _NO_OBJECT - state = inspect(obj) - dict_ = state.dict - - value = impl.get( - state, dict_, passive=PassiveFlag.PASSIVE_NO_FETCH - ) - if value is LoaderCallableStatus.PASSIVE_NO_RESULT: - return _EXPIRED_OBJECT - return value - - return get_corresponding_attr - else: - key = clause.key - if ( - self.target_cls - and key in inspect(self.target_cls).column_attrs - ): - util.warn( - f"Evaluating non-mapped column expression '{clause}' onto " - "ORM instances; this is a deprecated use case. Please " - "make use of the actual mapped columns in ORM-evaluated " - "UPDATE / DELETE expressions." - ) - - else: - raise UnevaluatableError(f"Cannot evaluate column: {clause}") + except KeyError as ke: + raise UnevaluatableError( + f"Cannot evaluate column: {clause}" + ) from ke + + if self.target_cls and not issubclass( + self.target_cls, parentmapper.class_ + ): + raise UnevaluatableError( + "Can't evaluate criteria against " + f"alternate class {parentmapper.class_}" + ) + + parentmapper._check_configure() + + # we'd like to use "proxy_key" annotation to get the "key", however + # in relationship primaryjoin cases proxy_key is sometimes deannotated + # and sometimes apparently not present in the first place (?). + # While I can stop it from being deannotated (though need to see if + # this breaks other things), not sure right now about cases where it's + # not there in the first place. can fix at some later point. + # key = clause._annotations["proxy_key"] + + # for now, use the old way + try: + key = parentmapper._columntoproperty[clause].key + except orm_exc.UnmappedColumnError as err: + raise UnevaluatableError( + f"Cannot evaluate expression: {err}" + ) from err + + # note this used to fall back to a simple `getattr(obj, key)` evaluator + # if impl was None; as of #8656, we ensure mappers are configured + # so that impl is available + impl = parentmapper.class_manager[key].impl def get_corresponding_attr(obj): if obj is None: return _NO_OBJECT - return getattr(obj, key, _EXPIRED_OBJECT) + state = inspect(obj) + dict_ = state.dict + + value = impl.get( + state, dict_, passive=PassiveFlag.PASSIVE_NO_FETCH + ) + if value is LoaderCallableStatus.PASSIVE_NO_RESULT: + return _EXPIRED_OBJECT + return value return get_corresponding_attr diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index bae381961c..86a0f82c56 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -2314,8 +2314,8 @@ class JoinCondition: """remove the parententity annotation from our join conditions which can leak in here based on some declarative patterns and maybe others. - We'd want to remove "parentmapper" also, but apparently there's - an exotic use case in _join_fixture_inh_selfref_w_entity + "parentmapper" is relied upon both by the ORM evaluator as well as + the use case in _join_fixture_inh_selfref_w_entity that relies upon it being present, see :ticket:`3364`. """ diff --git a/test/orm/dml/test_evaluator.py b/test/orm/dml/test_evaluator.py index 4b903b863c..0a5674740d 100644 --- a/test/orm/dml/test_evaluator.py +++ b/test/orm/dml/test_evaluator.py @@ -18,7 +18,6 @@ from sqlalchemy.orm import relationship from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import eq_ -from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ from sqlalchemy.testing.assertions import expect_raises_message @@ -114,19 +113,19 @@ class EvaluateTest(fixtures.MappedTest): ], ) - def test_warn_on_unannotated_matched_column(self): + def test_raise_on_unannotated_matched_column(self): + """test originally for warning emitted in #4073, + now updated for #8656.""" + User = self.classes.User compiler = evaluator.EvaluatorCompiler(User) - with expect_warnings( - r"Evaluating non-mapped column expression 'othername' " - "onto ORM instances; this is a deprecated use case." + with expect_raises_message( + evaluator.UnevaluatableError, + "Cannot evaluate column: othername", ): - meth = compiler.process(User.name == Column("othername", String)) - - u1 = User(id=5) - meth(u1) + compiler.process(User.name == Column("othername", String)) def test_raise_on_unannotated_unmatched_column(self): User = self.classes.User diff --git a/test/orm/dml/test_update_delete_where.py b/test/orm/dml/test_update_delete_where.py index c8e56e3c11..ebaba191ab 100644 --- a/test/orm/dml/test_update_delete_where.py +++ b/test/orm/dml/test_update_delete_where.py @@ -18,6 +18,7 @@ from sqlalchemy import String from sqlalchemy import testing from sqlalchemy import text from sqlalchemy import update +from sqlalchemy import values from sqlalchemy.orm import backref from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import joinedload @@ -237,6 +238,43 @@ class UpdateDeleteTest(fixtures.MappedTest): synchronize_session="fake", ) + @testing.requires.table_value_constructor + def test_update_against_external_non_mapped_cols(self): + """test #8656""" + User = self.classes.User + + data = values( + column("id", Integer), + column("name", String), + column("age_int", Integer), + name="myvalues", + ).data( + [ + (1, "new john", 35), + (3, "new jill", 39), + ] + ) + + # this statement will use "fetch" strategy, as "evaluate" will + # not be available + stmt = ( + update(User) + .where(User.id == data.c.id) + .values(age=data.c.age_int, name=data.c.name) + ) + s = fixture_session() + + john, jack, jill, jane = s.scalars( + select(User).order_by(User.id) + ).all() + + s.execute(stmt) + + eq_(john.name, "new john") + eq_(jill.name, "new jill") + eq_(john.age, 35) + eq_(jill.age, 39) + def test_illegal_operations(self): User = self.classes.User Address = self.classes.Address