303692 - Eliminate deprecated Bugzilla::DB routines from sanitycheck.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

sanitycheck.cgi:105:    SendSQL("UPDATE bugs SET votes = 0");
sanitycheck.cgi:106:    SendSQL("SELECT bug_id, SUM(vote_count) FROM votes " .
sanitycheck.cgi:109:    while (@row = FetchSQLData()) {
sanitycheck.cgi:114:        SendSQL("UPDATE bugs SET votes = $votes{$id} WHERE
bug_id = $id");
sanitycheck.cgi:127:    SendSQL("UPDATE groups SET last_changed = NOW() " .
$dbh->sql_limit(1));
sanitycheck.cgi:136:    SendSQL("SELECT userid FROM profiles");
sanitycheck.cgi:137:    while ((my $id) = FetchSQLData()) {
sanitycheck.cgi:153:    SendSQL("SELECT MAX(last_changed) FROM groups WHERE
last_changed < NOW() - " .
sanitycheck.cgi:155:    (my $cutoff) = FetchSQLData();
sanitycheck.cgi:157:    SendSQL("SELECT COUNT(*) FROM user_group_map");
sanitycheck.cgi:158:    (my $before) = FetchSQLData();
sanitycheck.cgi:160:    SendSQL("SELECT userid FROM profiles " .
sanitycheck.cgi:162:            "AND refreshed_when < " . SqlQuote($cutoff) . " " .
sanitycheck.cgi:165:    while ((my $id) = FetchSQLData()) {
sanitycheck.cgi:167:        PushGlobalSQLState();
sanitycheck.cgi:168:        SendSQL("DELETE FROM user_group_map WHERE " .
sanitycheck.cgi:170:        SendSQL("UPDATE profiles SET refreshed_when = 0
WHERE userid = $id");
sanitycheck.cgi:171:        PopGlobalSQLState();
sanitycheck.cgi:174:    SendSQL("SELECT COUNT(*) FROM user_group_map");
sanitycheck.cgi:175:    (my $after) = FetchSQLData();
sanitycheck.cgi:257:    SendSQL("SELECT bug_id FROM bugs
sanitycheck.cgi:262:    while (MoreSQLData()) {
sanitycheck.cgi:263:        push (@list, FetchOneColumn());
sanitycheck.cgi:355:        SendSQL("SELECT DISTINCT $refertable.$referfield" .
($keyname ? ", $refertable.$keyname" : '') . " " .
sanitycheck.cgi:362:        while (MoreSQLData()) {
sanitycheck.cgi:363:            my ($value, $key) = FetchSQLData();
sanitycheck.cgi:514:        SendSQL("SELECT DISTINCT $refertable.$referfield1,
$refertable.$referfield2" . ($keyname ? ", $refertable.$keyname" : '') . " " .
sanitycheck.cgi:523:        while (MoreSQLData()) {
sanitycheck.cgi:524:            my ($value1, $value2, $key) = FetchSQLData();
sanitycheck.cgi:564:SendSQL("SELECT userid, login_name FROM profiles");
sanitycheck.cgi:566:while (my ($id,$email) = (FetchSQLData())) {
sanitycheck.cgi:584:SendSQL("SELECT bug_id, votes, keywords FROM bugs " .
sanitycheck.cgi:591:while (@row = FetchSQLData()) {
sanitycheck.cgi:602:SendSQL("SELECT bug_id, SUM(vote_count) FROM votes " .
sanitycheck.cgi:605:while (@row = FetchSQLData()) {
sanitycheck.cgi:628:SendSQL("SELECT id, name FROM keyworddefs");
sanitycheck.cgi:629:while (@row = FetchSQLData()) {
sanitycheck.cgi:641:SendSQL("SELECT bug_id, keywordid FROM keywords ORDER BY
bug_id, keywordid");
sanitycheck.cgi:644:while (@row = FetchSQLData()) {
sanitycheck.cgi:665:SendSQL("SELECT keywords.bug_id, keyworddefs.name " .
sanitycheck.cgi:676:    my ($b, $k) = FetchSQLData();
sanitycheck.cgi:713:            SendSQL("UPDATE bugs SET keywords = " .
SqlQuote($k) .
sanitycheck.cgi:733:    SendSQL("SELECT DISTINCT bugs.bug_id " .
sanitycheck.cgi:739:    while (@row = FetchSQLData()) {
sanitycheck.cgi:767:my @open_states = map(SqlQuote($_), OpenStates());
sanitycheck.cgi:800:    SendSQL("SELECT COUNT( $field ) FROM $table WHERE $field
> NOW()");
sanitycheck.cgi:801:    my $c = FetchOneColumn();
sanitycheck.cgi:817:SendSQL("SELECT COUNT(product_id) FROM group_control_map
WHERE " .
sanitycheck.cgi:828:my $c = FetchOneColumn();
sanitycheck.cgi:869:SendSQL("SELECT bug_id " .
sanitycheck.cgi:874:while (@row = FetchSQLData()) {

Comment 1

[Tiago Ratto]

Attachment: V1 - Changed deprecated MySql functions to dbh.

I changed all the deprecated MySql functions to the dbh standard. Sanitycheck was tested in every way to ensure it's working as desired. Best regards.

Comment 2

[Frédéric Buclin]

Comment on attachment 201030 [details] [diff] [review]
V1 - Changed deprecated MySql functions to dbh.

>-    SendSQL("SELECT bug_id, SUM(vote_count) FROM votes " .
>-            $dbh->sql_group_by('bug_id'));
>+    my $sth = $dbh->prepare(q{SELECT bug_id,
>+                                     SUM(vote_count)
>+                                FROM votes
>+                               GROUP BY bug_id});

You have to use $dbh->sql_group_by()! Moreover, leave SUM() on the same line as bug_id.


>+    while (my $row = $sth>fetchrow_hashref()) {

$sth->, not $sth>.


>+        $votes{$row->{'bug_id'}} = $row->{'vote_count'};
>     }
>     foreach my $id (keys %votes) {
>+        $dbh->do(q{UPDATE bugs SET votes = ? WHERE bug_id = ?},
>+                 $votes{$id},$id);
>     }

The last two loops can be combined in a single one. This %votes hash is completely useless. Moreover, don't use $dbh->do() in a loop. Prepare your SQL query out of the loop and then execute it.


>+    my $time = $dbh->sql_interval('30 minute');
>+
>+    my $list = $dbh->selectcol_arrayref(qq{SELECT bug_id
>+                                             FROM bugs 
>+                                            WHERE (lastdiffed IS NULL
>+                                               OR lastdiffed < delta_ts)
>+                                              AND delta_ts < now() 
>+                                                  - $time
>+                                            ORDER BY bug_id});

Nit: Fix the indentation so that (lastdiffed IS NULL OR lastdiffed < delta_ts) is on one line.


>+        while (my $reference = $sth->fetchrow_hashref) {

Using a hash makes no sense. This makes the code over-complicated.


>+            if (($keyname) && ($keyname eq 'bug_id')) {
>+                $alert .= " (bug '".BugLink($reference->{"$keyname"}).
>+                          "'";
>+            } elsif (($keyname)) {
>+                
>+                $alert .= " ($keyname = ".$reference->{"$keyname"}.")";
>             }

I don't like this "optimization". Keep the old way: if ($keyname) { if () {} else {} }


> sub DoubleCrossCheck {

All subroutines should redefine $dbh.


>+        while (my $reference = $sth->fetchrow_hashref) {

Could you please stop using hashes? This makes the code really unreadable.

Same comment as above about the "optimization" done on tests.


>+my $sth = $dbh->prepare(q{SELECT userid,login_name FROM profiles});

Nit: missing whitespace after the comma.


OK, I stop the review here. Remove these awful hashes and resubmit a patch. This definitely makes the code too painful to read.

Comment 3

[Tiago Ratto]

Attachment: V2 - Changed from hashes to arrays and fixed minor problems.

Comment 4

[Frédéric Buclin]

Comment on attachment 201435 [details] [diff] [review]
V2 - Changed from hashes to arrays and fixed minor problems.

bitrotten (what a surprise!)

Comment 5

[Tiago Ratto]

Attachment: Patch V3 - Isn't bitrotten anymore !

Comment 6

[Frédéric Buclin]

Comment on attachment 203255 [details] [diff] [review]
Patch V3 - Isn't bitrotten anymore !

>+    my $query = q{UPDATE bugs SET votes=? WHERE bug_id = ?};
>+    while (my $row = $sth->fetchrow_arrayref()) {
>+        my ($votesum,$bug_id) = @$row;
>+        $dbh->prepare($query,$votesum,$bug_id);
>     }

You have to prepare your query outside the loop and then use $dbh->execute() in the loop. Don't pass parameters to $dbh->prepare() except the query itself! Parameters should be passed to $dbh->execute().


>+    my $time = $dbh->sql_interval('30 minute');

The list of parameters $dbh->sql_interval() takes has changed. So this patch no longer applies.


Bitrotten.

Comment 7

[Tiago Ratto]

Attachment: Patch - V4

Comment 8

[Tiago Ratto]

Attachment: Patch - V4

Sorry, selected the wrong file last time...

Comment 9

[Frédéric Buclin]

Comment on attachment 204694 [details] [diff] [review]
Patch - V4

>@@ -103,16 +103,18 @@ $template->put_header("Bugzilla Sanity C
>+    my $sth = $dbh->prepare(q{SELECT bug_id, SUM(vote_count)
>+                                FROM votes }. $dbh->sql_group_by('bug_id'));
>+    $sth->execute();
>     my %votes;
>+    while (my ($id, $v) = $sth->fetchrow_array) {
>         $votes{$id} = $v;
>     }
>+    my $query = q{UPDATE bugs SET votes = ? WHERE bug_id = ?};
>+    $sth = $dbh->prepare($query);
>     foreach my $id (keys %votes) {
>+        $sth->execute($votes{$id}, $id);
>     }

All this could be simplified either
1) by using selectall_hashref() and using bug_id as the key, or
2) by updating the 'bugs' table in the first WHILE loop instead of doing it twice.

The first suggestion would be my preferred one, but due to the large number of rows returned (= the number of bugs in the DB!), this would be too large amount of data in memory. So the second suggestion is the one to use here. In other words, this means that you don't need %votes anymore.


>@@ -195,22 +197,23 @@ if (defined $cgi->param('rescanallBugMai
>+    my $time = $dbh->sql_interval(30, 'MINUTE');
>+    
>+    my $list = $dbh->selectcol_arrayref(qq{
>+                                        SELECT bug_id
>+                                          FROM bugs 
>+                                         WHERE (lastdiffed IS NULL
>+                                            OR lastdiffed < delta_ts)
>+                                           AND delta_ts < now() - $time
>+                                      ORDER BY bug_id});

Nit: please don't align OR with WHERE and AND as OR is "internal" to the conditions in parens.
I would prefer something like:

WHERE (lastdiffed IS NULL
       OR lastdiffed < delta_ts)
  AND delta_ts < now() - $time


>+         
>+    Status(@$list . ' bugs found with possibly unsent mail.');

Please write scalar(@$list).


>@@ -564,9 +578,12 @@ if ($offervotecacherebuild) {
>+$sth = $dbh->prepare(q{SELECT id, name FROM keyworddefs});
>+$sth->execute;
>+
>+while (my @keywords = $sth->fetchrow_array) {
>+    my ($id, $name) = @keywords;

I don't expect the number of keywords to be very large, so you should catch them all at once using selectall_arrayref().


>@@ -576,12 +593,16 @@ while (@row = FetchSQLData()) {
>+$sth = $dbh->prepare(q{SELECT bug_id, keywordid
>+                         FROM keywords
>+                        ORDER BY bug_id,
>+                              keywordid});

Nit: please let keywordid on the same line as ORDER BY bug_id.


>@@ -601,18 +622,22 @@ if (defined $cgi->param('rebuildkeywordc
>+my $query = q{SELECT keywords.bug_id, keyworddefs.name
>+                FROM keywords
>+               INNER JOIN keyworddefs
>+                  ON keyworddefs.id = keywords.keywordid
>+               INNER JOIN bugs
>+                  ON keywords.bug_id = bugs.bug_id
>+               ORDER BY keywords.bug_id,
>+                     keyworddefs.name};

Please align SELECT FROM INNER JOIN and ORDER BY to the right:
    SELECT
      FROM
INNER JOIN
        ON
  ORDER BY

and move keyworddefs.name on the previous line.


>@@ -736,8 +762,10 @@ sub DateCheck {
>+    my $query = qq{SELECT COUNT($field)
>+                     FROM $table
>+                    WHERE $field > NOW()};
>+    my ($c) = @{$dbh->selectcol_arrayref($query)};

Don't use selectcol_arrayref() when the result is a single scalar!! Use fetchrow_array().


>+$query = qq{
>+     SELECT COUNT(product_id) 
>+       FROM group_control_map 
>+      WHERE membercontrol NOT IN(}. CONTROLMAPNA .
>+            q{, } . CONTROLMAPSHOWN . q{, } . CONTROLMAPDEFAULT .
>+            q{, } . CONTROLMAPMANDATORY .q{)
>+         OR othercontrol NOT IN(} . CONTROLMAPNA .
>+            q{, } . CONTROLMAPSHOWN . q{, } . CONTROLMAPDEFAULT .
>+            q{, } . CONTROLMAPMANDATORY . q{)
>+         OR ((membercontrol != othercontrol)
>+        AND (membercontrol != } . CONTROLMAPSHOWN . q{)
>+        AND ((membercontrol != } . CONTROLMAPDEFAULT . q{)
>+         OR (othercontrol = } . CONTROLMAPSHOWN . q{)))};

It would be cleaner to write something like:
my $groups = join(", ", (CONTROLMAPNA, CONTROLMAPSHOWN, CONTROLMAPDEFAULT, CONTROLMAPMANDATORY));
and then use $groups in $query.


>+
>+$sth = $dbh->prepare($query);
>+$sth->execute;
>+
>+my ($c) = $sth->fetchrow_array;

Write my $c = $dbh->selectrow_array($query);


>@@ -805,13 +839,14 @@ Status("Checking for unsent mail");
>+my $time = $dbh->sql_interval(30, 'MINUTE');
>+$sth = $dbh->prepare(qq{SELECT bug_id FROM bugs 
>+                         WHERE (lastdiffed IS NULL OR lastdiffed < delta_ts)
>+                           AND delta_ts < now() - $time
>+                         ORDER BY bug_id});

Move "FROM bugs" on its own line:
SELECT bug_id
  FROM bugs
 WHERE ...


>+$sth->execute;
>+  
>+while (my ($id) = $sth->fetchrow_array) {
>     push(@badbugs, $id);
> }

Here the number of rows returned by the query should be very small so you should catch them all at once using selectcol_arrayref().

Comment 10

[Gabriel Sales de Oliveira]

Attachment: fixed version

fixed patch.

Comment 11

[Frédéric Buclin]

Comment on attachment 216134 [details] [diff] [review]
fixed version

>-            if (!$exceptions{$value}) {
>-                my $alert = "Bad value "$value" found in $refertable.$referfield";
>-                if ($keyname) {
>-                    if ($keyname eq 'bug_id') {
>-                        $alert .= ' (bug ' . BugLink($key) . ')';
>-                    }
>-                    else {
>-                        $alert .= " ($keyname == '$key')";
>-                    }
>+            next if $exceptions{$value};
>+            my $alert = "Bad value "$value" found in $refertable.$referfield";
>+            if ($keyname) {
>+                if ($keyname eq 'bug_id') {
>+                    $alert .= ' (bug ' . BugLink($key) . ')';
>+                } else {
>+                    $alert .= " ($keyname == '$key')";
>                 }
>                 Alert($alert);
>                 $has_bad_references = 1;

Alert() and $has_bad_references are not in the "if ($keyname)" block.


>+        my $sth = $dbh->prepare($query);
>+        $sth->execute;
>+
>+        while (my ($value1, $value2, $key) = $sth->fetchrow_array) {
>             my $alert = "Bad values "$value1", "$value2" found in " .
>                 "$refertable.$referfield1 / $refertable.$referfield2";

As we are catching only bad values, I don't expect the list to be long. selectall_arrayref() seems appropriate here.


>-    if ($v != 0) {
>+    if ($v !=0) {

Nit: don't remove the whitespace after '='.


>+my $keywords = $dbh->selectall_hashref(q{SELECT id, name 
>+                                            FROM keyworddefs}, 'id');
>+
>+foreach my $id (keys %$keywords) {
>     if ($keywordids{$id}) {
>         Alert("Duplicate entry in keyworddefs for id $id");
>     }
>     $keywordids{$id} = 1;
>+    if ($keywords->{$id}->{'name'} =~ /[\s,]/) {

Don't use selectall_hashref() here. It's slow and make the code harder to write. Use selectall_arrayref() instead.


>-SendSQL("SELECT bug_id, keywordid FROM keywords ORDER BY bug_id, keywordid");
>+$sth = $dbh->prepare(q{SELECT bug_id, keywordid
>+                         FROM keywords
>+                        ORDER BY bug_id, keywordid});

Nit: aligned ORDER BY with SELECT and FROM:
  SELECT
    FROM
ORDER BY

This appears in several other places too.


>+            $dbh->do(q{UPDATE bugs 
>+                          SET keywords = ? 
>+                        WHERE bug_id = ?}, undef, $k, $b);

This SQL query is used inside a loop. For performance reasons, this SQL query must be prepared outside this loop.


>+    my $sth = $dbh->prepare(qq{SELECT DISTINCT bugs.bug_id
>+                                 FROM $middlesql 
>+                                ORDER BY bugs.bug_id});
>+    $sth->execute;
>     
>     my @badbugs = ();
>     
>+    while (my ($id) = $sth->fetchrow_array) {
>         push (@badbugs, $id);
>     }

Improve this code by using selectcol_arrayref: my $badbugs = $dbh->selectcol_arrayref(...).


>+    my $query = qq{SELECT COUNT($field)
>+                     FROM $table
>+                    WHERE $field > NOW()};
>+    $sth = $dbh->prepare($query);
>+    $sth->execute;
>+    my $c = $sth->fetchrow_array;

As said in my previous review, all this can be replaced by a single use of selectrow_array().


>+$query = qq{
>+     SELECT COUNT(product_id) 
>+       FROM group_control_map 
>+      WHERE membercontrol NOT IN( $groups )
>+         OR othercontrol NOT IN( $groups )
>+         OR ((membercontrol != othercontrol)
>+        AND (membercontrol != } . CONTROLMAPSHOWN . q{)
>+        AND ((membercontrol != } . CONTROLMAPDEFAULT . q{)
>+         OR (othercontrol = } . CONTROLMAPSHOWN . q{)))};

The indentation is wrong! The last two AND are part of the OR condition, so you should have something like:

WHERE ...
   OR ...
   OR (...
       AND ...
       AND (...
            OR ...
           )
      )

The last two ")" can be on the same line as the last OR.


>+my $bug_ids = $dbh->selectcol_arrayref(qq{
>+                  SELECT bug_id 
>+                    FROM bugs 
>+                   WHERE (lastdiffed IS NULL OR lastdiffed < delta_ts)
>+                     AND delta_ts < now() - $time
>+                ORDER BY bug_id});
> 
>+foreach my $id (@$bug_ids) {
>     push(@badbugs, $id);
> }

This is suboptimal. Simply write $badbugs = $dbh->selectcol_arrayref(...).

Comment 12

[Gabriel Sales de Oliveira]

Attachment: v5-fix

$sth_update is the best way to prepare the query outside the loop?

i fix the other issues.

Comment 13

[Frédéric Buclin]

Comment on attachment 216635 [details] [diff] [review]
v5-fix

>+    while (my ($id, $v) = $sth->fetchrow_array) {
>+        $dbh->do(q{UPDATE bugs SET votes = ? WHERE bug_id = ?},
>+                     undef, ($v, $id));
>     }

This SQL query must be prepared outside the loop.


>+my $keywords = $dbh->selectall_arrayref(q{SELECT id, name 
>+                                            FROM keyworddefs});
>+
>+foreach my $id (@$keywords) {
>+    my ($id, $name) = @$id;

Oops! Don't use $id twice. As each row of @$keywords is a keyword, use $keyword instead of the first $id.


>+    if (@$badbugs) {

Nit: write: if (scalar(@$badbugs)).


>+if (@$badbugs > 0) {
>     Alert("Bugs that have changes but no mail sent for at least half an hour: " .

Same comment here.

Comment 14

[Gabriel Sales de Oliveira]

Attachment: v5-fix-fix

fixed.

Comment 15

[Frédéric Buclin]

Comment on attachment 216667 [details] [diff] [review]
v5-fix-fix

Looks good. Works well (maybe because my DB has no error ;)). r=LpSolit

Comment 16

[Frédéric Buclin]

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.112; previous revision: 1.111
done