From: Mike Bayer Date: Fri, 21 Apr 2023 13:48:04 +0000 (-0400) Subject: try to omit unnecessary cols for ORM bulk insert + returning X-Git-Tag: rel_2_0_10~5^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=63f51491c5f0cb22883c800a065d7c4b4c54774e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git try to omit unnecessary cols for ORM bulk insert + returning Fixed bug in ORM bulk insert feature where additional unnecessary columns would be rendered in the INSERT statement if RETURNING of individual columns were requested. Fixes: #9685 Change-Id: Ibf5f06ab017215c7c9bd8850c3a006f73fe78c68 --- diff --git a/doc/build/changelog/unreleased_20/9685.rst b/doc/build/changelog/unreleased_20/9685.rst new file mode 100644 index 0000000000..7100a96c66 --- /dev/null +++ b/doc/build/changelog/unreleased_20/9685.rst @@ -0,0 +1,7 @@ +.. change:: + :tags: bug, orm + :tickets: 9685 + + Fixed bug in ORM bulk insert feature where additional unnecessary columns + would be rendered in the INSERT statement if RETURNING of individual columns + were requested. diff --git a/lib/sqlalchemy/orm/bulk_persistence.py b/lib/sqlalchemy/orm/bulk_persistence.py index f9d9d6a433..c096dc3e5d 100644 --- a/lib/sqlalchemy/orm/bulk_persistence.py +++ b/lib/sqlalchemy/orm/bulk_persistence.py @@ -464,8 +464,9 @@ class ORMDMLState(AbstractORMCompileState): compiler, orm_level_statement, dml_level_statement, + dml_mapper, + *, use_supplemental_cols=True, - dml_mapper=None, ): """establish ORM column handlers for an INSERT, UPDATE, or DELETE which uses explicit returning(). @@ -504,7 +505,17 @@ class ORMDMLState(AbstractORMCompileState): if use_supplemental_cols: dml_level_statement = dml_level_statement.return_defaults( - supplemental_cols=cols_to_return + # this is a little weird looking, but by passing + # primary key as the main list of cols, this tells + # return_defaults to omit server-default cols. Since + # we have cols_to_return, just return what we asked for + # (plus primary key, which ORM persistence needs since + # we likely set bookkeeping=True here, which is another + # whole thing...). We dont want to clutter the + # statement up with lots of other cols the user didn't + # ask for. see #9685 + *dml_mapper.primary_key, + supplemental_cols=cols_to_return, ) else: dml_level_statement = dml_level_statement.returning( @@ -1280,6 +1291,7 @@ class BulkORMInsert(ORMDMLState, InsertDMLState): compiler, orm_level_statement, statement, + dml_mapper=mapper, use_supplemental_cols=False, ) self.statement = statement @@ -1314,8 +1326,8 @@ class BulkORMInsert(ORMDMLState, InsertDMLState): compiler, orm_level_statement, statement, - use_supplemental_cols=True, dml_mapper=emit_insert_mapper, + use_supplemental_cols=True, ) self.statement = statement @@ -1425,6 +1437,7 @@ class BulkORMUpdate(BulkUDCompileState, UpdateDMLState): compiler, orm_level_statement, new_stmt, + dml_mapper=mapper, use_supplemental_cols=use_supplemental_cols, ) @@ -1795,6 +1808,7 @@ class BulkORMDelete(BulkUDCompileState, DeleteDMLState): compiler, orm_level_statement, new_stmt, + dml_mapper=mapper, use_supplemental_cols=use_supplemental_cols, ) diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index a331d4ed85..a12156eb52 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -1076,12 +1076,16 @@ def _emit_insert_statements( else: do_executemany = False - if not has_all_defaults and base_mapper._prefer_eager_defaults( - connection.dialect, table - ): - statement = statement.return_defaults( - *mapper._server_default_cols[table] - ) + if use_orm_insert_stmt is None: + if ( + not has_all_defaults + and base_mapper._prefer_eager_defaults( + connection.dialect, table + ) + ): + statement = statement.return_defaults( + *mapper._server_default_cols[table] + ) if mapper.version_id_col is not None: statement = statement.return_defaults(mapper.version_id_col) diff --git a/test/orm/dml/test_bulk_statements.py b/test/orm/dml/test_bulk_statements.py index b4628af670..7a9f3324f8 100644 --- a/test/orm/dml/test_bulk_statements.py +++ b/test/orm/dml/test_bulk_statements.py @@ -33,7 +33,7 @@ from sqlalchemy.testing.entities import ComparableEntity from sqlalchemy.testing.fixtures import fixture_session -class InsertStmtTest(fixtures.TestBase): +class InsertStmtTest(testing.AssertsExecutionResults, fixtures.TestBase): def test_no_returning_error(self, decl_base): class A(fixtures.ComparableEntity, decl_base): __tablename__ = "a" @@ -89,6 +89,48 @@ class InsertStmtTest(fixtures.TestBase): [("d3", 5), ("d4", 6)], ) + @testing.requires.insert_returning + def test_insert_returning_cols_dont_give_me_defaults(self, decl_base): + """test #9685""" + + class User(decl_base): + __tablename__ = "users" + + id: Mapped[int] = mapped_column(Identity(), primary_key=True) + + name: Mapped[str] = mapped_column() + other_thing: Mapped[Optional[str]] + server_thing: Mapped[str] = mapped_column(server_default="thing") + + decl_base.metadata.create_all(testing.db) + insert_stmt = insert(User).returning(User.id) + + s = fixture_session() + + with self.sql_execution_asserter() as asserter: + result = s.execute( + insert_stmt, + [ + {"name": "some name 1"}, + {"name": "some name 2"}, + {"name": "some name 3"}, + ], + ) + + eq_(result.all(), [(1,), (2,), (3,)]) + + asserter.assert_( + CompiledSQL( + "INSERT INTO users (name) VALUES (:name) " + "RETURNING users.id", + [ + {"name": "some name 1"}, + {"name": "some name 2"}, + {"name": "some name 3"}, + ], + ), + ) + @testing.requires.insert_returning def test_insert_from_select_col_property(self, decl_base): """test #9273""" @@ -191,17 +233,12 @@ class BulkDMLReturningInhTest: with self.sql_execution_asserter() as asserter: result = s.execute(stmt, values) - if inspect(B).single: - single_inh = ", a.bd, a.zcol, a.q" - else: - single_inh = "" - if use_returning: asserter.assert_( CompiledSQL( "INSERT INTO a (type, data, xcol) VALUES " "(:type, :data, :xcol) " - f"RETURNING a.id, a.type, a.data, a.xcol, a.y{single_inh}", + "RETURNING a.id, a.type, a.data, a.xcol, a.y", [ {"type": "a", "data": "d3", "xcol": 5}, {"type": "a", "data": "d4", "xcol": 6}, @@ -209,13 +246,13 @@ class BulkDMLReturningInhTest: ), CompiledSQL( "INSERT INTO a (type, data) VALUES (:type, :data) " - f"RETURNING a.id, a.type, a.data, a.xcol, a.y{single_inh}", + "RETURNING a.id, a.type, a.data, a.xcol, a.y", [{"type": "a", "data": "d5"}], ), CompiledSQL( "INSERT INTO a (type, data, xcol, y) " "VALUES (:type, :data, :xcol, :y) " - f"RETURNING a.id, a.type, a.data, a.xcol, a.y{single_inh}", + "RETURNING a.id, a.type, a.data, a.xcol, a.y", [ {"type": "a", "data": "d6", "xcol": 8, "y": 9}, {"type": "a", "data": "d7", "xcol": 12, "y": 12}, @@ -224,7 +261,7 @@ class BulkDMLReturningInhTest: CompiledSQL( "INSERT INTO a (type, data, xcol) " "VALUES (:type, :data, :xcol) " - f"RETURNING a.id, a.type, a.data, a.xcol, a.y{single_inh}", + "RETURNING a.id, a.type, a.data, a.xcol, a.y", [{"type": "a", "data": "d8", "xcol": 7}], ), ) @@ -258,17 +295,18 @@ class BulkDMLReturningInhTest: ) if use_returning: - eq_( - result.scalars().all(), - [ - A(data="d3", id=mock.ANY, type="a", x=5, y=None), - A(data="d4", id=mock.ANY, type="a", x=6, y=None), - A(data="d5", id=mock.ANY, type="a", x=None, y=None), - A(data="d6", id=mock.ANY, type="a", x=8, y=9), - A(data="d7", id=mock.ANY, type="a", x=12, y=12), - A(data="d8", id=mock.ANY, type="a", x=7, y=None), - ], - ) + with self.assert_statement_count(testing.db, 0): + eq_( + result.scalars().all(), + [ + A(data="d3", id=mock.ANY, type="a", x=5, y=None), + A(data="d4", id=mock.ANY, type="a", x=6, y=None), + A(data="d5", id=mock.ANY, type="a", x=None, y=None), + A(data="d6", id=mock.ANY, type="a", x=8, y=9), + A(data="d7", id=mock.ANY, type="a", x=12, y=12), + A(data="d8", id=mock.ANY, type="a", x=7, y=None), + ], + ) @testing.combinations( "strings",