Throudg using bugzilla, and maintaining our site it has become apparent to me that there is a lot of duplicate data in database that makes it difficult to maintain. For example, the components.value field exists not only in the components table, but also as a reference to the component in the bugs table. It would make sence if all tables that are referenced from other tables should be done so through use of a key. In the example outlined above of the components table being referenced from the bugs table, the components table should be augmented to have a primary key added to it with the bugs table having the component column replaced by a component_id column. This would mean that when the componets.value field was updated for whatever reason, no other field in the database need to change to keep the database current. I have started implementing these changes, and will submit patches as a followup to this bug report. I hope that this code will help ease the maintenance of existing databases. Regared,

The attachment 06 [details]/23/00 05:59 has not been fully tested, but appears to be working fine here. If anyone has any querys about the patch, please contact me. The next stage is to do the same with the products table.

I would do this for the versions table as well.

My main concern with a patch of this size and complexity is that it was made against the 2.10 version, and not against the current CVS version. There's been a LOT of changes made to 2.11 already since 2.10 was tarballed. Without a patch against the current CVS version, someone will likely be spending an entire day to put this in by hand to make sure it doesn't break anything.... Then again, perhaps that's what landfill is for. :)

I have completed the changes to use primary keys for products and components, along with the automatic database update script changes in checksetup.pl. These changes have not yet been fully testes, I presently know of only one minor bug at the moment. The columns in buglist display the id number and not the text version of products and components. If there are more bugs, then I am sorry, I'm new to bugzilla. I just hope that the changes are useful. I have also merged the changes with the head of CVS, so integration should be a lot easier. I'll post the diffs as an aattachment in a mo. Regards,

This looks both scary and very useful. We'll try to get this patch up on landfill as soon as we can to let people pound on it (as well as test the checksetup changes needed for the schema change). Unfortunately, we're backlogged on this stuff due to work constraints. Stay tuned...

Chris--another patch for landfill (btw you still need to give the ropes on populating that damn thing). We'll probably want to go over this one kind of carefully given the schema related stuff.

*** Bug 3725 has been marked as a duplicate of this bug. ***

this is part of a larger schema change i'm currently designing

ooh, this slipped through somewhere... getting it on the radar again.

Taking all of cyeh's Bugzilla bugs.

Unfortunately, this patch has suffered from an extreme case of bit-rot. I'm gonna start looking into how much/if it is salvagable. Mat Spencer, if you happen to have a current version of this laying around (against at least 2.14), I'd love to see it :)

Unless Mat comes forward with a new patch, I'll start working on this.

