From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Mon, 26 Nov 2018 05:59:01 +0000 (-0500)
Subject: Warn for lower-case column attribute on declarative
X-Git-Tag: rel_1_3_0b2~85^2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6ec40eca1a03a9156ee82f3ce75850778220b39e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Warn for lower-case column attribute on declarative

A warning is emitted in the case that a :func:`.column` object is applied to
a declarative class, as it seems likely this intended to be a
:class:`.Column` object.

Fixes: #4374
Change-Id: I2e617ef65547162e3ba6587c168548ad0cf6203d
---

diff --git a/doc/build/changelog/unreleased_12/4374.rst b/doc/build/changelog/unreleased_12/4374.rst
new file mode 100644
index 0000000000..716ff5b67b
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4374.rst
@@ -0,0 +1,7 @@
+.. change::
+   :tags: bug, orm, declarative
+   :tickets: 4374
+
+   A warning is emitted in the case that a :func:`.column` object is applied to
+   a declarative class, as it seems likely this intended to be a
+   :class:`.Column` object.
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index e5bc670b3b..f1d75bb4b2 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -303,6 +303,11 @@ class _MapperConfig(object):
                         if isinstance(ret, (Column, MapperProperty)) and \
                                 ret.doc is None:
                             ret.doc = obj.__doc__
+                    # here, the attribute is some other kind of property that
+                    # we assume is not part of the declarative mapping.
+                    # however, check for some more common mistakes
+                    else:
+                        self._warn_for_decl_attributes(base, name, obj)
 
         if inherited_table_args and not tablename:
             table_args = None
@@ -311,6 +316,14 @@ class _MapperConfig(object):
         self.tablename = tablename
         self.mapper_args_fn = mapper_args_fn
 
+    def _warn_for_decl_attributes(self, cls, key, c):
+        if isinstance(c, expression.ColumnClause):
+            util.warn(
+                "Attribute '%s' on class %s appears to be a non-schema "
+                "'sqlalchemy.sql.column()' "
+                "object; this won't be part of the declarative mapping" %
+                (key, cls))
+
     def _produce_column_copies(self, base):
         cls = self.cls
         dict_ = self.dict_
@@ -382,6 +395,7 @@ class _MapperConfig(object):
                 # and place the evaluated value onto the class.
                 if not k.startswith('__'):
                     dict_.pop(k)
+                    self._warn_for_decl_attributes(cls, k, value)
                     if not late_mapped:
                         setattr(cls, k, value)
                 continue
diff --git a/test/ext/declarative/test_basic.py b/test/ext/declarative/test_basic.py
index d2dc1a4250..e8a9a140cd 100644
--- a/test/ext/declarative/test_basic.py
+++ b/test/ext/declarative/test_basic.py
@@ -1,6 +1,7 @@
 
 from sqlalchemy.testing import eq_, assert_raises, \
     assert_raises_message, expect_warnings, is_
+from sqlalchemy.testing import assertions
 from sqlalchemy.ext import declarative as decl
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy import exc
@@ -175,6 +176,62 @@ class DeclarativeTest(DeclarativeTestBase):
         assert class_mapper(Bar).get_property('some_data').columns[0] \
             is t.c.data
 
+    def test_lower_case_c_column_warning(self):
+        with assertions.expect_warnings(
+            r"Attribute 'x' on class <class .*Foo.* appears to be a "
+            r"non-schema 'sqlalchemy.sql.column\(\)' object; ",
+        ):
+            class Foo(Base):
+                __tablename__ = 'foo'
+
+                id = Column(Integer, primary_key=True)
+                x = sa.sql.expression.column(Integer)
+                y = Column(Integer)
+
+        class MyMixin(object):
+            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; ",
+        ):
+            class Foo2(MyMixin, Base):
+                __tablename__ = 'foo2'
+
+                id = Column(Integer, primary_key=True)
+
+        with assertions.expect_warnings(
+            r"Attribute 'x' on class <class .*Foo3.* appears to be a "
+            r"non-schema 'sqlalchemy.sql.column\(\)' object; ",
+        ):
+            class Foo3(Base):
+                __tablename__ = 'foo3'
+
+                id = Column(Integer, primary_key=True)
+
+                @declared_attr
+                def x(cls):
+                    return sa.sql.expression.column(Integer)
+
+                y = Column(Integer)
+
+        with assertions.expect_warnings(
+            r"Attribute 'x' on class <class .*Foo4.* appears to be a "
+            r"non-schema 'sqlalchemy.sql.column\(\)' object; ",
+        ):
+            class MyMixin2(object):
+                @declared_attr
+                def x(cls):
+                    return sa.sql.expression.column(Integer)
+
+                y = Column(Integer)
+
+            class Foo4(MyMixin2, Base):
+                __tablename__ = 'foo4'
+
+                id = Column(Integer, primary_key=True)
+
     def test_column_named_twice(self):
         def go():
             class Foo(Base):