Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22401 closed Bug (duplicate)

SQL Function initial SQL data does not work.

Reported by: Julien GODIN Owned by: nobody
Component: Core (Management commands) Version: dev
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 Julien GODIN 10 years ago.
Potential patch

Download all attachments as: .zip

Change History (15)

by Julien GODIN, 10 years ago

Attachment: patch.patch added

Potential patch

comment:1 by Aymeric Augustin, 10 years ago

Component: UncategorizedCore (Management commands)
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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 by Julien GODIN, 10 years ago

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 by Aymeric Augustin, 10 years ago

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 by Anssi Kääriäinen, 10 years ago

+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 by Julien GODIN, 10 years ago

Proposed a pull request here :

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

It remove the regex technique and uses sqlparse.

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

Resolution: fixed
Status: newclosed

In 071c9337750b296d198cced56f3ffad0e176afb6:

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

comment:7 by Tim Graham, 10 years ago

Has patch: unset
Patch needs improvement: unset
Resolution: fixed
Severity: NormalRelease blocker
Status: closednew
Version: 1.6master

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

Resolution: fixed
Status: newclosed

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 by Aymeric Augustin, 10 years ago

Resolution: fixed
Status: closednew

comment:10 by Aymeric Augustin, 10 years ago

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 by Tim Graham, 10 years ago

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

comment:12 by Claude Paroz, 10 years ago

Resolution: duplicate
Severity: Release blockerNormal
Status: newclosed

This is really a duplicate of #3214.

comment:13 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

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