303690 - Eliminate deprecated Bugzilla::DB routines from collectstats.pl and whineatnews.pl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

collectstats.pl:145:                SendSQL("SELECT COUNT(bug_status) FROM bugs
WHERE bug_status='$status'");
collectstats.pl:147:                SendSQL("SELECT COUNT(bug_status) FROM bugs
WHERE bug_status='$status' AND product_id=$product_id");
collectstats.pl:150:            push @row, FetchOneColumn();
collectstats.pl:155:                SendSQL("SELECT COUNT(resolution) FROM bugs
WHERE resolution='$resolution'");
collectstats.pl:157:                SendSQL("SELECT COUNT(resolution) FROM bugs
WHERE resolution='$resolution' AND product_id=$product_id");
collectstats.pl:160:            push @row, FetchOneColumn();
collectstats.pl:277:        $and_product = " AND products.name = " .
SqlQuote($product);
collectstats.pl:285:    SendSQL("SELECT " . $dbh->sql_to_days('creation_ts') . "
AS start, " .
collectstats.pl:293:    my ($start, $end, $base) = FetchSQLData();
collectstats.pl:318:            SendSQL("SELECT bug_id FROM bugs $from_product " .
collectstats.pl:325:            while (@row = FetchSQLData()) {
collectstats.pl:351:                SendSQL("SELECT bugs_activity.removed " .
collectstats.pl:362:                if (@row = FetchSQLData()) {
collectstats.pl:365:                    SendSQL("SELECT bug_status FROM bugs
WHERE bug_id = $bug");
collectstats.pl:366:                    $status = FetchOneColumn();
collectstats.pl:374:                SendSQL("SELECT bugs_activity.removed " .
collectstats.pl:384:                if (@row = FetchSQLData()) {
collectstats.pl:387:                    SendSQL("SELECT resolution FROM bugs
WHERE bug_id = $bug");
collectstats.pl:388:                    $status = FetchOneColumn();

whineatnews.pl:41:SendSQL("SELECT bug_id, short_desc, login_name " .
whineatnews.pl:53:while (@row = FetchSQLData()) {

Comment 1

[Tiago Ratto]

Attachment: Patch v1

Comment 2

[Frédéric Buclin]

Comment on attachment 203375 [details] [diff] [review]
Patch v1

>Index: collectstats.pl

>+            my $result;
>+            if ($product eq "-All-") {
>+                $result= $dbh->selectcol_arrayref(qq{
>+                                SELECT COUNT(bug_status)
>+                                  FROM bugs
>+                                 WHERE bug_status = ?},undef,$status);

Doesn't make sense! Use $selectrow_array().


>+                $result= $dbh->selectcol_arrayref(qq{
>+                                SELECT COUNT(bug_status)
>+                                  FROM bugs
>+                                 WHERE bug_status = ?
>+                                   AND product_id = ?},undef,$status,
>+                                                  $product_id);

Same comment here. Moreover:
- add whitespaces after commas;
- enclose the list of parameters in parens;
- move all parameters on the same line:
AND product_id =?},
undef, ($status, $product_id));

This comment applies *everywhere*.

Another thing: instead of having two similar queries, merge them in a single one and add an additional condition if necessary.


>+            if ($product eq "-All-") {
>+                $result= $dbh->selectcol_arrayref(qq{
>+                                SELECT COUNT(resolution)
>+                                 FROM bugs
>+                                WHERE resolution = ?}, undef,
>+                                                  $resolution);
>             } else {
>+                $result = $dbh->selectcol_arrayref(qq{
>+                               SELECT COUNT(resolution)
>+                                 FROM bugs
>+                                WHERE resolution = ?
>+                                  AND product_id = ?}, undef,
>+                                                   $resolution,
>+                                                   $product_id);
>             }

Same comments here as above.


>+    if ($product ne '-All-') {
>+        $and_product = q{AND proucts.name = ? };

s/proucts/products/


>+        $from_product = q{INNER JOIN products
>+                                  ON bugs.product_id = products.id };
>+    }

If you don't understand what I meant above, here is a nice way of doing everything using a single query.


>+    my $query = q{SELECT }.$dbh->sql_to_days('creaton_ts').q{AS start, }. 

This won't work, because there is no whitespace before "AS".
Moreover, add *whitespaces* around ".": " foo " . " bar " . " baz ".


>+                           $dbh->sql_ti_days('current_date').q{ AS end, }.

s/sql_ti_days/sql_to_days/


>+                   WHERE }.$dbh->sql_to_days('creation_ts').qq{ != 'NULL'

We have a PostgreSQL bug here, see bug 316971. Your patch will bitrot again.


There are enough problems so far to stop my review here.


>Index: whineatnews.pl

>+my $query = qq{SELECT bug_id, short_desc, login_name

>+my $sth = $dbh->prepare($query);
>+$sth->execute;
> 
>+while (my $row = $sth->fetchrow_arrayref) {

Get all results directly using $dbh->selectall_arrayref() and then do a loop over the array.

Comment 3

[Tiago Ratto]

Attachment: Patch v2

Comment 4

[Frédéric Buclin]

Comment on attachment 204368 [details] [diff] [review]
Patch v2

>Index: collectstats.pl

>+            if ($product ne '-All-') {
>+                my $sql_product_id = $dbh->quote($product_id);
>+                $query .= qq{ AND product_id = $sql_product_id};
>             }

Don't use $dbh->quote()! Use placeholders! This remark applies at several other places too, including $day later in your patch.


>+    my $sth = $dbh->prepare($query);
>+    $sth->execute;
>+
>+    my ($start, $end, $base) = $sth->fetchrow_array;

Use $dbh->selectrow_array().


>+            $sth = $dbh->prepare($query);
>+
>+            my $result;
>+            $sth->execute;
>+
>+            while (($result) = $sth->fetchrow_array) {
>+                push(@bugs, $result);
>             }

Use $dbh->selectcol_arrayref.


>-                if (@row = FetchSQLData()) {
>-                    $status = $row[0];
>+                my @row = $dbh->selectrow_array($query, undef, $bug);
>+                if (@row) {
>+                    ($status) = @row;

To make things clear, write $bug_status instead of @row (only one element is returned).


>+                @row = $dbh->selectrow_array($query, undef, $bug);
>+                if (@row) {
>+                    ($status) = @row;

Same comment here, but using $bug_resolution.


>+                    ($status) = $dbh->selectrow_array(
>+                                         q{SELECT resolution
>+                                             FROM bugs
>+                                            WHERE bug_id = ?},
>+                                                      undef, $bug);

Fix the indentation of "undef, $bug" (undef should be right aligned with SELECT).



>Index: whineatnews.pl

>+my $query = q{SELECT bug_id, short_desc, login_name
>+                FROM bugs
>+          INNER JOIN profiles
>+                  ON userid = assigned_to
>+               WHERE (bug_status = 'NEW' OR bug_status = 'REOPENED')
>+                 AND }.$dbh->sql_to_days('NOW()').q{ - }.
>+                       $dbh->sql_to_days('delta_ts').q{ > }.
>+                       Param('whinedays').q{
>+            ORDER BY bug_id};

Use placeholders.

Comment 5

[Gabriel Sales de Oliveira]

Attachment: v3-diff

I also removed some trailing spaces...

Comment 6

[Frédéric Buclin]

Comment on attachment 216852 [details] [diff] [review]
v3-diff

>Index: collectstats.pl

>         foreach my $status ('NEW', 'ASSIGNED', 'REOPENED', 'UNCONFIRMED', 'RESOLVED', 'VERIFIED', 'CLOSED') {

>+                $result = $dbh->selectrow_array(
>+                                $query, undef, $status, $product_id);
>             } else {
>+                $result = $dbh->selectrow_array($query, undef, $status);
>             }

Prepare these SQL queries outside the FOREACH loop.


>         foreach my $resolution ('FIXED', 'INVALID', 'WONTFIX', 'LATER', 'REMIND', 'DUPLICATE', 'WORKSFORME', 'MOVED') {

>+                $result = $dbh->selectrow_array(
>+                                $query, undef, $resolution, $product_id);
>             } else {
>+                $result = $dbh->selectrow_array(
>+                                $query, undef, $resolution);
>             }

Same comment here.


>-    my $rows = $dbh->selectall_arrayref("SELECT dupe_of, dupe FROM duplicates");
>+    my $rows = $dbh->selectall_arrayref("SELECT dupe_of, dupe
>+                                           FROM duplicates");

No reason to change this line. It's short enough to remain on the same line.


>+         $and_product = qq{AND products.name = ?};
>+         $from_product = q{INNER JOIN products
>+                           ON bugs.product_id = products.id };

Keep whitespaces around these SQL fragments.


>+     }
>+
>     # Determine the start date from the date the first bug in the

Nit: } is misaligned (you added a whitespace).


>+                   WHERE } . $dbh->sql_to_days('creation_ts') .
>+                         qq{ != 'NULL' $and_product

If I wrote 'IS NOT NULL', that's because there is a good reason for that, see bug 316971. "!= 'NULL'" is syntaxically incorrect. So leave this alone.


>+        $dates = $dbh->selectrow_array($query, undef, $product);
>+    } else {
>+        $dates = $dbh->selectrow_array($query);
>+    }
>+
>+    my ($start, $end, $base) = @$dates;

This doesn't work! selectrow_array() returns an array, not a reference to an array.


>+            $query = qq{SELECT bug_id FROM bugs $from_product
>+                         WHERE bugs.creation_ts < from_days(?)
>+                           AND bugs.creation_ts >= from_days(?)

from_days() is not a valid function in PostgreSQL, see bug 316971. Leave $dbh->sql_from_days() alone.


I didn't review further as you brought back too many regressions.

Comment 7

[Gabriel Sales de Oliveira]

Attachment: v4-fix previous mistakes

Sorry about the last patch i just fixed the old issues and don't see the update...

Comment 8

[Frédéric Buclin]

Comment on attachment 217035 [details] [diff] [review]
v4-fix previous mistakes

>Index: collectstats.pl

>+        my $status_sql = q{SELECT COUNT(bug_status) 
>+                               FROM bugs 
>+                              WHERE bug_status = ?};

Nit: COUNT(*) is faster than COUNT(bug_status). Also, fix the indentation.


>+        my $reso_sql   = q{SELECT COUNT(resolution) 
>+                             FROM bugs 
>+                            WHERE resolution = ?};

Nit: same comment here.


>+        if ($product eq "-All-") {
>+            $sth_status = $dbh->prepare($status_sql);
>+            $sth_reso   = $dbh->prepare($reso_sql);
>+        } else {
>+            $status_sql .= q{ AND product_id = ?};
>+            $reso_sql   .= q{ AND product_id = ?};
>+            $sth_status = $dbh->prepare($status_sql);
>+            $sth_reso   = $dbh->prepare($reso_sql);
>+        }

$sth_status = $dbh->prepare($status_sql) and $sth_reso = $dbh->prepare($reso_sql) are common to both conditions, so move them out of the IF block.


>+            if ($product eq "-All-") {
>+                $sth_status->execute($status);
>             } else {
>+                $sth_status->execute($status, $product_id);
>             }

Better is to have a @values array and to push required values in it, and then do a single call to $sth_status->execute(@values) without using a IF block again.


>+            push (@row, $sth_status->fetchrow_array);

Nit: even better, write:
my $count = $dbh->selectrow_array($sth_status, undef, @values);
push(@row, $count);

This way, you combine ->execute() and ->fetchrow_array() in a single call. As $sth_status has already been prepared, it will not be prepared again.


>+                $sth_reso->execute($resolution);
>             } else {
>+                $sth_reso->execute($resolution, $product_id);
>             }
>+            push (@row, $sth_reso->fetchrow_array);

Same comment here.


>+                $dbh->sql_to_days("'1970-01-01'") .
>+                 qq{FROM bugs $from_product

Missing whitespace before FROM. This will probably make the SQL call to crash.


>+    my ($start, $end, $base);
>+    if ($product ne '-All-') {
>+        ($start, $end, $base) = $dbh->selectrow_array($query, undef, 
>+                                                      $product);
>+    } else {
>+        ($start, $end, $base) = $dbh->selectrow_array($query);
>+    }

Nit: Probably a better idea would be to use @values again, which would avoid this IF block.


>+            if ($product ne '-All-') {
>+                $result = $dbh->selectcol_arrayref($query, $product);
>+            } else {
>+                $result = $dbh->selectcol_arrayref($query);
>+            }

Nit: same comment here.


>+            foreach my $bug_id (@$result) {
>+                push(@bugs, $bug_id);
>             }

Write: push(@bugs, @$result). And $bug_ids would be better than $result.


>             for my $bug (@bugs) {
>+                $query = qq{
>+                        SELECT bugs_activity.removed 
>+                          FROM bugs_activity 
>+                    INNER JOIN fielddefs 
>+                            ON bugs_activity.fieldid = fielddefs.fieldid 
>+                         WHERE fielddefs.name = 'bug_status' 
>+                           AND bugs_activity.bug_id = ? 
>+                           AND bugs_activity.bug_when >= } .
>+                           $dbh->sql_from_days($day) .
>+                      " ORDER BY bugs_activity.bug_when " .
>+                          $dbh->sql_limit(1);

It doesn't make sense to redefine $query for each bug. Moreover, you should prepare the SQL query outside the FOR loop.


>+                my $bug_status = $dbh->selectrow_array($query, undef, $bug);
>+                if ($bug_status) {
>+                    $status = $bug_status;

$status is now useless as you already have $bug_status. Or said otherwise, $bug_status should be called $status.


>+                $query = qq{
>+                       SELECT bugs_activity.removed 
>+                         FROM bugs_activity 
>+                   INNER JOIN fielddefs 
>+                           ON bugs_activity.fieldid = fielddefs.fieldid 
>+                        WHERE fielddefs.name = 'resolution' 
>+                          AND bugs_activity.bug_id = ? 
>+                          AND bugs_activity.bug_when >= } . 
>+                          $dbh->sql_from_days($day) .
>+                   " ORDER BY bugs_activity.bug_when " .
>+                         $dbh->sql_limit(1);

Move and prepare $query outside the FOR loop.


>+                if ($bug_resolution) {
>+                    $status = @$bug_resolution;

Use $resolution or $bug_resolution instead of $status here. Using the same variable name is confusing.



>Index: whineatnews.pl

> my $dbh = Bugzilla->dbh;

Nit: In order to use Bugzilla->dbh, you should explicitly "use Bugzilla;", as globals.pl is going to be removed very soon now.

Comment 9

[Gabriel Sales de Oliveira]

Attachment: v5-version

you send a very nice solution... :)

Comment 10

[Frédéric Buclin]

Comment on attachment 217288 [details] [diff] [review]
v5-version

>Index: collectstats.pl

>+        my $sth_status;
>+        my $sth_reso;
>+        
>+        $sth_status = $dbh->prepare($status_sql);
>+        $sth_reso   = $dbh->prepare($reso_sql);

Nit: write:
 my $sth_status = $dbh->prepare($status_sql);
 my $sth_reso   = $dbh->prepare($reso_sql);

This was, you don't need the first two lines.


>         foreach my $status ('NEW', ...
>+            @values = ();
>+            push (@values, $status);

Nit: combine these two lines in a single one and write: @values = ($status);


>         foreach my $resolution ('FIXED', ...
>+            @values = ();
>+            push (@values, $resolution);

Same comment here.


>     if ($product ne '-All-') {
>+        $and_product = q{AND products.name = ?};
>+        $from_product = q{INNER JOIN products 
>+                          ON bugs.product_id = products.id};
>+    }
>+
>+    my @values = $product if ($product ne '-All-');

Move @values in the first IF block:

my @values = ();
if ($product ne '-All-') {
    ...
    push(@values, $product);
}

Isn't that cleaner? :-D


>+    my ($start, $end, $base) = $dbh->selectrow_array($query, undef, 
>+                                                     @values);

Nit: write it on one line.


>+            $query = qq{SELECT bug_id FROM bugs $from_product 
>+                         WHERE bugs.creation_ts < } . 
>+                         $dbh->sql_from_days($day - 1) . 
>+                         " AND bugs.creation_ts >= " . 
>+                         $dbh->sql_from_days($day - 2) . 
>+                        $and_product . " ORDER BY bug_id";

Nit: please don't mix qq{} and "". Choose one of them.
Nit #2: move FROM bugs $from_product on its own line.


>+            my $bug_ids = $dbh->selectcol_arrayref($query, @values);

Must be ($query, undef, @values)


>+            foreach my $bug_id (@$bug_ids) {
>+                push(@bugs, @$bug_id);
>             }

As I said in my previous review, write: push(@bugs, @$bug_ids).


>+                my $bug_status = $dbh->selectrow_array($sth_bug, undef, 
>+                                                       'bug_status', $bug);
>+                if ($bug_status) {
>+                    $status = $bug_status;
>                 } else {

Nit: replace $bug_status by $status. This way, you don't need to write $status = $bug_status.


>+                    $status = $dbh->selectrow_array(q{
>+                                SELECT bug_status 
>+                                  FROM bugs 
>+                                 WHERE bug_id = ?}, undef, $bug);

Prepare it outside the FOR loop.


>+                my $bug_resolution = $dbh->selectrow_array($sth_bug, undef, 
>+                                                         'resolution', $bug);
>+                if ($bug_resolution) {
>+                    $resolution = @$bug_resolution;
>                 } else {

Same comment as for the status.


>+                    $resolution = $dbh->selectrow_array(q{
>+                                SELECT resolution 
>+                                  FROM bugs 
>+                                 WHERE bug_id = ?}, undef, $bug);

Here too, prepare it outside the FOR loop.



>Index: whineatnews.pl

>+my $result = $dbh->selectall_arrayref($query, undef, 'NEW', 'REOPENED');

Nit: $bugs is more explicit than $result.


>+foreach my $row (@$result) {

Nit: $bug is more explicit than $row.

Comment 11

[Gabriel Sales de Oliveira]

Attachment: v5-fix

fixed version.

Comment 12

[Frédéric Buclin]

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

The patch works correctly, on both MySQL and PostgreSQL. r=LpSolit

Comment 13

[Frédéric Buclin]

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.48; previous revision: 1.47
done
Checking in whineatnews.pl;
/cvsroot/mozilla/webtools/bugzilla/whineatnews.pl,v  <--  whineatnews.pl
new revision: 1.21; previous revision: 1.20
done