Opened 4 years ago

Closed 4 years ago

Last modified 4 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: master
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 Changed 4 years ago by ris

Description: modified (diff)

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

comment:2 Changed 4 years ago by Tim Graham

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

Documentation seems appropriate.

comment:3 Changed 4 years ago by Markus Holtermann

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 Changed 4 years ago by Claude Paroz

#23541 was closed as a duplicate.

comment:5 Changed 4 years ago by 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?

comment:6 Changed 4 years ago by Markus Holtermann

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

comment:7 Changed 4 years ago by Markus Holtermann

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 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In b9a670b22799a44fe7d3467d1d21949f9f717593:

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

comment:9 Changed 4 years ago by 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

comment:10 Changed 4 years ago by Loic Bistuer

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 Changed 4 years ago by 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()])

comment:12 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by Markus Holtermann

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

comment:14 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

comment:15 Changed 4 years ago by Tim Graham <timograham@…>

In 85f6d893138c052f0fa24d7c2005b9c222af91b4:

Fixed #23426 -- Allowed parameters in migrations.RunSQL

Thanks tchaumeny and Loic for reviews.

comment:16 Changed 4 years ago by Tim Graham <timograham@…>

In d6a87eefd87250e68457488cfd62fb2cc8211b24:

Skip another test if sqlparse is not available

Refs #23426

comment:17 Changed 4 years ago by Tim Graham <timograham@…>

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