From af655e55d3cb15895901de7803726ee434389445 Mon Sep 17 00:00:00 2001 From: Carlos Sousa Date: Mon, 8 Jan 2024 14:50:02 -0500 Subject: [PATCH] Warn in execute when parameter is an empty list An empty sequence passed to any ``execute()`` method now raised a deprecation warning, since such an executemany is invalid. Pull request courtesy of Carlos Sousa. Fixes: #9647 Closes: #10406 Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/10406 Pull-request-sha: 087ba2d88d079b361e30e698e251c022b5780a3d Change-Id: I482e91a047477c3156a3ca806e5c1eefb6224b95 --- doc/build/changelog/unreleased_21/9647.rst | 8 ++++ lib/sqlalchemy/engine/_util_cy.py | 40 +++++++++---------- lib/sqlalchemy/orm/session.py | 2 +- .../testing/suite/test_deprecations.py | 2 +- lib/sqlalchemy/testing/suite/test_select.py | 10 ++--- test/dialect/oracle/test_types.py | 12 +----- test/engine/test_execute.py | 36 ++++++++++++++--- test/engine/test_processors.py | 10 ++++- test/orm/test_session.py | 18 +++++++++ test/perf/compiled_extensions/misc.py | 5 --- test/perf/compiled_extensions/row.py | 7 ---- test/requirements.py | 9 +---- 12 files changed, 92 insertions(+), 67 deletions(-) create mode 100644 doc/build/changelog/unreleased_21/9647.rst diff --git a/doc/build/changelog/unreleased_21/9647.rst b/doc/build/changelog/unreleased_21/9647.rst new file mode 100644 index 0000000000..f933b083b3 --- /dev/null +++ b/doc/build/changelog/unreleased_21/9647.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: change, engine + :tickets: 9647 + + An empty sequence passed to any ``execute()`` method now + raised a deprecation warning, since such an executemany + is invalid. + Pull request courtesy of Carlos Sousa. diff --git a/lib/sqlalchemy/engine/_util_cy.py b/lib/sqlalchemy/engine/_util_cy.py index 156fcce998..1eaf38f07d 100644 --- a/lib/sqlalchemy/engine/_util_cy.py +++ b/lib/sqlalchemy/engine/_util_cy.py @@ -11,11 +11,11 @@ from collections.abc import Mapping import operator from typing import Any from typing import Optional -from typing import Sequence from typing import Tuple from typing import TYPE_CHECKING -from sqlalchemy import exc +from .. import exc +from ..util import warn_deprecated if TYPE_CHECKING: from .interfaces import _CoreAnyExecuteParams @@ -47,7 +47,7 @@ _Empty_Tuple: Tuple[Any, ...] = cython.declare(tuple, ()) @cython.inline @cython.cfunc -def _is_mapping_or_tuple(value: object) -> cython.bint: +def _is_mapping_or_tuple(value: object, /) -> cython.bint: return ( isinstance(value, dict) or isinstance(value, tuple) @@ -57,22 +57,7 @@ def _is_mapping_or_tuple(value: object) -> cython.bint: ) -@cython.inline -@cython.cfunc -@cython.exceptval(0) -def _validate_execute_many_item(params: Sequence[Any]) -> cython.bint: - ret: cython.bint = 1 - if len(params) > 0: - if not _is_mapping_or_tuple(params[0]): - ret = 0 - raise exc.ArgumentError( - "List argument must consist only of tuples or dictionaries" - ) - return ret - - -# _is_mapping_or_tuple and _validate_execute_many_item could be -# inlined if pure python perf is a problem +# _is_mapping_or_tuple could be inlined if pure python perf is a problem def _distill_params_20( params: Optional[_CoreAnyExecuteParams], ) -> _CoreMultiExecuteParams: @@ -81,7 +66,17 @@ def _distill_params_20( # Assume list is more likely than tuple elif isinstance(params, list) or isinstance(params, tuple): # collections_abc.MutableSequence # avoid abc.__instancecheck__ - _validate_execute_many_item(params) + if len(params) == 0: + warn_deprecated( + "Empty parameter sequence passed to execute(). " + "This use is deprecated and will raise an exception in a " + "future SQLAlchemy release", + "2.1", + ) + elif not _is_mapping_or_tuple(params[0]): + raise exc.ArgumentError( + "List argument must consist only of tuples or dictionaries" + ) return params elif isinstance(params, dict) or isinstance(params, Mapping): # only do immutabledict or abc.__instancecheck__ for Mapping after @@ -98,7 +93,10 @@ def _distill_raw_params( return _Empty_Tuple elif isinstance(params, list): # collections_abc.MutableSequence # avoid abc.__instancecheck__ - _validate_execute_many_item(params) + if len(params) > 0 and not _is_mapping_or_tuple(params[0]): + raise exc.ArgumentError( + "List argument must consist only of tuples or dictionaries" + ) return params elif _is_mapping_or_tuple(params): return [params] # type: ignore[return-value] diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 13b906fe24..b77aa72d22 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -2252,7 +2252,7 @@ class Session(_SessionClassMethods, EventTarget): ) else: result = conn.execute( - statement, params or {}, execution_options=execution_options + statement, params, execution_options=execution_options ) if _scalar_result: diff --git a/lib/sqlalchemy/testing/suite/test_deprecations.py b/lib/sqlalchemy/testing/suite/test_deprecations.py index 07970c03ec..dc6a71a901 100644 --- a/lib/sqlalchemy/testing/suite/test_deprecations.py +++ b/lib/sqlalchemy/testing/suite/test_deprecations.py @@ -41,7 +41,7 @@ class DeprecatedCompoundSelectTest(fixtures.TablesTest): ], ) - def _assert_result(self, conn, select, result, params=()): + def _assert_result(self, conn, select, result, params=None): eq_(conn.execute(select, params).fetchall(), result) def test_plain_union(self, connection): diff --git a/lib/sqlalchemy/testing/suite/test_select.py b/lib/sqlalchemy/testing/suite/test_select.py index 866bf09cb5..8ab6d57bbe 100644 --- a/lib/sqlalchemy/testing/suite/test_select.py +++ b/lib/sqlalchemy/testing/suite/test_select.py @@ -204,7 +204,7 @@ class FetchLimitOffsetTest(fixtures.TablesTest): ) def _assert_result( - self, connection, select, result, params=(), set_=False + self, connection, select, result, params=None, set_=False ): if set_: query_res = connection.execute(select, params).fetchall() @@ -214,7 +214,7 @@ class FetchLimitOffsetTest(fixtures.TablesTest): else: eq_(connection.execute(select, params).fetchall(), result) - def _assert_result_str(self, select, result, params=()): + def _assert_result_str(self, select, result, params=None): with config.db.connect() as conn: eq_(conn.exec_driver_sql(select, params).fetchall(), result) @@ -734,7 +734,7 @@ class SameNamedSchemaTableTest(fixtures.TablesTest): class JoinTest(fixtures.TablesTest): __backend__ = True - def _assert_result(self, select, result, params=()): + def _assert_result(self, select, result, params=None): with config.db.connect() as conn: eq_(conn.execute(select, params).fetchall(), result) @@ -856,7 +856,7 @@ class CompoundSelectTest(fixtures.TablesTest): ], ) - def _assert_result(self, select, result, params=()): + def _assert_result(self, select, result, params=None): with config.db.connect() as conn: eq_(conn.execute(select, params).fetchall(), result) @@ -1121,7 +1121,7 @@ class ExpandingBoundInTest(fixtures.TablesTest): ], ) - def _assert_result(self, select, result, params=()): + def _assert_result(self, select, result, params=None): with config.db.connect() as conn: eq_(conn.execute(select, params).fetchall(), result) diff --git a/test/dialect/oracle/test_types.py b/test/dialect/oracle/test_types.py index 3bf78c105a..b8396df4fa 100644 --- a/test/dialect/oracle/test_types.py +++ b/test/dialect/oracle/test_types.py @@ -39,7 +39,6 @@ from sqlalchemy.dialects.oracle import cx_oracle from sqlalchemy.dialects.oracle import oracledb from sqlalchemy.sql import column from sqlalchemy.sql.sqltypes import NullType -from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ from sqlalchemy.testing import expect_raises_message @@ -1047,16 +1046,7 @@ class LOBFetchTest(fixtures.TablesTest): ) eq_(actual, self.data) - # this comes from cx_Oracle because these are raw - # cx_Oracle.Variable objects - if testing.requires.oracle5x.enabled: - assert_raises_message( - testing.db.dialect.dbapi.ProgrammingError, - "LOB variable no longer valid after subsequent fetch", - go, - ) - else: - go() + go() def test_lobs_with_convert_many_rows(self): # even with low arraysize, lobs are fine in autoconvert diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py index 122c08461d..31a9c4a70a 100644 --- a/test/engine/test_execute.py +++ b/test/engine/test_execute.py @@ -51,6 +51,7 @@ from sqlalchemy.testing import is_ from sqlalchemy.testing import is_false from sqlalchemy.testing import is_not from sqlalchemy.testing import is_true +from sqlalchemy.testing.assertions import expect_deprecated from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.provision import normalize_sequence from sqlalchemy.testing.schema import Column @@ -637,14 +638,37 @@ class ExecuteTest(fixtures.TablesTest): conn.close() def test_empty_insert(self, connection): - """test that execute() interprets [] as a list with no params""" + """test that execute() interprets [] as a list with no params and + warns since it has nothing to do with such an executemany. + """ users_autoinc = self.tables.users_autoinc - connection.execute( - users_autoinc.insert().values(user_name=bindparam("name", None)), - [], - ) - eq_(connection.execute(users_autoinc.select()).fetchall(), [(1, None)]) + with expect_deprecated( + r"Empty parameter sequence passed to execute\(\). " + "This use is deprecated and will raise an exception in a " + "future SQLAlchemy release" + ): + connection.execute( + users_autoinc.insert().values( + user_name=bindparam("name", None) + ), + [], + ) + + eq_(len(connection.execute(users_autoinc.select()).all()), 1) + + @testing.only_on("sqlite") + def test_raw_insert_with_empty_list(self, connection): + """exec_driver_sql instead does not raise if an empty list is passed. + Let the driver do that if it wants to. + """ + conn = connection + with expect_raises_message( + tsa.exc.ProgrammingError, "Incorrect number of bindings supplied" + ): + conn.exec_driver_sql( + "insert into users (user_id, user_name) values (?, ?)", [] + ) @testing.only_on("sqlite") def test_execute_compiled_favors_compiled_paramstyle(self): diff --git a/test/engine/test_processors.py b/test/engine/test_processors.py index d49396e99d..cdb518c969 100644 --- a/test/engine/test_processors.py +++ b/test/engine/test_processors.py @@ -10,6 +10,7 @@ from sqlalchemy.testing import eq_ from sqlalchemy.testing import expect_raises_message from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_none +from sqlalchemy.testing.assertions import expect_deprecated from sqlalchemy.util import immutabledict @@ -144,8 +145,13 @@ class _DistillArgsTest(fixtures.TestBase): eq_(self.module._distill_params_20(None), ()) def test_distill_20_empty_sequence(self): - eq_(self.module._distill_params_20(()), ()) - eq_(self.module._distill_params_20([]), []) + with expect_deprecated( + r"Empty parameter sequence passed to execute\(\). " + "This use is deprecated and will raise an exception in a " + "future SQLAlchemy release" + ): + eq_(self.module._distill_params_20(()), ()) + eq_(self.module._distill_params_20([]), []) def test_distill_20_sequence_sequence(self): eq_(self.module._distill_params_20(((1, 2, 3),)), ((1, 2, 3),)) diff --git a/test/orm/test_session.py b/test/orm/test_session.py index e08ab19c6e..6e9720774e 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -129,6 +129,24 @@ class ExecutionTest(_fixtures.FixtureTest): ): sess.scalar("select id from users where id=:id", {"id": 7}) + @testing.skip_if( + "oracle", "missing SELECT keyword [SQL: INSERT INTO tbl () VALUES ()]" + ) + def test_empty_list_execute(self, metadata, connection): + t = Table("tbl", metadata, Column("col", sa.Integer)) + t.create(connection) + sess = Session(bind=connection) + sess.execute(t.insert(), {"col": 42}) + + with assertions.expect_deprecated( + r"Empty parameter sequence passed to execute\(\). " + "This use is deprecated and will raise an exception in a " + "future SQLAlchemy release" + ): + sess.execute(t.insert(), []) + + eq_(len(sess.execute(sa.select(t.c.col)).all()), 2) + class TransScopingTest(_fixtures.FixtureTest): run_inserts = None diff --git a/test/perf/compiled_extensions/misc.py b/test/perf/compiled_extensions/misc.py index 01ff055b28..d051cca0b7 100644 --- a/test/perf/compiled_extensions/misc.py +++ b/test/perf/compiled_extensions/misc.py @@ -138,11 +138,6 @@ class DistillParam(Case): def none_20(self): self.impl._distill_params_20(None) - @test_case - def empty_sequence_20(self): - self.impl._distill_params_20(()) - self.impl._distill_params_20([]) - @test_case def list_20(self): self.impl._distill_params_20(self.list_tup) diff --git a/test/perf/compiled_extensions/row.py b/test/perf/compiled_extensions/row.py index 7fe8d00342..227bc8915b 100644 --- a/test/perf/compiled_extensions/row.py +++ b/test/perf/compiled_extensions/row.py @@ -14,12 +14,6 @@ class TupleGetter(Case): assert not py_util._is_compiled() return py_util.tuplegetter - @staticmethod - def c(): - from sqlalchemy import cresultproxy - - return cresultproxy.tuplegetter - @staticmethod def cython(): from sqlalchemy.engine import _util_cy @@ -29,7 +23,6 @@ class TupleGetter(Case): IMPLEMENTATIONS = { "python": python.__func__, - "c": c.__func__, "cython": cython.__func__, } diff --git a/test/requirements.py b/test/requirements.py index 2e80884bc1..f8f62fafaf 100644 --- a/test/requirements.py +++ b/test/requirements.py @@ -463,7 +463,7 @@ class DefaultRequirements(SuiteRequirements): @property def returning_star(self): - """backend supports RETURNING *""" + """backend supports ``RETURNING *``""" return skip_if(["oracle", "mssql"]) @@ -1870,13 +1870,6 @@ class DefaultRequirements(SuiteRequirements): return only_if(go) - @property - def oracle5x(self): - return only_if( - lambda config: against(config, "oracle+cx_oracle") - and config.db.dialect.cx_oracle_ver < (6,) - ) - @property def fail_on_oracledb_thin(self): def go(config): -- 2.47.2