Cross-db compatibility issue (SQLite can't insert multiple rows at once)

[Ryan J Ollos]

As Steffen pointed out in the ​mailing list thread, the plugin doesn't follow the Trac rules for DB API usage, as described in ​t:TracDev/DatabaseApi#RulesforDBAPIUsage. This will result in cross-db compatibility issues and the possibility of SQL injection.

Here is an example fix (untested, as I don't understand the plugin well enough to execute this pathway or write a test):

tracjsganttplugin/0.11/tracjsgantt/tracpm.py

@@ -2904 +2904 @@
                             values.append(t['id']) 
                             values.append(to_utimestamp(self.pm.start(t)))
                             values.append(to_utimestamp(self.pm.finish(t)))
-                    cursor.execute('INSERT INTO schedule' + \
-                                       ' (ticket, start, finish)' + \
-                                       ' VALUES %s' % valuesClause,
-                                   values)
-
+                    cursor.execute("""
+                        INSERT INTO schedule (ticket, start, finish)
+                        VALUES %s""" % valuesClause, values)
 
                     # Finally, add history records to schedule_change
                     # for newly scheduled tickets.

[mlm@…]

hi,

from trac.ini

[trac-jsgantt]
option.caption = Resource
option.colorby = priority
option.comp = 0
option.datedisplay = mm/dd/yyyy
option.dur = 0
option.enddate = 1
option.expandclosedtickets = 1
option.format = month
option.formats = day|week|month|quarter
option.hoursperday = 8.0
option.lwidth = 500
option.omitmilestone = 0
option.openlevel = 0
option.res = 0
option.schedule = asap
option.showdep = 1
option.startdate = 1
option.usermap = 0

[TracPM]
blocking = text
blocking.label = Blocking
complete = select
complete.label = % Complete
complete.options = 0|5|10|15|20|25|30|35|40|45|50|55|60|65|70|75|80|85|90|95|100
complete.order = 3
date_format = %m/%d/%Y
days_per_estimate = 0.125
default_estimate = 4.0
due_assign = text
due_assign.label = Start (MM/DD/YYYY)
due_assign.order = 1
due_close = text
due_close.label = End (MM/DD/YYYY)
due_close.order = 2
estimate_pad = 0.0
estimatedhours = text
estimatedhours.label = Estimated Number of Hours
estimatedhours.order = 1
estimatedhours.value = 0
fields.estimate = estimatedhours
fields.finish = due_close
fields.parent = parents
fields.pred = blockedby
fields.start = due_assign
fields.succ = blocking
fields.worked = totalhours
goal_ticket_type = milestone
hours = text
hours.label = Add Hours to Ticket
hours.order = 2
hours.value = 0
hours_per_estimate = 1
loe = select
loe.label = Estimate of Effort
loe.options = low|medium|high
loe.value = low
parent_format = %s
parents = text
parents.label = Parent Tickets
prim_developer = select
prim_developer.label = Primary Developer
prim_developer.options = na|allison|aracicot|chander|gcorradini|pmc2|rfsl|winkey
test_desc = textarea
test_desc.cols = 80
test_desc.format = wiki
test_desc.label = Testing Instructions
test_desc.rows = 10
totalhours = text
totalhours.label = Total Hours
totalhours.order = 4
totalhours.value = 0

[Ryan J Ollos]

The [ticket-custom] section can probably be inferred from what you've posted, but could you add that as well, just so we can be sure?

[anonymous]

[ticket-custom]
billable = checkbox
billable.label = Billable?
billable.order = 3
billable.value = 1
blockedby = text
blockedby.label = Blocked By
blocking = text
blocking.label = Blocking
due_assign = text
due_assign.label = Start (MM/DD/YYYY)
due_assign.order = 1
due_close = text
due_close.label = End (MM/DD/YYYY)
due_close.order = 2
estimatedhours = text
estimatedhours.label = Estimated Number of Hours
estimatedhours.order = 1
estimatedhours.value = 0
hours = text
hours.label = Add Hours to Ticket
hours.order = 2
hours.value = 0
parents = text
parents.label = Parent Tickets
totalhours = text
totalhours.label = Total Hours
totalhours.order = 4
totalhours.value = 0

[Ryan J Ollos]

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

Anyway, #11028 is a problem on Trac 1.0 when the TicketRescheduler component is enabled, but that won't be an issue for you since you've shown in your screen capture that it's not enabled.

I can't reproduce your specific issue. Maybe some more information is required. Could you post the [components] section from your trac.ini?

[mlm@…]

hi,

Tried to attach the complete trac.ini as a file, said it was spam - too many external links.

[components]
acct_mgr.htfile.htdigeststore = disabled
cmtekniktheme.* = enabled
crystalxtheme.* = enabled
githubsimple.* = enabled
mastertickets.* = enabled
skittlishtheme.* = enabled
themeengine.* = enabled
themeengine.admin.themeadminmodule = enabled
themeengine.api.themeenginesystem = enabled
themeengine.web_ui.themeenginemodule = enabled
timingandestimationplugin.* = enabled
trac.web.auth.loginmodule = disabled
tracjsgantt.* = enabled
tracjsgantt.tracjsgantt.tracjsganttchart = enabled
tracjsgantt.tracjsgantt.tracjsganttsupport = enabled
tracjsgantt.tracpm.projectsorter = enabled
tracjsgantt.tracpm.resourcescheduler = enabled
tracjsgantt.tracpm.simplecalendar = enabled
tracjsgantt.tracpm.simplesorter = enabled
tracjsgantt.tracpm.ticketrescheduler = disabled
tracjsgantt.tracpm.tracpm = enabled
tracopt.versioncontrol.git.* = enabled
tracopt.versioncontrol.svn.svn_fs.subversionconnector = enabled
tracopt.versioncontrol.svn.svn_prop.subversionmergepropertydiffrenderer = enabled
tracopt.versioncontrol.svn.svn_prop.subversionmergepropertyrenderer = enabled
tracopt.versioncontrol.svn.svn_prop.subversionpropertyrenderer = enabled
tracsubtickets.api.* = enabled
tracsubtickets.web_ui.* = enabled

[Ryan J Ollos]

Just a hint, you can wrap the sections you post in a code block to get nicer formatting (I've been editing your comments to add this):

{{{
#!ini
     <--- paste your content here, between the brackets
}}}

The use of the #!ini isn't necessary, and just gives syntax highlighting.

[Steffen Hoffmann]

Replying to rjollos:

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

No, actually it seems like my fault, because I didn't read the source good enough too, and my thread comment triggered this ticket of yours.

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, that would avoid the need 'valuesClause' and make code more obvious IMHO.

[Jun Omae]

Replying to hasienda:

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, ...

I think so, too. Should use cursor.executemany.

The inserting multiple rows feature is available in SQLite 3.7.11 or later. See ​http://sqlite.org/changes.html#version_3_7_11. However, the SQLite is 3.6.20 in CentOS 6.

$ cat /etc/redhat-release
CentOS release 6.4 (Final)
$ rpm -q sqlite
sqlite-3.6.20-1.el6.x86_64

[Chris Nelson]

Replying to hasienda:

Replying to rjollos:

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

No, actually it seems like my fault, because I didn't read the source good enough too, and my thread comment triggered this ticket of yours.

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, that would avoid the need 'valuesClause' and make code more obvious IMHO.

I'd be interested in seeing a patch that demonstrated that. I'm a DB API noob and have never found satisfying documentation so I extrapolate from examples I find and stick with things that work for me. Sometimes, those examples or that working code is not ideal.

[Chris Nelson]

I don't really see how this can be subject to SQL injection. The valuesClause is a list of triples each of which consists of:

• The numeric ticket ID from Trac
• The result of parsing two date strings, each is either a valid date or None

[Chris Nelson]

Considering

tracjsganttplugin/0.11/tracjsgantt/tracpm.py

@@ -2904 +2904 @@
                             values.append(t['id']) 
                             values.append(to_utimestamp(self.pm.start(t)))
                             values.append(to_utimestamp(self.pm.finish(t)))
-                    cursor.execute('INSERT INTO schedule' + \
-                                       ' (ticket, start, finish)' + \
-                                       ' VALUES %s' % valuesClause,
-                                   values)
-
+                    cursor.execute("""
+                        INSERT INTO schedule (ticket, start, finish)
+                        VALUES %s""" % valuesClause, values)
 
                     # Finally, add history records to schedule_change
                     # for newly scheduled tickets.

what does that """ do here? Doesn't """ just makes multi-line strings? If so, the after is semantically equivalent to the former.

​StackOverlfow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

[Chris Nelson]

Replying to rjollos:

As Steffen pointed out in the ​mailing list thread, the plugin doesn't follow the Trac rules for DB API usage, as described in ​t:TracDev/DatabaseApi#RulesforDBAPIUsage. This will result in cross-db compatibility issues and the possibility of SQL injection. ...

I don't see how I violate the Rules. I should not use "string formatting" to put values in the query. And I don't.

        valuesClause = ','.join(('(%s,%s,%s)',) * len(toInsert))
        values = []
        for t in tickets:
            if t['id'] in toInsert:
                values.append(t['id'])
                values.append(to_utimestamp(self.pm.start(t)))
                values.append(to_utimestamp(self.pm.finish(t)))
        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES %s' % valuesClause,
                       values)

I add valuesClause to the SQL with string formatting (%s) like the example from the Rules about a table name. valuesClause may be

• '(%s,%s,%s)'
• '(%s,%s,%s),(%s,%s,%s)'

and so on. (It can't be '' because we don't get to this code unless len(toInsert) is > 0.) So for two tickets to insert, I get

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES %s' % '(%s,%s,%s),(%s,%s,%s)',
                       values)

and values has six values in it (id, ts, ts, id, ts, ts). What's wrong with that?

[Chris Nelson]

Replying to ChrisNelson:

... So for two tickets to insert, I get

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES %s' % '(%s,%s,%s),(%s,%s,%s)',
                       values)

