From: Mike Bayer Date: Tue, 28 Jun 2022 22:55:19 +0000 (-0400) Subject: produce column copies up the whole hierarchy first X-Git-Tag: rel_1_4_40~43^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3ea6c365a2dd7060f0c6d462e2e7752c832f59af;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git produce column copies up the whole hierarchy first Fixed issue where a hierarchy of classes set up as an abstract or mixin declarative classes could not declare standalone columns on a superclass that would then be copied correctly to a :class:`_orm.declared_attr` callable that wanted to make use of them on a descendant class. Originally it looked like this would produce an ordering change, however an adjustment to the flow for produce_column_copies has avoided that for now. Fixes: #8190 Change-Id: I4e2ee74edb110793eb42691c3e4a0e0535fba7e9 (cherry picked from commit 9d12d493eb38f958c2d50da28f83ccc6de01f0dc) --- diff --git a/doc/build/changelog/unreleased_14/8190.rst b/doc/build/changelog/unreleased_14/8190.rst new file mode 100644 index 0000000000..934e44cf51 --- /dev/null +++ b/doc/build/changelog/unreleased_14/8190.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, orm, declarative + :tickets: 8190 + + Fixed issue where a hierarchy of classes set up as an abstract or mixin + declarative classes could not declare standalone columns on a superclass + that would then be copied correctly to a :class:`_orm.declared_attr` + callable that wanted to make use of them on a descendant class. diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py index ed4ccd1968..6e1c79745f 100644 --- a/lib/sqlalchemy/orm/decl_base.py +++ b/lib/sqlalchemy/orm/decl_base.py @@ -459,7 +459,14 @@ class _ClassScanMapperConfig(_MapperConfig): attribute_is_overridden = self._cls_attr_override_checker(self.cls) + bases = [] + for base in cls.__mro__: + # collect bases and make sure standalone columns are copied + # to be the column they will ultimately be on the class, + # so that declared_attr functions use the right columns. + # need to do this all the way up the hierarchy first + # (see #8190) class_mapped = ( base is not cls @@ -472,9 +479,34 @@ class _ClassScanMapperConfig(_MapperConfig): local_attributes_for_class = self._cls_attr_resolver(base) if not class_mapped and base is not cls: - self._produce_column_copies( - local_attributes_for_class, attribute_is_overridden + locally_collected_columns = self._produce_column_copies( + local_attributes_for_class, + attribute_is_overridden, + ) + else: + locally_collected_columns = {} + + bases.append( + ( + base, + class_mapped, + local_attributes_for_class, + locally_collected_columns, ) + ) + + for ( + base, + class_mapped, + local_attributes_for_class, + locally_collected_columns, + ) in bases: + + # this transfer can also take place as we scan each name + # for finer-grained control of how collected_attributes is + # populated, as this is what impacts column ordering. + # however it's simpler to get it out of the way here. + dict_.update(locally_collected_columns) for name, obj, is_dataclass in local_attributes_for_class(): if name == "__mapper_args__": @@ -640,6 +672,7 @@ class _ClassScanMapperConfig(_MapperConfig): ): cls = self.cls dict_ = self.dict_ + locally_collected_attributes = {} column_copies = self.column_copies # copy mixin columns to the mapped class @@ -664,7 +697,8 @@ class _ClassScanMapperConfig(_MapperConfig): column_copies[obj] = copy_ = obj._copy() copy_._creation_order = obj._creation_order setattr(cls, name, copy_) - dict_[name] = copy_ + locally_collected_attributes[name] = copy_ + return locally_collected_attributes def _extract_mappable_attributes(self): cls = self.cls diff --git a/test/orm/declarative/test_mixin.py b/test/orm/declarative/test_mixin.py index 78ab4dbfc3..f8e0cf5adb 100644 --- a/test/orm/declarative/test_mixin.py +++ b/test/orm/declarative/test_mixin.py @@ -2140,6 +2140,53 @@ class DeclaredAttrTest(DeclarativeTestBase, testing.AssertsCompiledSQL): "> :param_1", ) + def test_multilevel_mixin_attr_refers_to_column_copies(self): + """test #8190. + + This test is the same idea as test_mixin_attr_refers_to_column_copies + but tests the column copies from superclasses. + + """ + counter = mock.Mock() + + class SomeOtherMixin: + status = Column(String) + + class HasAddressCount(SomeOtherMixin): + id = Column(Integer, primary_key=True) + + @declared_attr + def address_count(cls): + counter(cls.id) + counter(cls.status) + return column_property( + select(func.count(Address.id)) + .where(Address.user_id == cls.id) + .where(cls.status == "some status") + .scalar_subquery() + ) + + class Address(Base): + __tablename__ = "address" + id = Column(Integer, primary_key=True) + user_id = Column(ForeignKey("user.id")) + + class User(Base, HasAddressCount): + __tablename__ = "user" + + eq_(counter.mock_calls, [mock.call(User.id), mock.call(User.status)]) + + sess = fixture_session() + self.assert_compile( + sess.query(User).having(User.address_count > 5), + "SELECT (SELECT count(address.id) AS count_1 FROM address " + 'WHERE address.user_id = "user".id AND "user".status = :param_1) ' + 'AS anon_1, "user".status AS user_status, "user".id AS user_id ' + 'FROM "user" HAVING (SELECT count(address.id) AS count_1 ' + 'FROM address WHERE address.user_id = "user".id ' + 'AND "user".status = :param_1) > :param_2', + ) + class AbstractTest(DeclarativeTestBase): def test_abstract_boolean(self):