Closed Bug 227155 Opened 20 years ago Closed 19 years ago
upgrade from 2
.16 .3 to 2 .17 .6 via cvs partially fails on "Migrating old chart data into database"
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
Assignee: zach → gerv
Severity: minor → major
Status: UNCONFIRMED → NEW
Component: Installation & Upgrading → Reporting/Charting
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
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
Attachment #139258 - Flags: review?(kiko)
Adding nomination for 2.17.7. Gerv
Status: NEW → ASSIGNED
Whiteboard: [wanted for 2.17.7]?
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
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.
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
Closed: 19 years ago
Resolution: --- → FIXED
True, but we'd annotate it like we do the template stuff
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.