]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
allow legacy forms with __allow_unmapped__
authorMike Bayer <mike_mp@zzzcomputing.com>
Fri, 21 Oct 2022 04:11:10 +0000 (00:11 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Fri, 21 Oct 2022 15:48:35 +0000 (11:48 -0400)
Improved support for legacy 1.4 mappings that use annotations which don't
include ``Mapped[]``, by ensuring the ``__allow_unmapped__`` attribute can
be used to allow such legacy annotations to pass through Annotated
Declarative without raising an error and without being interpreted in an
ORM runtime context. Additionally improved the error message generated when
this condition is detected, and added more documentation for how this
situation should be handled. Unfortunately the 1.4 WARN_SQLALCHEMY_20
migration warning cannot detect this particular configurational issue at
runtime with its current architecture.

Fixes: #8692
Change-Id: I5c642bcc1ebb7816f9470ec9bb0951550b6d55f1

doc/build/changelog/migration_20.rst
doc/build/changelog/unreleased_20/8692.rst [new file with mode: 0644]
doc/build/changelog/whatsnew_20.rst
doc/build/errors.rst
lib/sqlalchemy/orm/decl_base.py
lib/sqlalchemy/orm/util.py
test/orm/declarative/test_dc_transforms.py
test/orm/declarative/test_tm_future_annotations.py
test/orm/declarative/test_typed_mapping.py

index 3447fa07b11a96fd784eda280ae8477546bbd386..ab481f5d912c17ee1031b0edcf0499548ba8f814 100644 (file)
@@ -444,6 +444,86 @@ and all ``exc.RemovedIn20Warning`` occurrences set to raise an error,
 The sections that follow will detail the specific changes to make for all
 major API modifications.
 
+.. _migration_20_step_six:
+
+Migration to 2.0 Step Six - Add ``__allow_unmapped__`` to explicitly typed ORM models
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+SQLAlchemy 2.0 has new support for runtime interpretation of :pep:`484` typing annotations
+on ORM models.   A requirement of these annotations is that they must make use
+of the :class:`_orm.Mapped` generic container.  Annotations which don't use
+:class:`_orm.Mapped` which link to constructs such as :func:`_orm.relationship`
+will raise errors, as they suggest mis-configurations.
+
+SQLAlchemy applications that use the :ref:`Mypy plugin <mypy_toplevel>` with
+explicit annotations that don't use :class:`_orm.Mapped` in their annotations
+are subject to these errors, as would occur in the example below::
+
+    Base = declarative_base()
+
+
+    class Foo(Base):
+        __tablename__ = "foo"
+
+        id: int = Column(Integer, primary_key=True)
+
+        # will raise
+        bars: list["Bar"] = relationship("Bar", back_populates="foo")
+
+
+    class Bar(Base):
+        __tablename__ = "bar"
+
+        id: int = Column(Integer, primary_key=True)
+        foo_id = Column(ForeignKey("foo.id"))
+
+        # will raise
+        foo: Foo = relationship(Foo, back_populates="bars", cascade="all")
+
+Above, the ``Foo.bars`` and ``Bar.foo`` :func:`_orm.relationship` declarations
+will raise an error at class construction time because they don't use
+:class:`_orm.Mapped` (by contrast, the annotations that use
+:class:`_schema.Column` are ignored by 2.0, as these are able to be
+recognized as a legacy configuration style). To allow all annotations that
+don't use :class:`_orm.Mapped` to pass without error,
+the ``__allow_unmapped__`` attribute may be used on the class or any
+subclasses, which will cause the annotations in these cases to be
+ignored completely by the new Declarative system.
+
+The example below illustrates the application of ``__allow_unmapped__``
+to the Declarative ``Base`` class, where it will take effect for all classes
+that descend from ``Base``::
+
+    # qualify the base with __allow_unmapped__.  Can also be
+    # applied to classes directly if preferred
+    class Base:
+        __allow_unmapped__ = True
+
+
+    Base = declarative_base(cls=Base)
+
+    # existing mapping proceeds, Declarative will ignore any annotations
+    # which don't include ``Mapped[]``
+    class Foo(Base):
+        __tablename__ = "foo"
+
+        id: int = Column(Integer, primary_key=True)
+
+        bars: list["Bar"] = relationship("Bar", back_populates="foo")
+
+
+    class Bar(Base):
+        __tablename__ = "bar"
+
+        id: int = Column(Integer, primary_key=True)
+        foo_id = Column(ForeignKey("foo.id"))
+
+        foo: Foo = relationship(Foo, back_populates="bars", cascade="all")
+
+.. versionchanged:: 2.0.0beta3 - improved the ``__allow_unmapped__``
+   attribute support to allow for 1.4-style explicit annotated relationships
+   that don't use :class:`_orm.Mapped` to remain usable.
+
 
 2.0 Migration - Core Connection / Transaction
 ---------------------------------------------
diff --git a/doc/build/changelog/unreleased_20/8692.rst b/doc/build/changelog/unreleased_20/8692.rst
new file mode 100644 (file)
index 0000000..dbf0664
--- /dev/null
@@ -0,0 +1,13 @@
+.. change::
+    :tags: bug, orm, declarative
+    :tickets: 8692
+
+    Improved support for legacy 1.4 mappings that use annotations which don't
+    include ``Mapped[]``, by ensuring the ``__allow_unmapped__`` attribute can
+    be used to allow such legacy annotations to pass through Annotated
+    Declarative without raising an error and without being interpreted in an
+    ORM runtime context. Additionally improved the error message generated when
+    this condition is detected, and added more documentation for how this
+    situation should be handled. Unfortunately the 1.4 WARN_SQLALCHEMY_20
+    migration warning cannot detect this particular configurational issue at
+    runtime with its current architecture.
index 04c1be3e0a8d38c2188b09a5a6456b6868988d0a..abdebab5c8f6baf112fc579f7a354f7f23da1d09 100644 (file)
@@ -723,6 +723,25 @@ and :class:`_engine.Row` objects::
     :ref:`orm_declarative_table` - Updated Declarative documentation for
     Declarative generation and mapping of :class:`.Table` columns.
 
+.. _whatsnew_20_mypy_legacy_models:
+
+Using Legacy Mypy-Typed Models
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+SQLAlchemy applications that use the :ref:`Mypy plugin <mypy_toplevel>` with
+explicit annotations that don't use :class:`_orm.Mapped` in their annotations
+are subject to errors under the new system, as such annotations are flagged as
+errors when using constructs such as :func:`_orm.relationship`.
+
+The section :ref:`migration_20_step_six` illustrates how to temporarily
+disable these errors from being raised for a legacy ORM model that uses
+explicit annotations.
+
+.. seealso::
+
+    :ref:`migration_20_step_six`
+
+
 .. _whatsnew_20_dataclasses:
 
 Native Support for Dataclasses Mapped as ORM Models
index 3149906cb46ebacb4450a3b7a7f0b1f9b394dd37..87477db382f8307ea391335c2738e25f330dd0f5 100644 (file)
@@ -1312,6 +1312,30 @@ the :term:`detached` state.
    For background on buffering of the ``cursor`` results itself, see the
    section :ref:`engine_stream_results`.
 
+.. _error_zlpr:
+
+Type annotation can't be interpreted for Annotated Declarative Table form
+--------------------------------------------------------------------------
+
+SQLAlchemy 2.0 introduces a new
+:ref:`Annotated Declarative Table <orm_declarative_mapped_column>` declarative
+system which derives ORM mapped attribute information from :pep:`484`
+annotations within class definitions at runtime. A requirement of this form is
+that all ORM annotations must make use of a generic container called
+:class:`_orm.Mapped` to be properly annotated. Legacy SQLAlchemy mappings which
+include explicit :pep:`484` typing annotations, such as those which use the
+:ref:`legacy Mypy extension <mypy_toplevel>` for typing support, may include
+directives such as those for :func:`_orm.relationship` that don't include this
+generic.
+
+To resolve, the classes may be marked with the ``__allow_unmapped__`` boolean
+attribute until they can be fully migrated to the 2.0 syntax. See the migration
+notes at :ref:`migration_20_step_six` for an example.
+
+
+.. seealso::
+
+    :ref:`migration_20_step_six` - in the :ref:`migration_20_toplevel` document
 
 AsyncIO Exceptions
 ==================
index ef2c2f3c92d90c7fa52f30ae1ad83d2c009960c8..4e79ecc6fd190aaa7144669d43019d815eac46d4 100644 (file)
@@ -1199,6 +1199,11 @@ class _ClassScanMapperConfig(_MapperConfig):
             cls, "_sa_decl_prepare_nocascade", strict=True
         )
 
