From 7a96d2792dd7a65b8cc3a8af72d423c2c382b11d Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Thu, 5 Jun 2025 20:09:32 +0200 Subject: [PATCH] remove support for list of tuples in the normal execute The function that validates the arguments in the normal execute flow allowed by mistake list of tuples, that are not supported by the code since the 2.0 series. Change-Id: Ia401b0e19e72ed33b7d3d5718578cbed0d214c2a --- .../changelog/unreleased_20/no_tuples.rst | 8 ++++ lib/sqlalchemy/engine/_util_cy.py | 23 +++++++---- test/engine/test_processors.py | 39 +++++++------------ test/perf/compiled_extensions/misc.py | 24 ++++++------ 4 files changed, 50 insertions(+), 44 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/no_tuples.rst diff --git a/doc/build/changelog/unreleased_20/no_tuples.rst b/doc/build/changelog/unreleased_20/no_tuples.rst new file mode 100644 index 0000000000..c276b858df --- /dev/null +++ b/doc/build/changelog/unreleased_20/no_tuples.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: engine + + Improved validation of execution parameters passed to the + :meth:`_engine.Connection.execute` and similar methods to + provided a better error when tuples are passed in. + Previously the execution would fail with a difficult to + understand error message. diff --git a/lib/sqlalchemy/engine/_util_cy.py b/lib/sqlalchemy/engine/_util_cy.py index 6c45b22ef6..dd56c65d2a 100644 --- a/lib/sqlalchemy/engine/_util_cy.py +++ b/lib/sqlalchemy/engine/_util_cy.py @@ -57,7 +57,17 @@ def _is_mapping_or_tuple(value: object, /) -> cython.bint: ) -# _is_mapping_or_tuple could be inlined if pure python perf is a problem +@cython.inline +@cython.cfunc +def _is_mapping(value: object, /) -> cython.bint: + return ( + isinstance(value, dict) + or isinstance(value, Mapping) + # only do immutabledict or abc.__instancecheck__ for Mapping after + # we've checked for plain dictionaries and would otherwise raise + ) + + def _distill_params_20( params: Optional[_CoreAnyExecuteParams], ) -> _CoreMultiExecuteParams: @@ -73,19 +83,18 @@ def _distill_params_20( "future SQLAlchemy release", "2.1", ) - elif not _is_mapping_or_tuple(params[0]): + elif not _is_mapping(params[0]): raise exc.ArgumentError( - "List argument must consist only of tuples or dictionaries" + "List argument must consist only of dictionaries" ) return params - elif isinstance(params, dict) or isinstance(params, Mapping): - # only do immutabledict or abc.__instancecheck__ for Mapping after - # we've checked for plain dictionaries and would otherwise raise - return [params] + elif _is_mapping(params): + return [params] # type: ignore[list-item] else: raise exc.ArgumentError("mapping or list expected for parameters") +# _is_mapping_or_tuple could be inlined if pure python perf is a problem def _distill_raw_params( params: Optional[_DBAPIAnyExecuteParams], ) -> _DBAPIMultiExecuteParams: diff --git a/test/engine/test_processors.py b/test/engine/test_processors.py index cdb518c969..e43f642b44 100644 --- a/test/engine/test_processors.py +++ b/test/engine/test_processors.py @@ -153,13 +153,6 @@ class _DistillArgsTest(fixtures.TestBase): 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),)) - eq_(self.module._distill_params_20([(1, 2, 3)]), [(1, 2, 3)]) - - eq_(self.module._distill_params_20(((1, 2), (2, 3))), ((1, 2), (2, 3))) - eq_(self.module._distill_params_20([(1, 2), (2, 3)]), [(1, 2), (2, 3)]) - def test_distill_20_sequence_dict(self): eq_(self.module._distill_params_20(({"a": 1},)), ({"a": 1},)) eq_( @@ -171,27 +164,23 @@ class _DistillArgsTest(fixtures.TestBase): (MappingProxyType({"a": 1}),), ) - def test_distill_20_sequence_error(self): - with expect_raises_message( - exc.ArgumentError, - "List argument must consist only of tuples or dictionaries", - ): - self.module._distill_params_20((1, 2, 3)) + @combinations( + [(1, 2, 3)], + [([1, 2, 3],)], + [[1, 2, 3]], + [["a", "b"]], + [((1, 2, 3),)], + [[(1, 2, 3)]], + [((1, 2), (2, 3))], + [[(1, 2), (2, 3)]], + argnames="arg", + ) + def test_distill_20_sequence_error(self, arg): with expect_raises_message( exc.ArgumentError, - "List argument must consist only of tuples or dictionaries", - ): - self.module._distill_params_20(([1, 2, 3],)) - with expect_raises_message( - exc.ArgumentError, - "List argument must consist only of tuples or dictionaries", - ): - self.module._distill_params_20([1, 2, 3]) - with expect_raises_message( - exc.ArgumentError, - "List argument must consist only of tuples or dictionaries", + "List argument must consist only of dictionaries", ): - self.module._distill_params_20(["a", "b"]) + self.module._distill_params_20(arg) def test_distill_20_dict(self): eq_(self.module._distill_params_20({"foo": "bar"}), [{"foo": "bar"}]) diff --git a/test/perf/compiled_extensions/misc.py b/test/perf/compiled_extensions/misc.py index d051cca0b7..2ab6782e86 100644 --- a/test/perf/compiled_extensions/misc.py +++ b/test/perf/compiled_extensions/misc.py @@ -138,26 +138,22 @@ class DistillParam(Case): def none_20(self): self.impl._distill_params_20(None) - @test_case - def list_20(self): - self.impl._distill_params_20(self.list_tup) - - @test_case - def tuple_20(self): - self.impl._distill_params_20(self.tup_tup) - @test_case def list_dict_20(self): - self.impl._distill_params_20(self.list_tup) + self.impl._distill_params_20(self.list_dic) @test_case def tuple_dict_20(self): - self.impl._distill_params_20(self.dict) + self.impl._distill_params_20(self.tup_dic) @test_case def mapping_20(self): self.impl._distill_params_20(self.mapping) + @test_case + def dict_20(self): + self.impl._distill_params_20(self.dict) + @test_case def raw_none(self): self.impl._distill_raw_params(None) @@ -177,16 +173,20 @@ class DistillParam(Case): @test_case def raw_list_dict(self): - self.impl._distill_raw_params(self.list_tup) + self.impl._distill_raw_params(self.list_dic) @test_case def raw_tuple_dict(self): - self.impl._distill_raw_params(self.dict) + self.impl._distill_raw_params(self.tup_dic) @test_case def raw_mapping(self): self.impl._distill_raw_params(self.mapping) + @test_case + def raw_dict(self): + self.impl._distill_raw_params(self.mapping) + class AnonMap(Case): NUMBER = 5_000_000 -- 2.47.2