From: Mike Bayer Date: Mon, 17 Mar 2025 20:46:12 +0000 (-0400) Subject: ensure SQL expressions w/o bool pass through to correct typing error X-Git-Tag: rel_2_0_40~14 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9516633f5bdfb1e3e463a75f205d682def50e7f4;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git ensure SQL expressions w/o bool pass through to correct typing error Fixed regression which occurred as of 2.0.37 where the checked :class:`.ArgumentError` that's raised when an inappropriate type or object is used inside of a :class:`.Mapped` annotation would raise ``TypeError`` with "boolean value of this clause is not defined" if the object resolved into a SQL expression in a boolean context, for programs where future annotations mode was not enabled. This case is now handled explicitly and a new error message has also been tailored for this case. In addition, as there are at least half a dozen distinct error scenarios for intepretation of the :class:`.Mapped` construct, these scenarios have all been unified under a new subclass of :class:`.ArgumentError` called :class:`.MappedAnnotationError`, to provide some continuity between these different scenarios, even though specific messaging remains distinct. Fixes: #12329 Change-Id: I0193e3479c84a48b364df8655f050e2e84151122 (cherry picked from commit b19a09812c2b0806cc063e42993216fc1ead6ed2) --- diff --git a/doc/build/changelog/unreleased_20/12329.rst b/doc/build/changelog/unreleased_20/12329.rst new file mode 100644 index 0000000000..9e4d1519a5 --- /dev/null +++ b/doc/build/changelog/unreleased_20/12329.rst @@ -0,0 +1,16 @@ +.. change:: + :tags: bug, orm + :tickets: 12329 + + Fixed regression which occurred as of 2.0.37 where the checked + :class:`.ArgumentError` that's raised when an inappropriate type or object + is used inside of a :class:`.Mapped` annotation would raise ``TypeError`` + with "boolean value of this clause is not defined" if the object resolved + into a SQL expression in a boolean context, for programs where future + annotations mode was not enabled. This case is now handled explicitly and + a new error message has also been tailored for this case. In addition, as + there are at least half a dozen distinct error scenarios for intepretation + of the :class:`.Mapped` construct, these scenarios have all been unified + under a new subclass of :class:`.ArgumentError` called + :class:`.MappedAnnotationError`, to provide some continuity between these + different scenarios, even though specific messaging remains distinct. diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py index c480994d8f..f17717b53c 100644 --- a/lib/sqlalchemy/orm/decl_base.py +++ b/lib/sqlalchemy/orm/decl_base.py @@ -1577,7 +1577,7 @@ class _ClassScanMapperConfig(_MapperConfig): is_dataclass, ) except NameError as ne: - raise exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f"Could not resolve all types within mapped " f'annotation: "{annotation}". Ensure all ' f"types are written correctly and are " diff --git a/lib/sqlalchemy/orm/exc.py b/lib/sqlalchemy/orm/exc.py index 0494edf983..a2f7c9f78a 100644 --- a/lib/sqlalchemy/orm/exc.py +++ b/lib/sqlalchemy/orm/exc.py @@ -65,6 +65,15 @@ class FlushError(sa_exc.SQLAlchemyError): """A invalid condition was detected during flush().""" +class MappedAnnotationError(sa_exc.ArgumentError): + """Raised when ORM annotated declarative cannot interpret the + expression present inside of the :class:`.Mapped` construct. + + .. versionadded:: 2.0.40 + + """ + + class UnmappedError(sa_exc.InvalidRequestError): """Base for exceptions that involve expected mappings not present.""" diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index a41c520cdb..218285cab8 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -28,6 +28,7 @@ from typing import TypeVar from typing import Union from . import attributes +from . import exc as orm_exc from . import strategy_options from .base import _DeclarativeMapped from .base import class_mapper @@ -56,6 +57,7 @@ from ..sql.type_api import TypeEngine from ..util.typing import de_optionalize_union_types from ..util.typing import get_args from ..util.typing import includes_none +from ..util.typing import is_a_type from ..util.typing import is_fwd_ref from ..util.typing import is_pep593 from ..util.typing import is_pep695 @@ -862,16 +864,23 @@ class MappedColumn( isinstance(our_type, type) and issubclass(our_type, TypeEngine) ): - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f"The type provided inside the {self.column.key!r} " "attribute Mapped annotation is the SQLAlchemy type " f"{our_type}. Expected a Python type instead" ) - else: - raise sa_exc.ArgumentError( + elif is_a_type(our_type): + raise orm_exc.MappedAnnotationError( "Could not locate SQLAlchemy Core type for Python " f"type {our_type} inside the {self.column.key!r} " "attribute Mapped annotation" ) + else: + raise orm_exc.MappedAnnotationError( + f"The object provided inside the {self.column.key!r} " + "attribute Mapped annotation is not a Python type, " + f"it's the object {our_type!r}. Expected a Python " + "type." + ) self.column._set_type(new_sqltype) diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 48282b2d56..874c4f53b1 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -35,6 +35,7 @@ import weakref from . import attributes # noqa from . import exc +from . import exc as orm_exc from ._typing import _O from ._typing import insp_is_aliased_class from ._typing import insp_is_mapper @@ -2306,7 +2307,7 @@ def _extract_mapped_subtype( if raw_annotation is None: if required: - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f"Python typing annotation is required for attribute " f'"{cls.__name__}.{key}" when primary argument(s) for ' f'"{attr_cls.__name__}" construct are None or not present' @@ -2326,14 +2327,14 @@ def _extract_mapped_subtype( str_cleanup_fn=_cleanup_mapped_str_annotation, ) except _CleanupError as ce: - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f"Could not interpret annotation {raw_annotation}. " "Check that it uses names that are correctly imported at the " "module level. See chained stack trace for more hints." ) from ce except NameError as ne: if raiseerr and "Mapped[" in raw_annotation: # type: ignore - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f"Could not interpret annotation {raw_annotation}. " "Check that it uses names that are correctly imported at the " "module level. See chained stack trace for more hints." @@ -2362,7 +2363,7 @@ def _extract_mapped_subtype( ): return None - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( f'Type annotation for "{cls.__name__}.{key}" ' "can't be correctly interpreted for " "Annotated Declarative Table form. ORM annotations " @@ -2383,7 +2384,7 @@ def _extract_mapped_subtype( return annotated, None if len(annotated.__args__) != 1: - raise sa_exc.ArgumentError( + raise orm_exc.MappedAnnotationError( "Expected sub-type for Mapped[] annotation" ) diff --git a/lib/sqlalchemy/util/typing.py b/lib/sqlalchemy/util/typing.py index 32237833e7..c0f04f385b 100644 --- a/lib/sqlalchemy/util/typing.py +++ b/lib/sqlalchemy/util/typing.py @@ -572,7 +572,22 @@ def includes_none(type_: Any) -> bool: return any(includes_none(t) for t in pep695_values(type_)) if is_newtype(type_): return includes_none(type_.__supertype__) - return type_ in (NoneFwd, NoneType, None) + try: + return type_ in (NoneFwd, NoneType, None) + except TypeError: + # if type_ is Column, mapped_column(), etc. the use of "in" + # resolves to ``__eq__()`` which then gives us an expression object + # that can't resolve to boolean. just catch it all via exception + return False + + +def is_a_type(type_: Any) -> bool: + return ( + isinstance(type_, type) + or hasattr(type_, "__origin__") + or type_.__module__ in ("typing", "typing_extensions") + or type(type_).__mro__[0].__module__ in ("typing", "typing_extensions") + ) def is_union(type_: Any) -> TypeGuard[ArgsTypeProtocol]: diff --git a/test/orm/declarative/test_tm_future_annotations_sync.py b/test/orm/declarative/test_tm_future_annotations_sync.py index d0e5e05ac6..f41f9e2d98 100644 --- a/test/orm/declarative/test_tm_future_annotations_sync.py +++ b/test/orm/declarative/test_tm_future_annotations_sync.py @@ -13,6 +13,7 @@ import datetime from decimal import Decimal import enum import inspect as _py_inspect +import re import typing from typing import Any from typing import cast @@ -67,6 +68,7 @@ from sqlalchemy.orm import DeclarativeBase from sqlalchemy.orm import declared_attr from sqlalchemy.orm import deferred from sqlalchemy.orm import DynamicMapped +from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import foreign from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column @@ -614,19 +616,179 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): id: Mapped[int] = mapped_column(primary_key=True) data: Mapped[MyClass] = mapped_column() - def test_construct_lhs_sqlalchemy_type(self, decl_base): - with expect_raises_message( - sa_exc.ArgumentError, - "The type provided inside the 'data' attribute Mapped " - "annotation is the SQLAlchemy type .*BigInteger.*. Expected " - "a Python type instead", - ): + @testing.variation( + "argtype", + [ + "type", + ("column", testing.requires.python310), + ("mapped_column", testing.requires.python310), + "column_class", + "ref_to_type", + ("ref_to_column", testing.requires.python310), + ], + ) + def test_construct_lhs_sqlalchemy_type(self, decl_base, argtype): + """test for #12329. - class User(decl_base): - __tablename__ = "users" + of note here are all the different messages we have for when the + wrong thing is put into Mapped[], and in fact in #12329 we added + another one. - id: Mapped[int] = mapped_column(primary_key=True) - data: Mapped[BigInteger] = mapped_column() + This is a lot of different messages, but at the same time they + occur at different places in the interpretation of types. If + we were to centralize all these messages, we'd still likely end up + doing distinct messages for each scenario, so instead we added + a new ArgumentError subclass MappedAnnotationError that provides + some commonality to all of these cases. + + + """ + expect_future_annotations = "annotations" in globals() + + if argtype.type: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, type is + # a SQL type + "The type provided inside the 'data' attribute Mapped " + "annotation is the SQLAlchemy type .*BigInteger.*. Expected " + "a Python type instead", + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[BigInteger] = mapped_column() + + elif argtype.column: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # util.py -> _extract_mapped_subtype + ( + re.escape( + "Could not interpret annotation " + "Mapped[Column('q', BigInteger)]." + ) + if expect_future_annotations + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + else re.escape( + "The object provided inside the 'data' attribute " + "Mapped annotation is not a Python type, it's the " + "object Column('q', BigInteger(), table=None). " + "Expected a Python type." + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[Column("q", BigInteger)] = ( # noqa: F821 + mapped_column() + ) + + elif argtype.mapped_column: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + # interestingly, this raises at the same point for both + # future annotations mode and legacy annotations mode + r"The object provided inside the 'data' attribute " + "Mapped annotation is not a Python type, it's the object " + r"\. " + "Expected a Python type.", + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + big_integer: Mapped[int] = mapped_column() + data: Mapped[big_integer] = mapped_column() + + elif argtype.column_class: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, type is not + # a SQL type + re.escape( + "Could not locate SQLAlchemy Core type for Python type " + " inside the " + "'data' attribute Mapped annotation" + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[Column] = mapped_column() + + elif argtype.ref_to_type: + mytype = BigInteger + with expect_raises_message( + orm_exc.MappedAnnotationError, + ( + # decl_base.py -> _exract_mappable_attributes + re.escape( + "Could not resolve all types within mapped " + 'annotation: "Mapped[mytype]"' + ) + if expect_future_annotations + # properties.py -> _init_column_for_annotation, type is + # a SQL type + else re.escape( + "The type provided inside the 'data' attribute Mapped " + "annotation is the SQLAlchemy type " + ". " + "Expected a Python type instead" + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[mytype] = mapped_column() + + elif argtype.ref_to_column: + mycol = Column("q", BigInteger) + + with expect_raises_message( + orm_exc.MappedAnnotationError, + # decl_base.py -> _exract_mappable_attributes + ( + re.escape( + "Could not resolve all types within mapped " + 'annotation: "Mapped[mycol]"' + ) + if expect_future_annotations + else + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + re.escape( + "The object provided inside the 'data' attribute " + "Mapped " + "annotation is not a Python type, it's the object " + "Column('q', BigInteger(), table=None). " + "Expected a Python type." + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[mycol] = mapped_column() + + else: + argtype.fail() def test_construct_rhs_type_override_lhs(self, decl_base): class Element(decl_base): @@ -933,8 +1095,9 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): length = 99 if in_map.value else None else: with expect_raises_message( - exc.ArgumentError, - "Could not locate SQLAlchemy Core type for Python type", + orm_exc.MappedAnnotationError, + r"Could not locate SQLAlchemy Core type for Python type .*tat " + "inside the 'data' attribute Mapped annotation", ): declare() return @@ -2382,7 +2545,8 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): ) with expect_raises_message( - sa_exc.ArgumentError, "Could not locate SQLAlchemy Core type" + orm_exc.MappedAnnotationError, + "Could not locate SQLAlchemy Core type", ): class MyClass(Base): diff --git a/test/orm/declarative/test_typed_mapping.py b/test/orm/declarative/test_typed_mapping.py index f44e5cd63b..0ff4bc6039 100644 --- a/test/orm/declarative/test_typed_mapping.py +++ b/test/orm/declarative/test_typed_mapping.py @@ -4,6 +4,7 @@ import datetime from decimal import Decimal import enum import inspect as _py_inspect +import re import typing from typing import Any from typing import cast @@ -58,6 +59,7 @@ from sqlalchemy.orm import DeclarativeBase from sqlalchemy.orm import declared_attr from sqlalchemy.orm import deferred from sqlalchemy.orm import DynamicMapped +from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import foreign from sqlalchemy.orm import Mapped from sqlalchemy.orm import mapped_column @@ -605,19 +607,179 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): id: Mapped[int] = mapped_column(primary_key=True) data: Mapped[MyClass] = mapped_column() - def test_construct_lhs_sqlalchemy_type(self, decl_base): - with expect_raises_message( - sa_exc.ArgumentError, - "The type provided inside the 'data' attribute Mapped " - "annotation is the SQLAlchemy type .*BigInteger.*. Expected " - "a Python type instead", - ): + @testing.variation( + "argtype", + [ + "type", + ("column", testing.requires.python310), + ("mapped_column", testing.requires.python310), + "column_class", + "ref_to_type", + ("ref_to_column", testing.requires.python310), + ], + ) + def test_construct_lhs_sqlalchemy_type(self, decl_base, argtype): + """test for #12329. - class User(decl_base): - __tablename__ = "users" + of note here are all the different messages we have for when the + wrong thing is put into Mapped[], and in fact in #12329 we added + another one. - id: Mapped[int] = mapped_column(primary_key=True) - data: Mapped[BigInteger] = mapped_column() + This is a lot of different messages, but at the same time they + occur at different places in the interpretation of types. If + we were to centralize all these messages, we'd still likely end up + doing distinct messages for each scenario, so instead we added + a new ArgumentError subclass MappedAnnotationError that provides + some commonality to all of these cases. + + + """ + expect_future_annotations = "annotations" in globals() + + if argtype.type: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, type is + # a SQL type + "The type provided inside the 'data' attribute Mapped " + "annotation is the SQLAlchemy type .*BigInteger.*. Expected " + "a Python type instead", + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[BigInteger] = mapped_column() + + elif argtype.column: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # util.py -> _extract_mapped_subtype + ( + re.escape( + "Could not interpret annotation " + "Mapped[Column('q', BigInteger)]." + ) + if expect_future_annotations + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + else re.escape( + "The object provided inside the 'data' attribute " + "Mapped annotation is not a Python type, it's the " + "object Column('q', BigInteger(), table=None). " + "Expected a Python type." + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[Column("q", BigInteger)] = ( # noqa: F821 + mapped_column() + ) + + elif argtype.mapped_column: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + # interestingly, this raises at the same point for both + # future annotations mode and legacy annotations mode + r"The object provided inside the 'data' attribute " + "Mapped annotation is not a Python type, it's the object " + r"\. " + "Expected a Python type.", + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + big_integer: Mapped[int] = mapped_column() + data: Mapped[big_integer] = mapped_column() + + elif argtype.column_class: + with expect_raises_message( + orm_exc.MappedAnnotationError, + # properties.py -> _init_column_for_annotation, type is not + # a SQL type + re.escape( + "Could not locate SQLAlchemy Core type for Python type " + " inside the " + "'data' attribute Mapped annotation" + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[Column] = mapped_column() + + elif argtype.ref_to_type: + mytype = BigInteger + with expect_raises_message( + orm_exc.MappedAnnotationError, + ( + # decl_base.py -> _exract_mappable_attributes + re.escape( + "Could not resolve all types within mapped " + 'annotation: "Mapped[mytype]"' + ) + if expect_future_annotations + # properties.py -> _init_column_for_annotation, type is + # a SQL type + else re.escape( + "The type provided inside the 'data' attribute Mapped " + "annotation is the SQLAlchemy type " + ". " + "Expected a Python type instead" + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[mytype] = mapped_column() + + elif argtype.ref_to_column: + mycol = Column("q", BigInteger) + + with expect_raises_message( + orm_exc.MappedAnnotationError, + # decl_base.py -> _exract_mappable_attributes + ( + re.escape( + "Could not resolve all types within mapped " + 'annotation: "Mapped[mycol]"' + ) + if expect_future_annotations + else + # properties.py -> _init_column_for_annotation, object is + # not a SQL type or a python type, it's just some object + re.escape( + "The object provided inside the 'data' attribute " + "Mapped " + "annotation is not a Python type, it's the object " + "Column('q', BigInteger(), table=None). " + "Expected a Python type." + ) + ), + ): + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(primary_key=True) + data: Mapped[mycol] = mapped_column() + + else: + argtype.fail() def test_construct_rhs_type_override_lhs(self, decl_base): class Element(decl_base): @@ -924,8 +1086,9 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): length = 99 if in_map.value else None else: with expect_raises_message( - exc.ArgumentError, - "Could not locate SQLAlchemy Core type for Python type", + orm_exc.MappedAnnotationError, + r"Could not locate SQLAlchemy Core type for Python type .*tat " + "inside the 'data' attribute Mapped annotation", ): declare() return @@ -2373,7 +2536,8 @@ class MappedColumnTest(fixtures.TestBase, testing.AssertsCompiledSQL): ) with expect_raises_message( - sa_exc.ArgumentError, "Could not locate SQLAlchemy Core type" + orm_exc.MappedAnnotationError, + "Could not locate SQLAlchemy Core type", ): class MyClass(Base):