Re: [Django] #3214: [patch] raw sql file doesn't recognize quotes correctly

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: reopened
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by Xof):

* cc: Xof (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:26>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody

Type: Bug | Status: new

Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by sbarre):

* cc: sbarre (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:28>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by ris):

Can we get this fixed at some point please? It seems quite straightforward
now and it causes daily pain to so many people.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:29>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by russellm):

Sure - it will be fixed as soon as someone presents a patch. There's a
call to action in [#comment:18 Comment 18] and [#comment:19 Comment 19],
but nobody has presented a patch that implements that approach AFAICT from
the rest of the ticket discussion.

If someone wants this, you're going to have to write the patch.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:30>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):

* needs_tests: 1 => 0

Comment:

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

Open for review!

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:31>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by claudep):

Question raised by Mark on the PR: `With the migrations support, are .sql
files even needed any more?`

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:32>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by vzima):

Replying to [comment:32 claudep]:

> Question raised by Mark on the PR: `With the migrations support, are
.sql files even needed any more?`

I have not check the entire migration and south documentation, but what
about custom restraints, triggers and functions?

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:33>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by vzima):

* cc: vlastimil@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:34>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by Dmitri Bogomolov <4glitch@…>):

* cc: 4glitch@… (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:35>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0

Has patch: 1 | Patch needs improvement: 1

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by claudep):

* needs_better_patch: 0 => 1

Comment:

Repeating Andrew's answer on the pull request:
`They should go away, there's a RunSQL operation you can use in migrations
instead (that accepts either single statements or mutiple ones with its
own internal splitter regex). I didn't get around to removing the support
for them on syncdb-type apps, but they'll be ignored if you have
migrations.`

We should therefore test (sql like
[[https://github.com/claudep/django/blob/7ef5f573f0ee3dc615125c8195bde541c764b782/tests/initial_sql_regress/sql/simple.postgresql_psycopg2.sql|this]])
and document the new process replacing initial sql before closing this
ticket (or close this ticket and open new one).

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:36>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by timo):

How will the migrations framework handle
[https://docs.djangoproject.com/en/dev/howto/initial-data/#database-
backend-specific-sql-data Database-backend-specific SQL data]?

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:37>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by andrewgodwin):

Yep, this same bug will still apply to the splitter inside RunSQL (though
I believe it's marginally better than the old Django one).

timo: Database-specific SQL data will only work with apps without
migrations; those with migrations are expected to achieve the same result
using a migration to load in the data, which is much more portable and
forwards-compatible.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:38>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by aaugustin):

At least parts of this must have been fixed in #22401, see
071c9337750b296d198cced56f3ffad0e176afb6.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:39>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: rawsql | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by wdoekes):

Assuming sqlparse does the job, this can indeed be closed. Thanks!

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:40>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:

Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0

Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Changes (by aaugustin):

* keywords: rawsql => rawsql migrations
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* severity: Normal => Release blocker

Comment:

This ticket never tells which backend support multiple statements in
`cursor.execute()` and which don't. It would be interesting to check that
on the four officially supported databases before going any further...

----

If indeed some backends need splitting, in order to close this ticket, we
must implement a similar approach as #22401 in `RunSQL._split_sql`.

Splitting occurs only when explicitly setting `multiple=True`, which
Django itself never does, even in the test suite. We should add a test for
that (and skip it when sqlparse isn't available.

The patch will be easier if we include it in 1.7 because we won't need a
deprecation path. Therefore, marking as a release blocker.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:41>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by claudep):

SQLite: needs splitting
PostgreSQL: does not need splitting
MySQL: does not need splitting, but has issues when some multiline SQL
results are not read (`cursor.nextset()` has to be used to "consume"
results).
Oracle: ?

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:42>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by aaugustin):

It would be interesting to avoid the dependency on sqlparse for backends
that do not need it. Can we introduce a feature flag
"execute_supports_multiple_statements"?

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:43>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by claudep):

In my tentative pull request linked in comment:31, I used the approach
suggested by Russell in comment:19 (a custom backend method).

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:44>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by aaugustin):

The fix #22401 was reverted and it was then closed as a duplicate.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:45>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: rawsql migrations | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------

Comment (by aaugustin):

Oracle seems to choke on `\n`:

{{{
DatabaseError: ORA-00911: invalid character
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:46>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:

Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):

* stage: Accepted => Ready for checkin

Comment:

I think this is good to go: https://github.com/django/django/pull/2608

I ran the "test_run_sql" test on all four core database backends with and
without sqlparse. It's skipped in three cases and passes in five cases.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:47>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:48>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by claudep):

Thanks Aymeric, nice patch!

There are however test failures in initial_sql_regress, I had to apply
this:
{{{
--- a/tests/initial_sql_regress/tests.py
+++ b/tests/initial_sql_regress/tests.py
@@ -27,7 +27,10 @@ class InitialSQLTests(TestCase):
"""
connection = connections[DEFAULT_DB_ALIAS]
custom_sql = custom_sql_for_model(Simple, no_style(), connection)
- self.assertEqual(len(custom_sql), 9)
+ if connection.features.requires_sqlparse_for_splitting:
+ self.assertEqual(len(custom_sql), 9)
+ else:
+ self.assertEqual(len(custom_sql), 1)
with connection.cursor() as cursor:
for sql in custom_sql:
cursor.execute(sql)
}}}
Apart from that, I think it's RFC.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:49>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by aaugustin):

Thanks a lot for the review!

I don't think the assert on the number on statements is useful. I'll just
remove it.

What matters is that each statement creates an object and that 9 objects
are created.

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:50>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody

Type: Bug | Status: closed

Component: Database layer | Version: master

(models, ORM) | Resolution: fixed

Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Aymeric Augustin <aymeric.augustin@…>):

* status: new => closed
* resolution: => fixed

Comment:

In [changeset:"8b5b199e20ad2d8d3e91873ce0cd5d3035e05ece"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:51>

[Django]

#3214: [patch] raw sql file doesn't recognize quotes correctly
-------------------------------------+-------------------------------------
Reporter: shaunc <shaun@…> | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: rawsql migrations | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Aymeric Augustin <aymeric.augustin@…>):

In [changeset:"3bb0f118ca375f25cd0c03a5733ee2ef9d79dfa5"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/3214#comment:52>