dont assume ClauseElement in attributes, coercions

Fixed two distinct issues, each of which would come into play under certain
circumstances, most likely however one which is a common mis-configuration
in :class:`_hybrid.hybrid_property`, where the "expression" implementation
would return a non :class:`_sql.ClauseElement` such as a boolean value.
For both issues, 1.3's behavior was to silently ignore the
mis-configuration and ultimately attempt to interpret the value as a
SQL expression, which would lead to an incorrect query.

* Fixed issue regarding interaction of the attribute system with
hybrid_property, where if the ``__clause_element__()`` method of the
attribute returned a non-:class:`_sql.ClauseElement` object, an internal
``AttributeError`` would lead the attribute to return the ``expression``
function on the hybrid_property itself, as the attribute error was
against the name ``.expression`` which would invoke the ``__getattr__()``
method as a fallback. This now raises explicitly. In 1.3 the
non-:class:`_sql.ClauseElement` was returned directly.

* Fixed issue in SQL argument coercions system where passing the wrong
kind of object to methods that expect column expressions would fail if
the object were altogether not a SQLAlchemy object, such as a Python
function, in cases where the object were not just coerced into a bound
value. Again 1.3 did not have a comprehensive argument coercion system
so this case would also pass silently.

Fixes: #6350
Change-Id: I5bba0a6b27f45e5f8ebadfd6d511fa773388ef7c

5 files changed, 94 insertions, 2 deletions

diff --git a/doc/build/changelog/unreleased_14/6350.rst b/doc/build/changelog/unreleased_14/6350.rst
new file mode 100644
index 000000000..c2a911b7a
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/6350.rst
@@ -0,0 +1,28 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 6350
+
+    Fixed two distinct issues, each of which would come into play under certain
+    circumstances, most likely however one which is a common mis-configuration
+    in :class:`_hybrid.hybrid_property`, where the "expression" implementation
+    would return a non :class:`_sql.ClauseElement` such as a boolean value.
+    For both issues, 1.3's behavior was to silently ignore the
+    mis-configuration and ultimately attempt to interpret the value as a
+    SQL expression, which would lead to an incorrect query.
+
+    * Fixed issue regarding interaction of the attribute system with
+      hybrid_property, where if the ``__clause_element__()`` method of the
+      attribute returned a non-:class:`_sql.ClauseElement` object, an internal
+      ``AttributeError`` would lead the attribute to return the ``expression``
+      function on the hybrid_property itself, as the attribute error was
+      against the name ``.expression`` which would invoke the ``__getattr__()``
+      method as a fallback. This now raises explicitly. In 1.3 the
+      non-:class:`_sql.ClauseElement` was returned directly.
+
+    * Fixed issue in SQL argument coercions system where passing the wrong
+      kind of object to methods that expect column expressions would fail if
+      the object were altogether not a SQLAlchemy object, such as a Python
+      function, in cases where the object were not just coerced into a bound
+      value. Again 1.3 did not have a comprehensive argument coercion system
+      so this case would also pass silently.
+
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index 9e326db64..0c7bc4cf0 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -46,6 +46,7 @@ from .base import RELATED_OBJECT_OK  # noqa
 from .base import SQL_OK  # noqa
 from .base import state_str
 from .. import event
+from .. import exc
 from .. import inspection
 from .. import util
 from ..sql import base as sql_base
@@ -229,7 +230,20 @@ class QueryableAttribute(
                 "entity_namespace": self._entity_namespace,
             }
 
-        return self.comparator.__clause_element__()._annotate(annotations)
+        ce = self.comparator.__clause_element__()
+        try:
+            anno = ce._annotate
+        except AttributeError as ae:
+            util.raise_(
+                exc.InvalidRequestError(
+                    'When interpreting attribute "%s" as a SQL expression, '
+                    "expected __clause_element__() to return "
+                    "a ClauseElement object, got: %r" % (self, ce)
+                ),
+                from_=ae,
+            )
+        else:
+            return anno(annotations)
 
     @property
     def _entity_namespace(self):
