From: Mike Bayer Date: Fri, 27 May 2022 20:07:01 +0000 (-0400) Subject: remove "deannotate" from column_property expression X-Git-Tag: rel_1_4_37~6^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e247b3450945a46eff1a0c96de8d46384ac1d644;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git remove "deannotate" from column_property expression Fixed issue where using a :func:`_orm.column_property` construct containing a subquery against an already-mapped column attribute would not correctly apply ORM-compilation behaviors to the subquery, including that the "IN" expression added for a single-table inherits expression would fail to be included. This fix involves a few tweaks in the ORM adaptation logic, including a missing "parententity" adaptation on the mapper side. The specific mechanics here have a lot of moving parts so we will continue to add tests to assert these cases. In particular a more complete test for issue #2316 is added that was relying upon the deannotate happening here. Fixes: #8064 Change-Id: Ia85dd12dcf6e7c002b30de4a27d7aa66cb3cd20e (cherry picked from commit 8c5cc8ae255a7580e2ff339659cf66cd2c6e02c1) --- diff --git a/doc/build/changelog/unreleased_14/8064.rst b/doc/build/changelog/unreleased_14/8064.rst new file mode 100644 index 0000000000..ccac2ad03d --- /dev/null +++ b/doc/build/changelog/unreleased_14/8064.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 8064 + + Fixed issue where using a :func:`_orm.column_property` construct containing + a subquery against an already-mapped column attribute would not correctly + apply ORM-compilation behaviors to the subquery, including that the "IN" + expression added for a single-table inherits expression would fail to be + included. diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index ad68820125..21809e7e6e 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -1755,7 +1755,7 @@ class Mapper( col.key = col._tq_key_label = key self.columns.add(col, key) - for col in prop.columns + prop._orig_columns: + for col in prop.columns: for col in col.proxy_set: self._columntoproperty[col] = prop @@ -2091,9 +2091,9 @@ class Mapper( @HasMemoized.memoized_attribute def _single_table_criterion(self): if self.single and self.inherits and self.polymorphic_on is not None: - return self.polymorphic_on._annotate({"parentmapper": self}).in_( - m.polymorphic_identity for m in self.self_and_descendants - ) + return self.polymorphic_on._annotate( + {"parententity": self, "parentmapper": self} + ).in_(m.polymorphic_identity for m in self.self_and_descendants) else: return None diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index b5ac9b8794..d32af17464 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -20,7 +20,6 @@ from .descriptor_props import SynonymProperty from .interfaces import PropComparator from .interfaces import StrategizedProperty from .relationships import RelationshipProperty -from .util import _orm_full_deannotate from .. import log from .. import util from ..sql import coercions @@ -49,7 +48,6 @@ class ColumnProperty(StrategizedProperty): _links_to_entity = False __slots__ = ( - "_orig_columns", "columns", "group", "deferred", @@ -155,14 +153,8 @@ class ColumnProperty(StrategizedProperty): """ super(ColumnProperty, self).__init__() - self._orig_columns = [ - coercions.expect(roles.LabeledColumnExprRole, c) for c in columns - ] self.columns = [ - coercions.expect( - roles.LabeledColumnExprRole, _orm_full_deannotate(c) - ) - for c in columns + coercions.expect(roles.LabeledColumnExprRole, c) for c in columns ] self.group = kwargs.pop("group", None) self.deferred = kwargs.pop("deferred", False) diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 7f72c1fc08..4afcd0fb86 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -415,7 +415,7 @@ class ORMAdapter(sql_util.ColumnAdapter): def _include_fn(self, elem): entity = elem._annotations.get("parentmapper", None) - return not entity or entity.isa(self.mapper) + return not entity or entity.common_parent(self.mapper) class AliasedClass(object): diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py index 8c2e9d8de6..35fcabaab1 100644 --- a/lib/sqlalchemy/testing/fixtures.py +++ b/lib/sqlalchemy/testing/fixtures.py @@ -91,6 +91,10 @@ class TestBase(object): yield reg reg.dispose() + @config.fixture + def decl_base(self, registry): + return registry.generate_base() + @config.fixture() def future_connection(self, future_engine, connection): # integrate the future_engine and connection fixtures so diff --git a/test/orm/inheritance/test_single.py b/test/orm/inheritance/test_single.py index fbafdd85be..6f611eb3ad 100644 --- a/test/orm/inheritance/test_single.py +++ b/test/orm/inheritance/test_single.py @@ -13,6 +13,7 @@ from sqlalchemy import true from sqlalchemy import util from sqlalchemy.orm import aliased from sqlalchemy.orm import Bundle +from sqlalchemy.orm import column_property from sqlalchemy.orm import joinedload from sqlalchemy.orm import relationship from sqlalchemy.orm import Session @@ -1979,3 +1980,35 @@ class EagerDefaultEvalTestPolymorphic(EagerDefaultEvalTest): super(EagerDefaultEvalTestPolymorphic, cls).setup_classes( with_polymorphic="*" ) + + +class ColExprTest(AssertsCompiledSQL, fixtures.TestBase): + def test_discrim_on_column_prop(self, registry): + Base = registry.generate_base() + + class Employee(Base): + __tablename__ = "employee" + id = Column(Integer, primary_key=True) + type = Column(String(20)) + + __mapper_args__ = { + "polymorphic_on": "type", + "polymorphic_identity": "employee", + } + + class Engineer(Employee): + __mapper_args__ = {"polymorphic_identity": "engineer"} + + class Company(Base): + __tablename__ = "company" + id = Column(Integer, primary_key=True) + + max_engineer_id = column_property( + select(func.max(Engineer.id)).scalar_subquery() + ) + + self.assert_compile( + select(Company.max_engineer_id), + "SELECT (SELECT max(employee.id) AS max_1 FROM employee " + "WHERE employee.type IN (__[POSTCOMPILE_type_1])) AS anon_1", + ) diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 879cc2b817..b2a5ed33f3 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -179,6 +179,37 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): # has to lazy load the addresses self.assert_sql_count(testing.db, go, 1) + def test_column_property_adaptation(self, decl_base): + """test #2316 in support of #8064""" + + class A(decl_base): + __tablename__ = "a" + id = Column(Integer, primary_key=True) + type = Column(String(40), nullable=False) + __mapper_args__ = {"polymorphic_on": type} + + A.anything = column_property(A.id + 1000) + + class B(A): + __tablename__ = "b" + account_id = Column(Integer, ForeignKey("a.id"), primary_key=True) + x_id = Column(Integer, ForeignKey("x.id"), nullable=False) + __mapper_args__ = {"polymorphic_identity": "named"} + + class X(decl_base): + __tablename__ = "x" + id = Column(Integer, primary_key=True) + b = relationship("B") + + self.assert_compile( + select(X).options(joinedload(X.b)), + "SELECT x.id, a_1.id AS id_1, a_1.type, a_1.id + :id_2 AS anon_1, " + "b_1.account_id, b_1.x_id FROM x " + "LEFT OUTER JOIN " + "(a AS a_1 JOIN b AS b_1 ON a_1.id = b_1.account_id) " + "ON x.id = b_1.x_id", + ) + def test_no_render_in_subquery(self): """test #6378""" diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index 9e2a7f63a6..c8a87cf5b7 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -846,7 +846,8 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): ) @testing.combinations((True,), (False,)) - def test_add_column_prop_deannotate(self, autoalias): + def test_add_column_prop_adaption(self, autoalias): + """test ultimately from #2316 revised for #8064""" User, users = self.classes.User, self.tables.users Address, addresses = self.classes.Address, self.tables.addresses @@ -907,9 +908,13 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): "users_1.id = addresses.user_id", ) - def test_column_prop_deannotate(self): - """test that column property deannotates, - bringing expressions down to the original mapped columns. + def test_column_prop_stays_annotated(self): + """test ultimately from #2316 revised for #8064. + + previously column_property() would deannotate the given expression, + however this interfered with some compilation sceanrios. + + """ User, users = self.classes.User, self.tables.users m = self.mapper(User, users) @@ -921,14 +926,18 @@ class MapperTest(_fixtures.FixtureTest, AssertsCompiledSQL): m.add_property("y", column_property(expr2.scalar_subquery())) assert User.x.property.columns[0] is not expr - assert User.x.property.columns[0].element.left is users.c.name - # a deannotate needs to clone the base, in case - # the original one referenced annotated elements. - assert User.x.property.columns[0].element.right is not expr.right + + assert ( + User.x.property.columns[0].element.left + is User.name.comparator.expr + ) + + assert User.x.property.columns[0].element.right is expr.right assert User.y.property.columns[0] is not expr2 assert ( - User.y.property.columns[0].element._raw_columns[0] is users.c.name + User.y.property.columns[0].element._raw_columns[0] + is User.name.expression ) assert User.y.property.columns[0].element._raw_columns[1] is users.c.id