Opened 5 years ago

Closed 4 years ago

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

Reported by: Owned by: Ryan J Ollos Chris Nelson high TracJsGanttPlugin major Steffen Hoffmann

### Description (last modified by 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

diff --git a/tracjsganttplugin/0.11/tracjsgantt/tracpm.py b/tracjsganttplugin/0.
index 0f0c4ba..d5a90d7 100644
 a class TicketRescheduler(Component): 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.

### comment:1 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)

### comment:2 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)

### comment:3 Changed 5 years ago by 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
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
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


### comment:4 Changed 5 years ago by 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?

### comment:5 Changed 5 years ago by 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


### comment:6 follow-up:  9 Changed 5 years ago by 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?

### comment:7 Changed 5 years ago by 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.api.themeenginesystem = enabled
themeengine.web_ui.themeenginemodule = enabled
timingandestimationplugin.* = enabled
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


### comment:8 Changed 5 years ago by 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.

### comment:9 in reply to:  6 ; follow-ups:  10  11 Changed 5 years ago by Steffen Hoffmann

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.

### comment:10 in reply to:  9 Changed 5 years ago by Jun Omae

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


### comment:11 in reply to:  9 Changed 5 years ago by Chris Nelson

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.

### comment:12 Changed 4 years ago by Chris Nelson

Priority: normal → high normal → major

### comment:13 Changed 4 years ago by Chris Nelson

Status: new → accepted

### comment:14 Changed 4 years ago by 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
Last edited 4 years ago by Chris Nelson (previous) (diff)

Considering

• ## tracjsganttplugin/0.11/tracjsgantt/tracpm.py

diff --git a/tracjsganttplugin/0.11/tracjsgantt/tracpm.py b/tracjsganttplugin/0.
index 0f0c4ba..d5a90d7 100644
 a class TicketRescheduler(Component): 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.

### comment:16 in reply to:  description ; follow-up:  17 Changed 4 years ago by Chris Nelson

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?

Last edited 4 years ago by Chris Nelson (previous) (diff)

### comment:17 in reply to:  16 Changed 4 years ago by Chris Nelson

... 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.

### comment:18 follow-up:  19 Changed 4 years ago by 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


### comment:19 in reply to:  18 Changed 4 years ago by Chris Nelson

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

THANK YOU!

### comment:20 Changed 4 years ago by Chris Nelson

Summary: Cross-db compatibility issues and possibility of SQL injection → Cross-db compatibility issue (SQLite can't insert multiple rows at once)

### comment:21 Changed 4 years ago by 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)

### comment:22 Changed 4 years ago by 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.

### comment:23 Changed 4 years ago by 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.

### comment:24 in reply to:  15 ; follow-up:  25 Changed 4 years ago by Steffen Hoffmann

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.

### comment:25 in reply to:  24 ; follow-up:  26 Changed 4 years ago by Chris Nelson

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.

### comment:26 in reply to:  25 ; follow-up:  27 Changed 4 years ago by Chris Nelson

... 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)


### comment:27 in reply to:  26 Changed 4 years ago by Steffen Hoffmann

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.

### comment:28 Changed 4 years ago by 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.

### comment:29 Changed 4 years ago by Chris Nelson

Resolution: → fixed accepted → closed

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

### Modify Ticket

Change Properties