From: Mike Bayer Date: Wed, 5 Oct 2016 14:57:30 +0000 (-0400) Subject: Check for __clause_element__() in ORM insert/update X-Git-Tag: rel_1_1_0~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=09b685b24b19636e11169c181b45333f9739ca35;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 afdade444b..d7046dee31 100644 --- 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 978e6abf19..560872fc92 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 2f473fd314..99f938ebbe 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 56b0283756..24a33ee8dc 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 72b913b1a6..5b1f3b1b78 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 d5a9a71ae5..12d377e518 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__') +