+        expect_annotations_wo_mapped = (
+            self.allow_unmapped_annotations
+            or self.is_dataclass_prior_to_mapping
+        )
+
         for k in list(collected_attributes):
 
             if k in ("__table__", "__tablename__", "__mapper_args__"):
@@ -1275,15 +1280,28 @@ class _ClassScanMapperConfig(_MapperConfig):
                     ) = self.collected_annotations.get(
                         k, (None, None, None, False, None)
                     )
-                    value.declarative_scan(
-                        self.registry,
-                        cls,
-                        k,
-                        mapped_container,
-                        annotation,
-                        extracted_mapped_annotation,
-                        is_dataclass,
-                    )
+
+                    # issue #8692 - don't do any annotation interpretation if
+                    # an annotation were present and a container such as
+                    # Mapped[] etc. were not used.  If annotation is None,
+                    # do declarative_scan so that the property can raise
+                    # for required
+                    if mapped_container is not None or annotation is None:
+                        value.declarative_scan(
+                            self.registry,
+                            cls,
+                            k,
+                            mapped_container,
+                            annotation,
+                            extracted_mapped_annotation,
+                            is_dataclass,
+                        )
+                    else:
+                        # assert that we were expecting annotations
+                        # without Mapped[] were going to be passed.
+                        # otherwise an error should have been raised
+                        # by util._extract_mapped_subtype before we got here.
+                        assert expect_annotations_wo_mapped
 
                 if (
                     isinstance(value, (MapperProperty, _MapsColumns))
index b9d1b50e7a87574e86d66dd72dfe492285f399fd..481a71f8e908e78b0f5bd0846fba5c97b015e5a3 100644 (file)
@@ -2090,15 +2090,6 @@ def _extract_mapped_subtype(
         if not hasattr(annotated, "__origin__") or not is_origin_of_cls(
             annotated, _MappedAnnotationBase
         ):
-            anno_name = (
-                getattr(annotated, "__name__", None)
-                if not isinstance(annotated, str)
-                else None
-            )
-            if anno_name is None:
-                our_annotated_str = repr(annotated)
-            else:
-                our_annotated_str = anno_name
 
             if expect_mapped:
                 if getattr(annotated, "__origin__", None) is typing.ClassVar:
@@ -2107,29 +2098,22 @@ def _extract_mapped_subtype(
                 if not raiseerr:
                     return None
 
-                if attr_cls.__name__ == our_annotated_str or attr_cls is type(
-                    None
-                ):
-                    raise sa_exc.ArgumentError(
-                        f'Type annotation for "{cls.__name__}.{key}" '
-                        "should use the "
-                        f'syntax "Mapped[{our_annotated_str}]".  To leave '
-                        f"the attribute unmapped, use "
-                        f"ClassVar[{our_annotated_str}], assign a value to "
-                        f"the attribute, or "
-                        f"set __allow_unmapped__ = True on the class."
-                    )
-                else:
-                    raise sa_exc.ArgumentError(
-                        f'Type annotation for "{cls.__name__}.{key}" '
-                        "should use the "
-                        f'syntax "Mapped[{our_annotated_str}]" or '
-                        f'"{attr_cls.__name__}[{our_annotated_str}]".  To '
-                        f"leave the attribute unmapped, use "
-                        f"ClassVar[{our_annotated_str}], assign a value to "
-                        f"the attribute, or "
-                        f"set __allow_unmapped__ = True on the class."
-                    )
+                raise sa_exc.ArgumentError(
+                    f'Type annotation for "{cls.__name__}.{key}" '
+                    "can't be correctly interpreted for "
+                    "Annotated Declarative Table form.  ORM annotations "
+                    "should normally make use of the ``Mapped[]`` generic "
+                    "type, or other ORM-compatible generic type, as a "
+                    "container for the actual type, which indicates the "
+                    "intent that the attribute is mapped. "
+                    "Class variables that are not intended to be mapped "
+                    "by the ORM should use ClassVar[].  "
+                    "To allow Annotated Declarative to disregard legacy "
+                    "annotations which don't use Mapped[] to pass, set "
+                    '"__allow_unmapped__ = True" on the class or a '
+                    "superclass this class.",
+                    code="zlpr",
+                )
 
             else:
                 return annotated, None
index b467644bf58ea02bb7b0f869999f6c678f13cae0..ef62b7cb245024492e1a7ac3cd666ff9361a13bf 100644 (file)
@@ -49,6 +49,7 @@ from sqlalchemy.testing import is_false
 from sqlalchemy.testing import is_true
 from sqlalchemy.testing import ne_
 from sqlalchemy.util import compat
+from .test_typed_mapping import expect_annotation_syntax_error
 
 
 class DCTransformsTest(AssertsCompiledSQL, fixtures.TestBase):
@@ -372,12 +373,7 @@ class DCTransformsTest(AssertsCompiledSQL, fixtures.TestBase):
         """since I made this mistake in my own mapping video, lets have it
         raise an error"""
 
-        with expect_raises_message(
-            exc.ArgumentError,
-            r'Type annotation for "A.data" should '
-            r'use the syntax "Mapped\[str\]".  '
-            r"To leave the attribute unmapped,",
-        ):
+        with expect_annotation_syntax_error("A.data"):
 
             class A(dc_decl_base):
                 __tablename__ = "a"
index 45fb88f5c05e2dfedc16021154648207a3e09af5..24d666508a7bb082068e16bc91a611eda3b92490 100644 (file)
@@ -7,10 +7,13 @@ from typing import Set
 from typing import TypeVar
 from typing import Union
 
+from sqlalchemy import Column
 from sqlalchemy import exc
 from sqlalchemy import ForeignKey
 from sqlalchemy import Integer
 from sqlalchemy import Numeric
+from sqlalchemy import select
+from sqlalchemy import String
 from sqlalchemy import Table
 from sqlalchemy import testing
 from sqlalchemy.orm import attribute_keyed_dict
@@ -293,6 +296,33 @@ class RelationshipLHSTest(_RelationshipLHSTest):
 
         is_(a1.bs["foo"], b1)
 
+    def test_14_style_anno_accepted_w_allow_unmapped(self):
+        """test for #8692"""
+
+        class Base(DeclarativeBase):
+            __allow_unmapped__ = True
+
+        class A(Base):
+            __tablename__ = "a"
+
+            id: Mapped[int] = mapped_column(primary_key=True)
+            data: str = Column(String)
+            bs: List[B] = relationship("B", back_populates="a")
+
+        class B(Base):
+            __tablename__ = "b"
+            id: Mapped[int] = mapped_column(primary_key=True)
+            a_id: Mapped[int] = mapped_column(ForeignKey("a.id"))
+            data: Mapped[str]
+            a: A = relationship("A", back_populates="bs")
+
+        Base.registry.configure()
+
+        self.assert_compile(
+            select(A).join(A.bs),
+            "SELECT a.id, a.data FROM a JOIN b ON a.id = b.a_id",
+        )
+
     @testing.combinations(
         ("not_optional",),
         ("optional",),
index 1928b3812c99f4809fa47b9ff39a471dac9d79df..1b4be84d38793283a662706fdeb36cdd0625b3f9 100644 (file)
@@ -63,6 +63,15 @@ from sqlalchemy.util import compat
 from sqlalchemy.util.typing import Annotated
 
 
+def expect_annotation_syntax_error(name):
+    return expect_raises_message(
+        sa_exc.ArgumentError,
+        f'Type annotation for "{name}" '
+        "can't be correctly interpreted for "
+        "Annotated Declarative Table form.  ",
+    )
+
+
 class DeclarativeBaseTest(fixtures.TestBase):
     def test_class_getitem_as_declarative(self):
         T = TypeVar("T", bound="CommonBase")  # noqa
@@ -282,13 +291,7 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL):
         assert "old_column" in inspect(MyClass).attrs
 
     def test_i_have_plain_attrs_on_my_class_disallowed(self, decl_base):
-        with expect_raises_message(
-            sa_exc.ArgumentError,
-            r'Type annotation for "MyClass.status" should use the syntax '
-            r'"Mapped\[int\]".  To leave the attribute unmapped, use '
-            r"ClassVar\[int\], assign a value to the attribute, or "
-            r"set __allow_unmapped__ = True on the class.",
-        ):
+        with expect_annotation_syntax_error("MyClass.status"):
 
             class MyClass(decl_base):
                 __tablename__ = "mytable"
@@ -926,11 +929,7 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL):
                 is_true(optional_col.nullable)
 
     def test_missing_mapped_lhs(self, decl_base):
-        with expect_raises_message(
-            ArgumentError,
-            r'Type annotation for "User.name" should use the '
-            r'syntax "Mapped\[str\]" or "MappedColumn\[str\]"',
-        ):
+        with expect_annotation_syntax_error("User.name"):
 
             class User(decl_base):
                 __tablename__ = "users"
@@ -1213,7 +1212,18 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL):
                 id: Mapped[int] = mapped_column(primary_key=True)
                 bs = relationship()
 
-    def test_rudimentary_dataclasses_support(self, registry):
+    def test_legacy_dataclasses_not_currently_using_annotations(
+        self, registry
+    ):
+        """test if relationship() inspects annotations when using
+        the legacy dataclass style.
+
+        As of #8692, we are not looking at any annotations that don't use
+        ``Mapped[]``.   dataclass users should use MappedAsDataclass and
+        new conventions.
+
+        """
+
         @registry.mapped
         @dataclasses.dataclass
         class A:
@@ -1232,8 +1242,39 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL):
             id: Mapped[int] = mapped_column(primary_key=True)
             a_id = mapped_column(ForeignKey("a.id"))
 
