]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
use a private return class for the "catch all" relationship
authorMike Bayer <mike_mp@zzzcomputing.com>
Thu, 9 Nov 2023 15:27:19 +0000 (10:27 -0500)
committerFederico Caselli <cfederico87@gmail.com>
Wed, 20 Mar 2024 20:40:39 +0000 (21:40 +0100)
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

12 files changed:
doc/build/changelog/unreleased_20/10611.rst [new file with mode: 0644]
lib/sqlalchemy/ext/mypy/names.py
lib/sqlalchemy/orm/_orm_constructors.py
lib/sqlalchemy/orm/base.py
lib/sqlalchemy/orm/exc.py
lib/sqlalchemy/orm/interfaces.py
lib/sqlalchemy/orm/relationships.py
lib/sqlalchemy/orm/util.py
lib/sqlalchemy/util/langhelpers.py
test/orm/declarative/test_tm_future_annotations_sync.py
test/orm/declarative/test_typed_mapping.py
test/typing/plain_files/orm/relationship.py

diff --git a/doc/build/changelog/unreleased_20/10611.rst b/doc/build/changelog/unreleased_20/10611.rst
new file mode 100644 (file)
index 0000000..2627e4d
--- /dev/null
@@ -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.
index 35b4e2ba819cadc863b6114da212bea3c1b70dd2..fc3d708e7ddbf71e3863f30028fa64752f46d819 100644 (file)
@@ -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,
         {
index 6cf16507ba6f2686b1072bae9b6d20d8f7867a30..2639db2897f6357ecc9d0cf7a805eeb31998919d 100644 (file)
@@ -29,8 +29,8 @@ from .properties import MappedSQLExpression
 from .query import AliasOption
 from .relationships import _RelationshipArgumentType
 from .relationships import _RelationshipBackPopulatesArgument
+from .relationships import _RelationshipDeclared
 from .relationships import _RelationshipSecondaryArgument
-from .relationships import Relationship
 from .relationships import RelationshipProperty
 from .session import Session
 from .util import _ORMJoin
@@ -956,7 +956,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.
@@ -1762,7 +1762,7 @@ def relationship(
 
     """
 
-    return Relationship(
+    return _RelationshipDeclared(
         argument,
         secondary=secondary,
         uselist=uselist,
index 86af81cd6efab51046e8d7d7182ace0ecf221f4c..c9005298d827e34e3d9e84147d8e43ba3e0d71ef 100644 (file)
@@ -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
index 8ab831002abf5daa7dbd201edf3016c2a52eb494..39dd540112846c72336af6d19a389a75fd98afd5 100644 (file)
@@ -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),
                 ),
             )
 
index 36da1a31dba5329bc54de92c90902e0f159787cc..f5f6582202e1ac9f9bd4b871b03d78824ece8b3a 100644 (file)
@@ -152,13 +152,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(
@@ -184,7 +188,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"
         )
 
 
index 383bf24d450548152debe4851d125f694065554a..49b7079936b2e3c117df13f57bac21f8b7c1b5f1 100644 (file)
@@ -1795,19 +1795,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)
 
@@ -3518,11 +3516,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.
@@ -3540,3 +3536,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"
index 46bfb93fec1f124d07799590254fa4b140f90f29..81b6eb23a857c791314d8aae9ff37572fa457271 100644 (file)
@@ -2408,3 +2408,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)
index 4c05237c81c3b0bcf11053ff36ea47294b2a1645..31c205fbc68ed778025eba3c74edafc95a04aa2b 100644 (file)
@@ -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(
index 4ab2657529be6cf27f14004cf4e691a9b5ad1b5b..60f71947e0d6649cc3fb944cd7562e9b57b18bfd 100644 (file)
@@ -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 \<class 'sqlalchemy.orm."
-            r"relationships.Relationship'\> 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"
index 819b671a5a069f4a62447a593c853825faef4996..a1af50cbadb4c9acfc5dfb5093833389b12886c1 100644 (file)
@@ -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 \<class 'sqlalchemy.orm."
-            r"relationships.Relationship'\> 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"
index 6bfe19cc4e8ba1ddf15735421d5db7165a274821..5caf57de7bd69b9115fe5c474f6e04bdf910e66c 100644 (file)
@@ -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()