From: Mike Bayer Date: Thu, 9 Nov 2023 15:27:19 +0000 (-0500) Subject: use a private return class for the "catch all" relationship X-Git-Tag: rel_2_0_29~6^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=33344b0aa423c71a2d5c91257033c8bab2c547f9;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git use a private return class for the "catch all" relationship Fixed Declarative issue where typing a relationship using :class:`_orm.Relationship` rather than :class:`_orm.Mapped` would inadvertently pull in the "dynamic" relationship loader strategy for that attribute. Fixes: #10611 Change-Id: Ie4421050b583827fdf96c27ae9d7fe7ca596e77e (cherry picked from commit 7cfb940b1a9392f6e3784aff8f487c37ebcd588b) --- diff --git a/doc/build/changelog/unreleased_20/10611.rst b/doc/build/changelog/unreleased_20/10611.rst new file mode 100644 index 0000000000..2627e4d37c --- /dev/null +++ b/doc/build/changelog/unreleased_20/10611.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, orm + :tickets: 10611 + + Fixed Declarative issue where typing a relationship using + :class:`_orm.Relationship` rather than :class:`_orm.Mapped` would + inadvertently pull in the "dynamic" relationship loader strategy for that + attribute. diff --git a/lib/sqlalchemy/ext/mypy/names.py b/lib/sqlalchemy/ext/mypy/names.py index 35b4e2ba81..fc3d708e7d 100644 --- a/lib/sqlalchemy/ext/mypy/names.py +++ b/lib/sqlalchemy/ext/mypy/names.py @@ -58,6 +58,14 @@ NAMED_TYPE_BUILTINS_STR = "builtins.str" NAMED_TYPE_BUILTINS_LIST = "builtins.list" NAMED_TYPE_SQLA_MAPPED = "sqlalchemy.orm.base.Mapped" +_RelFullNames = { + "sqlalchemy.orm.relationships.Relationship", + "sqlalchemy.orm.relationships.RelationshipProperty", + "sqlalchemy.orm.relationships._RelationshipDeclared", + "sqlalchemy.orm.Relationship", + "sqlalchemy.orm.RelationshipProperty", +} + _lookup: Dict[str, Tuple[int, Set[str]]] = { "Column": ( COLUMN, @@ -66,24 +74,9 @@ _lookup: Dict[str, Tuple[int, Set[str]]] = { "sqlalchemy.sql.Column", }, ), - "Relationship": ( - RELATIONSHIP, - { - "sqlalchemy.orm.relationships.Relationship", - "sqlalchemy.orm.relationships.RelationshipProperty", - "sqlalchemy.orm.Relationship", - "sqlalchemy.orm.RelationshipProperty", - }, - ), - "RelationshipProperty": ( - RELATIONSHIP, - { - "sqlalchemy.orm.relationships.Relationship", - "sqlalchemy.orm.relationships.RelationshipProperty", - "sqlalchemy.orm.Relationship", - "sqlalchemy.orm.RelationshipProperty", - }, - ), + "Relationship": (RELATIONSHIP, _RelFullNames), + "RelationshipProperty": (RELATIONSHIP, _RelFullNames), + "_RelationshipDeclared": (RELATIONSHIP, _RelFullNames), "registry": ( REGISTRY, { diff --git a/lib/sqlalchemy/orm/_orm_constructors.py b/lib/sqlalchemy/orm/_orm_constructors.py index b9f618af0d..7cb536b297 100644 --- a/lib/sqlalchemy/orm/_orm_constructors.py +++ b/lib/sqlalchemy/orm/_orm_constructors.py @@ -28,8 +28,8 @@ from .properties import MappedColumn from .properties import MappedSQLExpression from .query import AliasOption from .relationships import _RelationshipArgumentType +from .relationships import _RelationshipDeclared from .relationships import _RelationshipSecondaryArgument -from .relationships import Relationship from .relationships import RelationshipProperty from .session import Session from .util import _ORMJoin @@ -950,7 +950,7 @@ def relationship( omit_join: Literal[None, False] = None, sync_backref: Optional[bool] = None, **kw: Any, -) -> Relationship[Any]: +) -> _RelationshipDeclared[Any]: """Provide a relationship between two mapped classes. This corresponds to a parent-child or associative table relationship. @@ -1756,7 +1756,7 @@ def relationship( """ - return Relationship( + return _RelationshipDeclared( argument, secondary=secondary, uselist=uselist, diff --git a/lib/sqlalchemy/orm/base.py b/lib/sqlalchemy/orm/base.py index 86af81cd6e..c9005298d8 100644 --- a/lib/sqlalchemy/orm/base.py +++ b/lib/sqlalchemy/orm/base.py @@ -21,6 +21,7 @@ from typing import Generic from typing import no_type_check from typing import Optional from typing import overload +from typing import Tuple from typing import Type from typing import TYPE_CHECKING from typing import TypeVar @@ -579,7 +580,7 @@ class InspectionAttr: """ - __slots__ = () + __slots__: Tuple[str, ...] = () is_selectable = False """Return True if this object is an instance of diff --git a/lib/sqlalchemy/orm/exc.py b/lib/sqlalchemy/orm/exc.py index 8ab831002a..39dd540112 100644 --- a/lib/sqlalchemy/orm/exc.py +++ b/lib/sqlalchemy/orm/exc.py @@ -16,6 +16,7 @@ from typing import Type from typing import TYPE_CHECKING from typing import TypeVar +from .util import _mapper_property_as_plain_name from .. import exc as sa_exc from .. import util from ..exc import MultipleResultsFound # noqa @@ -191,8 +192,8 @@ class LoaderStrategyException(sa_exc.InvalidRequestError): % ( util.clsname_as_plain_name(actual_strategy_type), requesting_property, - util.clsname_as_plain_name(applied_to_property_type), - util.clsname_as_plain_name(applies_to), + _mapper_property_as_plain_name(applied_to_property_type), + _mapper_property_as_plain_name(applies_to), ), ) diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index 2f090588fe..36336e7a2c 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -149,13 +149,17 @@ class ORMColumnDescription(TypedDict): class _IntrospectsAnnotations: __slots__ = () + @classmethod + def _mapper_property_name(cls) -> str: + return cls.__name__ + def found_in_pep593_annotated(self) -> Any: """return a copy of this object to use in declarative when the object is found inside of an Annotated object.""" raise NotImplementedError( - f"Use of the {self.__class__} construct inside of an " - f"Annotated object is not yet supported." + f"Use of the {self._mapper_property_name()!r} " + "construct inside of an Annotated object is not yet supported." ) def declarative_scan( @@ -181,7 +185,8 @@ class _IntrospectsAnnotations: raise sa_exc.ArgumentError( f"Python typing annotation is required for attribute " f'"{cls.__name__}.{key}" when primary argument(s) for ' - f'"{self.__class__.__name__}" construct are None or not present' + f'"{self._mapper_property_name()}" ' + "construct are None or not present" ) diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py index afff24c8cc..b5e33ffdbb 100644 --- a/lib/sqlalchemy/orm/relationships.py +++ b/lib/sqlalchemy/orm/relationships.py @@ -1755,19 +1755,17 @@ class RelationshipProperty( argument = extracted_mapped_annotation assert originating_module is not None - is_write_only = mapped_container is not None and issubclass( - mapped_container, WriteOnlyMapped - ) - if is_write_only: - self.lazy = "write_only" - self.strategy_key = (("lazy", self.lazy),) - - is_dynamic = mapped_container is not None and issubclass( - mapped_container, DynamicMapped - ) - if is_dynamic: - self.lazy = "dynamic" - self.strategy_key = (("lazy", self.lazy),) + if mapped_container is not None: + is_write_only = issubclass(mapped_container, WriteOnlyMapped) + is_dynamic = issubclass(mapped_container, DynamicMapped) + if is_write_only: + self.lazy = "write_only" + self.strategy_key = (("lazy", self.lazy),) + elif is_dynamic: + self.lazy = "dynamic" + self.strategy_key = (("lazy", self.lazy),) + else: + is_write_only = is_dynamic = False argument = de_optionalize_union_types(argument) @@ -3465,11 +3463,9 @@ _local_col_exclude = _ColInAnnotations("local", "should_not_adapt") _remote_col_exclude = _ColInAnnotations("remote", "should_not_adapt") -class Relationship( # type: ignore +class Relationship( RelationshipProperty[_T], _DeclarativeMapped[_T], - WriteOnlyMapped[_T], # not compatible with Mapped[_T] - DynamicMapped[_T], # not compatible with Mapped[_T] ): """Describes an object property that holds a single item or list of items that correspond to a related database table. @@ -3487,3 +3483,18 @@ class Relationship( # type: ignore inherit_cache = True """:meta private:""" + + +class _RelationshipDeclared( # type: ignore[misc] + Relationship[_T], + WriteOnlyMapped[_T], # not compatible with Mapped[_T] + DynamicMapped[_T], # not compatible with Mapped[_T] +): + """Relationship subclass used implicitly for declarative mapping.""" + + inherit_cache = True + """:meta private:""" + + @classmethod + def _mapper_property_name(cls) -> str: + return "Relationship" diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index f8431386e4..8e153e63db 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -2406,3 +2406,11 @@ def _extract_mapped_subtype( ) return annotated.__args__[0], annotated.__origin__ + + +def _mapper_property_as_plain_name(prop: Type[Any]) -> str: + if hasattr(prop, "_mapper_property_name"): + name = prop._mapper_property_name() + else: + name = None + return util.clsname_as_plain_name(prop, name) diff --git a/lib/sqlalchemy/util/langhelpers.py b/lib/sqlalchemy/util/langhelpers.py index 396a039771..4390ae1835 100644 --- a/lib/sqlalchemy/util/langhelpers.py +++ b/lib/sqlalchemy/util/langhelpers.py @@ -174,10 +174,11 @@ def string_or_unprintable(element: Any) -> str: return "unprintable element %r" % element -def clsname_as_plain_name(cls: Type[Any]) -> str: - return " ".join( - n.lower() for n in re.findall(r"([A-Z][a-z]+|SQL)", cls.__name__) - ) +def clsname_as_plain_name( + cls: Type[Any], use_name: Optional[str] = None +) -> str: + name = use_name or cls.__name__ + return " ".join(n.lower() for n in re.findall(r"([A-Z][a-z]+|SQL)", name)) def method_is_overridden( diff --git a/test/orm/declarative/test_tm_future_annotations_sync.py b/test/orm/declarative/test_tm_future_annotations_sync.py index 25e7781133..1a045ec1bf 100644 --- a/test/orm/declarative/test_tm_future_annotations_sync.py +++ b/test/orm/declarative/test_tm_future_annotations_sync.py @@ -68,14 +68,18 @@ from sqlalchemy.orm import foreign from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column from sqlalchemy.orm import MappedAsDataclass +from sqlalchemy.orm import Relationship from sqlalchemy.orm import relationship from sqlalchemy.orm import remote from sqlalchemy.orm import Session from sqlalchemy.orm import undefer from sqlalchemy.orm import WriteOnlyMapped +from sqlalchemy.orm.attributes import CollectionAttributeImpl from sqlalchemy.orm.collections import attribute_keyed_dict from sqlalchemy.orm.collections import KeyFuncDict +from sqlalchemy.orm.dynamic import DynamicAttributeImpl from sqlalchemy.orm.properties import MappedColumn +from sqlalchemy.orm.writeonly import WriteOnlyAttributeImpl from sqlalchemy.schema import CreateTable from sqlalchemy.sql.base import _NoArg from sqlalchemy.sql.sqltypes import Enum @@ -1185,8 +1189,7 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): with expect_raises_message( NotImplementedError, - r"Use of the \ construct inside of an Annotated " + r"Use of the 'Relationship' construct inside of an Annotated " r"object is not yet supported.", ): @@ -2491,6 +2494,42 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL): yield Base Base.registry.dispose() + @testing.combinations( + (Relationship, CollectionAttributeImpl), + (Mapped, CollectionAttributeImpl), + (WriteOnlyMapped, WriteOnlyAttributeImpl), + (DynamicMapped, DynamicAttributeImpl), + argnames="mapped_cls,implcls", + ) + def test_use_relationship(self, decl_base, mapped_cls, implcls): + """test #10611""" + + global B + + class B(decl_base): + __tablename__ = "b" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + a_id: Mapped[int] = mapped_column(ForeignKey("a.id")) + + class A(decl_base): + __tablename__ = "a" + + id: Mapped[int] = mapped_column(primary_key=True) + + # for future annotations support, need to write these + # directly in source code + if mapped_cls is Relationship: + bs: Relationship[List[B]] = relationship() + elif mapped_cls is Mapped: + bs: Mapped[List[B]] = relationship() + elif mapped_cls is WriteOnlyMapped: + bs: WriteOnlyMapped[List[B]] = relationship() + elif mapped_cls is DynamicMapped: + bs: DynamicMapped[List[B]] = relationship() + + decl_base.registry.configure() + assert isinstance(A.bs.impl, implcls) + def test_no_typing_in_rhs(self, decl_base): class A(decl_base): __tablename__ = "a" diff --git a/test/orm/declarative/test_typed_mapping.py b/test/orm/declarative/test_typed_mapping.py index 4afa33c731..175da29023 100644 --- a/test/orm/declarative/test_typed_mapping.py +++ b/test/orm/declarative/test_typed_mapping.py @@ -59,14 +59,18 @@ from sqlalchemy.orm import foreign from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column from sqlalchemy.orm import MappedAsDataclass +from sqlalchemy.orm import Relationship from sqlalchemy.orm import relationship from sqlalchemy.orm import remote from sqlalchemy.orm import Session from sqlalchemy.orm import undefer from sqlalchemy.orm import WriteOnlyMapped +from sqlalchemy.orm.attributes import CollectionAttributeImpl from sqlalchemy.orm.collections import attribute_keyed_dict from sqlalchemy.orm.collections import KeyFuncDict +from sqlalchemy.orm.dynamic import DynamicAttributeImpl from sqlalchemy.orm.properties import MappedColumn +from sqlalchemy.orm.writeonly import WriteOnlyAttributeImpl from sqlalchemy.schema import CreateTable from sqlalchemy.sql.base import _NoArg from sqlalchemy.sql.sqltypes import Enum @@ -1176,8 +1180,7 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): with expect_raises_message( NotImplementedError, - r"Use of the \ construct inside of an Annotated " + r"Use of the 'Relationship' construct inside of an Annotated " r"object is not yet supported.", ): @@ -2482,6 +2485,42 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL): yield Base Base.registry.dispose() + @testing.combinations( + (Relationship, CollectionAttributeImpl), + (Mapped, CollectionAttributeImpl), + (WriteOnlyMapped, WriteOnlyAttributeImpl), + (DynamicMapped, DynamicAttributeImpl), + argnames="mapped_cls,implcls", + ) + def test_use_relationship(self, decl_base, mapped_cls, implcls): + """test #10611""" + + # anno only: global B + + class B(decl_base): + __tablename__ = "b" + id: Mapped[int] = mapped_column(Integer, primary_key=True) + a_id: Mapped[int] = mapped_column(ForeignKey("a.id")) + + class A(decl_base): + __tablename__ = "a" + + id: Mapped[int] = mapped_column(primary_key=True) + + # for future annotations support, need to write these + # directly in source code + if mapped_cls is Relationship: + bs: Relationship[List[B]] = relationship() + elif mapped_cls is Mapped: + bs: Mapped[List[B]] = relationship() + elif mapped_cls is WriteOnlyMapped: + bs: WriteOnlyMapped[List[B]] = relationship() + elif mapped_cls is DynamicMapped: + bs: DynamicMapped[List[B]] = relationship() + + decl_base.registry.configure() + assert isinstance(A.bs.impl, implcls) + def test_no_typing_in_rhs(self, decl_base): class A(decl_base): __tablename__ = "a" diff --git a/test/typing/plain_files/orm/relationship.py b/test/typing/plain_files/orm/relationship.py index 6bfe19cc4e..5caf57de7b 100644 --- a/test/typing/plain_files/orm/relationship.py +++ b/test/typing/plain_files/orm/relationship.py @@ -21,6 +21,7 @@ from sqlalchemy.orm import joinedload from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column from sqlalchemy.orm import registry +from sqlalchemy.orm import Relationship from sqlalchemy.orm import relationship from sqlalchemy.orm import Session @@ -29,11 +30,22 @@ class Base(DeclarativeBase): pass +class Group(Base): + __tablename__ = "group" + + id: Mapped[int] = mapped_column(primary_key=True) + name: Mapped[str] = mapped_column() + + addresses_style_one_anno_only: Mapped[List["User"]] + addresses_style_two_anno_only: Mapped[Set["User"]] + + class User(Base): __tablename__ = "user" id: Mapped[int] = mapped_column(primary_key=True) name: Mapped[str] = mapped_column() + group_id = mapped_column(ForeignKey("group.id")) # this currently doesnt generate an error. not sure how to get the # overloads to hit this one, nor am i sure i really want to do that @@ -58,6 +70,19 @@ class Address(Base): user_style_one: Mapped[User] = relationship() user_style_two: Mapped["User"] = relationship() + rel_style_one: Relationship[List["MoreMail"]] = relationship() + # everything works even if using Relationship instead of Mapped + # users should use Mapped though + rel_style_one_anno_only: Relationship[Set["MoreMail"]] + + +class MoreMail(Base): + __tablename__ = "address" + + id = mapped_column(Integer, primary_key=True) + aggress_id = mapped_column(ForeignKey("address.id")) + email: Mapped[str] + class SelfReferential(Base): """test for #9150""" @@ -100,6 +125,18 @@ if typing.TYPE_CHECKING: # EXPECTED_RE_TYPE: sqlalchemy.orm.attributes.InstrumentedAttribute\[builtins.set\*?\[relationship.Address\]\] reveal_type(User.addresses_style_two) + # EXPECTED_RE_TYPE: sqlalchemy.*.InstrumentedAttribute\[builtins.list\*?\[relationship.User\]\] + reveal_type(Group.addresses_style_one_anno_only) + + # EXPECTED_RE_TYPE: sqlalchemy.orm.attributes.InstrumentedAttribute\[builtins.set\*?\[relationship.User\]\] + reveal_type(Group.addresses_style_two_anno_only) + + # EXPECTED_RE_TYPE: sqlalchemy.*.InstrumentedAttribute\[builtins.list\*?\[relationship.MoreMail\]\] + reveal_type(Address.rel_style_one) + + # EXPECTED_RE_TYPE: sqlalchemy.*.InstrumentedAttribute\[builtins.set\*?\[relationship.MoreMail\]\] + reveal_type(Address.rel_style_one_anno_only) + mapper_registry: registry = registry()