+        with expect_raises_message(
+            ArgumentError,
+            "relationship 'bs' expects a class or a mapper argument",
+        ):
+            registry.configure()
+
+    def test_14_style_anno_accepted_w_allow_unmapped(self):
+        """test for #8692"""
+
+        class Base(DeclarativeBase):
+            __allow_unmapped__ = True
+
+        class A(Base):
+            __tablename__ = "a"
+
+            id: Mapped[int] = mapped_column(primary_key=True)
+            data: str = Column(String)
+            bs: List["B"] = relationship("B", back_populates="a")  # noqa: F821
+
+        class B(Base):
+            __tablename__ = "b"
+            id: Mapped[int] = mapped_column(primary_key=True)
+            a_id: Mapped[int] = mapped_column(ForeignKey("a.id"))
+            data: Mapped[str]
+            a: A = relationship("A", back_populates="bs")
+
         self.assert_compile(
-            select(A).join(A.bs), "SELECT a.id FROM a JOIN b ON a.id = b.a_id"
+            select(A).join(A.bs),
+            "SELECT a.id, a.data FROM a JOIN b ON a.id = b.a_id",
+        )
+        self.assert_compile(
+            select(B).join(B.a),
+            "SELECT b.id, b.a_id, b.data FROM b JOIN a ON a.id = b.a_id",
         )
 
     @testing.combinations(
@@ -1287,11 +1328,7 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL):
 
     def test_wrong_annotation_type_one(self, decl_base):
 
