From: Mike Bayer Date: Fri, 23 Apr 2021 14:19:39 +0000 (-0400) Subject: dont assume ClauseElement in attributes, coercions X-Git-Tag: rel_1_4_12~33^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=373e960cb4448d1498d0a47fd35c97f83170f87e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 --- diff --git a/doc/build/changelog/unreleased_14/6350.rst b/doc/build/changelog/unreleased_14/6350.rst new file mode 100644 index 0000000000..c2a911b7a7 --- /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 9e326db64a..0c7bc4cf0f 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 c00262fd5a..b7aba9d747 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 d44e284a34..c9373f9aa0 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 9973110712..37f75594b1 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 " + "