Comment on attachment 48817 [details] [diff] [review] partial patch This partial patch has the checksetup.pl logic in it and should allow searching for and changing bugs. I haven't touched anything else yet. I wanted to get this up here so people could look it over and possibly tell me if I'm doing something wrong (I also thought it'd be nice to have a copy stored someplace I could retrieve it if something happened to my working copy :). Please note that this is far from being ready for use in a production environment.

Comment on attachment 49062 [details] [diff] [review] add sanitycheck.cgi, editproducts.cgi, editcomponents.cgi This still isn't ready for production, but I think it's getting close.

> product_id meduimint not null, Eh? Also I noticed you converted the names from varchar to tinytext. This may well be the same size (no idea), but I believe varchar is more DB independent and consistent with what exists elsewhere.

In the current patch it looks like the update code in checksetup.pl only matches on component name rather than on component name and product_id. The relevant update line should probably change to something like: + $dbh->do("UPDATE bugs SET component_id = $component_id " . + "WHERE component = " . $dbh->quote($component) . + "AND product_id = " . $dbh->quote($product_id)); with appropriate code to get product_id from the components table beforehand.

The latest patch is now available on landfill for testing... http://landfill.tequilarista.org/bz43600/

Another thing ... it is very important you rename the primary key "id" rather than "xxx_id", as it is for keywords and soon resolutions. This is because the edit*.cgi code that will eventually support this stuff expects it to be. That simplifies the code.

OK, v6 fixes all known issues and (I believe) incorporates everyone's suggestions that have been mentioned so far. Ready for the infamous review :)

Comment on attachment 54007 [details] [diff] [review] patch v6: Ready for review Needs work - still contains "meduimint".

We are currently trying to wrap up Bugzilla 2.16. We are now close enough to release time that anything that wasn't already ranked at P1 isn't going to make the cut. Thus this is being retargetted at 2.18. If you strongly disagree with this retargetting, please comment, however, be aware that we only have about 2 weeks left to review and test anything at this point, and we intend to devote this time to the remaining bugs that were designated as release blockers.

Another revision... fixes some errors I found and updates to current cvs. Patch applied to http://landfill.tequilarista.org/bz43600/

This bug blocks a "we would like for 2.16 bug" so retargeting... it very well may get bumped back to 2.18 again.

Comment on attachment 59004 [details] [diff] [review] patch v8: More love There has got to be some way to break up this patch to make it easier to review. As it is, this patch is insanely huge. Hmm, maybe I should review one script a day. Anyway, here are my comments on the checksetup.pl portion: There some more uses in checksetup.pl of the old fields in the products and components tables, for example in the block starting at line 2188, in the block starting at line 2216, and the code that creates an initial test product and component starting at line 1716. >-$sth = $dbh->prepare("SELECT product FROM products"); >+$sth = $dbh->prepare("SELECT description FROM products"); Not sure why description is used here, but the return values are never used, so this probably doesn't matter. Still, product_id may be trivially more performant. --- Btw- This patch could also use a refresh to the tip.

> There has got to be some way to break up this patch to make it easier > to review. As it is, this patch is insanely huge. I know, but I don't know of any way to make it easier to digest... it all has to be done at the same time... > There some more uses in checksetup.pl of the old fields in the products > and components tables, for example in the block starting at line 2188, in > the block starting at line 2216 Those blocks really have to use the old names. They only get run if you are upgrading from a really old install and if that's the case, then the part that converts everything to use the new field names at the end of the file hasn't run yet. > and the code that creates an initial test > product and component starting at line 1716 Oops, that's fixed > Not sure why description is used here It really has to be something that's gonna exist in old version and new versions... if I used product_id, then when checksetup.pl was run for the first time when upgrading, it would either error out (most likely) or create the TestProduct (even though there's real data). > Btw- This patch could also use a refresh to the tip. Done :)

schema changes + huge patch + 2.16 freeze mode = this is gonna have to wait for checkin. If anyone wants to continue reviewing it, go for it, but it won't be checked in until 2.16 ships at this point.

