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

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect, P1)

Description

[Ralf Hauser]

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!

Comment 1

[Ralf Hauser]

happened while running .checksetup.pl

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[Gervase Markham [:gerv]]

Attachment: 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

Comment 4

[Gervase Markham [:gerv]]

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

Comment 5

[Gervase Markham [:gerv]]

Adding nomination for 2.17.7.

Gerv

Comment 6

[Christian Reis]

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

Comment 7

[Gervase Markham [:gerv]]

> 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

Comment 8

[Bradley Baetz (:bbaetz)]

$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

Comment 9

[Matthew Tuck [:CodeMachine]]

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

Comment 10

[Gervase Markham [:gerv]]

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

Comment 11

[Bradley Baetz (:bbaetz)]

True, but we'd annotate it like we do the template stuff