and values has six values in it (id, ts, ts, id, ts, ts). What's wrong with that?

To carry this one step further, since the % will be processed before the function call, this evaluates to

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES (%s,%s,%s),(%s,%s,%s)',
                       values)

and we're using the DB API to fill in the %s's with the elements of values.

[Jun Omae]

INSERT statement with multiple rows is not cross-db compatibility. Instead, should use cursor.executemany().

$ sqlite3
SQLite version 3.3.6
Enter ".help" for instructions
sqlite> CREATE TABLE test (id INTEGER, col1 TEXT);
sqlite> INSERT INTO test (id, col1) VALUES (1, '111'), (2, '222');
SQL error: near ",": syntax error
sqlite> INSERT INTO test (id, col1) VALUES (1, '111');
sqlite> SELECT * FROM test;
1|111

[Chris Nelson]

Replying to jun66j5:

INSERT statement with multiple rows is not cross-db compatibility. ...

THANK YOU!

[Chris Nelson]

Short form:

• Must use executemany() to INSERT multiple rows for cross-db compatibility (not supported in SQLlite)
• Cannot use executemany() to SELECT (not supported at all)

[Chris Nelson]

Considering, for example, ​some MySQL help, (and ​pyodbc) the structure of my values has to change from

(r1v1, r1v2, ..., r1vn, r2v1, r2v2, ...)

