From: Mike Bayer Date: Wed, 11 Oct 2023 14:28:34 +0000 (-0400) Subject: dont enter do_executemany if implicit_returning is False X-Git-Tag: rel_2_0_22~6 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=20c1b1b1ede5d66e38ca1c4ae4a708fc33245b1d;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git dont enter do_executemany if implicit_returning is False Fixed regression in recently revised "insertmanyvalues" feature (likely issue :ticket:`9618`) where the ORM would inadvertently attempt to interpret a non-RETURNING result as one with RETURNING, in the case where the ``implicit_returning=False`` parameter were applied to the mapped :class:`.Table`, indicating that "insertmanyvalues" cannot be used if the primary key values are not provided. This includes a refinement to insertmanyvalues where we consider return_defaults() with supplemental_cols to be the same as an explicit returning(), since that's the purpose of this. This saves us some extra exceptions that could be thrown per-dialect if implicit_returning were set to False in some cases. Fixed issue within some dialects where the dialect could incorrectly return an empty result set for an INSERT statement that does not actually return rows at all, due to artfacts from pre- or post-fetching the primary key of the row or rows still being present. Affected dialects included asyncpg, all mssql dialects. Fixes: #10453 Change-Id: Ie2e7e4f4cd9180558f9da315d21895347ec6d4f7 --- diff --git a/doc/build/changelog/unreleased_20/10453.rst b/doc/build/changelog/unreleased_20/10453.rst new file mode 100644 index 0000000000..93ed84a241 --- /dev/null +++ b/doc/build/changelog/unreleased_20/10453.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, orm + :tickets: 10453 + + Fixed regression in recently revised "insertmanyvalues" feature (likely + issue :ticket:`9618`) where the ORM would inadvertently attempt to + interpret a non-RETURNING result as one with RETURNING, in the case where + the ``implicit_returning=False`` parameter were applied to the mapped + :class:`.Table`, indicating that "insertmanyvalues" cannot be used if the + primary key values are not provided. + +.. change:: + :tags: bug, engine + + Fixed issue within some dialects where the dialect could incorrectly return + an empty result set for an INSERT statement that does not actually return + rows at all, due to artfacts from pre- or post-fetching the primary key of + the row or rows still being present. Affected dialects included asyncpg, + all mssql dialects. diff --git a/lib/sqlalchemy/dialects/mssql/base.py b/lib/sqlalchemy/dialects/mssql/base.py index 6d46687e43..f23d05fcf5 100644 --- a/lib/sqlalchemy/dialects/mssql/base.py +++ b/lib/sqlalchemy/dialects/mssql/base.py @@ -1942,6 +1942,7 @@ class MSExecutionContext(default.DefaultExecutionContext): row = self.cursor.fetchall()[0] self._lastrowid = int(row[0]) + self.cursor_fetch_strategy = _cursor._NO_CURSOR_DML elif ( self.compiled is not None and is_sql_compiler(self.compiled) diff --git a/lib/sqlalchemy/dialects/mssql/pyodbc.py b/lib/sqlalchemy/dialects/mssql/pyodbc.py index 49d203ee6e..b711998e29 100644 --- a/lib/sqlalchemy/dialects/mssql/pyodbc.py +++ b/lib/sqlalchemy/dialects/mssql/pyodbc.py @@ -365,6 +365,7 @@ from ... import exc from ... import types as sqltypes from ... import util from ...connectors.pyodbc import PyODBCConnector +from ...engine import cursor as _cursor class _ms_numeric_pyodbc: @@ -593,6 +594,8 @@ class MSExecutionContext_pyodbc(MSExecutionContext): self.cursor.nextset() self._lastrowid = int(row[0]) + + self.cursor_fetch_strategy = _cursor._NO_CURSOR_DML else: super().post_exec() diff --git a/lib/sqlalchemy/dialects/postgresql/asyncpg.py b/lib/sqlalchemy/dialects/postgresql/asyncpg.py index 1554879a5c..4fa188cb02 100644 --- a/lib/sqlalchemy/dialects/postgresql/asyncpg.py +++ b/lib/sqlalchemy/dialects/postgresql/asyncpg.py @@ -566,6 +566,7 @@ class AsyncAdapt_asyncpg_cursor: async def _executemany(self, operation, seq_of_parameters): adapt_connection = self._adapt_connection + self.description = None async with adapt_connection._execute_mutex: await adapt_connection._check_type_cache_invalidation( self._invalidate_schema_cache_asof diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py index e9f1efbe63..553d8f0bea 100644 --- a/lib/sqlalchemy/engine/default.py +++ b/lib/sqlalchemy/engine/default.py @@ -71,6 +71,7 @@ if typing.TYPE_CHECKING: from types import ModuleType from .base import Engine + from .cursor import ResultFetchStrategy from .interfaces import _CoreMultiExecuteParams from .interfaces import _CoreSingleExecuteParams from .interfaces import _DBAPICursorDescription @@ -1853,7 +1854,7 @@ class DefaultExecutionContext(ExecutionContext): def _setup_dml_or_text_result(self): compiled = cast(SQLCompiler, self.compiled) - strategy = self.cursor_fetch_strategy + strategy: ResultFetchStrategy = self.cursor_fetch_strategy if self.isinsert: if ( @@ -1882,9 +1883,15 @@ class DefaultExecutionContext(ExecutionContext): strategy = _cursor.BufferedRowCursorFetchStrategy( self.cursor, self.execution_options ) - cursor_description = ( - strategy.alternate_cursor_description or self.cursor.description - ) + + if strategy is _cursor._NO_CURSOR_DML: + cursor_description = None + else: + cursor_description = ( + strategy.alternate_cursor_description + or self.cursor.description + ) + if cursor_description is None: strategy = _cursor._NO_CURSOR_DML elif self._num_sentinel_cols: diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index 31fbdf3438..6729b479f9 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -1082,7 +1082,7 @@ def _emit_insert_statements( records = list(records) if returning_is_required_anyway or ( - not hasvalue and len(records) > 1 + table.implicit_returning and not hasvalue and len(records) > 1 ): if ( deterministic_results_reqd diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py index 298c50ec0f..e51403eced 100644 --- a/lib/sqlalchemy/sql/crud.py +++ b/lib/sqlalchemy/sql/crud.py @@ -1578,7 +1578,11 @@ def _get_returning_modifiers(compiler, stmt, compile_state, toplevel): should_implicit_return_defaults = ( implicit_returning and stmt._return_defaults ) - explicit_returning = should_implicit_return_defaults or stmt._returning + explicit_returning = ( + should_implicit_return_defaults + or stmt._returning + or stmt._supplemental_returning + ) use_insertmanyvalues = ( toplevel and compiler.for_executemany diff --git a/lib/sqlalchemy/sql/dml.py b/lib/sqlalchemy/sql/dml.py index 921aed3f9e..4ca6ed338f 100644 --- a/lib/sqlalchemy/sql/dml.py +++ b/lib/sqlalchemy/sql/dml.py @@ -567,7 +567,8 @@ class UpdateBase( 3. :meth:`.UpdateBase.return_defaults` can be called against any backend. Backends that don't support RETURNING will skip the usage - of the feature, rather than raising an exception. The return value + of the feature, rather than raising an exception, *unless* + ``supplemental_cols`` is passed. The return value of :attr:`_engine.CursorResult.returned_defaults` will be ``None`` for backends that don't support RETURNING or for which the target :class:`.Table` sets :paramref:`.Table.implicit_returning` to diff --git a/lib/sqlalchemy/testing/suite/test_insert.py b/lib/sqlalchemy/testing/suite/test_insert.py index a893e30334..0faf591da4 100644 --- a/lib/sqlalchemy/testing/suite/test_insert.py +++ b/lib/sqlalchemy/testing/suite/test_insert.py @@ -104,6 +104,15 @@ class InsertBehaviorTest(fixtures.TablesTest): Column("id", Integer, primary_key=True, autoincrement=False), Column("data", String(50)), ) + Table( + "no_implicit_returning", + metadata, + Column( + "id", Integer, primary_key=True, test_needs_autoincrement=True + ), + Column("data", String(50)), + implicit_returning=False, + ) Table( "includes_defaults", metadata, @@ -119,6 +128,33 @@ class InsertBehaviorTest(fixtures.TablesTest): ), ) + @testing.variation("style", ["plain", "return_defaults"]) + @testing.variation("executemany", [True, False]) + def test_no_results_for_non_returning_insert( + self, connection, style, executemany + ): + """test another INSERT issue found during #10453""" + + table = self.tables.no_implicit_returning + + stmt = table.insert() + if style.return_defaults: + stmt = stmt.return_defaults() + + if executemany: + data = [ + {"data": "d1"}, + {"data": "d2"}, + {"data": "d3"}, + {"data": "d4"}, + {"data": "d5"}, + ] + else: + data = {"data": "d1"} + + r = connection.execute(stmt, data) + assert not r.returns_rows + @requirements.autoincrement_insert def test_autoclose_on_insert(self, connection): r = connection.execute( diff --git a/test/orm/dml/test_bulk_statements.py b/test/orm/dml/test_bulk_statements.py index d9c91a5707..7246a9749f 100644 --- a/test/orm/dml/test_bulk_statements.py +++ b/test/orm/dml/test_bulk_statements.py @@ -48,18 +48,21 @@ class InsertStmtTest(testing.AssertsExecutionResults, fixtures.TestBase): @testing.variation( "style", [ + ("default", testing.requires.insert_returning), "no_executemany", ("no_sort_by", testing.requires.insert_returning), ("all_enabled", testing.requires.insert_returning), ], ) @testing.variation("sort_by_parameter_order", [True, False]) + @testing.variation("enable_implicit_returning", [True, False]) def test_no_returning_error( self, decl_base, testing_engine, style: testing.Variation, sort_by_parameter_order, + enable_implicit_returning, ): class A(ComparableEntity, decl_base): __tablename__ = "a" @@ -67,22 +70,30 @@ class InsertStmtTest(testing.AssertsExecutionResults, fixtures.TestBase): data: Mapped[str] x: Mapped[Optional[int]] = mapped_column("xcol") + if not enable_implicit_returning: + __table_args__ = {"implicit_returning": False} + engine = testing_engine() - if style.no_executemany: + if style.default: + pass + elif style.no_executemany: engine.dialect.use_insertmanyvalues = False + engine.dialect.use_insertmanyvalues_wo_returning = False engine.dialect.insert_executemany_returning = False engine.dialect.insert_executemany_returning_sort_by_parameter_order = ( # noqa: E501 False ) elif style.no_sort_by: engine.dialect.use_insertmanyvalues = True + engine.dialect.use_insertmanyvalues_wo_returning = True engine.dialect.insert_executemany_returning = True engine.dialect.insert_executemany_returning_sort_by_parameter_order = ( # noqa: E501 False ) elif style.all_enabled: engine.dialect.use_insertmanyvalues = True + engine.dialect.use_insertmanyvalues_wo_returning = True engine.dialect.insert_executemany_returning = True engine.dialect.insert_executemany_returning_sort_by_parameter_order = ( # noqa: E501 True @@ -93,8 +104,10 @@ class InsertStmtTest(testing.AssertsExecutionResults, fixtures.TestBase): decl_base.metadata.create_all(engine) s = Session(engine) - if style.all_enabled or ( - style.no_sort_by and not sort_by_parameter_order + if ( + style.all_enabled + or (style.no_sort_by and not sort_by_parameter_order) + or style.default ): result = s.scalars( insert(A).returning( diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py index 5ca34d9174..1a5b697b8e 100644 --- a/test/orm/test_unitofworkv2.py +++ b/test/orm/test_unitofworkv2.py @@ -3160,7 +3160,9 @@ class EagerDefaultsSettingTest( t = Table( "test", metadata, - Column("id", Integer, primary_key=True), + Column( + "id", Integer, primary_key=True, test_needs_autoincrement=True + ), Column( "foo", Integer, @@ -3284,6 +3286,126 @@ class EagerDefaultsSettingTest( ) ) + def test_eager_default_setting_inserts_no_pks( + self, + setup_mappers, + eager_defaults_variations, + implicit_returning_variations, + connection, + ): + """test for #10453. + + This is essentially a variation from test_eager_default_setting, + as a separate test because there are too many new conditions by + introducing this variant. + + """ + Thing = setup_mappers + s = Session(connection) + + t1, t2 = (Thing(bar=6), Thing(bar=6)) + + s.add_all([t1, t2]) + + expected_eager_defaults = eager_defaults_variations.eager_defaults or ( + ( + eager_defaults_variations.auto + or eager_defaults_variations.unspecified + ) + and connection.dialect.insert_executemany_returning + and bool(implicit_returning_variations) + ) + expect_returning = connection.dialect.insert_returning and bool( + implicit_returning_variations + ) + + with self.sql_execution_asserter(connection) as asserter: + s.flush() + + asserter.assert_( + Conditional( + expect_returning, + [ + Conditional( + connection.dialect.insert_executemany_returning, + [ + Conditional( + expected_eager_defaults, + [ + CompiledSQL( + "INSERT INTO test (bar) " + "VALUES (:bar) " + "RETURNING test.id, test.foo", + [ + {"bar": 6}, + {"bar": 6}, + ], + ) + ], + [ + CompiledSQL( + "INSERT INTO test (bar) " + "VALUES (:bar) " + "RETURNING test.id", + [ + {"bar": 6}, + {"bar": 6}, + ], + ) + ], + ) + ], + [ + CompiledSQL( + "INSERT INTO test (bar) " + "VALUES (:bar) " + "RETURNING test.id, test.foo", + {"bar": 6}, + ), + CompiledSQL( + "INSERT INTO test (bar) " + "VALUES (:bar) " + "RETURNING test.id, test.foo", + {"bar": 6}, + ), + ], + ), + ], + [ + CompiledSQL( + "INSERT INTO test (bar) VALUES (:bar)", + [ + {"bar": 6}, + ], + enable_returning=False, + ), + CompiledSQL( + "INSERT INTO test (bar) VALUES (:bar)", + [ + {"bar": 6}, + ], + enable_returning=False, + ), + Conditional( + expected_eager_defaults and not expect_returning, + [ + CompiledSQL( + "SELECT test.foo AS test_foo " + "FROM test WHERE test.id = :pk_1", + [{"pk_1": 1}], + ), + CompiledSQL( + "SELECT test.foo AS test_foo " + "FROM test WHERE test.id = :pk_1", + [{"pk_1": 2}], + ), + ], + [], + ), + ], + ) + ) + def test_eager_default_setting_updates( self, setup_mappers, diff --git a/test/sql/test_returning.py b/test/sql/test_returning.py index 7d40fa76f3..4d55c435db 100644 --- a/test/sql/test_returning.py +++ b/test/sql/test_returning.py @@ -1295,6 +1295,16 @@ class InsertManyReturningTest(fixtures.TablesTest): Column("strval", String(50)), ) + Table( + "no_implicit_returning", + metadata, + Column( + "id", Integer, primary_key=True, test_needs_autoincrement=True + ), + Column("data", String(30)), + implicit_returning=False, + ) + @testing.combinations( ( lambda table: (table.c.strval + "hi",), @@ -1416,6 +1426,61 @@ class InsertManyReturningTest(fixtures.TablesTest): ], ) + @testing.variation( + "style", + ["no_cols", "cols", "cols_plus_supplemental", "normal_returning"], + ) + def test_no_executemany_w_no_implicit_returning(self, connection, style): + """test a refinement made during fixes for #10453; + return_defaults() with 'supplemental_cols' is considered to be an + explicit returning case, bypassing the implicit_returning parameter. + + """ + t1 = self.tables.no_implicit_returning + + if style.cols_plus_supplemental: + result = connection.execute( + t1.insert().return_defaults( + t1.c.id, supplemental_cols=[t1.c.data] + ), + [ + {"data": "d1"}, + {"data": "d2"}, + {"data": "d3"}, + ], + ) + eq_(result.scalars().all(), ["d1", "d2", "d3"]) + elif style.normal_returning: + result = connection.execute( + t1.insert().returning(t1.c.data), + [ + {"data": "d1"}, + {"data": "d2"}, + {"data": "d3"}, + ], + ) + eq_(result.scalars().all(), ["d1", "d2", "d3"]) + elif style.cols: + result = connection.execute( + t1.insert().return_defaults(t1.c.id), + [ + {"data": "d1"}, + {"data": "d2"}, + {"data": "d3"}, + ], + ) + assert not result.returns_rows + elif style.no_cols: + result = connection.execute( + t1.insert().return_defaults(t1.c.id), + [ + {"data": "d1"}, + {"data": "d2"}, + {"data": "d3"}, + ], + ) + assert not result.returns_rows + def test_insert_executemany_type_test(self, connection): t1 = self.tables.type_cases result = connection.execute(