Check for __clause_element__() in ORM insert/update

ORM attributes can now be assigned any object that is has a
``__clause_element__()`` attribute, which will result in inline
SQL the way any :class:`.ClauseElement` class does.  This covers other
mapped attributes not otherwise transformed by further expression
constructs.

As part of this, it was considered that we could add
__clause_element__() to ClauseElement, however this causes endless loops
in a "while" pattern and this pattern has been identified in third
party libraries.  Add a test to ensure we never make that change.

Change-Id: I9e15b3f1c4883fd3909acbf7dc81d034c6e3ce1d
Fixes: #3802

6 files changed, 54 insertions, 7 deletions

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index afdade444..d7046dee3 100644
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -22,6 +22,16 @@
     :version: 1.1.0
 
     .. change::
+        :tags: bug, orm
+        :tickets: 3802
+
+        ORM attributes can now be assigned any object that is has a
+        ``__clause_element__()`` attribute, which will result in inline
+        SQL the way any :class:`.ClauseElement` class does.  This covers other
+        mapped attributes not otherwise transformed by further expression
+        constructs.
+
+    .. change::
         :tags: feature, orm
         :tickets: 3812
 
diff --git a/doc/build/changelog/migration_11.rst b/doc/build/changelog/migration_11.rst
index 978e6abf1..560872fc9 100644
--- a/doc/build/changelog/migration_11.rst
+++ b/doc/build/changelog/migration_11.rst
@@ -214,7 +214,8 @@ Specific checks added for passing mapped classes, instances as SQL literals
 The typing system now has specific checks for passing of SQLAlchemy
 "inspectable" objects in contexts where they would otherwise be handled as
 literal values.   Any SQLAlchemy built-in object that is legal to pass as a
-SQL value includes a method ``__clause_element__()`` which provides a
+SQL value (which is not already a :class:`.ClauseElement` instance)
+includes a method ``__clause_element__()`` which provides a
 valid SQL expression for that object.  For SQLAlchemy objects that
 don't provide this, such as mapped classes, mappers, and mapped
 instances, a more informative error message is emitted rather than
@@ -2210,7 +2211,6 @@ the issue.
 
 :ticket:`3809`
 
-
 .. _change_2528:
 
 A UNION or similar of SELECTs with LIMIT/OFFSET/ORDER BY now parenthesizes the embedded selects
diff --git a/lib/sqlalchemy/ext/hybrid.py b/lib/sqlalchemy/ext/hybrid.py
index 2f473fd31..99f938ebb 100644
--- a/lib/sqlalchemy/ext/hybrid.py
+++ b/lib/sqlalchemy/ext/hybrid.py
@@ -809,7 +809,7 @@ class Comparator(interfaces.PropComparator):
 
     def __clause_element__(self):
         expr = self.expression
-        while hasattr(expr, '__clause_element__'):
+        if hasattr(expr, '__clause_element__'):
             expr = expr.__clause_element__()
         return expr
 
diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py
index 56b028375..24a33ee8d 100644
--- a/lib/sqlalchemy/orm/persistence.py
+++ b/lib/sqlalchemy/orm/persistence.py
@@ -388,8 +388,10 @@ def _collect_insert_commands(
             col = propkey_to_col[propkey]
             if value is None and propkey not in eval_none and not render_nulls:
                 continue
-            elif not bulk and isinstance(value, sql.ClauseElement):
-                value_params[col.key] = value
+            elif not bulk and hasattr(value, '__clause_element__') or \
+                    isinstance(value, sql.ClauseElement):
+                value_params[col.key] = value.__clause_element__() \
+                    if hasattr(value, '__clause_element__') else value
             else:
                 params[col.key] = value
 
@@ -462,8 +464,10 @@ def _collect_update_commands(
                 value = state_dict[propkey]
                 col = propkey_to_col[propkey]
 
-                if isinstance(value, sql.ClauseElement):
-                    value_params[col] = value
+                if hasattr(value, '__clause_element__') or \
+                        isinstance(value, sql.ClauseElement):
+                    value_params[col] = value.__clause_element__() \
+                        if hasattr(value, '__clause_element__') else value
                 # guard against values that generate non-__nonzero__
                 # objects for __eq__()
                 elif state.manager[propkey].impl.is_equal(
diff --git a/test/orm/test_unitofwork.py b/test/orm/test_unitofwork.py
index 72b913b1a..5b1f3b1b7 100644
--- a/test/orm/test_unitofwork.py
+++ b/test/orm/test_unitofwork.py
@@ -482,6 +482,29 @@ class ClauseAttributesTest(fixtures.MappedTest):
         assert 'value' not in hb.__dict__
         eq_(hb.value, False)
 
+    def test_clauseelement_accessor(self):
+        class Thing(object):
+            def __init__(self, value):
+                self.value = value
+
+            def __clause_element__(self):
+                return literal_column(str(self.value))
+
+        User = self.classes.User
+
+        u = User(id=5, name='test', counter=Thing(3))
+
+        session = create_session()
+        session.add(u)
+        session.flush()
+
+        u.counter = Thing(5)
+        session.flush()
+
+        def go():
+            eq_(u.counter, 5)
+        self.sql_count_(1, go)
+
 
 class PassiveDeletesTest(fixtures.MappedTest):
     __requires__ = ('foreign_keys',)
diff --git a/test/sql/test_inspect.py b/test/sql/test_inspect.py
index d5a9a71ae..12d377e51 100644
--- a/test/sql/test_inspect.py
+++ b/test/sql/test_inspect.py
@@ -32,3 +32,13 @@ class TestCoreInspection(fixtures.TestBase):
         is_(inspect(c), c)
         assert not c.is_selectable
         assert not hasattr(c, 'selectable')
+
+    def test_no_clause_element_on_clauseelement(self):
+        # re [ticket:3802], there are in the wild examples
+        # of looping over __clause_element__, therefore the
+        # absense of __clause_element__ as a test for "this is the clause
+        # element" must be maintained
+
+        x = Column('foo', Integer)
+        assert not hasattr(x, '__clause_element__')
+