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

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index afdade444b269153528cb9d7d52b5f24c6179bf5..d7046dee315d84c49d97bbaab4c863efdef96ca8 100644 (file)
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -21,6 +21,16 @@
 .. changelog::
     :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 978e6abf19742a75f47b4df26ea9226b421bf14a..560872fc92cb1d4cf75cfd9994511030b3a1101f 100644 (file)
--- 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 2f473fd314068367be02779e395b902f2b0d1827..99f938ebbeebcb1a5563567617a94fc7de314b34 100644 (file)
--- 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 56b028375694f865631bb2719781fcf463159bd4..24a33ee8dc61e65f474b5df56cf4219ac15efc9b 100644 (file)
--- 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 72b913b1a6dcf4dea00963709c056887d4ca7812..5b1f3b1b78d65a0c4247dde261f2be18907356bd 100644 (file)
--- 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 d5a9a71ae5914a1d11bae04b11f1ad8f47888e70..12d377e5187b0178ad614ffeabaf068ec5bfb954 100644 (file)
--- 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__')
+