Opened 8 years ago

Closed 3 years ago

#4680 closed Bug (fixed)

Fixes problem in initial_sql where "--" is in a string

Reported by: Tim Chase Owned by: anonymous
Component: Core (Management commands) Version: master
Severity: Normal Keywords: initial-sql manage.py sprintdec01
Cc: code.djangoproject@…, claude@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the core/management.py get_custom_sql_for_model() function, if the initial SQL has a "--" in a string, it chokes.

The attached diff should fix the problem. Test cases:

select '--' as foo; -> select '--' as foo;
select '--' as foo; -- select stuff -> select '--' as foo;
select '--' as foo; -- select stuff -- because I want to -> select '--' as foo;
select * foo; -- select stuff -- because I want to -> select * foo;
select * foo; -- select stuff -> select * foo;

Attachments (4)

initial_sql.diff (946 bytes) - added by Tim Chase 8 years ago.
"svn diff" against trunk
django-ignore-comments-in-sql.patch (5.2 KB) - added by shaleh 8 years ago.
updated patch. Seems simpler than the previous one. Added test casesto simple.sql as well as a unittest for custom_sql_for_model itself
4680-test.diff (2.4 KB) - added by claudep 3 years ago.
Just a test case demonstrating the issue is still present
4680-3.diff (4.8 KB) - added by claudep 3 years ago.
Possible resolution of the issue

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by Tim Chase

"svn diff" against trunk

comment:1 Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 8 years ago by mboersma

  • Owner changed from nobody to mboersma
  • Status changed from new to assigned

comment:3 Changed 8 years ago by anonymous

  • Owner changed from mboersma to anonymous
  • Status changed from assigned to new

Changed 8 years ago by shaleh

updated patch. Seems simpler than the previous one. Added test casesto simple.sql as well as a unittest for custom_sql_for_model itself

comment:4 Changed 8 years ago by shaleh

  • Keywords sprintdec01 added
  • Needs tests unset

added tests

comment:5 Changed 8 years ago by Simon G <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

The first patch doesn't work (as demonstrated by the tests in the second patch). I don't like the code changes in the second patch, though. Having to a linear scan through every character, one at a time, in the initial SQL strings seems inefficient. There must be a better way.

comment:7 Changed 8 years ago by shaleh

Yeah, I was not entirely happy either BUT how often is this called? Is it really in a time efficient section? In other words, while it may not be 100% pretty do we really care? My understanding is this code only happens at application initialization time from manage.py or similar. We could make a sample sql file that had a thousand or two entries and see just how long it takes.

I am willing to help get this finalized, but I do not see a clear regex only solution out of this.

comment:8 Changed 7 years ago by adrian

  • Component changed from Tools to django-admin.py

comment:9 Changed 6 years ago by jammycakes

There are a couple of related problems here:

  1. It chokes on strings that contain a semicolon
  2. It also chokes when you're trying to define stored procedures, e.g.
delimiter $$

create procedure DoSomething()
begin
   select count(*) from MyTable ;
end

$$

delimiter ;

Wouldn't a better solution be to spawn a separate process (mysql -e /path/to/sql/file) to handle the initial SQL as a batch than to try and reinvent the database engine's parser?

comment:10 Changed 6 years ago by anonymous

This seems to have been fixed in bug #6394.

comment:11 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by claudep

Just a test case demonstrating the issue is still present

Changed 3 years ago by claudep

Possible resolution of the issue

comment:12 Changed 3 years ago by claudep

  • Cc claude@… added
  • Easy pickings unset
  • Patch needs improvement unset
  • UI/UX unset

A possible patch, inspired from the initial patch regex to clean comments.

comment:13 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 3 years ago by Claude Paroz <claude@…>

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

In [423244bc6b670abc2b7d6896add5c1baf0b4ef2a]:

Fixed #4680 -- Improved initial_sql parsing

In particular, allow the '--' sequence to be present in string
values without being interpreted as comment marker.
Thanks Tim Chase for the report and shaleh for the initial patch.

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