Comment on attachment 67206 [details] [diff] [review] Patch v9: Minor checksetup changes Walkthrough, this probably doesn't apply any more, blah, blah, etc ;) >Index: bug_form.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v >retrieving revision 1.86 >diff -u -r1.86 bug_form.pl >@@ -75,6 +75,8 @@ > delta_ts, > sum(votes.count) > from bugs left join votes using(bug_id) >+left join products on bugs.product_id = products.id >+left join components on bugs.component_id = components.id > where bugs.bug_id = $id > group by bugs.bug_id"; no, not a left join. A bug must always be in a valid product, and left joins are slow. As long as there is a sanity check for this, of course. > >Index: buglist.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v >retrieving revision 1.156 >diff -u -r1.156 buglist.cgi >--- buglist.cgi 20 Jan 2002 01:44:36 -0000 1.156 >+++ buglist.cgi 31 Jan 2002 03:28:51 -0000 >@@ -162,6 +162,8 @@ > unshift(@supptables, > ("profiles map_assigned_to", > "profiles map_reporter", >+ "products map_products", >+ "components map_components", See, you didn't need an outer join here ;) >Index: checksetup.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v >retrieving revision 1.115 >diff -u -r1.115 checksetup.pl >--- checksetup.pl 30 Jan 2002 14:14:11 -0000 1.115 >+++ checksetup.pl 31 Jan 2002 03:29:07 -0000 >@@ -1845,8 +1853,6 @@ > AddField('products', 'milestoneurl', 'tinytext not null'); > AddField('components', 'initialqacontact', 'tinytext not null'); > AddField('components', 'description', 'mediumtext not null'); >-ChangeFieldType('components', 'program', 'varchar(64)'); >- > Don't you need to leave that in if the field exists, so the thelater conversion code will work? ... Oh, I see - you've done that later. NM then >-ChangeFieldType ('bugs', 'product', 'varchar(64) not null'); >-ChangeFieldType ('components', 'program', 'varchar(64)'); >-ChangeFieldType ('products', 'product', 'varchar(64)'); >-ChangeFieldType ('versions', 'program', 'varchar(64) not null'); Ditto, although maybe thats not needed here since all the comparisons will still work. This part isn't handled later. > # 2000-01-16 Added a "keywords" field to the bugs table, which > # contains a string copy of the entries of the keywords table for this > # bug. This is so that I can easily sort and display a keywords >@@ -2595,6 +2584,67 @@ > # Add a field for the attachment ID to the bugs_activity table, so installations > # using the attachment manager can record changes to attachments. > AddField("bugs_activity", "attach_id", "mediumint null"); >+ >+# 2001-10-?? jake@acutex.net >+# Use integer ID's for products and components. >+# http://bugzilla.mozilla.org/show_bug.cgi?id=43600 >+if (GetFieldDef("products", "product")) { >+ # Pick up changes that may have to be done to older versions and used >+ # to be further up in the file. >+ ChangeFieldType ('products', 'product', 'varchar(64) not null'); >+ >+ AddField("products", "id", "smallint not null auto_increment primary key"); >+ AddField("components", "product_id", "smallint not null"); >+ AddField("versions", "product_id", "smallint not null"); >+ AddField("milestones", "product_id", "smallint not null"); >+ AddField("bugs", "product_id", "smallint not null"); >+ AddField("attachstatusdefs", "product_id", "smallint not null"); >+ print "Updating database to use product ID's in Bugzilla's tables.\n"; >+ my $sth = $dbh->prepare("SELECT id, product FROM products"); >+ $sth->execute; >+ while (my ($product_id, $product) = $sth->fetchrow_array()) { >+ $dbh->do("UPDATE components SET product_id = $product_id " . >+ "WHERE program = " . $dbh->quote($product)); >+ $dbh->do("UPDATE versions SET product_id = $product_id " . >+ "WHERE program = " . $dbh->quote($product)); >+ $dbh->do("UPDATE milestones SET product_id = $product_id " . >+ "WHERE product = " . $dbh->quote($product)); >+ $dbh->do("UPDATE bugs SET product_id = $product_id " . >+ "WHERE product = " . $dbh->quote($product)); >+ $dbh->do("UPDATE attachstatusdefs SET product_id = $product_id " . >+ "WHERE product = " . $dbh->quote($product)); >+ } Do you need to deal with errors in the tables here? ie UPDATE foo SET product_id=1 WHERE product_id=0; Or just leave them as is? For these tables, it may not be needed, but it probably is for bugs. >+ AddField("components", "id", "smallint not null auto_increment primary key"); >+ AddField("bugs", "component_id", "smallint not null"); >+ print "Updating the database to use component ID's in the bugs table.\n"; no ' here.... >+ $sth = $dbh->prepare("SELECT id, value, product_id FROM components"); >+ $sth->execute; >+ while (my ($component_id, $component, $product_id) = $sth->fetchrow_array()) { >+ $dbh->do("UPDATE bugs SET component_id = $component_id " . >+ "WHERE component = " . $dbh->quote($component) . >+ " AND product_id = $product_id"); >+ } Here you probably do need a sanity check, else things won't show up. Maybe just print a warning? We do recommend that people run sanity checks before and after upgrading, but this one won't have been picked up. When I test this, I'll create an invalid bug, and see what happens to it. >+ print "Fixing Indexes and Uniqueness.\n"; >+ $dbh->do("ALTER TABLE milestones DROP PRIMARY KEY"); milestones don't have a primary key - you need to drop the UNIQUE index instead. <skipping edit* for the moment> >Index: globals.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v >retrieving revision 1.134 >diff -u -r1.134 globals.pl >--- globals.pl 30 Jan 2002 14:14:12 -0000 1.134 >+++ globals.pl 31 Jan 2002 03:29:16 -0000 >@@ -91,12 +91,12 @@ > # Joe Robins, 7/5/00 > $::superusergroupset = "9223372036854775807"; > >-#sub die_with_dignity { >-# my ($err_msg) = @_; >-# print $err_msg; >-# confess($err_msg); >-#} >-#$::SIG{__DIE__} = \&die_with_dignity; >+sub die_with_dignity { >+ my ($err_msg) = @_; >+ print $err_msg; >+ confess($err_msg); >+} >+$::SIG{__DIE__} = \&die_with_dignity; > You probably don't want that in the final patch... > sub ConnectToDatabase { > my ($useshadow) = (@_); >@@ -460,7 +460,9 @@ > > sub GenerateVersionTable { > ConnectToDatabase(); >- SendSQL("select value, program from versions order by value"); >+ SendSQL("SELECT versions.value, products.name FROM versions " . >+ "LEFT JOIN products ON products.id = versions.product_id " . >+ "ORDER BY versions.value"); again, why the left join? Versions w/o products are invalid, and there should be a sanity check for them. > my @line; > my %varray; > my %carray; >@@ -472,7 +474,9 @@ > push @{$::versions{$p1}}, $v; > $varray{$v} = 1; > } >- SendSQL("select value, program from components order by value"); >+ SendSQL("SELECT components.name, products.name FROM components " . >+ "LEFT JOIN products ON products.id = components.product_id " . >+ "ORDER BY components.name"); ditto. >@@ -582,7 +586,9 @@ > > if ($dotargetmilestone) { > # reading target milestones in from the database - matthew@zeroknowledge.com >- SendSQL("SELECT value, product FROM milestones ORDER BY sortkey, value"); >+ SendSQL("SELECT milestones.value, products.name FROM milestones " . >+ "LEFT JOIN products ON products.id = milestones.product_id " . >+ "ORDER BY milestones.sortkey, value"); I'll stop commenting on these now... Basically, you don't need left joins anywhere >@@ -1365,7 +1414,7 @@ > "FROM profiles " . > "LEFT JOIN votes ON profiles.userid = votes.who " . > "LEFT JOIN bugs USING(bug_id) " . >- "LEFT JOIN products USING(product)" . >+ "LEFT JOIN products ON products.id = bugs.product_id " . ... except that you can leave it here because the old code did that. This may be fixed as part of the 'clean up the schema' stuff I want for 2.18. >Index: post_bug.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v >retrieving revision 1.38 >diff -u -r1.38 post_bug.cgi >--- post_bug.cgi 20 Jan 2002 01:44:40 -0000 1.38 >+++ post_bug.cgi 31 Jan 2002 03:29:16 -0000 >@@ -87,9 +88,11 @@ > } > } > >-if (!defined $::FORM{'component'} || $::FORM{'component'} eq "") { >- PuntTryAgain("You must choose a component that corresponds to this bug. " . >- "If necessary, just guess."); >+my $component_id = get_component_id($product, $::FORM{component}); >+if (! $component_id) { >+ DisplayError("The component you selected (" . html_quote($::FORM{component}) . >+ ") doesn't exist."); >+ exit; > } > why this change? Shouldn't it still be PuntTryAgain? >Index: process_bug.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v >retrieving revision 1.114 >diff -u -r1.114 process_bug.cgi >--- process_bug.cgi 22 Jan 2002 15:12:11 -0000 1.114 >+++ process_bug.cgi 31 Jan 2002 03:29:20 -0000 >@@ -584,6 +585,17 @@ > } > } > >+if (defined $::FORM{'component'} && $::FORM{'component'} ne $::dontchange) { >+ my $comp_id = get_component_id($::FORM{'product'}, $::FORM{'component'}); >+ DoComma(); >+ $::query .= "component_id = $comp_id"; >+} >+ >+if (defined $::FORM{'product'} && $::FORM{'product'} ne $::dontchange) { >+ my $prod_id = get_product_id($::FORM{'product'}); >+ DoComma(); >+ $::query .= "product_id = $prod_id"; >+} What if the product isn't valid (or is that caught by earlier checks via the versioncache)? >Index: reports.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/reports.cgi,v >retrieving revision 1.50 >diff -u -r1.50 reports.cgi >--- reports.cgi 20 Jan 2002 01:44:43 -0000 1.50 >+++ reports.cgi 31 Jan 2002 03:29:22 -0000 >@@ -186,7 +186,7 @@ > if (Param('usetargetmilestone')) { > print "<option value=\"most_doomed_for_milestone\">Most Doomed"; > } >- print "<option value=\"most_recently_doomed\">Most Recently Doomed"; >+# print "<option value=\"most_recently_doomed\">Most Recently Doomed"; ?? > > if ($FORM{'product'} ne "-All-" ) { >- $query .= "and bugs.product=".SqlQuote($FORM{'product'}); >+ $query .= "and products.name=".SqlQuote($FORM{'product'}); Don't you need the extra table here, too? >Index: sanitycheck.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v >retrieving revision 1.40 >diff -u -r1.40 sanitycheck.cgi >--- sanitycheck.cgi 20 Jan 2002 01:44:43 -0000 1.40 >+++ sanitycheck.cgi 31 Jan 2002 03:29:23 -0000 >@@ -179,15 +179,15 @@ [crosscheck changes] Shouldn't there be a check matching the components table up with the bugs table, too? Is that covered in the later check? >- my $link = "buglist.cgi?product=" . url_quote($product) . >- "&component=" . url_quote($component); >- Alert(qq{Bug(s) found with invalid product/component: $product/$component (<a href="$link">bug list</a>)}); >+# my $link = "buglist.cgi?product=" . url_quote($product) . >+# "&component=" . url_quote($component); >+# Alert(qq{Bug(s) found with invalid product/component: $product/$component (<a href="$link">bug list</a>)}); >+ Alert(qq{Bug(s) found with invalid product/component ID: $product_id/$component_id}); > } Is this needed? (And can't this all be rewritten as a left join WHERE foo=NULL?)

