From: Mike Bayer Date: Fri, 25 Aug 2023 16:12:06 +0000 (-0400) Subject: add explicit support for aliased ORM models in UPDATE/DELETE X-Git-Tag: rel_2_0_21~30 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d18ccdc997185b74;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git add explicit support for aliased ORM models in UPDATE/DELETE Adjusted the ORM's interpretation of UPDATE/DELETE targets to not interfere with the target table passed to the statement, such as for :class:`_orm.aliased` constructs. Cases like ORM session synchonize using "SELECT" statements such as with MySQL/ MariaDB will still have issues with UPDATE/DELETE of this form so it's best to disable synchonize_session when using DML statements of this type. A separate issue to identify RETURNING should be used for ORM UPDATE of an aliased() with fetch strategy and that these columns should come back was attempted here, but is failing tests and is beyond the scope of the immediate issue. also updates the mssql URL in config to suit current preferences :) Change-Id: If1609ad500abb10d430717786142fb430d1c9265 --- diff --git a/doc/build/changelog/unreleased_20/10279.rst b/doc/build/changelog/unreleased_20/10279.rst new file mode 100644 index 0000000000..4a851cbdd2 --- /dev/null +++ b/doc/build/changelog/unreleased_20/10279.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 10279 + + Adjusted the ORM's interpretation of UPDATE/DELETE targets to not interfere + with the target table passed to the statement, such as for + :class:`_orm.aliased` constructs. Cases like ORM session synchonize using + "SELECT" statements such as with MySQL/ MariaDB will still have issues with + UPDATE/DELETE of this form so it's best to disable synchonize_session when + using DML statements of this type. diff --git a/lib/sqlalchemy/orm/bulk_persistence.py b/lib/sqlalchemy/orm/bulk_persistence.py index 6b35c4a50d..5f9db51b56 100644 --- a/lib/sqlalchemy/orm/bulk_persistence.py +++ b/lib/sqlalchemy/orm/bulk_persistence.py @@ -1438,7 +1438,6 @@ class BulkORMUpdate(BulkUDCompileState, UpdateDMLState): self._resolved_values = dict(self._resolved_values) new_stmt = statement._clone() - new_stmt.table = mapper.local_table # note if the statement has _multi_values, these # are passed through to the new statement, which will then raise @@ -1499,9 +1498,7 @@ class BulkORMUpdate(BulkUDCompileState, UpdateDMLState): # over and over again. so perhaps if it could be RETURNING just # the elements that were based on a SQL expression and not # a constant. For now it doesn't quite seem worth it - new_stmt = new_stmt.return_defaults( - *(list(mapper.local_table.primary_key)) - ) + new_stmt = new_stmt.return_defaults(*new_stmt.table.primary_key) if toplevel: new_stmt = self._setup_orm_returning( @@ -1860,7 +1857,6 @@ class BulkORMDelete(BulkUDCompileState, DeleteDMLState): ) new_stmt = statement._clone() - new_stmt.table = mapper.local_table new_crit = cls._adjust_for_extra_criteria( self.global_attributes, mapper diff --git a/lib/sqlalchemy/sql/crud.py b/lib/sqlalchemy/sql/crud.py index 4191069fb4..544f6771a2 100644 --- a/lib/sqlalchemy/sql/crud.py +++ b/lib/sqlalchemy/sql/crud.py @@ -647,6 +647,9 @@ def _scan_cols( compiler_implicit_returning = compiler.implicit_returning + # TODO - see TODO(return_defaults_columns) below + # cols_in_params = set() + for c in cols: # scan through every column in the target table @@ -672,6 +675,9 @@ def _scan_cols( kw, ) + # TODO - see TODO(return_defaults_columns) below + # cols_in_params.add(c) + elif isinsert: # no parameter is present and it's an insert. @@ -764,6 +770,19 @@ def _scan_cols( if c in remaining_supplemental ) + # TODO(return_defaults_columns): there can still be more columns in + # _return_defaults_columns in the case that they are from something like an + # aliased of the table. we can add them here, however this breaks other ORM + # things. so this is for another day. see + # test/orm/dml/test_update_delete_where.py -> test_update_from_alias + + # if stmt._return_defaults_columns: + # compiler_implicit_returning.extend( + # set(stmt._return_defaults_columns) + # .difference(compiler_implicit_returning) + # .difference(cols_in_params) + # ) + return (use_insertmanyvalues, use_sentinel_columns) diff --git a/setup.cfg b/setup.cfg index d45fb5f6c9..5c68605d08 100644 --- a/setup.cfg +++ b/setup.cfg @@ -168,7 +168,7 @@ asyncmy = mysql+asyncmy://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4 asyncmy_fallback = mysql+asyncmy://scott:tiger@127.0.0.1:3306/test?charset=utf8mb4&async_fallback=true mariadb = mariadb+mysqldb://scott:tiger@127.0.0.1:3306/test mariadb_connector = mariadb+mariadbconnector://scott:tiger@127.0.0.1:3306/test -mssql = mssql+pyodbc://scott:tiger^5HHH@mssql2017:1433/test?driver=ODBC+Driver+13+for+SQL+Server +mssql = mssql+pyodbc://scott:tiger^5HHH@mssql2017:1433/test?driver=ODBC+Driver+18+for+SQL+Server&TrustServerCertificate=yes pymssql = mssql+pymssql://scott:tiger^5HHH@mssql2017:1433/test docker_mssql = mssql+pyodbc://scott:tiger^5HHH@127.0.0.1:1433/test?driver=ODBC+Driver+17+for+SQL+Server oracle = oracle+cx_oracle://scott:tiger@oracle18c/xe diff --git a/test/orm/dml/test_update_delete_where.py b/test/orm/dml/test_update_delete_where.py index 89d1e5c7fb..9efb82a1dd 100644 --- a/test/orm/dml/test_update_delete_where.py +++ b/test/orm/dml/test_update_delete_where.py @@ -19,6 +19,7 @@ from sqlalchemy import testing from sqlalchemy import text from sqlalchemy import update from sqlalchemy import values +from sqlalchemy.orm import aliased from sqlalchemy.orm import backref from sqlalchemy.orm import exc as orm_exc from sqlalchemy.orm import immediateload @@ -831,6 +832,7 @@ class UpdateDeleteTest(fixtures.MappedTest): sess = fixture_session() john, jack, jill, jane = sess.query(User).order_by(User.id).all() + sess.query(User).filter(User.age > 29).update( {"age": User.age - 10}, synchronize_session="evaluate" ) @@ -2279,6 +2281,84 @@ class UpdateDeleteFromTest(fixtures.MappedTest): properties={"user": relationship(User, backref="documents")}, ) + @testing.requires.update_from_using_alias + @testing.combinations( + False, + ("fetch", testing.requires.update_returning), + ("auto", testing.requires.update_returning), + argnames="synchronize_session", + ) + def test_update_from_alias(self, synchronize_session): + Document = self.classes.Document + s = fixture_session() + + d1 = aliased(Document) + + with self.sql_execution_asserter() as asserter: + s.execute( + update(d1).where(d1.title == "baz").values(flag=True), + execution_options={"synchronize_session": synchronize_session}, + ) + + if True: + # TODO: see note in crud.py line 770. RETURNING should be here + # if synchronize_session="fetch" however there are more issues + # with this. + # if synchronize_session is False: + asserter.assert_( + CompiledSQL( + "UPDATE documents AS documents_1 SET flag=:flag " + "WHERE documents_1.title = :title_1", + [{"flag": True, "title_1": "baz"}], + ) + ) + else: + asserter.assert_( + CompiledSQL( + "UPDATE documents AS documents_1 SET flag=:flag " + "WHERE documents_1.title = :title_1 " + "RETURNING documents_1.id", + [{"flag": True, "title_1": "baz"}], + ) + ) + + @testing.requires.delete_using_alias + @testing.combinations( + False, + ("fetch", testing.requires.delete_returning), + ("auto", testing.requires.delete_returning), + argnames="synchronize_session", + ) + def test_delete_using_alias(self, synchronize_session): + Document = self.classes.Document + s = fixture_session() + + d1 = aliased(Document) + + with self.sql_execution_asserter() as asserter: + s.execute( + delete(d1).where(d1.title == "baz"), + execution_options={"synchronize_session": synchronize_session}, + ) + + if synchronize_session is False: + asserter.assert_( + CompiledSQL( + "DELETE FROM documents AS documents_1 " + "WHERE documents_1.title = :title_1", + [{"title_1": "baz"}], + ) + ) + else: + asserter.assert_( + CompiledSQL( + "DELETE FROM documents AS documents_1 " + "WHERE documents_1.title = :title_1 " + "RETURNING documents_1.id", + [{"title_1": "baz"}], + ) + ) + @testing.requires.update_from def test_update_from_joined_subq_test(self): Document = self.classes.Document diff --git a/test/orm/test_core_compilation.py b/test/orm/test_core_compilation.py index 06482562b9..dd0d597b22 100644 --- a/test/orm/test_core_compilation.py +++ b/test/orm/test_core_compilation.py @@ -487,7 +487,7 @@ class PropagateAttrsTest(QueryTest): class DMLTest(QueryTest, AssertsCompiledSQL): - __dialect__ = "default" + __dialect__ = "default_enhanced" @testing.variation("stmt_type", ["update", "delete"]) def test_dml_ctes(self, stmt_type: testing.Variation): @@ -519,6 +519,45 @@ class DMLTest(QueryTest, AssertsCompiledSQL): else: stmt_type.fail() + @testing.variation("stmt_type", ["core", "orm"]) + def test_aliased_update(self, stmt_type: testing.Variation): + """test #10279""" + if stmt_type.orm: + User = self.classes.User + u1 = aliased(User) + stmt = update(u1).where(u1.name == "xyz").values(name="newname") + elif stmt_type.core: + user_table = self.tables.users + u1 = user_table.alias() + stmt = update(u1).where(u1.c.name == "xyz").values(name="newname") + else: + stmt_type.fail() + + self.assert_compile( + stmt, + "UPDATE users AS users_1 SET name=:name " + "WHERE users_1.name = :name_1", + ) + + @testing.variation("stmt_type", ["core", "orm"]) + def test_aliased_delete(self, stmt_type: testing.Variation): + """test #10279""" + if stmt_type.orm: + User = self.classes.User + u1 = aliased(User) + stmt = delete(u1).where(u1.name == "xyz") + elif stmt_type.core: + user_table = self.tables.users + u1 = user_table.alias() + stmt = delete(u1).where(u1.c.name == "xyz") + else: + stmt_type.fail() + + self.assert_compile( + stmt, + "DELETE FROM users AS users_1 " "WHERE users_1.name = :name_1", + ) + @testing.variation("stmt_type", ["core", "orm"]) def test_add_cte(self, stmt_type: testing.Variation): """test #10167""" diff --git a/test/requirements.py b/test/requirements.py index e0941da1b9..95083aed0b 100644 --- a/test/requirements.py +++ b/test/requirements.py @@ -491,6 +491,15 @@ class DefaultRequirements(SuiteRequirements): "Backend does not support UPDATE..FROM", ) + @property + def update_from_using_alias(self): + """Target must support UPDATE..FROM syntax against an alias""" + + return skip_if( + ["oracle", "sqlite<3.33.0", "mssql"], + "Backend does not support UPDATE..FROM with an alias", + ) + @property def delete_using(self): """Target must support DELETE FROM..FROM or DELETE..USING syntax""" @@ -499,6 +508,14 @@ class DefaultRequirements(SuiteRequirements): "Backend does not support DELETE..USING or equivalent", ) + @property + def delete_using_alias(self): + """Target must support DELETE FROM against an alias""" + return only_on( + ["postgresql", "sqlite"], + "Backend does not support DELETE..USING/FROM with an alias", + ) + @property def update_where_target_in_subquery(self): """Target must support UPDATE (or DELETE) where the same table is