upgrade from 2.16.3 to 2.17.6 via cvs partially fails on "Migrating old chart data into database"

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Reporting/Charting
P1
major
RESOLVED FIXED
14 years ago
5 years ago

People

(Reporter: Ralf Hauser, Assigned: gerv)

Tracking

unspecified
Bugzilla 2.18
x86
Linux
Bug Flags:
approval +

Details

(Whiteboard: [fixed in 2.17.7], URL)

Attachments

(1 attachment)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031129
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031129

Migrating old chart data into database ...
[Mon Dec  1 10:07:29 2003] checksetup.pl: DBD::mysql::db do failed: Duplicate
entry '1-15-0-0' for key 1 at ./checksetup.pl line 3832.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.



Expected Results:  
no duplicate key

no clue whether that will hurt me down the road!
(Reporter)

Comment 1

14 years ago
happened while running .checksetup.pl
(Assignee)

Comment 2

14 years ago
This happens because one day long ago you ran collectstats.pl twice in one day.
The new code doesn't like that at all.

I hadn't envisaged this scenario - it makes fixing this issue imperative.

Gerv
Assignee: zach → gerv
Severity: minor → major
Status: UNCONFIRMED → NEW
Component: Installation & Upgrading → Reporting/Charting
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
(Assignee)

Comment 3

14 years ago
Created attachment 139258 [details] [diff] [review]
Patch v.1

We need to delete before insertion both in the migration and the general
collectstats.pl case, to avoid SQL errors from duplicate entries (in the first
instance) and running collectstats.pl twice in a day (in the second.)

Gerv
(Assignee)

Comment 4

14 years ago
Comment on attachment 139258 [details] [diff] [review]
Patch v.1

Kiko, you've looked at charting code before. Can you review this for 2.17.7?

Gerv
Attachment #139258 - Flags: review?(kiko)
(Assignee)

Comment 5

14 years ago
Adding nomination for 2.17.7.

Gerv
Status: NEW → ASSIGNED
Whiteboard: [wanted for 2.17.7]?

Comment 6

14 years ago
Comment on attachment 139258 [details] [diff] [review]
Patch v.1

Instead of deleting, would it be simpler in the first case to simply check for
duplicate lines in the file?

In the second case, would it make sense to check if a run has been made today,
and if so, update instead of insert?

I guess it doesn't matter *that* much -- just make sure that this is the best
approach.

Any reason why you're using dbh->quote in the prepare call and not in the
execute call in collectstats.pl? You do it differently in checksetup.pl.. I'd
expect you used a ? marked and called

$deletesth->execute($series_id, $today);

(I also thought dbh->quote() was called automatically when you used ?s, btw).
(Assignee)

Comment 7

14 years ago
> Instead of deleting, would it be simpler in the first case to simply check for
> duplicate lines in the file?

I can't see a way it would be _simpler_, given that the current additional code
is eight lines. I don't know whether people would more often set their clocks
forward by mistake, or back by mistake, so we can't really tell whether a "keep
the first" or "keep the last" policy would be better.

> In the second case, would it make sense to check if a run has been made today,
> and if so, update instead of insert?

Deleting is arguably faster; there's not much in it. Checking and updating or
inserting requires writing 3 SQL statements; this version requires 2. 

> Any reason why you're using dbh->quote in the prepare call and not in the
> execute call in collectstats.pl? 

Yes; series_id is internally generated and guaranteed to be a number.

> (I also thought dbh->quote() was called automatically when you used ?s, btw).

I thought that, but I've had taint problems before. Perhaps bbaetz can clarify.

Gerv
$dbh->quote() escapes values.

placeholders may internally quote, or may use a db function so that they're
passe dindirectly as parameters, saving parsing overhead

The taint checks check all parameters going into a DBI function. placeholders
are parmaters. Thus you need to ->quote them.

Yes, this sucks a fair bit, and one day I'd like to remove that, and instead ban
$ within the SQL statement itself. But we can't do that yet. (Given the
internals of the DBI<->DBD glue, its not really possible to only check the
statement).

The other argument is that tainting isn't only checking for SQL injection
attaks, but that we should validate it first for security/can the user see the
bug/etc.

_HOWEVER_, in your case you are actually inserting $today into the SQL directly,
so the quote is needed. Unless I'm missing something
Attachment #139258 - Flags: review?(kiko) → review+
Flags: approval+
Whiteboard: [wanted for 2.17.7]? → [wanted for 2.17.7]
> Yes, this sucks a fair bit, and one day I'd like to remove that, and instead
> ban $ within the SQL statement itself. But we can't do that yet.

I doubt you'll ever be able to do this, at least in a 100% ban/testing suite
sense.  My new admineditor code uses placeholders, and still has to do this
sometimes, because it's generic, and the set of fields you're dealing with is
*NOT* fixed, nor is the table.
(Assignee)

Comment 10

14 years ago
Fixed.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.258; previous revision: 1.257
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.34; previous revision: 1.33
done

Gerv
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
True, but we'd annotate it like we do the template stuff
Whiteboard: [wanted for 2.17.7] → [fixed in 2.17.7]
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.