Merge lp:~therve/storm/delete-with-using into lp:storm

[Thomas Herve]

I think I'm happy enough with the branch. I added support to MySQL and Postgres, and it looks simple enough.

[lp:~therve/storm/delete-with-using]

359. By Thomas Herve on 2010-11-13

Fix tests naming

[James Henstridge (jamesh)]

I notice that you don't do a state.push/pop on auto_tables like compile_select() does. I guess it isn't as important since DELETE statements can't be nested (can they?), but it wouldn't hurt.

Also, the use of build_tables() and manually adding tables from state.auto_tables looks fishy. For compile_select() the automatically generated table list comes from build_tables, so I think there is a problem with how it is being used here.

I think the way it should work is:

 1. just run compile() on Delete.table to generate the "FROM xxx" clause.
 2. use build_tables(compile, Undef, Undef, state) to generate the "USING xxx" clause,
    plus some way to ensure Delete.table does not appear in this list.

Now on to the test changes: While we've used the "if cls_name.startswith(...):" pattern to disable tests for some backends, perhaps it would be cleaner to add a "supports_delete_using()" method that the backend test classes can override to say whether the test should run.

Lastly, it would be good to include a test for the case where the backend does not support "DELETE ... USING" to ensure we give a sensible error rather than generating nonsense SQL like we currently do.

review: Needs Fixing

Unmerged revisions

359. By Thomas Herve on 2010-11-13

Fix tests naming

358. By Thomas Herve on 2010-11-13

Cleanup

357. By Thomas Herve on 2010-11-13

Add one test with 3 tables

356. By Thomas Herve on 2010-11-13

Use localized compile, add support for mysql

355. By Thomas Herve on 2010-11-13

Merge forward

354. By Thomas Herve on 2009-12-27

Support USING in DELETE

=== modified file 'storm/databases/mysql.py'
--- storm/databases/mysql.py	2009-09-21 17:03:08 +0000
+++ storm/databases/mysql.py	2010-11-13 12:51:17 +0000
@@ -31,13 +31,12 @@
     MySQLdb = dummy
 
 from storm.expr import (
-    compile, Insert, Select, compile_select, Undef, And, Eq,
-    SQLRaw, SQLToken, is_safe_token)
+    compile, Insert, Select, Delete, compile_select, Undef, SQLToken,
+    is_safe_token, EXPR, build_tables, TABLE)
 from storm.variables import Variable
 from storm.database import Database, Connection, Result
 from storm.exceptions import (
     install_exceptions, DatabaseModuleError, OperationalError)
-from storm.variables import IntVariable
 
 
 install_exceptions(MySQLdb)
@@ -51,6 +50,31 @@
         select.limit = sys.maxint
     return compile_select(compile, select, state)
 
+
[e-mail](Delete)
+def compile_delete(compile, delete, state):
+    tokens = ["DELETE FROM "]
+    state.push("context", EXPR)
+    where = None
+    if delete.where is not Undef:
+        where = compile(delete.where, state, raw=True)
+    # Compile later for auto_tables support.
+    state.context = TABLE
+    tokens.append(build_tables(compile, delete.table,
+                               delete.default_table, state))
+    if state.auto_tables:
+        tables = set(
+            compile(table, state, token=True) for table in state.auto_tables)
+        if tables:
+            tokens.append(" USING ")
+            tokens.append(", ".join(tables))
+    if where is not None:
+        tokens.append(" WHERE ")
+        tokens.append(where)
+    state.pop()
+    return "".join(tokens)
+
+
 @compile.when(SQLToken)
 def compile_sql_token_mysql(compile, expr, state):
     """MySQL uses ` as the escape character by default."""
 
=== modified file 'storm/databases/postgres.py'
--- storm/databases/postgres.py	2010-10-23 21:02:22 +0000
+++ storm/databases/postgres.py	2010-11-13 12:51:17 +0000
@@ -39,10 +39,10 @@
     psycopg2 = dummy
 
 from storm.expr import (
-    Undef, Expr, SetExpr, Select, Insert, Alias, And, Eq, FuncExpr, SQLRaw,
-    Sequence, Like, SQLToken, COLUMN, COLUMN_NAME, COLUMN_PREFIX, TABLE,
-    compile, compile_select, compile_insert, compile_set_expr, compile_like,
-    compile_sql_token)
+    Undef, Expr, SetExpr, Select, Insert, Delete, Alias, And, Eq, FuncExpr,
+    SQLRaw, Sequence, Like, SQLToken, COLUMN, COLUMN_NAME, COLUMN_PREFIX,
+    TABLE, compile, compile_select, compile_insert, compile_set_expr,
+    compile_like, compile_sql_token, EXPR, build_tables)
 from storm.variables import Variable, ListVariable
 from storm.database import Database, Connection, Result
 from storm.exceptions import (
@@ -183,6 +183,32 @@
     return compile_insert(compile, insert, state)
 
 
[e-mail](Delete)
+def compile_delete(compile, delete, state):
+    tokens = ["DELETE FROM "]
+    state.push("context", EXPR)
+    where = None
+    if delete.where is not Undef:
+        where = compile(delete.where, state, raw=True)
+    # Compile later for auto_tables support.
+    state.context = TABLE
+    tokens.append(build_tables(compile, delete.table,
+                               delete.default_table, state))
+    if state.auto_tables:
+        tables = set(
+            compile(table, state, token=True) for table in state.auto_tables)
+        # Remove the target table from the USING clause
+        tables.discard(tokens[1])
+        if tables:
+            tokens.append(" USING ")
+            tokens.append(", ".join(tables))
+    if where is not None:
+        tokens.append(" WHERE ")
+        tokens.append(where)
+    state.pop()
+    return "".join(tokens)
+
+
 @compile.when(Sequence)
 def compile_sequence_postgres(compile, sequence, state):
     return "nextval('%s')" % sequence.name
 
=== modified file 'storm/expr.py'
--- storm/expr.py	2009-11-02 11:11:20 +0000
+++ storm/expr.py	2010-11-13 12:51:17 +0000
@@ -768,6 +768,7 @@
         self.table = table
         self.default_table = default_table
 
+
 @compile.when(Delete)
 def compile_delete(compile, delete, state):
     tokens = ["DELETE FROM ", None]
 
=== modified file 'tests/store/base.py'
--- tests/store/base.py	2010-10-19 13:36:26 +0000
+++ tests/store/base.py	2010-11-13 12:51:17 +0000
@@ -1153,6 +1153,33 @@
                           (30, "Title 10"),
                          ])
 
+    def test_find_remove_with_using(self):
+        """
+        Postgres and MySQL support C{USING} tables in C{DELETE}, allowing to
+        select more tables when removing rows.
+        """
+        cls_name = self.__class__.__name__
+        if cls_name.startswith("SQLite"):
+            return
+        result = self.store.find(
+            Bar, Bar.foo_id == Foo.id, Foo.title == u"Title 20")
+        result.remove()
+        self.assertEquals(sorted(self.store.find(Bar.id)), [100, 300])
+
+    def test_find_remove_with_using_multi_tables(self):
+        """
+        The C{USING} syntax supports several tables which can be used to build
+        check criteria.
+        """
+        cls_name = self.__class__.__name__
+        if cls_name.startswith("SQLite"):
+            return
+        result = self.store.find(
+            Bar, Bar.id == Link.bar_id, Link.foo_id == Foo.id, Foo.title ==
+            u"Title 10")
+        result.remove()
+        self.assertEquals(sorted(self.store.find(Bar.id)), [100, 200])
+
     def test_find_cached(self):
         foo = self.store.get(Foo, 20)
         bar = self.store.get(Bar, 200)