migrations.RunSQL's function signature implies it won't do any parameter substitution

[ris]

Bit of an odd one here, and probably comes down to a matter of opinion.

migrations.RunSQL not taking any params= argument seems to imply that it doesn't do any parameter substitution on the supplied SQL, which would mean that "%"s can be used freely in the SQL.

This of course isn't the case and doing

    migrations.RunSQL("UPDATE city_table SET description = 'silly' WHERE name ILIKE '%camelot%'")

will screw up because psycopg2 will be confused about the "%"s.

Either RunSQL should accept params= and this should be documented or RunSQL should attempt to nullify this by doing something like .replace ( "%" , "%%" ) to the SQL string to escape any.

[ris]

This bug seems awfully petty now that I've submitted it.

[Tim Graham]

Documentation seems appropriate.

[Markus Holtermann]

A patch is rather simple to get either support or not support for parameters. I would consider the way it is implemented right now to be a bug.

To allow using '%' as wildcards in SQL, the following works for me:

django/db/migrations/operations/special.py

@@ -66 +66 @@
     def database_forwards(self, app_label, schema_editor, from_state, to_state):
         statements = schema_editor.connection.ops.prepare_sql_script(self.sql)
         for statement in statements:
-            schema_editor.execute(statement)
+            schema_editor.execute(statement, params=None)
 
     def database_backwards(self, app_label, schema_editor, from_state, to_state):
         if self.reverse_sql is None:
             raise NotImplementedError("You cannot reverse this operation")
         statements = schema_editor.connection.ops.prepare_sql_script(self.reverse_sql)
         for statement in statements:
-            schema_editor.execute(statement)
+            schema_editor.execute(statement, params=None)
 
     def describe(self):
         return "Raw SQL operation"

Allowing params to be passed to RunSQL a few more changes need to be done:

django/db/migrations/operations/special.py

@@ -50 +50 @@
     by this SQL change, in case it's custom column/table creation/deletion.
     """
 
-    def __init__(self, sql, reverse_sql=None, state_operations=None):
+    def __init__(self, sql, reverse_sql=None, state_operations=None, params=None, reverse_params=None):
         self.sql = sql
         self.reverse_sql = reverse_sql
         self.state_operations = state_operations or []
+        self.params = params
+        self.reverse_params = reverse_params
 
     @property
     def reversible(self):
@@ -66 +68 @@
     def database_forwards(self, app_label, schema_editor, from_state, to_state):
         statements = schema_editor.connection.ops.prepare_sql_script(self.sql)
         for statement in statements:
-            schema_editor.execute(statement)
+            schema_editor.execute(statement, params=self.params)
 
     def database_backwards(self, app_label, schema_editor, from_state, to_state):
         if self.reverse_sql is None:
             raise NotImplementedError("You cannot reverse this operation")
         statements = schema_editor.connection.ops.prepare_sql_script(self.reverse_sql)
         for statement in statements:
-            schema_editor.execute(statement)
+            schema_editor.execute(statement, params=self.reverse_params)
 
     def describe(self):
         return "Raw SQL operation"

Which way should I go and should it be backported to 1.7?

[Claude Paroz]

#23541 was closed as a duplicate.

[Claude Paroz]

I think we should be consistent with how the related issue with cursor.execute has been fixed in Django 1.6. See how it's documented now: ​https://docs.djangoproject.com/en/1.7/topics/db/sql/#executing-custom-sql-directly
If we don't provide parameters, it should be possible to include single % chars without Django trying any extrapolation (e.g. passing None to execute, like the first example in the previous comment). We can then allow passing parameters to RunSQL and document it like we did for cursor.execute (% has to be doubled when and only when params are provided).
While the first fix should be backported to 1.7.1, the second could be delayed in 1.8 as a new functionality. Thoughts about that plan?

[Markus Holtermann]

It's fine with me. 1. patch for 1.7.x and second for 1.8

[Markus Holtermann]

A pull request for 1.7.1 is at ​https://github.com/django/django/pull/3266
A pull request for 1.8 is at ​https://github.com/django/django/pull/3267

[Tim Graham <timograham@…>]

In b9a670b22799a44fe7d3467d1d21949f9f717593:

Fixed #23426 -- Don't require double percent sign in RunSQL without parameters

[Tim Graham <timograham@…>]

In ae14c75014379d1cf405509b65eb986c0cb3b0f3:

[1.7.x] Fixed #23426 -- Don't require double percent sign in RunSQL without parameters

Backport of b9a670b227 from master

[Loic Bistuer]

Reopening for the 1.8 branch.

The implementation proposed in PR3267 won't play well with multiple statements (e.g. two INSERT statements separated by a semicolon).

Possible fix, make params and reverse_params a list whose length must match the number of SQL statements.

[Markus Holtermann]

After trying to implement the parameter mapping as initially suggested by Loic in multiple differnt ways, we came to the conclusion that every attempt we made had some weakness that worked contrary to what people would expect. Loic proposed passing parameters together with the SQL query in a 2-tuple which is what I implemented now. Here's how it works (the behavior for the sql and reverse_sql parameters are exactly the same):

---

If sql is a string, it is passed to schema_editor.connection.ops.prepare_sql_script() where a multi statement string is split into a list of single statements or a single statement is returned as a list with only itself.

If sql is a list/tuple (they behave the same here and in the following situations) Django will iterate over it and has xxx choices how to interpret each element. If the element ...

1. ... is a string, Django will interpret it as a single statements (e.g. no ; that separates two INSERT statements)
2. ... is a list/tuple with 1 element Django behaves as in 1.
3. ... is a list/tuple with 2 elements Django will consider the first element a single statement and the second the parameters as expected and accepted by cursor.execute() and defined in ​PEP-249
4. ... is a list/tuple with 0 or >2 elements Django will raise a ValueError

---

On IRC ThomasC raised the question that it might be worth taking a look into 1. and 2. and pass the string through prepare_sql_script(). This would involve some changes but should be possible. I'm not sure though if this behavior is worth implementing. One usecase would be something like migrations.RunSQL([file1.read(), file2.read()])

[Loic Bistuer]

I wouldn't allow 2 as it doesn't bring much to the table, only:

• "SQL 1; SQL 2; ..." used for backwards compat and to allow reading SQL from a file.
• ["SQL 1", "SQL 2", ...] used to build a sequence programatically.
• [("SQL 1", params), ("SQL 2", params), ...] used to pass params to statements.

[Markus Holtermann]

Thanks for the comment. I removed the 1-tuple support. Waiting for Jenkins to test.

[Tim Graham <timograham@…>]

In 85f6d893138c052f0fa24d7c2005b9c222af91b4:

Fixed #23426 -- Allowed parameters in migrations.RunSQL

Thanks tchaumeny and Loic for reviews.

[Tim Graham <timograham@…>]

In d6a87eefd87250e68457488cfd62fb2cc8211b24:

Skip another test if sqlparse is not available

Refs #23426

[Tim Graham <timograham@…>]

In 4ef9618e12510a84a4d21440b1e18ab704ee6d73:

Avoided requiring sqlparse for a test.

Refs #23426. Thanks Markus Holtermann for the suggestion.