From abeea1d82db34232bbef01e98fa4d1de0f583eb6 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sat, 4 Aug 2018 13:45:07 -0400 Subject: [PATCH] Include UPDATE/DELETE extra_froms in correlation Fixed bug where the multi-table support for UPDATE and DELETE statements did not consider the additional FROM elements as targets for correlation, when a correlated SELECT were also combined with the statement. This change now includes that a SELECT statement in the WHERE clause for such a statement will try to auto-correlate back to these additional tables in the parent UPDATE/DELETE or unconditionally correlate if :meth:`.Select.correlate` is used. Note that auto-correlation raises an error if the SELECT statement would have no FROM clauses as a result, which can now occur if the parent UPDATE/DELETE specifies the same tables in its additional set of tables ; specify :meth:`.Select.correlate` explicitly to resolve. Change-Id: Ie11eaad7e49af3f59df11691b104d6359341bdae Fixes: #4313 --- doc/build/changelog/unreleased_12/4313.rst | 15 +++++ lib/sqlalchemy/sql/compiler.py | 27 ++++---- test/sql/test_delete.py | 76 +++++++++++++++++++++- test/sql/test_update.py | 58 ++++++++++++++++- 4 files changed, 161 insertions(+), 15 deletions(-) create mode 100644 doc/build/changelog/unreleased_12/4313.rst diff --git a/doc/build/changelog/unreleased_12/4313.rst b/doc/build/changelog/unreleased_12/4313.rst new file mode 100644 index 0000000000..9817420fa2 --- /dev/null +++ b/doc/build/changelog/unreleased_12/4313.rst @@ -0,0 +1,15 @@ +.. change:: + :tags: bug, sql + :tickets: 4313 + + Fixed bug where the multi-table support for UPDATE and DELETE statements + did not consider the additional FROM elements as targets for correlation, + when a correlated SELECT were also combined with the statement. This + change now includes that a SELECT statement in the WHERE clause for such a + statement will try to auto-correlate back to these additional tables in the + parent UPDATE/DELETE or unconditionally correlate if + :meth:`.Select.correlate` is used. Note that auto-correlation raises an + error if the SELECT statement would have no FROM clauses as a result, which + can now occur if the parent UPDATE/DELETE specifies the same tables in its + additional set of tables; specify :meth:`.Select.correlate` explicitly to + resolve. diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index ae1dd2c7cb..529e28ba6d 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -2180,11 +2180,6 @@ class SQLCompiler(Compiled): def visit_update(self, update_stmt, asfrom=False, **kw): toplevel = not self.stack - self.stack.append( - {'correlate_froms': {update_stmt.table}, - "asfrom_froms": {update_stmt.table}, - "selectable": update_stmt}) - extra_froms = update_stmt._extra_froms is_multitable = bool(extra_froms) @@ -2194,8 +2189,15 @@ class SQLCompiler(Compiled): render_extra_froms = [ f for f in extra_froms if f not in main_froms ] + correlate_froms = main_froms.union(extra_froms) else: render_extra_froms = [] + correlate_froms = {update_stmt.table} + + self.stack.append( + {'correlate_froms': correlate_froms, + "asfrom_froms": correlate_froms, + "selectable": update_stmt}) text = "UPDATE " @@ -2289,14 +2291,15 @@ class SQLCompiler(Compiled): def visit_delete(self, delete_stmt, asfrom=False, **kw): toplevel = not self.stack - self.stack.append({'correlate_froms': {delete_stmt.table}, - "asfrom_froms": {delete_stmt.table}, - "selectable": delete_stmt}) - crud._setup_crud_params(self, delete_stmt, crud.ISDELETE, **kw) extra_froms = delete_stmt._extra_froms + correlate_froms = {delete_stmt.table}.union(extra_froms) + self.stack.append({'correlate_froms': correlate_froms, + "asfrom_froms": correlate_froms, + "selectable": delete_stmt}) + text = "DELETE " if delete_stmt._prefixes: @@ -2401,9 +2404,9 @@ class StrSQLCompiler(SQLCompiler): for t in extra_froms) def delete_extra_from_clause(self, update_stmt, - from_table, extra_froms, - from_hints, - **kw): + from_table, extra_froms, + from_hints, + **kw): return ', ' + ', '.join( t._compiler_dispatch(self, asfrom=True, fromhints=from_hints, **kw) diff --git a/test/sql/test_delete.py b/test/sql/test_delete.py index 7d18db9c9c..91f2c2cdc2 100644 --- a/test/sql/test_delete.py +++ b/test/sql/test_delete.py @@ -1,10 +1,13 @@ #! coding:utf-8 from sqlalchemy import Integer, String, ForeignKey, delete, select, and_, \ - or_ + or_, exists from sqlalchemy.dialects import mysql +from sqlalchemy.engine import default from sqlalchemy import testing -from sqlalchemy.testing import AssertsCompiledSQL, fixtures, eq_ +from sqlalchemy import exc +from sqlalchemy.testing import AssertsCompiledSQL, fixtures, eq_, \ + assert_raises_message from sqlalchemy.testing.schema import Table, Column @@ -103,6 +106,75 @@ class DeleteTest(_DeleteTestBase, fixtures.TablesTest, AssertsCompiledSQL): ')') +class DeleteFromCompileTest( + _DeleteTestBase, fixtures.TablesTest, AssertsCompiledSQL): + # DELETE FROM is also tested by individual dialects since there is no + # consistent syntax. here we use the StrSQLcompiler which has a fake + # syntax. + + __dialect__ = 'default_enhanced' + + def test_delete_extra_froms(self): + table1, table2 = self.tables.mytable, self.tables.myothertable + + stmt = table1.delete().where(table1.c.myid == table2.c.otherid) + self.assert_compile( + stmt, + "DELETE FROM mytable , myothertable " + "WHERE mytable.myid = myothertable.otherid", + ) + + def test_correlation_to_extra(self): + table1, table2 = self.tables.mytable, self.tables.myothertable + + stmt = table1.delete().where( + table1.c.myid == table2.c.otherid).where( + ~exists().where(table2.c.otherid == table1.c.myid). + where(table2.c.othername == 'x').correlate(table2) + ) + + self.assert_compile( + stmt, + "DELETE FROM mytable , myothertable WHERE mytable.myid = " + "myothertable.otherid AND NOT (EXISTS " + "(SELECT * FROM mytable WHERE myothertable.otherid = " + "mytable.myid AND myothertable.othername = :othername_1))", + ) + + def test_dont_correlate_to_extra(self): + table1, table2 = self.tables.mytable, self.tables.myothertable + + stmt = table1.delete().where( + table1.c.myid == table2.c.otherid).where( + ~exists().where(table2.c.otherid == table1.c.myid). + where(table2.c.othername == 'x').correlate() + ) + + self.assert_compile( + stmt, + "DELETE FROM mytable , myothertable WHERE mytable.myid = " + "myothertable.otherid AND NOT (EXISTS " + "(SELECT * FROM myothertable, mytable " + "WHERE myothertable.otherid = " + "mytable.myid AND myothertable.othername = :othername_1))", + ) + + def test_autocorrelate_error(self): + table1, table2 = self.tables.mytable, self.tables.myothertable + + stmt = table1.delete().where( + table1.c.myid == table2.c.otherid).where( + ~exists().where(table2.c.otherid == table1.c.myid). + where(table2.c.othername == 'x') + ) + + assert_raises_message( + exc.InvalidRequestError, + ".*returned no FROM clauses due to auto-correlation.*", + stmt.compile, dialect=default.StrCompileDialect() + ) + + class DeleteFromRoundTripTest(fixtures.TablesTest): __backend__ = True diff --git a/test/sql/test_update.py b/test/sql/test_update.py index cc5b4962bf..138581061b 100644 --- a/test/sql/test_update.py +++ b/test/sql/test_update.py @@ -1,5 +1,5 @@ from sqlalchemy import Integer, String, ForeignKey, and_, or_, func, \ - literal, update, table, bindparam, column, select, exc + literal, update, table, bindparam, column, select, exc, exists from sqlalchemy import testing from sqlalchemy.dialects import mysql from sqlalchemy.engine import default @@ -591,6 +591,62 @@ class UpdateFromCompileTest(_UpdateFromTestBase, fixtures.TablesTest, 'AND anon_1.email_address = :email_address_1', checkparams=checkparams) + def test_correlation_to_extra(self): + users, addresses = self.tables.users, self.tables.addresses + + stmt = users.update().values(name="newname").where( + users.c.id == addresses.c.user_id + ).where( + ~exists().where( + addresses.c.user_id == users.c.id + ).where(addresses.c.email_address == 'foo').correlate(addresses) + ) + + self.assert_compile( + stmt, + "UPDATE users SET name=:name FROM addresses WHERE " + "users.id = addresses.user_id AND NOT " + "(EXISTS (SELECT * FROM users WHERE addresses.user_id = users.id " + "AND addresses.email_address = :email_address_1))" + ) + + def test_dont_correlate_to_extra(self): + users, addresses = self.tables.users, self.tables.addresses + + stmt = users.update().values(name="newname").where( + users.c.id == addresses.c.user_id + ).where( + ~exists().where( + addresses.c.user_id == users.c.id + ).where(addresses.c.email_address == 'foo').correlate() + ) + + self.assert_compile( + stmt, + "UPDATE users SET name=:name FROM addresses WHERE " + "users.id = addresses.user_id AND NOT " + "(EXISTS (SELECT * FROM addresses, users " + "WHERE addresses.user_id = users.id " + "AND addresses.email_address = :email_address_1))" + ) + + def test_autocorrelate_error(self): + users, addresses = self.tables.users, self.tables.addresses + + stmt = users.update().values(name="newname").where( + users.c.id == addresses.c.user_id + ).where( + ~exists().where( + addresses.c.user_id == users.c.id + ).where(addresses.c.email_address == 'foo') + ) + + assert_raises_message( + exc.InvalidRequestError, + ".*returned no FROM clauses due to auto-correlation.*", + stmt.compile, dialect=default.StrCompileDialect() + ) + class UpdateFromRoundTripTest(_UpdateFromTestBase, fixtures.TablesTest): __backend__ = True -- 2.47.2