Regarding: LEFT JOINs I need to join the bugs table to products table in order to get the name (I don't think "Product: 3" would mean much to anybody... unless I just get the product_id from the initial query and then run another one to get the name. Regarding: ChangeFieldType in checksetup.pl The ones that I pulled and didn't replace are for fields that will be dropped during this running of checksetup.pl. It seems silly to change the field type only to turn around and drop the field. Regarding: Dealing w/table errors in checksetup.pl It seems that setting the product/component_id field to 1 if it equals 0 is worse than leaving it 0. Perhaps I should report if there are any 0's. Regarding: &die_with_dignity Correct, this should be commented out again before checking in... in fact, I never intended for that sub to get checked in at all, but I left it there when I was doing the taint-patch for processmail for testing purposes, and it went in. Regarding: sanitycheck.cgi To be honest, I really don't understand sanitycheck.cgi. I only wanted to make it work, but at some point it will have to be done better. Everything else is addressed.

Yes, you need a join, but not a left join. the diufference is that if a product id in the bugs table doesn't exist in the products tabe, then with a left join you'll get NULL for the name, and for a normal join you won't get a result. Left joints are slower because they can't really use indexes to do the join, at least not entirely. Don't you have to change the type so that the conversion code works correctly? People should be running sanitycheck before upgrading, and I guess if the bugs won't show up (because of the inner join) then poele will try to work out what is wrong ;) It may stuff up later conversion code unless it has a valid value, though. I'll look at the rest later.