to

[ (r1v1, r1v2, ... r1vn), (r2v1, ... r2vn) ...]`

Easy enough.

[Chris Nelson]

In 13868:

Use executemany() when inserting multiple rows. Refs #11027.

Required for cross-db compatibility.

Light testing shows this still works for me in PostgreSQL so it
doesn't seem I broke anything. I need SQLite feedback, though.

[Steffen Hoffmann]

Replying to ChrisNelson:

what does that """ do here? Doesn't """ just makes multi-line strings? If so, the after is semantically equivalent to the former.

Mostly readability. You can achieve the same by positioning multi-line SQL statements aligned like done in the triple-quoted string example.

​StackOverflow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

Definitely. You'll find plenty of examples relying on string concatenation inside braces, while your style has been rather surprising to me.

Anyway, the main point has been about multiple inserts, what we obviously failed to make clear in ticket description as well as early comments. Sorry for that.

[Chris Nelson]

Replying to hasienda:

Replying to ChrisNelson: ...

​StackOverflow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

Definitely. You'll find plenty of examples relying on string concatenation inside braces, while your style has been rather surprising to me. ...

Perhaps emacs's Python mode indents

#py
f('Foo' + \
  ' Bar')

better than

#py
f('Foo'
  ' Bar')

? I'd have to check.

[Chris Nelson]

Replying to ChrisNelson:

... Perhaps emacs's Python mode indents

#py
f('Foo' + \
  ' Bar')

better than

#py
f('Foo'
  ' Bar')

? I'd have to check.

I suppose the definition of "better" is open to discussion. emacs gives me:

f('Foo' + \
      ' Bar')

g('Stuff'
  ' and nonsense')

I rather like

f('Foo' + \
      ' Bar',
  theSecondArg)

[Steffen Hoffmann]

Replying to ChrisNelson:

I suppose the definition of "better" is open to discussion. emacs gives me:

Agreed. At that time it was a minor consent, that SQL statements formatted like

INSERT into table (col1, col2)
VALUES (val1, val2)

improved source readability compared to semantically identical oneliners and line wrapping on PEP8 style, subject to alterations by personal tast, sure. OTOH the '+ \' is allowed but technically obsolete, definitely. Just that.

[Chris Nelson]

Any chance of getting this validated in SQLite? I don't have a system to do it on. I may just close it and hope for the best.

[Chris Nelson]

No feedback in two weeks. I'm going to call this good. We can reopen if it's still an issue.