diff --git a/lib/sqlalchemy/sql/coercions.py b/lib/sqlalchemy/sql/coercions.py
index c00262fd5..b7aba9d74 100644
--- a/lib/sqlalchemy/sql/coercions.py
+++ b/lib/sqlalchemy/sql/coercions.py
@@ -313,7 +313,7 @@ class _ColumnCoercions(object):
     def _implicit_coercions(
         self, original_element, resolved, argname=None, **kw
     ):
-        if not resolved.is_clause_element:
+        if not getattr(resolved, "is_clause_element", False):
             self._raise_for_expected(original_element, argname, resolved)
         elif resolved._is_select_statement:
             self._warn_for_scalar_subquery_coercion()
diff --git a/test/ext/test_hybrid.py b/test/ext/test_hybrid.py
index d44e284a3..c9373f9aa 100644
--- a/test/ext/test_hybrid.py
+++ b/test/ext/test_hybrid.py
@@ -1,5 +1,6 @@
 from decimal import Decimal
 
+from sqlalchemy import exc
 from sqlalchemy import ForeignKey
 from sqlalchemy import func
 from sqlalchemy import inspect
@@ -8,6 +9,7 @@ from sqlalchemy import literal_column
 from sqlalchemy import Numeric
 from sqlalchemy import select
 from sqlalchemy import String
+from sqlalchemy import testing
 from sqlalchemy.ext import hybrid
 from sqlalchemy.ext.declarative import declarative_base
 from sqlalchemy.orm import aliased
@@ -191,6 +193,24 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
 
         return A
 
+    def _wrong_expr_fixture(self):
+        Base = declarative_base()
+
+        class A(Base):
+            __tablename__ = "a"
+            id = Column(Integer, primary_key=True)
+            _value = Column("value", String)
+
+            @hybrid.hybrid_property
+            def value(self):
+                return self._value is not None
+
+            @value.expression
+            def value(cls):
+                return cls._value is not None
+
+        return A
+
     def _relationship_fixture(self):
         Base = declarative_base()
 
@@ -244,6 +264,19 @@ class PropertyExpressionTest(fixtures.TestBase, AssertsCompiledSQL):
             A.value.__clause_element__(), "foo(a.value) + bar(a.value)"
         )
 
+    def test_expression_isnt_clause_element(self):
+        A = self._wrong_expr_fixture()
+
+        from sqlalchemy.sql import coercions, roles
+
+        with testing.expect_raises_message(
+            exc.InvalidRequestError,
+            'When interpreting attribute "A.value" as a SQL expression, '
+            r"expected __clause_element__\(\) to return a "
+            "ClauseElement object, got: True",
+        ):
+            coercions.expect(roles.ExpressionElementRole, A.value)
+
     def test_any(self):
         A, B = self._relationship_fixture()
         sess = fixture_session()
diff --git a/test/sql/test_roles.py b/test/sql/test_roles.py
index 997311071..37f75594b 100644
--- a/test/sql/test_roles.py
+++ b/test/sql/test_roles.py
@@ -209,6 +209,23 @@ class RoleTest(fixtures.TestBase):
         ):
             expect(roles.ExpressionElementRole, t.select().alias())
 
+    def test_raise_on_regular_python_obj_for_expr(self):
+        """test #6350"""
+
+        def some_function():
+            pass
+
+        class Thing(object):
+            def __clause_element__(self):
+                return some_function
+
+        with testing.expect_raises_message(
+            exc.ArgumentError,
+            r"SQL expression element expected, got "
+            "<function .*some_function .* resolved from <.*Thing object .*",
+        ):
+            expect(roles.ExpressionElementRole, Thing())
+
     def test_statement_text_coercion(self):
         with testing.expect_deprecated_20(
             "Using plain strings to indicate SQL statements"