*** Bug 137504 has been marked as a duplicate of this bug. ***

-> me - I have a patch (w/o the fix for bug 153540) This fixes my review comments above, and also changes files which Jake didn't do. Note that the edit* changes are a bit ugly, but the code is ugly to begin with. It can be cleaned up in the Great Edit* Rewrite, though. Its mainly stuff to avoid code duplication, and the like, though - nothing major. The side effects of this patch include: - a few more checks in various places (edit*, mainly) that the supplied product/component actually exists. This is needed because the various selects will no longer just return no match, since getting the product id is often a separate step (we usually need it more than once per script) - buglist.cgi no longer does a whole lot of joins unless it actually needs to. I've wanted to do this for a while, but the overhead of possibly joining 5 extra tables for each search with this patch means that its going to become a massive overhead, instead of just a large one. - *NB* bugs without a valid product or component WILL NOT BE VISIBLE. This is to avoid a LEFT JOIN. Theres been a sanitycheck for it for quite a while, and I've added an message from bug_form.pl for when this happens. I could use a LEFT JOIN for bug_form.pl/Bug.pm without much affect, I guess, but LEFT JOINS are _slow_, so I don't want to use them for buglist.cgi. (This is also one of the reasons why I removed unneeded joins, esp the QA one) I think I've tested most things, but this patch is fairly invasive, so it will need careful testing of everything. I'll apply it to landfill once justdave gives me write perms to jake's old testing directory.

