43600 - Duplicate field names in bugzilla tables (component IDs etc.)

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Mat Spencer]

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,

Comment 1

[Mat Spencer]

Attachment: Add components.component_id remove bugs.component, replace with bugs.component_id

Comment 2

[Mat Spencer]

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.

Comment 3

[Dave Miller [:justdave]]

I would do this for the versions table as well.

Comment 4

[Dave Miller [:justdave]]

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

Comment 5

[Mat Spencer]

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,

Comment 6

[Mat Spencer]

Attachment: product and component ids

Comment 7

[Tara Hernandez]

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

Comment 8

[Tara Hernandez]

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.

Comment 9

[Chris Yeh]

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

Comment 10

[Chris Yeh]

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

Comment 11

[Dave Miller [:justdave]]

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

Comment 12

[Dave Miller [:justdave]]

Taking all of cyeh's Bugzilla bugs.

Comment 13

[Jacob Steenhagen]

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

Comment 14

[Jacob Steenhagen]

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

Comment 15

[Jacob Steenhagen]

Attachment: partial patch

Comment 16

[Jacob Steenhagen]

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 17

[Jacob Steenhagen]

Attachment: add sanitycheck.cgi, editproducts.cgi, editcomponents.cgi

Comment 18

[Jacob Steenhagen]

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.

Comment 19

[Matthew Tuck [:CodeMachine]]

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

Comment 20

[Stephen Lee]

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.

Comment 21

[Jacob Steenhagen]

Attachment: add reports.cgi, other minor fixes

Comment 22

[Jacob Steenhagen]

Attachment: same thing, but as diff -u

Comment 23

[Jacob Steenhagen]

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

Comment 24

[Matthew Tuck [:CodeMachine]]

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.

Comment 25

[Jacob Steenhagen]

Attachment: patch v5

Comment 26

[Jacob Steenhagen]

Attachment: patch v6: Ready for review

Comment 27

[Jacob Steenhagen]

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 28

[Matthew Tuck [:CodeMachine]]

Comment on attachment 54007 [details] [diff] [review]
patch v6: Ready for review

Needs work - still contains "meduimint".

Comment 29

[Jacob Steenhagen]

Attachment: Patch v7: Use smallint

Comment 30

[Dave Miller [:justdave]]

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.

Comment 31

[Jacob Steenhagen]

Attachment: patch v8: More love

Another revision... fixes some errors I found and updates to current cvs.

Patch applied to http://landfill.tequilarista.org/bz43600/

Comment 32

[Jacob Steenhagen]

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 33

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 34

[Jacob Steenhagen]

Attachment: Patch v9: Minor checksetup changes

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

Comment 35

[Dave Miller [:justdave]]

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 36

[Bradley Baetz (:bbaetz)]

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-?? [e-mail]
>+# 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 - [e-mail]
>-        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?)

Comment 37

[Jacob Steenhagen]

Attachment: Patch v10: Address review comments

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.

Comment 38

[Bradley Baetz (:bbaetz)]

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.

Comment 39

[Andreas Krüger]

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

Comment 40

[Bradley Baetz (:bbaetz)]

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

Comment 41

[Bradley Baetz (:bbaetz)]

Attachment: v11

See previous comment for details.

Comment 42

[Bradley Baetz (:bbaetz)]

Attachment: v12

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.

Comment 43

[Bradley Baetz (:bbaetz)]

Attachment: v13

Fixes cvs conflict with current trunk

Comment 44

[Bradley Baetz (:bbaetz)]

Attachment: v14

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

Comment 45

[Joel Peshkin]

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;

Comment 46

[Bradley Baetz (:bbaetz)]

Attachment: v15

Fixes milestones/versions

Comment 47

[Joel Peshkin]

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.

Comment 48

[Bradley Baetz (:bbaetz)]

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 49

[Joel Peshkin]

Comment on attachment 93376 [details] [diff] [review]
v15

oops - Missing an "AS" in buglis.cgi breaks email search.  Line 555 fix is
known.

Comment 50

[Bradley Baetz (:bbaetz)]

Attachment: v16

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)

Comment 51

[Joel Peshkin]

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;
+}

Comment 52

[Bradley Baetz (:bbaetz)]

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

Comment 53

[Joel Peshkin]

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.  

Comment 54

[Bradley Baetz (:bbaetz)]

Attachment: v17

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

Comment 55

[Joel Peshkin]

Tried v17

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

Comment 56

[Bradley Baetz (:bbaetz)]

Drat. I need to play the game attachments/keywords do.

Except that they're broken for some of this stuff. Hmm...

Comment 57

[Bradley Baetz (:bbaetz)]

Attachment: v18

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.

Comment 58

[Bradley Baetz (:bbaetz)]

Attachment: v19

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

Comment 59

[J. Paul Reed [:preed]]

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.

Comment 60

[Bradley Baetz (:bbaetz)]

Attachment: v20

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 61

[J. Paul Reed [:preed]]

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

Comment 62

[Bradley Baetz (:bbaetz)]

Attachment: v21 - this is it

This is it - I mean it :)

Comment 63

[Bradley Baetz (:bbaetz)]

Comment on attachment 94829 [details] [diff] [review]
v21 - this is it

Carrying forward r=jaypee, and r=joel

Comment 64

[Bradley Baetz (:bbaetz)]

Checked in!

Please file any regression bugs on me.

Comment 65

[Dave Miller [:justdave]]

(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 66

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18

Comment 67

[timeless]

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

Comment 68

[Frédéric Buclin]

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.