From: Mike Bayer Date: Fri, 20 Jun 2025 13:20:27 +0000 (-0400) Subject: Extend #12168 to relationship collections X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=95ad9fe42b888d7cdea60b7a71c833cbb8667887;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Extend #12168 to relationship collections As seen in [1] we need to extend the "no default set" case a bit more to help get dataclasses to act more like traditional mapped classes. [1] https://github.com/sqlalchemy/sqlalchemy/issues/9410#issuecomment-2989758246 Fixes: #12168 Change-Id: Icb11faf2203460ab5bca4f2eb5b08b7563cff758 --- diff --git a/doc/build/changelog/migration_21.rst b/doc/build/changelog/migration_21.rst index 5dcc9bea09..dd3d81a140 100644 --- a/doc/build/changelog/migration_21.rst +++ b/doc/build/changelog/migration_21.rst @@ -136,8 +136,8 @@ lambdas which do the same:: .. _change_12168: -ORM Mapped Dataclasses no longer populate implicit ``default`` in ``__dict__`` ------------------------------------------------------------------------------- +ORM Mapped Dataclasses no longer populate implicit ``default``, collection-based ``default_factory`` in ``__dict__`` +-------------------------------------------------------------------------------------------------------------------- This behavioral change addresses a widely reported issue with SQLAlchemy's :ref:`orm_declarative_native_dataclasses` feature that was introduced in 2.0. @@ -290,6 +290,39 @@ on the instance, delivered via descriptor:: >>> so.status default_status +default_factory for collection-based relationships internally uses DONT_SET +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +A late add to the behavioral change brings equivalent behavior to the +use of the :paramref:`_orm.relationship.default_factory` parameter with +collection-based relationships. This attribute is `documented ` +as being limited to exactly the collection class that's stated on the left side +of the annotation, which is now enforced at mapper configuration time:: + + class Parent(Base): + __tablename__ = "parents" + + id: Mapped[int] = mapped_column(primary_key=True, init=False) + name: Mapped[str] + + children: Mapped[list["Child"]] = relationship(default_factory=list) + +With the above mapping, the actual +:paramref:`_orm.relationship.default_factory` parameter is replaced internally +to instead use the same ``DONT_SET`` constant that's applied to +:paramref:`_orm.relationship.default` for many-to-one relationships. +SQLAlchemy's existing collection-on-attribute access behavior occurs as always +on access:: + + >>> p1 = Parent(name="p1") + >>> p1.children + [] + +This change to :paramref:`_orm.relationship.default_factory` accommodates a +similar merge-based condition where an empty collection would be forced into +a new object that in fact wants a merged collection to arrive. + + Related Changes ^^^^^^^^^^^^^^^ diff --git a/doc/build/changelog/unreleased_21/12168.rst b/doc/build/changelog/unreleased_21/12168.rst index 6521733eae..ee63cd14fe 100644 --- a/doc/build/changelog/unreleased_21/12168.rst +++ b/doc/build/changelog/unreleased_21/12168.rst @@ -4,14 +4,18 @@ A significant behavioral change has been made to the behavior of the :paramref:`_orm.mapped_column.default` and - :paramref:`_orm.relationship.default` parameters, when used with - SQLAlchemy's :ref:`orm_declarative_native_dataclasses` feature introduced - in 2.0, where the given value (assumed to be an immutable scalar value) is - no longer passed to the ``@dataclass`` API as a real default, instead a - token that leaves the value un-set in the object's ``__dict__`` is used, in - conjunction with a descriptor-level default. This prevents an un-set - default value from overriding a default that was actually set elsewhere, - such as in relationship / foreign key assignment patterns as well as in + :paramref:`_orm.relationship.default` parameters, as well as the + :paramref:`_orm.relationship.default_factory` parameter with + collection-based relationships, when used with SQLAlchemy's + :ref:`orm_declarative_native_dataclasses` feature introduced in 2.0, where + the given value (assumed to be an immutable scalar value for + :paramref:`_orm.mapped_column.default` and a simple collection class for + :paramref:`_orm.relationship.default_factory`) is no longer passed to the + ``@dataclass`` API as a real default, instead a token that leaves the value + un-set in the object's ``__dict__`` is used, in conjunction with a + descriptor-level default. This prevents an un-set default value from + overriding a default that was actually set elsewhere, such as in + relationship / foreign key assignment patterns as well as in :meth:`_orm.Session.merge` scenarios. See the full writeup in the :ref:`whatsnew_21_toplevel` document which includes guidance on how to re-enable the 2.0 version of the behavior if needed. diff --git a/doc/build/orm/dataclasses.rst b/doc/build/orm/dataclasses.rst index 7f377ca399..f8af3fb8d6 100644 --- a/doc/build/orm/dataclasses.rst +++ b/doc/build/orm/dataclasses.rst @@ -388,6 +388,7 @@ attributes from non-dataclass mixins to be part of the dataclass. +.. _orm_declarative_dc_relationships: Relationship Configuration ^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index e8886a1181..24b5af7795 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -1947,6 +1947,10 @@ class _CollectionAttributeImpl(_HasCollectionAdapter, _AttributeImpl): pop: bool = False, _adapt: bool = True, ) -> None: + + if value is DONT_SET: + return + iterable = orig_iterable = value new_keys = None diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index 9045e09a7c..266acbc472 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -384,6 +384,8 @@ class _DataclassDefaultsDontSet(_DCAttributeOptions): _default_scalar_value: Any + _disable_dataclass_default_factory: bool = False + def _get_dataclass_setup_options( self, decl_scan: _ClassScanMapperConfig, @@ -391,21 +393,35 @@ class _DataclassDefaultsDontSet(_DCAttributeOptions): dataclass_setup_arguments: _DataclassArguments, ) -> _AttributeOptions: + disable_descriptor_defaults = getattr( + decl_scan.cls, "_sa_disable_descriptor_defaults", False + ) + dataclasses_default = self._attribute_options.dataclasses_default + dataclasses_default_factory = ( + self._attribute_options.dataclasses_default_factory + ) + if ( dataclasses_default is not _NoArg.NO_ARG and not callable(dataclasses_default) - and not getattr( - decl_scan.cls, "_sa_disable_descriptor_defaults", False - ) + and not disable_descriptor_defaults ): self._default_scalar_value = ( self._attribute_options.dataclasses_default ) return self._attribute_options._replace( - dataclasses_default=DONT_SET + dataclasses_default=DONT_SET, + ) + elif ( + self._disable_dataclass_default_factory + and dataclasses_default_factory is not _NoArg.NO_ARG + and not disable_descriptor_defaults + ): + return self._attribute_options._replace( + dataclasses_default=DONT_SET, + dataclasses_default_factory=_NoArg.NO_ARG, ) - return self._attribute_options diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index 481af4f360..eba022685c 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -399,6 +399,7 @@ class RelationshipProperty( direction: RelationshipDirection _init_args: _RelationshipArgs + _disable_dataclass_default_factory = True def __init__( self, @@ -1431,8 +1432,11 @@ class RelationshipProperty( criterion = adapt_source(criterion) return criterion + def _format_as_string(self, class_: type, key: str) -> str: + return f"{class_.__name__}.{key}" + def __str__(self) -> str: - return str(self.parent.class_.__name__) + "." + self.key + return self._format_as_string(self.parent.class_, self.key) def merge( self, @@ -1896,6 +1900,18 @@ class RelationshipProperty( if self.argument is None: self.argument = cast("_RelationshipArgumentType[_T]", argument) + if ( + self._attribute_options.dataclasses_default_factory + is not _NoArg.NO_ARG + and self._attribute_options.dataclasses_default_factory + is not self.collection_class + ): + raise sa_exc.ArgumentError( + f"For relationship {self._format_as_string(cls, key)} using " + "dataclass options, default_factory must be exactly " + f"{self.collection_class}" + ) + @util.preload_module("sqlalchemy.orm.mapper") def _setup_entity(self, __argument: Any = None, /) -> None: if "entity" in self.__dict__: diff --git a/test/orm/declarative/test_dc_transforms.py b/test/orm/declarative/test_dc_transforms.py index 34b9d1982b..2d20c102aa 100644 --- a/test/orm/declarative/test_dc_transforms.py +++ b/test/orm/declarative/test_dc_transforms.py @@ -909,68 +909,43 @@ class DCTransformsTest(AssertsCompiledSQL, fixtures.TestBase): class RelationshipDefaultFactoryTest(fixtures.TestBase): - def test_list(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - - bs: Mapped[List["B"]] = relationship( # noqa: F821 - default_factory=lambda: [B(data="hi")] - ) - - class B(dc_decl_base): - __tablename__ = "b" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] - - a1 = A() - eq_(a1.bs[0].data, "hi") - - def test_set(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - - bs: Mapped[Set["B"]] = relationship( # noqa: F821 - default_factory=lambda: {B(data="hi")} - ) - - class B(dc_decl_base, unsafe_hash=True): - __tablename__ = "b" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] - a1 = A() - eq_(a1.bs.pop().data, "hi") - - def test_oh_no_mismatch(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) + @testing.variation("collection_type", ["list", "set", "list_set_mismatch"]) + def test_no_funny_business( + self, + dc_decl_base: Type[MappedAsDataclass], + collection_type: testing.Variation, + ): + if collection_type.list: + expected = "list" + else: + expected = "set" - bs: Mapped[Set["B"]] = relationship( # noqa: F821 - default_factory=lambda: [B(data="hi")] - ) + with expect_raises_message( + exc.ArgumentError, + f"For relationship A.bs using dataclass options, " + f"default_factory must be exactly ", + ): - class B(dc_decl_base, unsafe_hash=True): - __tablename__ = "b" + class A(dc_decl_base): + __tablename__ = "a" - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] + id: Mapped[int] = mapped_column(primary_key=True, init=False) - # old school collection mismatch error FTW - with expect_raises_message( - TypeError, "Incompatible collection type: list is not set-like" - ): - A() + if collection_type.list: + bs: Mapped[List["B"]] = relationship( # noqa: F821 + default_factory=lambda: [B(data="hi")] # noqa: F821 + ) + elif collection_type.set: + bs: Mapped[Set["B"]] = relationship( # noqa: F821 + default_factory=lambda: {B(data="hi")} # noqa: F821 + ) + elif collection_type.list_set_mismatch: + bs: Mapped[Set["B"]] = relationship( # noqa: F821 + default_factory=list + ) + else: + collection_type.fail() def test_one_to_one_example(self, dc_decl_base: Type[MappedAsDataclass]): """test example in the relationship docs will derive uselist=False @@ -2530,6 +2505,72 @@ class UseDescriptorDefaultsTest(fixtures.TestBase, testing.AssertsCompiledSQL): # field was explicit so is overridden by merge eq_(u3.status, "default_status") + @testing.variation("use_attr_init", [True, False]) + def test_collection_merge_scenario(self, dc_decl_base, use_attr_init): + if use_attr_init: + attr_init_kw = {} + else: + attr_init_kw = {"init": False} + + class MyClass(dc_decl_base): + __tablename__ = "myclass" + + id: Mapped[int] = mapped_column( + primary_key=True, autoincrement=False + ) + name: Mapped[str] + things: Mapped[List["Thing"]] = relationship( + cascade="all, delete-orphan", + default_factory=list, + **attr_init_kw, + ) + + class Thing(dc_decl_base): + __tablename__ = "thing" + id: Mapped[int] = mapped_column( + primary_key=True, autoincrement=False + ) + my_id: Mapped[int] = mapped_column( + ForeignKey("myclass.id"), init=False + ) + name: Mapped[str] + + dc_decl_base.metadata.create_all(testing.db) + + with Session(testing.db) as sess: + if use_attr_init: + u1 = MyClass(id=1, name="x", things=[Thing(id=1, name="t1")]) + else: + u1 = MyClass(id=1, name="x") + u1.things = [Thing(id=1, name="t1")] + sess.add(u1) + + sess.flush() + + u2 = sess.merge(MyClass(id=1, name="y")) + is_(u2, u1) + eq_(u2.name, "y") + + if MyClass.use_descriptor_defaults: + tt = Thing(id=1, name="t1") + tt.my_id = 1 + eq_(u2.things, [tt]) + else: + eq_(u2.things, []) + + if use_attr_init: + u3 = sess.merge(MyClass(id=1, name="z", things=[])) + else: + mc = MyClass(id=1, name="z") + mc.things = [] + u3 = sess.merge(mc) + + is_(u3, u1) + eq_(u3.name, "z") + + # field was explicit so is overridden by merge + eq_(u3.things, []) + class SynonymDescriptorDefaultTest(AssertsCompiledSQL, fixtures.TestBase): """test new behaviors for synonyms given dataclasses descriptor defaults diff --git a/test/orm/declarative/test_dc_transforms_future_anno_sync.py b/test/orm/declarative/test_dc_transforms_future_anno_sync.py index d1f319e240..455d63428b 100644 --- a/test/orm/declarative/test_dc_transforms_future_anno_sync.py +++ b/test/orm/declarative/test_dc_transforms_future_anno_sync.py @@ -922,68 +922,43 @@ class DCTransformsTest(AssertsCompiledSQL, fixtures.TestBase): class RelationshipDefaultFactoryTest(fixtures.TestBase): - def test_list(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - - bs: Mapped[List["B"]] = relationship( # noqa: F821 - default_factory=lambda: [B(data="hi")] - ) - - class B(dc_decl_base): - __tablename__ = "b" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] - - a1 = A() - eq_(a1.bs[0].data, "hi") - - def test_set(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - - bs: Mapped[Set["B"]] = relationship( # noqa: F821 - default_factory=lambda: {B(data="hi")} - ) - - class B(dc_decl_base, unsafe_hash=True): - __tablename__ = "b" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] - a1 = A() - eq_(a1.bs.pop().data, "hi") - - def test_oh_no_mismatch(self, dc_decl_base: Type[MappedAsDataclass]): - class A(dc_decl_base): - __tablename__ = "a" - - id: Mapped[int] = mapped_column(primary_key=True, init=False) + @testing.variation("collection_type", ["list", "set", "list_set_mismatch"]) + def test_no_funny_business( + self, + dc_decl_base: Type[MappedAsDataclass], + collection_type: testing.Variation, + ): + if collection_type.list: + expected = "list" + else: + expected = "set" - bs: Mapped[Set["B"]] = relationship( # noqa: F821 - default_factory=lambda: [B(data="hi")] - ) + with expect_raises_message( + exc.ArgumentError, + f"For relationship A.bs using dataclass options, " + f"default_factory must be exactly ", + ): - class B(dc_decl_base, unsafe_hash=True): - __tablename__ = "b" + class A(dc_decl_base): + __tablename__ = "a" - id: Mapped[int] = mapped_column(primary_key=True, init=False) - a_id = mapped_column(ForeignKey("a.id"), init=False) - data: Mapped[str] + id: Mapped[int] = mapped_column(primary_key=True, init=False) - # old school collection mismatch error FTW - with expect_raises_message( - TypeError, "Incompatible collection type: list is not set-like" - ): - A() + if collection_type.list: + bs: Mapped[List["B"]] = relationship( # noqa: F821 + default_factory=lambda: [B(data="hi")] # noqa: F821 + ) + elif collection_type.set: + bs: Mapped[Set["B"]] = relationship( # noqa: F821 + default_factory=lambda: {B(data="hi")} # noqa: F821 + ) + elif collection_type.list_set_mismatch: + bs: Mapped[Set["B"]] = relationship( # noqa: F821 + default_factory=list + ) + else: + collection_type.fail() def test_one_to_one_example(self, dc_decl_base: Type[MappedAsDataclass]): """test example in the relationship docs will derive uselist=False @@ -2549,6 +2524,72 @@ class UseDescriptorDefaultsTest(fixtures.TestBase, testing.AssertsCompiledSQL): # field was explicit so is overridden by merge eq_(u3.status, "default_status") + @testing.variation("use_attr_init", [True, False]) + def test_collection_merge_scenario(self, dc_decl_base, use_attr_init): + if use_attr_init: + attr_init_kw = {} + else: + attr_init_kw = {"init": False} + + class MyClass(dc_decl_base): + __tablename__ = "myclass" + + id: Mapped[int] = mapped_column( + primary_key=True, autoincrement=False + ) + name: Mapped[str] + things: Mapped[List["Thing"]] = relationship( + cascade="all, delete-orphan", + default_factory=list, + **attr_init_kw, + ) + + class Thing(dc_decl_base): + __tablename__ = "thing" + id: Mapped[int] = mapped_column( + primary_key=True, autoincrement=False + ) + my_id: Mapped[int] = mapped_column( + ForeignKey("myclass.id"), init=False + ) + name: Mapped[str] + + dc_decl_base.metadata.create_all(testing.db) + + with Session(testing.db) as sess: + if use_attr_init: + u1 = MyClass(id=1, name="x", things=[Thing(id=1, name="t1")]) + else: + u1 = MyClass(id=1, name="x") + u1.things = [Thing(id=1, name="t1")] + sess.add(u1) + + sess.flush() + + u2 = sess.merge(MyClass(id=1, name="y")) + is_(u2, u1) + eq_(u2.name, "y") + + if MyClass.use_descriptor_defaults: + tt = Thing(id=1, name="t1") + tt.my_id = 1 + eq_(u2.things, [tt]) + else: + eq_(u2.things, []) + + if use_attr_init: + u3 = sess.merge(MyClass(id=1, name="z", things=[])) + else: + mc = MyClass(id=1, name="z") + mc.things = [] + u3 = sess.merge(mc) + + is_(u3, u1) + eq_(u3.name, "z") + + # field was explicit so is overridden by merge + eq_(u3.things, []) + class SynonymDescriptorDefaultTest(AssertsCompiledSQL, fixtures.TestBase): """test new behaviors for synonyms given dataclasses descriptor defaults