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!
happened while running .checksetup.pl
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
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
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
Adding nomination for 2.17.7. Gerv
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).
> 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
> 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.
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
True, but we'd annotate it like we do the template stuff