303693 - Eliminate deprecated Bugzilla::DB routines from describe*.cgi, duplicates.cgi, quips.cgi, report.cgi, request.cgi and showdependency*.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

describecomponents.cgi:89:SendSQL("SELECT name, initialowner, initialqacontact,
description FROM " .
describecomponents.cgi:91:while (MoreSQLData()) {
describecomponents.cgi:93:      FetchSQLData();

describekeywords.cgi:40:SendSQL("SELECT keyworddefs.name, keyworddefs.description,
describekeywords.cgi:50:while (MoreSQLData()) {
describekeywords.cgi:51:    my ($name, $description, $bugs) = FetchSQLData();

duplicates.cgi:235:    SendSQL($query->getSQL());
duplicates.cgi:237:    while (MoreSQLData()) {
duplicates.cgi:241:            $short_desc, $bug_status, $resolution) =
FetchSQLData();

quips.cgi:48:    SendSQL("SELECT quipid, userid, quip, approved FROM quips");
quips.cgi:52:    while (MoreSQLData()) {
quips.cgi:53:        my ($quipid, $userid, $quip, $approved) = FetchSQLData();
quips.cgi:63:            SendSQL("SELECT login_name FROM profiles WHERE userid =
$userid");
quips.cgi:64:            $users->{$userid} = FetchOneColumn();
quips.cgi:83:    SendSQL("INSERT INTO quips (userid, quip, approved) VALUES " .
quips.cgi:84:           '(' . $userid . ', ' . SqlQuote($comment) . ', ' .
$approved . ')');
quips.cgi:91:    SendSQL("SELECT quipid, approved FROM quips");
quips.cgi:94:    while (MoreSQLData()) {
quips.cgi:95:        my ($quipid, $approved) = FetchSQLData();
quips.cgi:108:    SendSQL("UPDATE quips SET approved = 1 WHERE quipid IN (" .
quips.cgi:110:    SendSQL("UPDATE quips SET approved = 0 WHERE quipid IN (" .
quips.cgi:125:    SendSQL("SELECT quip FROM quips WHERE quipid = $quipid");
quips.cgi:126:    $vars->{'deleted_quip'} = FetchSQLData();
quips.cgi:127:    SendSQL("DELETE FROM quips WHERE quipid = $quipid");

report.cgi:149:SendSQL($query);
report.cgi:165:while (MoreSQLData()) {
report.cgi:166:    my ($row, $col, $tbl) = FetchSQLData();

request.cgi:149:                                         
SqlQuote($cgi->param('requester'))));
request.cgi:155:                            SqlQuote($cgi->param('requestee'))));
request.cgi:195:        push(@criteria, "flagtypes.name = " . SqlQuote($form_type));
request.cgi:243:    SendSQL($query);
request.cgi:245:    while (MoreSQLData()) {
request.cgi:246:        my @data = FetchSQLData();
request.cgi:265:    SendSQL("SELECT DISTINCT(name) FROM flagtypes ORDER BY name");
request.cgi:266:    push(@types, FetchOneColumn()) while MoreSQLData();

showdependencygraph.cgi:124:    SendSQL("SELECT blocked, dependson FROM
dependencies");
showdependencygraph.cgi:126:    while (MoreSQLData()) {
showdependencygraph.cgi:127:        my ($blocked, $dependson) = FetchSQLData();
showdependencygraph.cgi:139:        SendSQL("SELECT blocked, dependson
showdependencygraph.cgi:142:        while (MoreSQLData()) {
showdependencygraph.cgi:143:            my ($blocked, $dependson) = FetchSQLData();
showdependencygraph.cgi:168:    SendSQL("SELECT bug_status, resolution,
short_desc FROM bugs " .
showdependencygraph.cgi:170:    ($stat, $resolution, $summary) = FetchSQLData();

showdependencytree.cgi:152:        SendSQL("SELECT 1,
showdependencytree.cgi:169:         $bug->{'assignee_email'}) = FetchSQLData();
showdependencytree.cgi:185:    SendSQL("  SELECT $relationship
showdependencytree.cgi:191:    push(@dependencies, FetchOneColumn()) while
MoreSQLData();

Comment 1

[Frédéric Buclin]

maybe this bug should be splitted to avoid bitrot as much as possible.

Comment 2

[Teemu Mannermaa (:wicked)]

(In reply to comment #1)
> maybe this bug should be splitted to avoid bitrot as much as possible.

I don't think this is needed because 1) there's not so many changed lines and 2)
these files seems to not change so often.

Comment 3

[Teemu Mannermaa (:wicked)]

Attachment: Rewrite deprecated SQL code, V1

Comment 4

[Frédéric Buclin]

Comment on attachment 199294 [details] [diff] [review]
Rewrite deprecated SQL code, V1

>Index: quips.cgi

>-    SendSQL("INSERT INTO quips (userid, quip, approved) VALUES " .
>-           '(' . $userid . ', ' . SqlQuote($comment) . ', ' . $approved . ')');
>+    $dbh->do("INSERT INTO quips (userid, quip, approved) VALUES (?, ?, ?)",
>+             undef, ($userid, $comment, $approved));

Shouldn't you trick_taint $comment?



>Index: request.cgi

>         push(@criteria, $dbh->sql_istrcmp('requesters.login_name',
>-                                          SqlQuote($cgi->param('requester'))));
>+                        $dbh->quote($cgi->param('requester'))));

Same comment here for $cgi->param('requester').


>             push(@criteria, $dbh->sql_istrcmp('requestees.login_name',
>-                            SqlQuote($cgi->param('requestee'))));
>+                            $dbh->quote($cgi->param('requestee'))));

and here.


>+    my $flagtypes = $dbh->selectall_arrayref(
>+                         "SELECT DISTINCT(name) FROM flagtypes ORDER BY name");

Nit: selectcol_arrayref()



>Index: showdependencytree.cgi

>+        ($bug->{'exists'},
>+         $bug->{'status'},
>+         $bug->{'summary'},
>+         $bug->{'milestone'},
>+         $bug->{'assignee_id'},
>+         $bug->{'assignee_email'}) = $dbh->selectrow_array(
>+                "SELECT 1,
>                         bug_status, 
>                         short_desc, 
>                         $milestone_column, 
>                         assignee.userid, 
>                         assignee.login_name
>+                   FROM bugs
>              INNER JOIN profiles AS assignee
>                      ON bugs.assigned_to = assignee.userid
>                   WHERE bugs.bug_id = $id");

write WHERE bugs.bug_id = ?", undef, $id);



>+    my $deps = $dbh->selectall_arrayref(
>+            "  SELECT $relationship
>                  FROM dependencies 
>                 WHERE $bug_type = $id 
>              ORDER BY $relationship");

Same comment here.


>     my @dependencies = ();
>+    foreach my $dep (@$deps) {
>+        push(@dependencies, @$dep)
>+    }
>     return @dependencies;

Couldn't you write "return @$deps" instead? This would be a nice cleanup.

Comment 5

[Teemu Mannermaa (:wicked)]

Attachment: Rewrite deprecated SQL code, V1.5

Comment 6

[Frédéric Buclin]

Comment on attachment 199955 [details] [diff] [review]
Rewrite deprecated SQL code, V1.5

>Index: describekeywords.cgi

>+my $keyword_defs = $dbh->selectall_arrayref(
>+                     q{SELECT keyworddefs.name, keyworddefs.description,
>+                              COUNT(keywords.bug_id)
>+                         FROM keyworddefs
>+                    LEFT JOIN keywords
>+                           ON keyworddefs.id = keywords.keywordid } .
>+           $dbh->sql_group_by('keyworddefs.id',
>+                              'keyworddefs.name, keyworddefs.description') .
>+                   " ORDER BY keyworddefs.name");
> 
> my @keywords;
> 
>+foreach my $keyword_def (@$keyword_defs) {
>+    my ($name, $description, $bugs) = @$keyword_def;
>    
>     push (@keywords, { name => $name, 
>                        description => $description,

Nit: instead of doing this, use {'Slice' => {}} and write
"COUNT(keywords.bug_id) AS bugcount".
Then you can write "$vars->{'keywords'} = $keyword_defs" directly.



>Index: quips.cgi

>     foreach my $quipid (@quipids) {
>         my $userid = $quips->{$quipid}{'userid'};
>         if ($userid && not defined $users->{$userid}) {
>+            ($users->{$userid}) = $dbh->selectrow_array(
>+                            "SELECT login_name FROM profiles WHERE userid = ?",
>+                             undef, $userid);
>         }
>     }

You should prepare the DBI call before entering the loop (my $sth =
$dbh->prepare(...)).



>Index: request.cgi

>     my @types = ("all");
>+    my $flagtypes = $dbh->selectcol_arrayref(
>+                         "SELECT DISTINCT(name) FROM flagtypes ORDER BY name");
>     
>-    $vars->{'types'} = \@types;
>+    $vars->{'types'} = \@$flagtypes;

err... don't forget "all" already defined in @types. Write: push(@types,
@$flagtypes) instead.



>Index: showdependencygraph.cgi

>     foreach my $id (@stack) {
>+        my $dependencies = $dbh->selectall_arrayref(
>+              q{SELECT blocked, dependson
>+                  FROM dependencies
>+                 WHERE blocked = ? or dependson = ?}, undef, ($id, $id));

You should prepare the DBI call outside the loop.


>+    ($stat, $resolution, $summary) = $dbh->selectrow_array(
>+        q{SELECT bug_status, resolution, short_desc
>+            FROM bugs
>+           WHERE bugs.bug_id = ?}, undef, $k);

2 things here: first, prepare the DBI call outside the loop. And secondly
(nit), add "my" in front of ($stat, $resolution, $summary) which will allow you
to remove the useless declarations preceding this.

Comment 7

[Teemu Mannermaa (:wicked)]

Attachment: Rewrite deprecated SQL code, V2

All review comments addresses. When I was testing my last patch I was wondering why request.cgi didn't show all option for flags. Didn't realize it was because of my patch. Ooops.. :)

Comment 8

[Frédéric Buclin]

Comment on attachment 200846 [details] [diff] [review]
Rewrite deprecated SQL code, V2

>Index: showdependencytree.cgi

Nit: you should define $dbh in each subroutine where it's used. This can be fixed on checkin. r=LpSolit

Comment 9

[Frédéric Buclin]

Checking in describecomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/describecomponents.cgi,v  <--  describecomponents.cgi
new revision: 1.33; previous revision: 1.32
done
Checking in describekeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/describekeywords.cgi,v  <--  describekeywords.cgi
new revision: 1.17; previous revision: 1.16
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.49; previous revision: 1.48
done
Checking in quips.cgi;
/cvsroot/mozilla/webtools/bugzilla/quips.cgi,v  <--  quips.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v  <--  report.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.28; previous revision: 1.27
done
Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.45; previous revision: 1.44
done
Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.36; previous revision: 1.35
done