Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#23426 closed Cleanup/optimization (fixed)

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

Reported by: ris Owned by: Markus Holtermann
Component: Migrations Version: dev
Severity: Normal Keywords: migrations sql runsql params escape
Cc: info+coding@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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.

Change History (17)

comment:1 by ris, 9 years ago

Description: modified (diff)

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

comment:2 by Tim Graham, 9 years ago

Component: MigrationsDocumentation
Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Documentation seems appropriate.

comment:3 by Markus Holtermann, 9 years ago

Cc: info+coding@… added
Owner: changed from nobody to Markus Holtermann
Status: newassigned

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

    diff --git a/django/db/migrations/operations/special.py b/django/db/migrations/operations/special.py
    index fe5f00a..bfe4180 100644
    a b class RunSQL(Operation):  
    6666    def database_forwards(self, app_label, schema_editor, from_state, to_state):
    6767        statements = schema_editor.connection.ops.prepare_sql_script(self.sql)
    6868        for statement in statements:
    69             schema_editor.execute(statement)
     69            schema_editor.execute(statement, params=None)
    7070
    7171    def database_backwards(self, app_label, schema_editor, from_state, to_state):
    7272        if self.reverse_sql is None:
    7373            raise NotImplementedError("You cannot reverse this operation")
    7474        statements = schema_editor.connection.ops.prepare_sql_script(self.reverse_sql)
    7575        for statement in statements:
    76             schema_editor.execute(statement)
     76            schema_editor.execute(statement, params=None)
    7777
    7878    def describe(self):
    7979        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

    diff --git a/django/db/migrations/operations/special.py b/django/db/migrations/operations/special.py
    index fe5f00a..f6e7ea9 100644
    a b class RunSQL(Operation):  
    5050    by this SQL change, in case it's custom column/table creation/deletion.
    5151    """
    5252
    53     def __init__(self, sql, reverse_sql=None, state_operations=None):
     53    def __init__(self, sql, reverse_sql=None, state_operations=None, params=None, reverse_params=None):
    5454        self.sql = sql
    5555        self.reverse_sql = reverse_sql
    5656        self.state_operations = state_operations or []
     57        self.params = params
     58        self.reverse_params = reverse_params
    5759
    5860    @property
    5961    def reversible(self):
    class RunSQL(Operation):  
    6668    def database_forwards(self, app_label, schema_editor, from_state, to_state):
    6769        statements = schema_editor.connection.ops.prepare_sql_script(self.sql)
    6870        for statement in statements:
    69             schema_editor.execute(statement)
     71            schema_editor.execute(statement, params=self.params)
    7072
    7173    def database_backwards(self, app_label, schema_editor, from_state, to_state):
    7274        if self.reverse_sql is None:
    7375            raise NotImplementedError("You cannot reverse this operation")
    7476        statements = schema_editor.connection.ops.prepare_sql_script(self.reverse_sql)
    7577        for statement in statements:
    76             schema_editor.execute(statement)
     78            schema_editor.execute(statement, params=self.reverse_params)
    7779
    7880    def describe(self):
    7981        return "Raw SQL operation"

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

comment:4 by Claude Paroz, 9 years ago

#23541 was closed as a duplicate.

comment:5 by Claude Paroz, 9 years ago

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?

comment:6 by Markus Holtermann, 9 years ago

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

comment:7 by Markus Holtermann, 9 years ago

Component: DocumentationMigrations
Has patch: set

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

comment:8 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In b9a670b22799a44fe7d3467d1d21949f9f717593:

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

comment:9 by Tim Graham <timograham@…>, 9 years ago

In ae14c75014379d1cf405509b65eb986c0cb3b0f3:

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

Backport of b9a670b227 from master

comment:10 by Loic Bistuer, 9 years ago

Easy pickings: unset
Patch needs improvement: set
Version: 1.7master

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.

comment:11 by Markus Holtermann, 9 years ago

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()])

comment:12 by Loic Bistuer, 9 years ago

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.

comment:13 by Markus Holtermann, 9 years ago

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

comment:14 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:15 by Tim Graham <timograham@…>, 9 years ago

In 85f6d893138c052f0fa24d7c2005b9c222af91b4:

Fixed #23426 -- Allowed parameters in migrations.RunSQL

Thanks tchaumeny and Loic for reviews.

comment:16 by Tim Graham <timograham@…>, 9 years ago

In d6a87eefd87250e68457488cfd62fb2cc8211b24:

Skip another test if sqlparse is not available

Refs #23426

comment:17 by Tim Graham <timograham@…>, 9 years ago

In 4ef9618e12510a84a4d21440b1e18ab704ee6d73:

Avoided requiring sqlparse for a test.

Refs #23426. Thanks Markus Holtermann for the suggestion.

Note: See TracTickets for help on using tickets.
Back to Top