Status

()

defect
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: harriv-bmo, Assigned: gerv)

Tracking

unspecified
Bugzilla 2.18
x86
Linux
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Pants.

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

Gerv
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?
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. harriv-bmo@prosys.fi - 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
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...)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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.
> 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
ITs $val because ita cna be the username too, under some evil backwards compat code.
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?
> 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

Blocks: 211164
*** Bug 215935 has been marked as a duplicate of this bug. ***
Posted patch Patch v.1 (obsolete) — Splinter Review
Doh! All my fault. I forgot to get the creator from the DB, and then was
surprised when it was undefined...

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

bbaetz: can you review this one-liner?

Gerv
Attachment #130999 - Flags: review?(bbaetz)
Attachment #130999 - Flags: review?(bbaetz) → review+
Flags: approval?
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
Hmm. We're not really logging in here, so passing it to Bugzilla::Search is
probably the best, I think.
Posted patch Patch v.2 (obsolete) — Splinter Review
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
Attachment #130999 - Attachment is obsolete: true
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.
Attachment #131278 - Flags: review-
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.
Flags: approval?
okay, so it has been reviewed. :)  But it waws negative anyway, so more to the
point.
Posted patch Patch v.3Splinter Review
It still seems to work using a local $user.

Gerv
Attachment #131278 - Attachment is obsolete: true
Attachment #131467 - Flags: review?(bbaetz)
Attachment #131467 - Flags: review?(bbaetz) → review+
Flags: approval?
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?
caduvall: that's a different issue.

Gerv
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 2.18
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
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
*** Bug 223711 has been marked as a duplicate of this bug. ***
*** Bug 211164 has been marked as a duplicate of this bug. ***
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.