From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 24 Mar 2023 15:11:54 +0000 (-0400)
Subject: warn for all unmapped expressions
X-Git-Tag: rel_2_0_8~10^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d998b550a1857bd9f3b2418e0ac1592c4cb70ef9;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

warn for all unmapped expressions

Expanded the warning emitted when a plain :func:`_sql.column` object is
present in a Declarative mapping to include any arbitrary SQL expression
that is not declared within an appropriate property type such as
:func:`_orm.column_property`, :func:`_orm.deferred`, etc. These attributes
are otherwise not mapped at all and remain unchanged within the class
dictionary. As it seems likely that such an expression is usually not
what's intended, this case now warns for all such otherwise ignored
expressions, rather than just the :func:`_sql.column` case.

Fixes: #9537
Change-Id: Ic4ca7a071a28adf4ea8680690025d927522a0805
---

diff --git a/doc/build/changelog/unreleased_20/9537.rst b/doc/build/changelog/unreleased_20/9537.rst
new file mode 100644
index 0000000000..9d39d479e3
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/9537.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 9537
+
+    Expanded the warning emitted when a plain :func:`_sql.column` object is
+    present in a Declarative mapping to include any arbitrary SQL expression
+    that is not declared within an appropriate property type such as
+    :func:`_orm.column_property`, :func:`_orm.deferred`, etc. These attributes
+    are otherwise not mapped at all and remain unchanged within the class
+    dictionary. As it seems likely that such an expression is usually not
+    what's intended, this case now warns for all such otherwise ignored
+    expressions, rather than just the :func:`_sql.column` case.
diff --git a/lib/sqlalchemy/orm/decl_base.py b/lib/sqlalchemy/orm/decl_base.py
index d01aad4395..6be5142765 100644
--- a/lib/sqlalchemy/orm/decl_base.py
+++ b/lib/sqlalchemy/orm/decl_base.py
@@ -1277,11 +1277,14 @@ class _ClassScanMapperConfig(_MapperConfig):
     def _warn_for_decl_attributes(
         self, cls: Type[Any], key: str, c: Any
     ) -> None:
-        if isinstance(c, expression.ColumnClause):
+        if isinstance(c, expression.ColumnElement):
             util.warn(
                 f"Attribute '{key}' on class {cls} appears to "
-                "be a non-schema 'sqlalchemy.sql.column()' "
-                "object; this won't be part of the declarative mapping"
+                "be a non-schema SQLAlchemy expression "
+                "object; this won't be part of the declarative mapping. "
+                "To map arbitrary expressions, use ``column_property()`` "
+                "or a similar function such as ``deferred()``, "
+                "``query_expression()`` etc. "
             )
 
     def _produce_column_copies(
diff --git a/test/orm/declarative/test_basic.py b/test/orm/declarative/test_basic.py
index 4aca4daa6d..2d712c823e 100644
--- a/test/orm/declarative/test_basic.py
+++ b/test/orm/declarative/test_basic.py
@@ -1301,10 +1301,10 @@ class DeclarativeMultiBaseTest(
             class_mapper(Bar).get_property("some_data").columns[0] is t.c.data
         )
 
-    def test_lower_case_c_column_warning(self):
+    def test_non_sql_expression_warning_one(self):
         with assertions.expect_warnings(
             r"Attribute 'x' on class <class .*Foo.* appears to be a "
-            r"non-schema 'sqlalchemy.sql.column\(\)' object; "
+            r"non-schema SQLAlchemy expression object; "
         ):
 
             class Foo(Base):
@@ -1314,13 +1314,14 @@ class DeclarativeMultiBaseTest(
                 x = sa.sql.expression.column(Integer)
                 y = Column(Integer)
 
+    def test_non_sql_expression_warning_two(self):
         class MyMixin:
             x = sa.sql.expression.column(Integer)
             y = Column(Integer)
 
         with assertions.expect_warnings(
             r"Attribute 'x' on class <class .*MyMixin.* appears to be a "
-            r"non-schema 'sqlalchemy.sql.column\(\)' object; "
+            r"non-schema SQLAlchemy expression object; "
         ):
 
             class Foo2(MyMixin, Base):
@@ -1328,9 +1329,10 @@ class DeclarativeMultiBaseTest(
 
                 id = Column(Integer, primary_key=True)
 
+    def test_non_sql_expression_warning_three(self):
         with assertions.expect_warnings(
             r"Attribute 'x' on class <class .*Foo3.* appears to be a "
-            r"non-schema 'sqlalchemy.sql.column\(\)' object; "
+            r"non-schema SQLAlchemy expression object; "
         ):
 
             class Foo3(Base):
@@ -1344,9 +1346,10 @@ class DeclarativeMultiBaseTest(
 
                 y = Column(Integer)
 
+    def test_non_sql_expression_warning_four(self):
         with assertions.expect_warnings(
             r"Attribute 'x' on class <class .*Foo4.* appears to be a "
-            r"non-schema 'sqlalchemy.sql.column\(\)' object; "
+            r"non-schema SQLAlchemy expression object; "
         ):
 
             class MyMixin2:
@@ -1361,6 +1364,25 @@ class DeclarativeMultiBaseTest(
 
                 id = Column(Integer, primary_key=True)
 
+    def test_non_sql_expression_warning_five(self):
+
+        # test for #9537
+        with assertions.expect_warnings(
+            r"Attribute 'x' on class <class .*Foo5.* appears to be a "
+            r"non-schema SQLAlchemy expression object; ",
+            r"Attribute 'y' on class <class .*Foo5.* appears to be a "
+            r"non-schema SQLAlchemy expression object; ",
+            raise_on_any_unexpected=True,
+        ):
+
+            class Foo5(Base):
+                __tablename__ = "foo5"
+
+                id = Column(Integer, primary_key=True)
+                x = Column("x", String()).collate("some collation")
+                y = Column("y", Integer) + 5
+                z = "im not a sqlalchemy thing"
+
     def test_column_named_twice(self):
         with expect_warnings(
             "On class 'Foo', Column object 'x' named directly multiple "
diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py
index 99cfdc3837..27f1d70525 100644
--- a/test/orm/inheritance/test_basic.py
+++ b/test/orm/inheritance/test_basic.py
@@ -198,11 +198,9 @@ class PolyExpressionEagerLoad(fixtures.DeclarativeMappedTest):
             child_id = Column(Integer, ForeignKey("a.id"))
             child = relationship("A")
 
-            p_a = case((discriminator == "a", "a"), else_="b")
-
             __mapper_args__ = {
                 "polymorphic_identity": "a",
-                "polymorphic_on": p_a,
+                "polymorphic_on": case((discriminator == "a", "a"), else_="b"),
             }
 
         class B(A):