210735 - collectstats.pl broken

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Harri Vartiainen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3.1) Gecko/20030425
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3.1) Gecko/20030425

After updating newest version from CVS (including fix for bug 16009),
collectstats.pl gives following error messages:

./collectstats.pl 
[Thu Jun 26 08:53:53 2003] collectstats.pl: Use of uninitialized value in
pattern match (m//) at Bugzilla/Util.pm line 49.
DBD::mysql::st execute failed: Duplicate entry '32-2003-06-26 00:00:00' for key
1 [for statement ``INSERT INTO series_data (series_id, date, value) VALUES (?,
'2003-06-26', ?)'']) at ./collectstats.pl line 472
        main::CollectSeriesData() called at ./collectstats.pl line 85

 This happens with or without --regenerate switch.

Reproducible: Always

Steps to Reproduce:
1. Run ./checksetup.pl




Redhat Linux 8.0
Perl 5.8.0 with following modules:
Checking for       AppConfig (v1.52)   ok: found v1.52
Checking for             CGI (v2.93)   ok: found v2.93
Checking for    Data::Dumper (any)     ok: found v2.12
Checking for    Date::Format (v2.21)   ok: found v2.22
Checking for             DBI (v1.32)   ok: found v1.35
Checking for      DBD::mysql (v2.1010) ok: found v2.1017
Checking for      File::Spec (v0.82)   ok: found v0.83
Checking for      File::Temp (any)     ok: found v0.13
Checking for        Template (v2.08)   ok: found v2.08
Checking for      Text::Wrap (v2001.0131) ok: found v2001.0929

Comment 1

[Gervase Markham [:gerv]]

Pants.

I'll see if I can figure out what's going on this evening.

Gerv

Comment 2

[Jussi Sirpoma]

I ran collect stats from a cron job. I do not get the output from those scripts
but I'm getting zeros inserted to the series_data table. The data/mining files
seem to be ok. 

So can this be the cause for that?

Comment 3

[Gervase Markham [:gerv]]

The uninitialised value is a problem with Bugzilla/User.pm, from bbaetz' checkin
on the 3rd of June. Basically, he trick_taints something which is undef. CCing him.

The other thing is that you are only allowed to run collectstats.pl once per
day. Yes, this should be documented. [e-mail] - if you only run
collectstats.pl once per day, are you getting stats collected?

Jussi: your problem seems different; please file another bug. Remember, you can
only run collectstats once per day. the data/mining files are a completely
different mechanism - I would not be surprised if they still worked.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

You need to REPLACE (or start a transaction, DELETE for that date, then
INSERT...), rather than error out that way.

The undef warning is yours:

        $::vars->{'user'} =
                      new Bugzilla::User($serieses->{$series_id}->{'creator'});

Apart from '$serieses', if the creator is undef, then that will fail...

(Oh, and don't expect that to log you in, either. We can let Bugzilla::user be a
setter if thats what you need, or just pass it to Bugzilla::Search. Currently
the search code does |Bugzilla->user| to get the user for auth purposes, so
that'll miss anything the creator can see which is protected. Unless that was
the goal...)

Comment 5

[Gervase Markham [:gerv]]

The undef warning is not mine. Careful use of Carp reveals the following line in
User.pm to be the culprit:

 70     # We're checking for validity here, so any value is OK
 71     trick_taint($val);

$val is undef, and trick_taint doesn't like undefs - hence the warning. Creating
a User with a single parameter ought to be valid. Your 3rd of June checkin added
that line of code, I believe.

Re: REPLACE, I was told not to use it as it is MySQL-specific.

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

REPLACE is mysql specific, but in this case we have an alternative (which I
mentioned). If you don't mind dropping existing data, you can use that for mysql
too, but it needs trasactions to be atomic.

Yes, its my code which is calling trick_taint, but thats because you're passing
undef in as the userid, which isn't valid.

> Creating a User with a single parameter ought to be valid

