From 335cdea7c72c279f8cf9de278ac96f7dda2bca15 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 27 May 2022 16:07:01 -0400 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_14/8064.rst | 9 ++++++ lib/sqlalchemy/orm/mapper.py | 8 +++--- lib/sqlalchemy/orm/properties.py | 11 +------- lib/sqlalchemy/orm/util.py | 2 +- test/orm/inheritance/test_single.py | 33 ++++++++++++++++++++++ test/orm/test_eager_relations.py | 31 ++++++++++++++++++++ test/orm/test_mapper.py | 27 ++++++++++++------ 7 files changed, 97 insertions(+), 24 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/8064.rst 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 2d3bceb928..2a227f9daa 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -1911,7 +1911,7 @@ class Mapper( self.columns.add(col, key) - for col in prop.columns + prop._orig_columns: + for col in prop.columns: for proxy_col in col.proxy_set: self._columntoproperty[proxy_col] = prop @@ -2260,9 +2260,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 7655f3ae2f..d77d6e63c0 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -38,7 +38,6 @@ from .interfaces import MapperProperty from .interfaces import PropComparator from .interfaces import StrategizedProperty from .relationships import Relationship -from .util import _orm_full_deannotate from .. import exc as sa_exc from .. import ForeignKey from .. import log @@ -103,7 +102,6 @@ class ColumnProperty( _links_to_entity = False columns: List[NamedColumn[Any]] - _orig_columns: List[NamedColumn[Any]] _is_polymorphic_discriminator: bool @@ -112,7 +110,6 @@ class ColumnProperty( comparator_factory: Type[PropComparator[_T]] __slots__ = ( - "_orig_columns", "columns", "group", "deferred", @@ -148,14 +145,8 @@ class ColumnProperty( attribute_options=attribute_options ) columns = (column,) + additional_columns - self._orig_columns = [ - coercions.expect(roles.LabeledColumnExprRole, c) for c in columns - ] self.columns = [ - _orm_full_deannotate( - coercions.expect(roles.LabeledColumnExprRole, c) - ) - for c in columns + coercions.expect(roles.LabeledColumnExprRole, c) for c in columns ] self.group = group self.deferred = deferred diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 520c95672f..c8c802d9f2 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -475,7 +475,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( diff --git a/test/orm/inheritance/test_single.py b/test/orm/inheritance/test_single.py index efb3c99798..86422000f7 100644 --- a/test/orm/inheritance/test_single.py +++ b/test/orm/inheritance/test_single.py @@ -13,6 +13,7 @@ from sqlalchemy import testing from sqlalchemy import true 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 @@ -1935,3 +1936,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 8c82450da0..a5bd4c75a2 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 d8cc48939b..f11ad31009 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -879,7 +879,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 @@ -940,9 +941,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) @@ -954,14 +959,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 -- 2.47.2