From: Mike Bayer Date: Fri, 21 Oct 2022 04:11:10 +0000 (-0400) Subject: allow legacy forms with __allow_unmapped__ X-Git-Tag: rel_2_0_0b3~36^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8370edb4ccca2055635107855b58c795fccbeaf1;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git allow legacy forms with __allow_unmapped__ 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 --- diff --git a/doc/build/changelog/migration_20.rst b/doc/build/changelog/migration_20.rst index 3447fa07b1..ab481f5d91 100644 --- a/doc/build/changelog/migration_20.rst +++ b/doc/build/changelog/migration_20.rst @@ -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 ` 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 index 0000000000..dbf06641a6 --- /dev/null +++ b/doc/build/changelog/unreleased_20/8692.rst @@ -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. diff --git a/doc/build/changelog/whatsnew_20.rst b/doc/build/changelog/whatsnew_20.rst index 04c1be3e0a..abdebab5c8 100644 --- a/doc/build/changelog/whatsnew_20.rst +++ b/doc/build/changelog/whatsnew_20.rst @@ -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 ` 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 diff --git a/doc/build/errors.rst b/doc/build/errors.rst index 3149906cb4..87477db382 100644 --- a/doc/build/errors.rst +++ b/doc/build/errors.rst @@ -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 ` 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 ` 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 ================== diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py index ef2c2f3c92..4e79ecc6fd 100644 --- a/lib/sqlalchemy/orm/decl_base.py +++ b/lib/sqlalchemy/orm/decl_base.py @@ -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)) diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index b9d1b50e7a..481a71f8e9 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -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 diff --git a/test/orm/declarative/test_dc_transforms.py b/test/orm/declarative/test_dc_transforms.py index b467644bf5..ef62b7cb24 100644 --- a/test/orm/declarative/test_dc_transforms.py +++ b/test/orm/declarative/test_dc_transforms.py @@ -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" diff --git a/test/orm/declarative/test_tm_future_annotations.py b/test/orm/declarative/test_tm_future_annotations.py index 45fb88f5c0..24d666508a 100644 --- a/test/orm/declarative/test_tm_future_annotations.py +++ b/test/orm/declarative/test_tm_future_annotations.py @@ -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",), diff --git a/test/orm/declarative/test_typed_mapping.py b/test/orm/declarative/test_typed_mapping.py index 1928b3812c..1b4be84d38 100644 --- a/test/orm/declarative/test_typed_mapping.py +++ b/test/orm/declarative/test_typed_mapping.py @@ -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"