See previous comment for details.

OK, this adds the indexes, and fixes a couple of minor bugs (eg couldn't just change the component from the mass change page) I'll put this up on landfill once it comes back to life. This is now ready for review, I think.

Fixes cvs conflict with current trunk

... and updated for cvs conflicts again....

Comment on attachment 93287 [details] [diff] [review] v14 Editing milestones broken. Editmilestones.cgi line 311 need $ Editversions.cgi has several queries with old schema probably shouldn't check in with email diabled.. # disable email flag for offline debugging work -my $enableSendMail = 1; +my $enableSendMail = 0;

Fixes milestones/versions

Comment on attachment 93376 [details] [diff] [review] v15 Needs second review. Should also test on site with large number of bugs assigned to versions and miletones to convert.

Theres one more fix needed - change line 555 of buglist.cgi to read: ' push(@supptables, "profiles AS map_$f");' (ie add AS) I also realised that I broke contrib/, and while we don't noramlly care about that, I should probably make an attempt to fix bug_email, so I'll do that too.

Comment on attachment 93376 [details] [diff] [review] v15 oops - Missing an "AS" in buglis.cgi breaks email search. Line 555 fix is known.

OK, searching on user name has been fixed. bug_email now 'works' as well as it used to, ie not very well (see, eg, bug 160631)

If a bug is created and no component is specified, get_component_id dies and causes the user to see a fatal rather than an error instructing the user to back up and select a component. +my $component_id = get_component_id($product, $::FORM{component}); +if (!$component_id) { DisplayError("You must choose a component that corresponds to this bug. If necessary, just guess."); +sub get_component_id { + my ($prod, $comp) = @_; + PushGlobalSQLState(); + SendSQL("SELECT components.id " . + "FROM components, products " . + "WHERE products.id = components.product_id " . + " AND products.name = " . SqlQuote($prod) . + " AND components.name = " . SqlQuote($comp)); + my ($comp_id) = FetchSQLData(); + PopGlobalSQLState(); + detaint_natural($comp_id) || die "get_component_id() returned a non-integer"; + return $comp_id; +}

How do you validly select a product w/o a component? But yeah, I probably need to remove that |die|.

When a user enters a new bug, they normally click on the product and are taken to a page where they often touch nothing above the summary. If they neglect to select a component, they are normally told so and told to go back and pick one.

OK, this no longer does a die. The trick_taint stuff is there for processmail; see bug 161402

Tried v17 Most works, but boolean queries where component contains a substring don't work anymore.

Drat. I need to play the game attachments/keywords do. Except that they're broken for some of this stuff. Hmm...

OK, fixing the component/product stuff wasn't hard, once I sorted out the fielddefs mess - see bug 161865. Note that searching for product changed to/from is broken, in the same way that searching for cc changed to/from is broken. There has got to be a better way to do this - ideas? Also, the fielddefs db change has been removed, so if you want to apply this over a previous version, manualy rename component_id/product_id back to component/product in the fielddefs table.

OK, try this. Re the buglist search stuff, can I just say 'yuck'.

Comment on attachment 94721 [details] [diff] [review] v19 IN SUMMARY: As I'm sure everyone knows, this is a huge patch and thus very hard to review; it is with great reservation that I give this an r=, not so much because I don't trust the patch authors, but moreso because I don't trust my review. I did the best I could with the context I had... I hope that's "good enough." This gets my r= *assuming* the two following issues are addressed: -- The get_*_id() functions assert() the fact that they are receiving digits; bbaetz and I "discussed" ;-) this on IRC, and he said he'd add it. -- This patch seems to have a lot of cruft from other patches; unless there's a reason not to, these other patches should be removed from this one... this patch is huge enough as it is... <fingers shaking> r=preed </shaking> ... >Index: CGI.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v >retrieving revision 1.164 >diff -u -r1.164 CGI.pl >- if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) { >+ if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:\"\[\] \t\r\n]/) { Ok, according to bbaetz on IRC, this is for bug 160631; please take it out of the patch for this bug then. >Index: collectstats.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v >retrieving revision 1.20 >diff -u -r1.20 collectstats.pl >--- collectstats.pl 6 Jun 2001 21:17:40 -0000 1.20 >+++ collectstats.pl 10 Aug 2002 00:40:29 -0000 >@@ -67,6 +67,9 @@ > my $dir = shift; > my $product = shift; > my $when = localtime (time); >+ my $product_id = get_product_id($product) unless $product eq '-All-'; >+ >+ die "Unknown product $product" unless $product_id; According to bbaetz on IRC, the die should be: >+ die "Unknown product $product" unless ($product_id or $product eq '-All-'); >Index: globals.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v >retrieving revision 1.183 >diff -u -r1.183 globals.pl >+sub get_product_id { >+ my ($prod) = @_; >+ PushGlobalSQLState(); >+ SendSQL("SELECT id FROM products WHERE name = " . SqlQuote($prod)); >+ my ($prod_id) = FetchSQLData(); >+ PopGlobalSQLState(); >+ return $prod_id; >+} >+ >+sub get_product_name { >+ my ($prod_id) = @_; >+ PushGlobalSQLState(); >+ SendSQL("SELECT name FROM products WHERE id = $prod_id"); >+ my ($prod) = FetchSQLData(); >+ PopGlobalSQLState(); >+ return $prod; >+} >+ >+sub get_component_id { >+ my ($prod, $comp) = @_; >+ PushGlobalSQLState(); >+ SendSQL("SELECT id FROM components " . >+ "WHERE product_id = $prod AND name = " . SqlQuote($comp)); >+ my ($comp_id) = FetchSQLData(); >+ PopGlobalSQLState(); >+ return $comp_id; >+} >+ >+sub get_component_name { >+ my ($comp_id) = @_; >+ PushGlobalSQLState(); >+ SendSQL("SELECT name FROM components WHERE id = $comp_id"); >+ my ($comp) = FetchSQLData(); >+ PopGlobalSQLState(); >+ return $comp; >+} Everywhere where you assume you're getting an id, you should assert() that fact; of course, since we don't have assert(), something like: die "NaN" if ($whatever_id !~ /^\d+$/); Just an extra security check since we don't SqlQuote those IDs because we assume they will be numbers; this also takes care of the issue of chaining these function (i.e. get_component_id(get_product_id($prodid), $comp)) when get_product_id() returns undef because $prod doesn't exit. nit: it would be nice to combine these functions if at all possible; do we make any restrictions on component or product names (i.e. must have at least one letter in them, etc.)? If so, these could be collapsed into get_component/product_info or something like that. >Index: processmail >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/processmail,v >retrieving revision 1.84 >diff -u -r1.84 processmail >--- processmail 11 Jul 2002 08:05:05 -0000 1.84 >+++ processmail 10 Aug 2002 00:40:43 -0000 > # disable email flag for offline debugging work >-my $enableSendMail = 1; >+my $enableSendMail = 0; Turn this back on, if it hasn't been already. >- $word = SqlQuote(quotemeta($word)); >+ $word = &::SqlQuote(quotemeta($word)); What does this have to do with this bug? If nothing, again, please remove... this patch is big enough as it is.

OK, this should be it. I've updated to current cvs, fixed a minor merge conflict in duplicates.cgi (mainly code movement, but one bug fix when searching for duplicates for specific products), and added the die calls jp wanted.

Comment on attachment 94819 [details] [diff] [review] v20 Per a conversation on IRC, bbaetz is gonna take out the fix for bug 160631 (re: comment 59) and add in the digit die() in get_component_id. r=preed

This is it - I mean it :)

Comment on attachment 94829 [details] [diff] [review] v21 - this is it Carrying forward r=jaypee, and r=joel

Checked in! Please file any regression bugs on me.

(In reply to comment #64) > Please file any regression bugs on me. Bug 281845 filed for the broken user-delete code (which actually worked before this was checked in :)

Comment on attachment 214082 [details] [diff] [review] docs patch about 'Bugzilla Database Tables' section, for 2.18 >+components: This stores name, the description and initial >+owner/qacontact of the component, and which product the components >+belong to. ... and _to_ which product the components belong ||. (don't end a sentence with a preposition.)

Doc patch submitted on the 2.18 branch: Checking in docs/xml/customization.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v <-- customization.xml new revision: 1.12.2.13; previous revision: 1.12.2.12 done This part of the documentation has been removed on newer branches.