I don't know what that means. I'm also not sure what that code is doing,
post-User.pm.

Comment 7

[Gervase Markham [:gerv]]

> Yes, its my code which is calling trick_taint, but thats because you're passing
> undef in as the userid, which isn't valid.

Oops, sorry, I miscounted. I saw three "shift" calls in the User.pm create
method, and a trick_taint of the third one, and assumed that, as I was passing
in a single parameter, you were trick_tainting something that was always undef.

In fact, the first is the invocant, and the second is added by the new sub, and
the third is, in fact, the userid (although calling it $val didn't make it very
clear.)

Sorry about that.

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

ITs $val because ita cna be the username too, under some evil backwards compat code.

Comment 9

[Harri Vartiainen]

Reply to comment #3:

 There are no error message in my daily.cron result mail.

 But shouldn't it be working with "--rebuild" switch even when collectstats have 
been run earlier?

Comment 10

[Gervase Markham [:gerv]]

> But shouldn't it be working with "--rebuild" switch even when collectstats have 
> been run earlier?

The series data generation runs whether or not this switch is given. This is
because --regenerate regenerates the old stats, not the new ones; the two are
separate (although one is migrated to the other.)

Gerv

Comment 11

[Dave Miller [:justdave]]

*** Bug 215935 has been marked as a duplicate of this bug. ***

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Doh! All my fault. I forgot to get the creator from the DB, and then was
surprised when it was undefined...

Gerv

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 130999 [details] [diff] [review]
Patch v.1

bbaetz: can you review this one-liner?

Gerv

Comment 14

[Gervase Markham [:gerv]]

bbaetz said:

> (Oh, and don't expect that to log you in, either. We can let Bugzilla::user be a
> setter if thats what you need, or just pass it to Bugzilla::Search. Currently
> the search code does |Bugzilla->user| to get the user for auth purposes, so
> that'll miss anything the creator can see which is protected. Unless that was
> the goal...)

I missed this first time round. No, that's not the goal. :-)

I want the code to be able to masquerade as the creator; I assume this requires
a login. So what's the best suggestion of those you gave which achieves that?

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Hmm. We're not really logging in here, so passing it to Bugzilla::Search is
probably the best, I think.

Comment 16

[Gervase Markham [:gerv]]

Attachment: Patch v.2

This one definitely fixes the permissions bug as well - it makes it possible to
pass a user to Bugzilla::Search, so collectstats.pl can pretend to be lots of
different people.

I've tested it, and it seems to work.

Gerv

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 131278 [details] [diff] [review]
Patch v.2

Should be using a local $user instead of $::vars{'user'}. You shouldn't need to
set $::vars for that after this patch; if you do it means that you missed
somewhere.

Comment 18

[Dave Miller [:justdave]]

you've posted a new patch since you requested approval, and obsoleted the one
that was here at the time.  The new patch has not been reviewed yet, so removing
the approval request.

Comment 19

[Dave Miller [:justdave]]

okay, so it has been reviewed. :)  But it waws negative anyway, so more to the
point.

Comment 20

[Gervase Markham [:gerv]]

Attachment: Patch v.3

It still seems to work using a local $user.

Gerv

Comment 21

[chaduv]

The attached patch just kills the undef warning reported, but not the error 
caused by running collectstats twice in one day. Would it be reasonable to 
check for it already having been run once, and fail with a useful message 
rather than the DB query failing and erroring out?

Comment 22

[Gervase Markham [:gerv]]

caduvall: that's a different issue.

Gerv

Comment 23

[Gervase Markham [:gerv]]

Fixed.

Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.47; previous revision: 1.46
done

Gerv

Comment 24

[Gervase Markham [:gerv]]

*** Bug 223711 has been marked as a duplicate of this bug. ***

Comment 25

[Gervase Markham [:gerv]]

*** Bug 211164 has been marked as a duplicate of this bug. ***