Closed
Bug 303696
Opened 19 years ago
Closed 18 years ago
Eliminate deprecated Bugzilla::DB routines from process_bug.cgi
Categories
(Bugzilla :: Bugzilla-General, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: wicked, Assigned: batosti)
References
Details
Attachments
(1 file, 6 obsolete files)
|
29.38 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
These lines need to rewritten to use DBI:
process_bug.cgi:185: SendSQL("SELECT delta_ts FROM bugs WHERE bug_id = " .
process_bug.cgi:187: my $delta_ts = FetchOneColumn();
process_bug.cgi:241: SendSQL("SELECT name FROM products INNER JOIN bugs " .
process_bug.cgi:244: $oldproduct = FetchSQLData();
process_bug.cgi:326: SendSQL("SELECT defaultmilestone FROM
products " .
process_bug.cgi:327: "WHERE name = " . SqlQuote($prod));
process_bug.cgi:328: $defaults{'target_milestone'} =
FetchOneColumn();
process_bug.cgi:462: SendSQL("SELECT reporter, assigned_to, qa_contact
FROM bugs
process_bug.cgi:464: ($reporterid, $ownerid, $qacontactid) =
(FetchSQLData());
process_bug.cgi:532: SendSQL("SELECT reporter FROM bugs WHERE bug_id = $dupe");
process_bug.cgi:533: my $reporter = FetchOneColumn();
process_bug.cgi:541: SendSQL("SELECT cclist_accessible FROM bugs WHERE bug_id
= $original");
process_bug.cgi:542: $vars->{'cclist_accessible'} = FetchOneColumn();
process_bug.cgi:640: SqlQuote($str) . " ELSE
'UNCONFIRMED' END";
process_bug.cgi:667: my @open_state = map(SqlQuote($_), OpenStates());
process_bug.cgi:671: SqlQuote($str) .
" ELSE " .
process_bug.cgi:675: $::query .= "bug_status = " . SqlQuote($str);
process_bug.cgi:690: $::query .= "resolution = " . SqlQuote($str);
process_bug.cgi:707:SendSQL("SELECT groups.id, isactive FROM groups INNER JOIN
user_group_map " .
process_bug.cgi:711:while (my ($b, $isactive) = FetchSQLData()) {
process_bug.cgi:734: $::query .= "$field = " .
SqlQuote(trim($cgi->param($field)));
process_bug.cgi:749: SendSQL("SELECT DISTINCT product_id FROM bugs WHERE
bug_id IN (" .
process_bug.cgi:751: $prod_id = FetchOneColumn();
process_bug.cgi:752: $prod_id = undef if (FetchOneColumn());
process_bug.cgi:801: SendSQL("SELECT group_id FROM bug_group_map WHERE bug_id
= " .
process_bug.cgi:803: my ($havegroup) = FetchSQLData();
process_bug.cgi:828: SendSQL("UPDATE longdescs SET isprivate =
$private " .
process_bug.cgi:980: SendSQL("SELECT initialowner FROM components " .
process_bug.cgi:982: $assignee = FetchOneColumn();
process_bug.cgi:986: SendSQL("SELECT initialqacontact FROM components " .
process_bug.cgi:988: $qacontact = FetchOneColumn();
process_bug.cgi:1097: $::query .= "$field = " . SqlQuote($er_time);
process_bug.cgi:1107: $::query .= SqlQuote($cgi->param('deadline'));
process_bug.cgi:1120: SendSQL("SELECT delta_ts, " . join(',', @::log_columns) .
process_bug.cgi:1122: my @row = FetchSQLData();
process_bug.cgi:1131: SendSQL("SELECT $target FROM dependencies WHERE $me =
$i ORDER BY $target");
process_bug.cgi:1133: while (MoreSQLData()) {
process_bug.cgi:1134: push(@list, FetchOneColumn());
process_bug.cgi:1145: my $sql_timestamp = SqlQuote($timestamp);
process_bug.cgi:1152: SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp
WHERE bug_id = $i");
process_bug.cgi:1321: SendSQL("select now()");
process_bug.cgi:1322: $timestamp = FetchOneColumn();
process_bug.cgi:1323: my $sql_timestamp = SqlQuote($timestamp);
process_bug.cgi:1351: SendSQL("DELETE FROM keywords WHERE bug_id = $id");
process_bug.cgi:1356: SendSQL("DELETE FROM keywords
process_bug.cgi:1361: SendSQL("INSERT INTO keywords
process_bug.cgi:1367: SendSQL("SELECT keyworddefs.name
process_bug.cgi:1373: while (MoreSQLData()) {
process_bug.cgi:1374: push(@list, FetchOneColumn());
process_bug.cgi:1383: SendSQL($query);
process_bug.cgi:1387: SendSQL("SELECT resolution FROM bugs WHERE bug_id = $id");
process_bug.cgi:1388: my $resolution = FetchOneColumn();
process_bug.cgi:1390: SendSQL("DELETE FROM duplicates WHERE dupe = $id");
process_bug.cgi:1400: SendSQL("SELECT id, membercontrol
process_bug.cgi:1404: while (MoreSQLData()) {
process_bug.cgi:1405: my ($group, $control) = FetchSQLData();
process_bug.cgi:1422: SendSQL("INSERT INTO bug_group_map (bug_id,
group_id)
process_bug.cgi:1434: SendSQL("DELETE FROM bug_group_map
process_bug.cgi:1454: SendSQL("SELECT who FROM cc WHERE bug_id = $id");
process_bug.cgi:1455: while (MoreSQLData()) {
process_bug.cgi:1456: $oncc{FetchOneColumn()} = 1;
process_bug.cgi:1463: SendSQL("INSERT INTO cc (bug_id, who)
VALUES ($id, $pid)");
process_bug.cgi:1471: SendSQL("DELETE FROM cc WHERE bug_id = $id
AND who = $pid");
process_bug.cgi:1527: SendSQL("delete from dependencies where $me
= $id");
process_bug.cgi:1529: SendSQL("insert into dependencies ($me,
$target) values ($id, $i)");
process_bug.cgi:1559: SendSQL("SELECT DISTINCT groups.id, isactive, " .
process_bug.cgi:1585: while (MoreSQLData()) {
process_bug.cgi:1587: $useringroup, $bugingroup) = FetchSQLData();
process_bug.cgi:1641: SendSQL("INSERT INTO bug_group_map (bug_id,
group_id) VALUES " .
process_bug.cgi:1645: SendSQL("DELETE FROM bug_group_map WHERE
bug_id = $id " .
process_bug.cgi:1742: SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp
WHERE bug_id = $id");
process_bug.cgi:1748: SendSQL("SELECT reporter FROM bugs WHERE bug_id = " .
process_bug.cgi:1750: my $reporter = FetchOneColumn();
process_bug.cgi:1751: SendSQL("SELECT reporter FROM bugs WHERE bug_id = " .
process_bug.cgi:1753: my $isreporter = FetchOneColumn();
process_bug.cgi:1754: SendSQL("SELECT who FROM cc WHERE bug_id = " .
process_bug.cgi:1756: my $isoncc = FetchOneColumn();
process_bug.cgi:1763: SendSQL("INSERT INTO cc (who, bug_id) " .
process_bug.cgi:1773: SendSQL("INSERT INTO duplicates VALUES ($duplicate, " .
|
||
Comment 1•19 years ago
|
||
hum.... I much prefer writing the patch than reviewing this monster. :-p
Assignee: general → LpSolit
Target Milestone: --- → Bugzilla 2.22
|
|
||
Updated•19 years ago
|
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
|
Assignee | |
Comment 2•19 years ago
|
||
|
|
||
Comment 3•19 years ago
|
||
Comment on attachment 202425 [details] [diff] [review] batosti_v1 (double) quotes are fine. No need to change them everywhere. Only remove deprecated routines, that's the goal of this bug! (and I prefer quotes that q{} anyway)
Attachment #202425 -
Flags: review?(LpSolit) → review-
|
|
Assignee | |
Comment 4•19 years ago
|
||
Attachment #202425 -
Attachment is obsolete: true
Attachment #203138 -
Flags: review?(LpSolit)
|
|
||
Comment 5•19 years ago
|
||
Comment on attachment 203138 [details] [diff] [review] batosti_v1_fix A few things before I review this patch more seriously: >+ my $result = $dbh->selectrow_arrayref(q{SELECT reporter, >+ assigned_to, >+ qa_contact >+ FROM bugs >+ WHERE bug_id = ? }, I'm not sure that having one field per line makes the query really more readable. >+my $result = $dbh->selectall_arrayref(q{SELECT groups.id, isactive >+ FROM groups >+ WHERE id IN(?) >+ AND isbuggroup = 1}, This won't work. IN(?) will quote the whole list instead of each element one by one. >+ my $value = trim($cgi->param($field)); >+ trick_taint($value); >+ push @values, $value; Please add parens: push(@values, $value). I know this is correct, but I think it's nice to clearly indicate what is added to the list. For instance: is "push @foo, $bar if ($baz)" equals to "push(@foo, $bar if ($baz))" or "push(@foo, $bar) if ($baz)"? ;) >+ my $sth = $dbh->prepare(q{SELECT DISTINCT product_id >+ FROM bugs >+ WHERE bug_id IN (?)}. >+ $dbh->sql_limit(2), undef, >+ join(',', @idlist)); As I said, this won't work. And keep join() on the previous line. Do you really like reading code like this? > sub LogDependencyActivity { > my ($i, $oldstr, $target, $me, $timestamp) = (@_); >- my $sql_timestamp = SqlQuote($timestamp); >+ my $sql_timestamp = $dbh->quote($timestamp); If your queries are correctly written, you should never have to quote anything. This is done by placeholders. >+ $dbh->do(q{UPDATE bugs >+ SET delta_ts = ? >+ WHERE bug_id = ?}, >+ undef, $sql_timestamp, $i); When you pass several parameters to the query, enclose these parameters in parens. => undef, ($sql_timestamp, $i)); >+ my $isreporter = $dbh->selectrow_array(q{SELECT reporter >+ FROM bugs >+ WHERE bug_id = ? >+ AND reporter = ?}, >+ undef, $duplicate, >+ $reporter); Please keep all variables on the same line, unless the list is really long, which isn't the case here. If you are too far on the right, you are allowed to start the query on a new line, closer to the left.
Attachment #203138 -
Flags: review?(LpSolit) → review-
|
|
Assignee | |
Comment 6•19 years ago
|
||
Attachment #203138 -
Attachment is obsolete: true
Attachment #203448 -
Flags: review?(LpSolit)
|
|
||
Comment 7•19 years ago
|
||
Comment on attachment 203448 [details] [diff] [review] batosti_v1_anotherfix bitrot. Also, make sure no new code has been added meanwhile. Hunk #15 FAILED at 868. Hunk #19 succeeded at 1060 with fuzz 1 (offset 28 lines). Hunk #20 FAILED at 1132. Hunk #27 FAILED at 1622. Hunk #31 succeeded at 1727 with fuzz 1 (offset 98 lines).
Attachment #203448 -
Flags: review?(LpSolit) → review-
|
||
Updated•19 years ago
|
Severity: normal → enhancement
|
|
Assignee | |
Comment 8•18 years ago
|
||
Attachment #203448 -
Attachment is obsolete: true
Attachment #216867 -
Flags: review?(LpSolit)
|
|
||
Comment 9•18 years ago
|
||
Comment on attachment 216867 [details] [diff] [review] batosti_v2 >- my $check_can_enter = >- $dbh->selectrow_array("SELECT 1 FROM bugs >- INNER JOIN products >- ON bugs.product_id = products.id >- WHERE products.name != ? >- AND bugs.bug_id IN >- (" . join(',', @idlist) . ") " . >- $dbh->sql_limit(1), >- undef, $prod); >+ my $check_can_enter = $dbh->selectrow_array( >+ q{SELECT 1 FROM bugs INNER JOIN products >+ ON bugs.product_id = products.id WHERE products.name != ? >+ AND bugs.bug_id IN(} . join(',', @idlist) . q{) } . >+ $dbh->sql_limit(1), undef, $prod); When the SQL query requires more than 2-3 lines, you should respect the usual rules about writing SQL queries, meaning that you should keep the original query, and fix only the indentation. You could as well leave it alone. >+ $defaults{'target_milestone'} = $dbh->selectcol_arrayref( >+ q{SELECT defaultmilestone FROM products >+ WHERE name = ?}, undef, $prod); selectcol_arrayref() returns a reference to an array, not an array itself. Use selectrow_array() instead. >+ my $result = $dbh->selectrow_arrayref( >+ q{SELECT reporter, assigned_to, qa_contact FROM bugs >+ WHERE bug_id = ? }, undef, $bugid); >+ ($reporterid, $ownerid, $qacontactid) = @$result; Get rid of $result and use selectrow_array(): ($reporterid, $ownerid, $qacontactid) = $dbh->selectrow_array(...) >+ $vars->{'cclist_accessible'} = @{$dbh->selectcol_arrayref( >+ q{SELECT cclist_accessible FROM bugs WHERE bug_id = ?}, >+ undef, $original)}; Use selectrow_array(). >- my $sth = $dbh->prepare("UPDATE bugs >- SET bug_status = 'RESOLVED', >- resolution = 'MOVED', >- delta_ts = ? >+ my $sth = $dbh->prepare("UPDATE bugs SET bug_status = 'RESOLVED', >+ resolution = 'MOVED', delta_ts = ? > WHERE bug_id = ?"); Nit: leave it alone. > $::query .= "bug_status = CASE WHEN everconfirmed = 1 THEN " . >- SqlQuote($str) . " ELSE 'UNCONFIRMED' END"; >- } elsif (is_open_state($str)) { >+ "? ELSE 'UNCONFIRMED' END"; >+ push(@values, $str); I don't think this will work. You can only use placeholders in the WHERE part of the SQL query. >- } elsif (is_open_state($str)) { >+ } elsif (IsOpenedState($str)) { IsOpenedState() has been renamed as is_open_state(). Could you *PLEASE* test your patches before posting them?? >- my @open_state = map(SqlQuote($_), BUG_STATE_OPEN); >+ my @open_state = map($dbh->quote($_), OpenStates()); OpenStates() has been replaced by BUG_STATE_OPEN. I'm very surprised that this kind of changes doesn't appear obvious to you. I'm not going to review this patch any further. These last two examples clearly show that you don't test your patches, and that you introduce regressions even when the changes seem obvious. Before posting a new patch, *TEST IT*! And think about the changes you are doing!
Attachment #216867 -
Flags: review?(LpSolit) → review-
|
|
Assignee | |
Comment 10•18 years ago
|
||
Attachment #216867 -
Attachment is obsolete: true
Attachment #217023 -
Flags: review?(LpSolit)
|
|
||
Comment 11•18 years ago
|
||
Comment on attachment 217023 [details] [diff] [review] batosti_v2_fix >Index: process_bug.cgi >+ $vars->{'cclist_accessible'} = @{$dbh->selectrow_array( >+ q{SELECT cclist_accessible FROM bugs WHERE bug_id = ?}, >+ undef, $original)}; selectrow_array already returns either a scalar or an array. Do not write @{}. In ChangeStatus(), you forgot to change: my $cond = SqlQuote($str); > $::query .= "bug_status = CASE WHEN bug_status IN($open_state) THEN " . > $cond . " ELSE bug_status END"; >+ push(@values, ($open_state, $str)); $open_state and $cond are already inserted in $::query directly, so you don't have to add them to @values. >+my $result = $dbh->selectall_arrayref( >+ q{SELECT groups.id, isactive FROM groups WHERE id IN(?) >+ AND isbuggroup = 1}, undef, $grouplist); You cannot use placeholders in IN() as the whole list would be quoted. You have to leave $grouplist in IN(). >+foreach my $row (@$result) { Nit: maybe $groups would be more explicit than $result, and $group would also be more explicit than $row, unless they are already in use. You forgot to change: # Add custom fields data to the query that will update the database. $::query .= "$field = " . SqlQuote(trim($cgi->param($field))). Make sure to remove all SqlQuote() calls from the file! >+ $assignee = DBNameToIdAndCheck(trim($cgi->param('assigned_to'))); >+ $::query .= "assigned_to = ?"; >+ push(@values, $assignee); $assignee is already defined. No need to call DBNameToIdAndCheck() again. >+ $::query .= "$field = ?"; >+ detaint_natural($er_time); "estimated_time" and "remaining_time" are not integers, but floating numbers. detaint_natural() doesn't work here. Use trick_taint() instead. >+ $timestamp = $dbh->selectrow_array(q{SELECT NOW()}); >+ my $sql_timestamp = $dbh->quote($timestamp); Get rid of $sql_timestamp. > foreach my $keyword (@keywordlist) { > if ($keywordaction ne "makeexact") { >+ $dbh->do(q{DELETE FROM keywords WHERE bug_id = ? AND keywordid = ?}, >+ undef, $id, $keyword); > $changed = 1; > } > if ($keywordaction ne "delete") { >+ $dbh->do(q{INSERT INTO keywords (bug_id, keywordid) >+ VALUES (?, ?)}, undef, $id, $keyword); > $changed = 1; > } > } Prepare the two SQL queries outside the FOREACH loop. >+ $query .= " WHERE bug_id = ?"; >+ detaint_natural($id); >+ push(@values, $id); $id is already detained. No need to use detaint_natural() here. >+ my $resolution = $dbh->selectcol_arrayref( >+ q{SELECT resolution FROM bugs WHERE bug_id = ?}, undef, $id); Use selectrow_array(). >+ my $result = $dbh->selectall_arrayref( >+ q{SELECT id, membercontrol FROM groups LEFT JOIN group_control_map >+ ON id = group_id AND product_id = ? WHERE isactive != 0}, >+ undef, $newproduct_id); Nit: this is a pretty long SQL query. You should write it using our usual conventions: SELECT FROM LEFT JOIN ON WHERE Moreover, I would prefer $groups rather than $result, and $group rather than $row. >+ $dbh->do(q{INSERT INTO bug_group_map (bug_id, group_id) >+ VALUES (?, ?)}, undef, $id, $grouptoadd); Prepare it outside the FOREACH loop. >+ $dbh->do(q{DELETE FROM bug_group_map WHERE bug_id = ? >+ AND group_id = ?}, undef, $id, $grouptodel); Same comment as above. >+ my $result = $dbh->selectcol_arrayref(q{SELECT who FROM cc >+ WHERE bug_id = ?}, undef, $id); >+ foreach my $who (@$result) { Nit: $ccs or $cc_list is more explicit than $result. > foreach my $pid (keys %cc_add) { > # If this person isn't already on the cc list, add them > if (! $oncc{$pid}) { >+ $dbh->do(q{INSERT INTO cc (bug_id, who) VALUES (?, ?)}, >+ undef, $id, $pid); > push (@added, $cc_add{$pid}); > $oncc{$pid} = 1; > } Prepare the SQL query outside the FOREACH loop. > foreach my $pid (keys %cc_remove) { > # If the person is on the cc list, remove them > if ($oncc{$pid}) { >+ $dbh->do(q{DELETE FROM cc WHERE bug_id = ? AND who = ?}, >+ undef, $id, $pid); > push (@removed, $cc_remove{$pid}); > $oncc{$pid} = 0; > } Same comment as above. > foreach my $i (@{$deps{$target}}) { >+ $dbh->do(qq{INSERT INTO dependencies ($me, $target) >+ VALUES (?, ?)}, undef, $id, $i); > } Same comment as above. >+ my $sth = $dbh->prepare( >+ qq{SELECT DISTINCT groups.id, isactive, >+ oldcontrolmap.membercontrol, >+ newcontrolmap.membercontrol, >+ CASE WHEN groups.id IN (?) THEN 1 ELSE 0 END, You cannot use placeholders in IN(). Moreover, don't align CASE WHEN with SELECT as it's part of SELECT itself: SELECT DISTINCT foo CASE WHEN bar CASE WHEN baz FROM foo2 LEFT JOIN bar2 ON baz2 WHERE foo3 >+ $sth->execute($grouplist, $oldhash{'product_id'}, >+ $newproduct_id, $id); The number of groups per bug is small, so you can use $selectall_arrayref() directly. >+ $dbh->do(q{INSERT INTO bug_group_map (bug_id, group_id) >+ VALUES (?, ?)}, undef, $id, $groupid); > } elsif ($thisdel) { > push(@DefGroupsRemoved, GroupIdToName($groupid)); >+ $dbh->do(q{DELETE FROM bug_group_map WHERE bug_id = ? >+ AND group_id = ?}, undef, $id, $groupid); Prepare both SQL queries outside the FOREACH loop. >+ $dbh->do(q{UPDATE bugs SET delta_ts = ? WHERE bug_id = ?}, >+ undef, $sql_timestamp, $id); Now that you are using placeholders, you don't need the quotes timestamp. Use $timestamp here and remove $sql_timestamp completely from the code. >+ my $id = $cgi->param('id'); >+ detaint_natural($id); $id is already equals to $cgi->param('id') (you used it above), and is already detainted. So these two lines are useless.
Attachment #217023 -
Flags: review?(LpSolit) → review-
|
|
Assignee | |
Comment 12•18 years ago
|
||
Attachment #217023 -
Attachment is obsolete: true
Attachment #217428 -
Flags: review?(LpSolit)
|
|
||
Comment 13•18 years ago
|
||
Comment on attachment 217428 [details] [diff] [review] batosti_v3 > $::query .= "bug_status = CASE WHEN bug_status IN($open_state) THEN " . > $cond . " ELSE bug_status END"; >+ push(@values, $str); Do not push values into @values if you don't have a corresponding placeholder. $str must not be pushed. >+my $groups = $dbh->selectall_arrayref( >+ qq{SELECT groups.id, isactive FROM groups WHERE id IN($grouplist) >+ AND isbuggroup = 1}, undef); Remove 'undef', and start the second line with WHERE, to make it clearer. >+ $::query .= "$field = ?"; >+ push(@values, $cgi->param($field)); $cgi->param() must be detainted. >+ my $sth_insert = >+ $dbh->prepare(q{INSERT INTO keywords (bug_id, keywordid) >+ VALUES (?, ?)}); > foreach my $keyword (@keywordlist) { > > if ($keywordaction ne "delete") { >- SendSQL("INSERT INTO keywords >- (bug_id, keywordid) VALUES ($id, $keyword)"); > $changed = 1; > } Huh? You forgot to replace this SQL call by $sth_insert->execute(...). >+ $sth = $dbh->prepare(q{DELETE FROM bug_group_map WHERE bug_id = ? >+ AND group_id = ?}); Nit: starting the second line with WHERE would be clearer. >+ my @cc_list = $dbh->selectrow_array(q{SELECT who FROM cc >+ WHERE bug_id = ?}, There are several entries for a given bug ID. You have to use selectcol_arrayref(). >+ my $groups = $dbh->selectall_arrayref( >+ qq{SELECT DISTINCT groups.id, isactive, >+ oldcontrolmap.membercontrol, >+ newcontrolmap.membercontrol, >+ CASE WHEN groups.id IN ($grouplist) THEN 1 ELSE 0 END, >+ CASE WHEN bug_group_map.group_id IS NOT NULL >+ THEN 1 ELSE 0 END >+ FROM groups >+ EFT JOIN group_control_map AS oldcontrolmap EFT -> LEFT I will test the next version of your patch.
Attachment #217428 -
Flags: review?(LpSolit) → review-
|
|
Assignee | |
Comment 14•18 years ago
|
||
Attachment #217428 -
Attachment is obsolete: true
Attachment #217470 -
Flags: review?(LpSolit)
|
|
||
Comment 15•18 years ago
|
||
Comment on attachment 217470 [details] [diff] [review] batosti_v3_fix Looks good. And works correctly. r=LpSolit
Attachment #217470 -
Flags: review?(LpSolit) → review+
|
|
||
Updated•18 years ago
|
Flags: approval?
|
||
Updated•18 years ago
|
Flags: approval? → approval+
|
|
||
Comment 16•18 years ago
|
||
Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.313; previous revision: 1.312 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•