From 8b5b199e20ad2d8d3e91873ce0cd5d3035e05ece Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Sat, 26 Apr 2014 10:22:48 +0200 Subject: [PATCH] Fixed #3214 -- Stopped parsing SQL with regex. Avoided introducing a new regex-based SQL splitter in the migrations framework, before we're bound by backwards compatibility. Adapted this change to the legacy "initial SQL data" feature, even though it's already deprecated, in order to facilitate the transition to migrations. sqlparse becomes mandatory for RunSQL on some databases (all but PostgreSQL). There's no API to provide a single statement and tell Django not to attempt splitting. Since we have a more robust splitting implementation, that seems like a good tradeoff. It's easier to add a new keyword argument later if necessary than to remove one. Many people contributed to both tickets, thank you all, and especially Claude for the review. Refs #22401. --- django/core/management/sql.py | 5 ++- django/db/backends/__init__.py | 34 +++++++++++++++++++ .../db/backends/postgresql_psycopg2/base.py | 1 + .../postgresql_psycopg2/operations.py | 3 ++ django/db/migrations/operations/special.py | 28 +++------------ .../contributing/writing-code/unit-tests.txt | 2 ++ docs/ref/migration-operations.txt | 17 +++++----- docs/releases/1.7.txt | 8 +++++ tests/initial_sql_regress/tests.py | 1 - tests/migrations/test_operations.py | 15 +++++++- tests/requirements/base.txt | 1 + 11 files changed, 77 insertions(+), 38 deletions(-) diff --git a/django/core/management/sql.py b/django/core/management/sql.py index c789701c47..4832f27b07 100644 --- a/django/core/management/sql.py +++ b/django/core/management/sql.py @@ -155,6 +155,7 @@ def sql_all(app_config, style, connection): def _split_statements(content): + # Private API only called from code that emits a RemovedInDjango19Warning. comment_re = re.compile(r"^((?:'[^']*'|[^'])*?)--.*$") statements = [] statement = [] @@ -202,9 +203,7 @@ def custom_sql_for_model(model, style, connection): for sql_file in sql_files: if os.path.exists(sql_file): with codecs.open(sql_file, 'r' if six.PY3 else 'U', encoding=settings.FILE_CHARSET) as fp: - # Some backends can't execute more than one SQL statement at a time, - # so split into separate statements. - output.extend(_split_statements(fp.read())) + output.extend(connection.ops.prepare_sql_script(fp.read(), _allow_fallback=True)) return output diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index d9f9bf1c07..b0bb9ad9bd 100644 --- a/django/db/backends/__init__.py +++ b/django/db/backends/__init__.py @@ -1,5 +1,6 @@ import datetime import time +import warnings try: from django.utils.six.moves import _thread as thread @@ -16,6 +17,7 @@ from django.db.backends.signals import connection_created from django.db.backends import utils from django.db.transaction import TransactionManagementError from django.db.utils import DatabaseError, DatabaseErrorWrapper, ProgrammingError +from django.utils.deprecation import RemovedInDjango19Warning from django.utils.functional import cached_property from django.utils import six from django.utils import timezone @@ -599,6 +601,10 @@ class BaseDatabaseFeatures(object): # Does 'a' LIKE 'A' match? has_case_insensitive_like = True + # Does the backend require the sqlparse library for splitting multi-line + # statements before executing them? + requires_sqlparse_for_splitting = True + def __init__(self, connection): self.connection = connection @@ -867,6 +873,34 @@ class BaseDatabaseOperations(object): """ return 'DEFAULT' + def prepare_sql_script(self, sql, _allow_fallback=False): + """ + Takes a SQL script that may contain multiple lines and returns a list + of statements to feed to successive cursor.execute() calls. + + Since few databases are able to process raw SQL scripts in a single + cursor.execute() call and PEP 249 doesn't talk about this use case, + the default implementation is conservative. + """ + # Remove _allow_fallback and keep only 'return ...' in Django 1.9. + try: + # This import must stay inside the method because it's optional. + import sqlparse + except ImportError: + if _allow_fallback: + # Without sqlparse, fall back to the legacy (and buggy) logic. + warnings.warn( + "Providing intial SQL data on a %s database will require " + "sqlparse in Django 1.9." % self.connection.vendor, + RemovedInDjango19Warning) + from django.core.management.sql import _split_statements + return _split_statements(sql) + else: + raise + else: + return [sqlparse.format(statement, strip_comments=True) + for statement in sqlparse.split(sql) if statement] + def process_clob(self, value): """ Returns the value of a CLOB column, for backends that return a locator diff --git a/django/db/backends/postgresql_psycopg2/base.py b/django/db/backends/postgresql_psycopg2/base.py index 3ceb670230..df883e3ebb 100644 --- a/django/db/backends/postgresql_psycopg2/base.py +++ b/django/db/backends/postgresql_psycopg2/base.py @@ -58,6 +58,7 @@ class DatabaseFeatures(BaseDatabaseFeatures): nulls_order_largest = True closed_cursor_error_class = InterfaceError has_case_insensitive_like = False + requires_sqlparse_for_splitting = False class DatabaseWrapper(BaseDatabaseWrapper): diff --git a/django/db/backends/postgresql_psycopg2/operations.py b/django/db/backends/postgresql_psycopg2/operations.py index 9285e6eeca..b9d0231768 100644 --- a/django/db/backends/postgresql_psycopg2/operations.py +++ b/django/db/backends/postgresql_psycopg2/operations.py @@ -93,6 +93,9 @@ class DatabaseOperations(BaseDatabaseOperations): def no_limit_value(self): return None + def prepare_sql_script(self, sql, _allow_fallback=False): + return [sql] + def quote_name(self, name): if name.startswith('"') and name.endswith('"'): return name # Quoting once is enough. diff --git a/django/db/migrations/operations/special.py b/django/db/migrations/operations/special.py index 4f0c494f33..f87585f0db 100644 --- a/django/db/migrations/operations/special.py +++ b/django/db/migrations/operations/special.py @@ -1,4 +1,3 @@ -import re from .base import Operation @@ -43,20 +42,16 @@ class SeparateDatabaseAndState(Operation): class RunSQL(Operation): """ - Runs some raw SQL - a single statement by default, but it will attempt - to parse and split it into multiple statements if multiple=True. - - A reverse SQL statement may be provided. + Runs some raw SQL. A reverse SQL statement may be provided. Also accepts a list of operations that represent the state change effected by this SQL change, in case it's custom column/table creation/deletion. """ - def __init__(self, sql, reverse_sql=None, state_operations=None, multiple=False): + def __init__(self, sql, reverse_sql=None, state_operations=None): self.sql = sql self.reverse_sql = reverse_sql self.state_operations = state_operations or [] - self.multiple = multiple @property def reversible(self): @@ -66,30 +61,15 @@ class RunSQL(Operation): for state_operation in self.state_operations: state_operation.state_forwards(app_label, state) - def _split_sql(self, sql): - regex = r"(?mx) ([^';]* (?:'[^']*'[^';]*)*)" - comment_regex = r"(?mx) (?:^\s*$)|(?:--.*$)" - # First, strip comments - sql = "\n".join([x.strip().replace("%", "%%") for x in re.split(comment_regex, sql) if x.strip()]) - # Now get each statement - for st in re.split(regex, sql)[1:][::2]: - yield st - def database_forwards(self, app_label, schema_editor, from_state, to_state): - if self.multiple: - statements = self._split_sql(self.sql) - else: - statements = [self.sql] + statements = schema_editor.connection.ops.prepare_sql_script(self.sql) for statement in statements: schema_editor.execute(statement) 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") - if self.multiple: - statements = self._split_sql(self.reverse_sql) - else: - statements = [self.reverse_sql] + statements = schema_editor.connection.ops.prepare_sql_script(self.reverse_sql) for statement in statements: schema_editor.execute(statement) diff --git a/docs/internals/contributing/writing-code/unit-tests.txt b/docs/internals/contributing/writing-code/unit-tests.txt index 3edc588c18..3571fb0c26 100644 --- a/docs/internals/contributing/writing-code/unit-tests.txt +++ b/docs/internals/contributing/writing-code/unit-tests.txt @@ -166,6 +166,7 @@ dependencies: * memcached_, plus a :ref:`supported Python binding ` * gettext_ (:ref:`gettext_on_windows`) * selenium_ +* sqlparse_ You can find these dependencies in `pip requirements files`_ inside the ``tests/requirements`` directory of the Django source tree and install them @@ -197,6 +198,7 @@ associated tests will be skipped. .. _memcached: http://memcached.org/ .. _gettext: http://www.gnu.org/software/gettext/manual/gettext.html .. _selenium: https://pypi.python.org/pypi/selenium +.. _sqlparse: https://pypi.python.org/pypi/sqlparse .. _pip requirements files: http://www.pip-installer.org/en/latest/cookbook.html#requirements-files Code coverage diff --git a/docs/ref/migration-operations.txt b/docs/ref/migration-operations.txt index 2a5f46a93c..331ccd5c06 100644 --- a/docs/ref/migration-operations.txt +++ b/docs/ref/migration-operations.txt @@ -167,25 +167,23 @@ Changes a field's name (and, unless ``db_column`` is set, its column name). Special Operations ================== +.. _operation-run-sql: + RunSQL ------ :: - RunSQL(sql, reverse_sql=None, state_operations=None, multiple=False) + RunSQL(sql, reverse_sql=None, state_operations=None) Allows running of arbitrary SQL on the database - useful for more advanced features of database backends that Django doesn't support directly, like partial indexes. -``sql``, and ``reverse_sql`` if provided, should be strings of SQL to run on the -database. They will be passed to the database as a single SQL statement unless -``multiple`` is set to ``True``, in which case they will be split into separate -statements manually by the operation before being passed through. - -In some extreme cases, the built-in statement splitter may not be able to split -correctly, in which case you should manually split the SQL into multiple calls -to ``RunSQL``. +``sql``, and ``reverse_sql`` if provided, should be strings of SQL to run on +the database. On most database backends (all but PostgreSQL), Django will +split the SQL into individual statements prior to executing them. This +requires installing the sqlparse_ Python library. The ``state_operations`` argument is so you can supply operations that are equivalent to the SQL in terms of project state; for example, if you are @@ -194,6 +192,7 @@ operation here so that the autodetector still has an up-to-date state of the model (otherwise, when you next run ``makemigrations``, it won't see any operation that adds that field and so will try to run it again). +.. _sqlparse: https://pypi.python.org/pypi/sqlparse .. _operation-run-python: diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 0828585550..a34d0d6fe9 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -636,6 +636,14 @@ Management Commands * :djadmin:`collectstatic` command with symlink option is now supported on Windows NT 6 (Windows Vista and newer). +* :ref:`initial-sql` now works better if the sqlparse_ Python library is + installed. + + Note that it's deprecated in favor of the :ref:`RunSQL ` + operation of migrations, which benefits from the improved behavior. + +.. _sqlparse: https://pypi.python.org/pypi/sqlparse + Models ^^^^^^ diff --git a/tests/initial_sql_regress/tests.py b/tests/initial_sql_regress/tests.py index 428d993667..ebbe36d35d 100644 --- a/tests/initial_sql_regress/tests.py +++ b/tests/initial_sql_regress/tests.py @@ -27,7 +27,6 @@ class InitialSQLTests(TestCase): """ connection = connections[DEFAULT_DB_ALIAS] custom_sql = custom_sql_for_model(Simple, no_style(), connection) - self.assertEqual(len(custom_sql), 9) with connection.cursor() as cursor: for sql in custom_sql: cursor.execute(sql) diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index e1b4682d58..d294bdcd26 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -1,5 +1,10 @@ import unittest +try: + import sqlparse +except ImportError: + sqlparse = None + from django.db import connection, migrations, models, router from django.db.migrations.migration import Migration from django.db.migrations.state import ProjectState @@ -640,6 +645,7 @@ class OperationTests(MigrationTestBase): operation.database_backwards("test_alinto", editor, new_state, project_state) self.assertIndexNotExists("test_alinto_pony", ["pink", "weight"]) + @unittest.skipIf(sqlparse is None and connection.features.requires_sqlparse_for_splitting, "Missing sqlparse") def test_run_sql(self): """ Tests the RunSQL operation. @@ -647,7 +653,10 @@ class OperationTests(MigrationTestBase): project_state = self.set_up_test_model("test_runsql") # Create the operation operation = migrations.RunSQL( - "CREATE TABLE i_love_ponies (id int, special_thing int)", + # Use a multi-line string with a commment to test splitting on SQLite and MySQL respectively + "CREATE TABLE i_love_ponies (id int, special_thing int);\n" + "INSERT INTO i_love_ponies (id, special_thing) VALUES (1, 42); -- this is magic!\n" + "INSERT INTO i_love_ponies (id, special_thing) VALUES (2, 51);\n", "DROP TABLE i_love_ponies", state_operations=[migrations.CreateModel("SomethingElse", [("id", models.AutoField(primary_key=True))])], ) @@ -661,6 +670,10 @@ class OperationTests(MigrationTestBase): with connection.schema_editor() as editor: operation.database_forwards("test_runsql", editor, project_state, new_state) self.assertTableExists("i_love_ponies") + # Make sure all the SQL was processed + with connection.cursor() as cursor: + cursor.execute("SELECT COUNT(*) FROM i_love_ponies") + self.assertEqual(cursor.fetchall()[0][0], 2) # And test reversal self.assertTrue(operation.reversible) with connection.schema_editor() as editor: diff --git a/tests/requirements/base.txt b/tests/requirements/base.txt index 3d982bc00a..c3f77234fd 100644 --- a/tests/requirements/base.txt +++ b/tests/requirements/base.txt @@ -5,3 +5,4 @@ Pillow PyYAML pytz > dev selenium +sqlparse