-        with expect_raises_message(
-            sa_exc.ArgumentError,
-            r"Type annotation for \"A.data\" should use the "
-            r"syntax \"Mapped\['B'\]\" or \"Relationship\['B'\]\"",
-        ):
+        with expect_annotation_syntax_error("A.data"):
 
             class A(decl_base):
                 __tablename__ = "a"
@@ -1301,11 +1338,7 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL):
 
     def test_wrong_annotation_type_two(self, decl_base):
 
-        with expect_raises_message(
-            sa_exc.ArgumentError,
-            r"Type annotation for \"A.data\" should use the "
-            r"syntax \"Mapped\[B\]\" or \"Relationship\[B\]\"",
-        ):
+        with expect_annotation_syntax_error("A.data"):
 
             class B(decl_base):
                 __tablename__ = "b"
@@ -1320,12 +1353,7 @@ class RelationshipLHSTest(fixtures.TestBase, testing.AssertsCompiledSQL):
 
     def test_wrong_annotation_type_three(self, decl_base):
 
-        with expect_raises_message(
-            sa_exc.ArgumentError,
-            r"Type annotation for \"A.data\" should use the "
-            r"syntax \"Mapped\['List\[B\]'\]\" or "
-            r"\"Relationship\['List\[B\]'\]\"",
-        ):
+        with expect_annotation_syntax_error("A.data"):
 
             class B(decl_base):
                 __tablename__ = "b"
@@ -1585,11 +1613,7 @@ class CompositeTest(fixtures.TestBase, testing.AssertsCompiledSQL):
             state: str
             zip_: str
 
-        with expect_raises_message(
-            ArgumentError,
-            r"Type annotation for \"User.address\" should use the syntax "
-            r"\"Mapped\['Address'\]\"",
-        ):
+        with expect_annotation_syntax_error("User.address"):
 
             class User(decl_base):
                 __tablename__ = "user"