From: Federico Caselli Date: Thu, 23 Jan 2020 22:51:38 +0000 (-0500) Subject: Deprecate empty or_() and and_() X-Git-Tag: rel_1_4_0b1~543 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=1de64504d8e68e2c0d14669c7638cf6f6d74973f;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Deprecate empty or_() and and_() Creating an :func:`.and_` or :func:`.or_` construct with no arguments or empty ``*args`` will now emit a deprecation warning, as the SQL produced is a no-op (i.e. it renders as a blank string). This behavior is considered to be non-intuitive, so for empty or possibly empty :func:`.and_` or :func:`.or_` constructs, an appropriate default boolean should be included, such as ``and_(True, *args)`` or ``or_(False, *args)``. As has been the case for many major versions of SQLAlchemy, these particular boolean values will not render if the ``*args`` portion is non-empty. As there are some internal cases where an empty and_() construct is used in order to build an optional WHERE expression, a private utility function is added to suit this use case. Co-authored-by: Mike Bayer Fixes: #5054 Closes: #5062 Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/5062 Pull-request-sha: 5ca2f27281977d74e390148c0fb8deaa0e0e4ad9 Change-Id: I599b9c8befa64d9a59a35ad7dd84ff400e3aa647 --- diff --git a/doc/build/changelog/unreleased_14/5054.rst b/doc/build/changelog/unreleased_14/5054.rst new file mode 100644 index 0000000000..cf4aafebdb --- /dev/null +++ b/doc/build/changelog/unreleased_14/5054.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, sql + :tickets: 5054 + + Creating an :func:`.and_` or :func:`.or_` construct with no arguments or + empty ``*args`` will now emit a deprecation warning, as the SQL produced is + a no-op (i.e. it renders as a blank string). This behavior is considered to + be non-intuitive, so for empty or possibly empty :func:`.and_` or + :func:`.or_` constructs, an appropriate default boolean should be included, + such as ``and_(True, *args)`` or ``or_(False, *args)``. As has been the + case for many major versions of SQLAlchemy, these particular boolean + values will not render if the ``*args`` portion is non-empty. diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index 06aab11013..31b8b0a208 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -30,8 +30,10 @@ from .. import sql from .. import util from ..sql import coercions from ..sql import expression +from ..sql import operators from ..sql import roles from ..sql.base import _from_objects +from ..sql.elements import BooleanClauseList def _bulk_insert( @@ -864,15 +866,15 @@ def _emit_update_statements( ) def update_stmt(): - clause = sql.and_() + clauses = BooleanClauseList._construct_raw(operators.and_) for col in mapper._pks_by_table[table]: - clause.clauses.append( + clauses.clauses.append( col == sql.bindparam(col._label, type_=col.type) ) if needs_version_id: - clause.clauses.append( + clauses.clauses.append( mapper.version_id_col == sql.bindparam( mapper.version_id_col._label, @@ -880,7 +882,7 @@ def _emit_update_statements( ) ) - stmt = table.update(clause) + stmt = table.update(clauses) return stmt cached_stmt = base_mapper._memo(("update", table), update_stmt) @@ -1180,15 +1182,15 @@ def _emit_post_update_statements( ) def update_stmt(): - clause = sql.and_() + clauses = BooleanClauseList._construct_raw(operators.and_) for col in mapper._pks_by_table[table]: - clause.clauses.append( + clauses.clauses.append( col == sql.bindparam(col._label, type_=col.type) ) if needs_version_id: - clause.clauses.append( + clauses.clauses.append( mapper.version_id_col == sql.bindparam( mapper.version_id_col._label, @@ -1196,7 +1198,7 @@ def _emit_post_update_statements( ) ) - stmt = table.update(clause) + stmt = table.update(clauses) if mapper.version_id_col is not None: stmt = stmt.return_defaults(mapper.version_id_col) @@ -1295,21 +1297,22 @@ def _emit_delete_statements( ) def delete_stmt(): - clause = sql.and_() + clauses = BooleanClauseList._construct_raw(operators.and_) + for col in mapper._pks_by_table[table]: - clause.clauses.append( + clauses.clauses.append( col == sql.bindparam(col.key, type_=col.type) ) if need_version_id: - clause.clauses.append( + clauses.clauses.append( mapper.version_id_col == sql.bindparam( mapper.version_id_col.key, type_=mapper.version_id_col.type ) ) - return table.delete(clause) + return table.delete(clauses) statement = base_mapper._memo(("delete", table), delete_stmt) for connection, recs in groupby(delete, lambda rec: rec[1]): # connection diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index 648394ed2e..a99c6ca356 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -2131,32 +2131,64 @@ class BooleanClauseList(ClauseList, ColumnElement): @classmethod def _construct(cls, operator, continue_on, skip_on, *clauses, **kw): + + has_continue_on = None + special_elements = (continue_on, skip_on) convert_clauses = [] - clauses = [ - coercions.expect(roles.WhereHavingRole, clause) - for clause in util.coerce_generator_arg(clauses) - ] - for clause in clauses: + for clause in util.coerce_generator_arg(clauses): + clause = coercions.expect(roles.WhereHavingRole, clause) - if isinstance(clause, continue_on): - continue + # elements that are not the continue/skip are the most + # common, try to have only one isinstance() call for that case. + if not isinstance(clause, special_elements): + convert_clauses.append(clause) elif isinstance(clause, skip_on): + # instance of skip_on, e.g. and_(x, y, False, z), cancels + # the rest out return clause.self_group(against=operators._asbool) - - convert_clauses.append(clause) - - if len(convert_clauses) == 1: + elif has_continue_on is None: + # instance of continue_on, like and_(x, y, True, z), store it + # if we didn't find one already, we will use it if there + # are no other expressions here. + has_continue_on = clause + + lcc = len(convert_clauses) + + if lcc > 1: + # multiple elements. Return regular BooleanClauseList + # which will link elements against the operator. + return cls._construct_raw( + operator, + [c.self_group(against=operator) for c in convert_clauses], + ) + elif lcc == 1: + # just one element. return it as a single boolean element, + # not a list and discard the operator. return convert_clauses[0].self_group(against=operators._asbool) - elif not convert_clauses and clauses: - return clauses[0].self_group(against=operators._asbool) - - convert_clauses = [ - c.self_group(against=operator) for c in convert_clauses - ] + elif not lcc and has_continue_on is not None: + # no elements but we had a "continue", just return the continue + # as a boolean element, discard the operator. + return has_continue_on.self_group(against=operators._asbool) + else: + # no elements period. deprecated use case. return an empty + # ClauseList construct that generates nothing unless it has + # elements added to it. + util.warn_deprecated( + "Invoking %(name)s() without arguments is deprecated, and " + "will be disallowed in a future release. For an empty " + "%(name)s() construct, use %(name)s(%(continue_on)s, *args)." + % { + "name": operator.__name__, + "continue_on": "True" if continue_on is True_ else "False", + } + ) + return cls._construct_raw(operator) + @classmethod + def _construct_raw(cls, operator, clauses=None): self = cls.__new__(cls) - self.clauses = convert_clauses + self.clauses = clauses if clauses else [] self.group = True self.operator = operator self.group_contents = True @@ -2198,6 +2230,25 @@ class BooleanClauseList(ClauseList, ColumnElement): where(users_table.c.name == 'wendy').\ where(users_table.c.enrolled == True) + The :func:`.and_` construct must be given at least one positional + argument in order to be valid; a :func:`.and_` construct with no + arguments is ambiguous. To produce an "empty" or dynamically + generated :func:`.and_` expression, from a given list of expressions, + a "default" element of ``True`` should be specified:: + + criteria = and_(True, *expressions) + + The above expression will compile to SQL as the expression ``true`` + or ``1 = 1``, depending on backend, if no other expressions are + present. If expressions are present, then the ``True`` value is + ignored as it does not affect the outcome of an AND expression that + has other elements. + + .. deprecated:: 1.4 The :func:`.and_` element now requires that at + least one argument is passed; creating the :func:`.and_` construct + with no arguments is deprecated, and will emit a deprecation warning + while continuing to produce a blank SQL string. + .. seealso:: :func:`.or_` @@ -2230,6 +2281,25 @@ class BooleanClauseList(ClauseList, ColumnElement): (users_table.c.name == 'jack') ) + The :func:`.or_` construct must be given at least one positional + argument in order to be valid; a :func:`.or_` construct with no + arguments is ambiguous. To produce an "empty" or dynamically + generated :func:`.or_` expression, from a given list of expressions, + a "default" element of ``False`` should be specified:: + + or_criteria = or_(False, *expressions) + + The above expression will compile to SQL as the expression ``false`` + or ``0 = 1``, depending on backend, if no other expressions are + present. If expressions are present, then the ``False`` value is + ignored as it does not affect the outcome of an OR expression which + has other elements. + + .. deprecated:: 1.4 The :func:`.or_` element now requires that at + least one argument is passed; creating the :func:`.or_` construct + with no arguments is deprecated, and will emit a deprecation warning + while continuing to produce a blank SQL string. + .. seealso:: :func:`.and_` diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py index 2c6473166c..ac9f034feb 100644 --- a/test/engine/test_execute.py +++ b/test/engine/test_execute.py @@ -390,9 +390,9 @@ class ExecuteTest(fixtures.TestBase): for obj in ( Table("foo", MetaData(), Column("x", Integer)), Column("x", Integer), - tsa.and_(), + tsa.and_(True), + tsa.and_(True).compile(), column("foo"), - tsa.and_().compile(), column("foo").compile(), MetaData(), Integer(), diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index b49fb455cd..b53acf61ea 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -70,7 +70,9 @@ from sqlalchemy.ext.compiler import compiles from sqlalchemy.sql import column from sqlalchemy.sql import compiler from sqlalchemy.sql import label +from sqlalchemy.sql import operators from sqlalchemy.sql import table +from sqlalchemy.sql.elements import BooleanClauseList from sqlalchemy.sql.expression import ClauseList from sqlalchemy.sql.expression import HasPrefixes from sqlalchemy.testing import assert_raises @@ -1391,12 +1393,22 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL): select([t]).where(and_(or_(t.c.x == 12, and_(or_(t.c.x == 8))))), "SELECT t.x FROM t WHERE t.x = :x_1 OR t.x = :x_2", ) + self.assert_compile( + select([t]).where( + and_(or_(or_(t.c.x == 12), and_(or_(and_(t.c.x == 8))))) + ), + "SELECT t.x FROM t WHERE t.x = :x_1 OR t.x = :x_2", + ) self.assert_compile( select([t]).where( and_( or_( or_(t.c.x == 12), - and_(or_(), or_(and_(t.c.x == 8)), and_()), + and_( + BooleanClauseList._construct_raw(operators.or_), + or_(and_(t.c.x == 8)), + BooleanClauseList._construct_raw(operators.and_), + ), ) ) ), @@ -1451,11 +1463,15 @@ class SelectTest(fixtures.TestBase, AssertsCompiledSQL): def test_where_empty(self): self.assert_compile( - select([table1.c.myid]).where(and_()), + select([table1.c.myid]).where( + BooleanClauseList._construct_raw(operators.and_) + ), "SELECT mytable.myid FROM mytable", ) self.assert_compile( - select([table1.c.myid]).where(or_()), + select([table1.c.myid]).where( + BooleanClauseList._construct_raw(operators.or_) + ), "SELECT mytable.myid FROM mytable", ) diff --git a/test/sql/test_delete.py b/test/sql/test_delete.py index 1f4c49c562..5dcc0d112b 100644 --- a/test/sql/test_delete.py +++ b/test/sql/test_delete.py @@ -15,6 +15,7 @@ from sqlalchemy.engine import default from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ +from sqlalchemy.testing import expect_deprecated from sqlalchemy.testing import fixtures from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table @@ -77,12 +78,14 @@ class DeleteTest(_DeleteTestBase, fixtures.TablesTest, AssertsCompiledSQL): def test_where_empty(self): table1 = self.tables.mytable - self.assert_compile( - table1.delete().where(and_()), "DELETE FROM mytable" - ) - self.assert_compile( - table1.delete().where(or_()), "DELETE FROM mytable" - ) + with expect_deprecated(): + self.assert_compile( + table1.delete().where(and_()), "DELETE FROM mytable" + ) + with expect_deprecated(): + self.assert_compile( + table1.delete().where(or_()), "DELETE FROM mytable" + ) def test_prefix_with(self): table1 = self.tables.mytable diff --git a/test/sql/test_deprecations.py b/test/sql/test_deprecations.py index ec7b2ac38e..4e88dcdb86 100644 --- a/test/sql/test_deprecations.py +++ b/test/sql/test_deprecations.py @@ -1,6 +1,7 @@ #! coding: utf-8 from sqlalchemy import alias +from sqlalchemy import and_ from sqlalchemy import bindparam from sqlalchemy import CHAR from sqlalchemy import column @@ -14,6 +15,7 @@ from sqlalchemy import join from sqlalchemy import literal_column from sqlalchemy import MetaData from sqlalchemy import null +from sqlalchemy import or_ from sqlalchemy import select from sqlalchemy import sql from sqlalchemy import String @@ -42,7 +44,7 @@ from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table -class DeprecationWarningsTest(fixtures.TestBase): +class DeprecationWarningsTest(fixtures.TestBase, AssertsCompiledSQL): __backend__ = True def test_ident_preparer_force(self): @@ -122,6 +124,14 @@ class DeprecationWarningsTest(fixtures.TestBase): ): select([column("x")], for_update=True) + def test_empty_and_or(self): + with testing.expect_deprecated( + r"Invoking and_\(\) without arguments is deprecated, and " + r"will be disallowed in a future release. For an empty " + r"and_\(\) construct, use and_\(True, \*args\)" + ): + self.assert_compile(or_(and_()), "") + class ConvertUnicodeDeprecationTest(fixtures.TestBase): diff --git a/test/sql/test_operators.py b/test/sql/test_operators.py index a90b03b387..d3afc2deef 100644 --- a/test/sql/test_operators.py +++ b/test/sql/test_operators.py @@ -42,6 +42,7 @@ from sqlalchemy.sql import sqltypes from sqlalchemy.sql import table from sqlalchemy.sql import true from sqlalchemy.sql.elements import BindParameter +from sqlalchemy.sql.elements import BooleanClauseList from sqlalchemy.sql.elements import Label from sqlalchemy.sql.expression import BinaryExpression from sqlalchemy.sql.expression import ClauseList @@ -51,6 +52,7 @@ from sqlalchemy.sql.expression import tuple_ from sqlalchemy.sql.expression import UnaryExpression from sqlalchemy.sql.expression import union from sqlalchemy.testing import assert_raises_message +from sqlalchemy.testing import combinations from sqlalchemy.testing import eq_ from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures @@ -1062,14 +1064,67 @@ class ConjunctionTest(fixtures.TestBase, testing.AssertsCompiledSQL): __dialect__ = default.DefaultDialect(supports_native_boolean=True) - def test_one(self): + def test_single_bool_one(self): self.assert_compile(~and_(true()), "false") - def test_two(self): + def test_single_bool_two(self): + self.assert_compile(~and_(True), "false") + + def test_single_bool_three(self): self.assert_compile(or_(~and_(true())), "false") - def test_three(self): - self.assert_compile(or_(and_()), "") + def test_single_bool_four(self): + self.assert_compile(~or_(false()), "true") + + def test_single_bool_five(self): + self.assert_compile(~or_(False), "true") + + def test_single_bool_six(self): + self.assert_compile(and_(~or_(false())), "true") + + def test_single_bool_seven(self): + self.assert_compile(and_(True), "true") + + def test_single_bool_eight(self): + self.assert_compile(or_(False), "false") + + def test_single_bool_nine(self): + self.assert_compile( + and_(True), + "1 = 1", + dialect=default.DefaultDialect(supports_native_boolean=False), + ) + + def test_single_bool_ten(self): + self.assert_compile( + or_(False), + "0 = 1", + dialect=default.DefaultDialect(supports_native_boolean=False), + ) + + @combinations((and_, "and_", "True"), (or_, "or_", "False")) + def test_empty_clauses(self, op, str_op, str_continue): + # these warning classes will change to ArgumentError when the + # deprecated behavior is disabled + assert_raises_message( + exc.SADeprecationWarning, + r"Invoking %(str_op)s\(\) without arguments is deprecated, and " + r"will be disallowed in a future release. For an empty " + r"%(str_op)s\(\) construct, use " + r"%(str_op)s\(%(str_continue)s, \*args\)\." + % {"str_op": str_op, "str_continue": str_continue}, + op, + ) + + def test_empty_and_raw(self): + self.assert_compile( + BooleanClauseList._construct_raw(operators.and_), "" + ) + + def test_empty_or_raw(self): + self.assert_compile( + BooleanClauseList._construct_raw(operators.and_), "" + ) def test_four(self): x = column("x") diff --git a/test/sql/test_selectable.py b/test/sql/test_selectable.py index be7e28b5d3..c3655efd2d 100644 --- a/test/sql/test_selectable.py +++ b/test/sql/test_selectable.py @@ -32,6 +32,7 @@ from sqlalchemy.sql import Alias from sqlalchemy.sql import base from sqlalchemy.sql import column from sqlalchemy.sql import elements +from sqlalchemy.sql import operators from sqlalchemy.sql import table from sqlalchemy.sql import util as sql_util from sqlalchemy.sql import visitors @@ -2678,7 +2679,8 @@ class ReprTest(fixtures.TestBase): elements.True_(), elements.False_(), elements.ClauseList(), - elements.BooleanClauseList.and_(), + elements.BooleanClauseList._construct_raw(operators.and_), + elements.BooleanClauseList._construct_raw(operators.or_), elements.Tuple(), elements.Case([]), elements.Extract("foo", column("x")), diff --git a/test/sql/test_update.py b/test/sql/test_update.py index e625b7d9ce..0313db8327 100644 --- a/test/sql/test_update.py +++ b/test/sql/test_update.py @@ -1,4 +1,3 @@ -from sqlalchemy import and_ from sqlalchemy import bindparam from sqlalchemy import column from sqlalchemy import exc @@ -8,7 +7,6 @@ from sqlalchemy import func from sqlalchemy import Integer from sqlalchemy import literal from sqlalchemy import MetaData -from sqlalchemy import or_ from sqlalchemy import select from sqlalchemy import String from sqlalchemy import table @@ -18,6 +16,8 @@ from sqlalchemy import update from sqlalchemy import util from sqlalchemy.dialects import mysql from sqlalchemy.engine import default +from sqlalchemy.sql import operators +from sqlalchemy.sql.elements import BooleanClauseList from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL @@ -634,12 +634,16 @@ class UpdateTest(_UpdateFromTestBase, fixtures.TablesTest, AssertsCompiledSQL): def test_where_empty(self): table1 = self.tables.mytable self.assert_compile( - table1.update().where(and_()), + table1.update().where( + BooleanClauseList._construct_raw(operators.and_) + ), "UPDATE mytable SET myid=:myid, name=:name, " "description=:description", ) self.assert_compile( - table1.update().where(or_()), + table1.update().where( + BooleanClauseList._construct_raw(operators.or_) + ), "UPDATE mytable SET myid=:myid, name=:name, " "description=:description", )