Opened 16 months ago

Closed 16 months ago

Last modified 15 months ago

#22401 closed Bug (duplicate)

SQL Function initial SQL data does not work.

Reported by: pouete Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I created a model and trying to add some functions to it :
Full trace :

Failed to install custom SQL for frontend.Alarm model: unterminated dollar-quoted string at or near "$$ BEGIN RETURN + 1;"
LINE 1: ... FUNCTION increment(i integer) RETURNS integer AS $$ BEGIN R...

Here is the function I am trying to create :
( stolen from here : http://www.postgresql.org/docs/9.1/static/sql-createfunction.html )

CREATE OR REPLACE FUNCTION increment(i integer) RETURNS integer AS $$
       BEGIN
                RETURN i + 1;
       END;
$$ LANGUAGE plpgsql;

The problem is in the _split_statements(content) function where it checks if the lines ends with ";" and close the statement if so.
The problem is that PLSQL functions needs ";" to work.

Attachments (1)

patch.patch (1.1 KB) - added by pouete 16 months ago.
Potential patch

Download all attachments as: .zip

Change History (15)

Changed 16 months ago by pouete

Potential patch

comment:1 Changed 16 months ago by aaugustin

  • Component changed from Uncategorized to Core (Management commands)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

Hardcoding CREATE and LANGUAGE doesn't sound like a good solution. What if there's simply a ; at the end of a line in a multi-line string?

Parsing SQL with regex is worse than parsing HTML with regex... If we want to fix these edge cases, we must fix them with proper parsing, not hacks.

comment:2 Changed 16 months ago by pouete

So, to provide a summary of the situation :

  • No regex.
  • parse the SQL properly.

Should we be using this ? :
http://code.google.com/p/python-sqlparse/
as i do not see any other solution so far.

comment:3 Changed 16 months ago by aaugustin

Actually sqlparse is here now: https://github.com/andialbrecht/sqlparse

It supports Python 2.5+ and 3.2+, which is good, and provides the exact feature we need:
https://sqlparse.readthedocs.org/en/latest/api/?highlight=split#sqlparse.split

I'm +0 on introducing this as on optional dependency and +1 on deprecating the current regex-based logic and eventually turning it into a mandatory dependency when you want to load initial SQL (which most projects don't)!

comment:4 Changed 16 months ago by akaariai

+1 on deprecating regex SQL parsing. I don't think we should be maintaining any other half-baked solution to SQL parsing either. So, I guess that makes me +1 on requiring sqlparse for initial SQL.

comment:5 Changed 16 months ago by pouete

Proposed a pull request here :

https://github.com/django/django/pull/2529

It remove the regex technique and uses sqlparse.

comment:6 Changed 16 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 071c9337750b296d198cced56f3ffad0e176afb6:

Fixed #22401 -- Deprecated regular expression parsing of initial SQL in favor of installing sqlparse.

comment:7 Changed 16 months ago by timo

  • Has patch unset
  • Patch needs improvement unset
  • Resolution fixed deleted
  • Severity changed from Normal to Release blocker
  • Status changed from closed to new
  • Version changed from 1.6 to master

There's a regression here because sqlparse does not strip trailing comments the way the regex does (see #4680 for when this behavior was added). tests/initial_sql_regress/sql/simple.sql contains a trailing comment on line 2 and this causes MySQL to error out with "Commands out of sync; you can't run this command now". Jenkins doesn't have sqlparse installed, so it's still using the regex logic.

You can reproduce this behavior with the following code:

>>> from django.db import connection
>>> cursor = connection.cursor()
>>> cursor.execute("SELECT 1; --")
>>> cursor.execute("SELECT 2;")
Traceback ...
django.db.utils.ProgrammingError: (2014, "Commands out of sync; you can't run this command now")

I've realized that initial SQL data is deprecated (pending confirmation from Andrew Godwin in #22444) so I'm inclined just to revert this and mark it "won't fix".

comment:8 Changed 16 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 47927eb786f432cb069f0b00fd810c465a78fd71:

Revert "Fixed #22401 -- Deprecated regular expression parsing of initial SQL in favor of installing sqlparse."

This reverts commit 071c9337750b296d198cced56f3ffad0e176afb6.

This introduced a regression on MySQL and custom SQL is deprecated.

comment:9 Changed 16 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

comment:10 Changed 16 months ago by aaugustin

Possible solution:

>>> import sqlparse
>>> sqlparse.split("""
...     SELECT 1; -- comment
...     SELECT 2;
... """)
['SELECT 1; -- comment', 'SELECT 2;', '']
>>> [sqlparse.format(stmt, strip_comments=True) for stmt in _ if stmt]
['SELECT 1;', 'SELECT 2;']

comment:11 Changed 16 months ago by timo

Aymeric, you think it's worth addressing this even though the feature is deprecated?

comment:12 Changed 16 months ago by claudep

  • Resolution set to duplicate
  • Severity changed from Release blocker to Normal
  • Status changed from new to closed

This is really a duplicate of #3214.

comment:13 Changed 15 months ago by Aymeric Augustin <aymeric.augustin@…>

In 8b5b199e20ad2d8d3e91873ce0cd5d3035e05ece:

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.

comment:14 Changed 15 months ago by Aymeric Augustin <aymeric.augustin@…>

In 3bb0f118ca375f25cd0c03a5733ee2ef9d79dfa5:

[1.7.x] 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.

Backport of 8b5b199 from master

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