Closed Bug 43600 Opened 24 years ago Closed 22 years ago

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


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




Bugzilla 2.18


(Reporter: mbs, Assigned: bbaetz)


(Blocks 2 open bugs, )


(Whiteboard: [schema])


(2 files, 22 obsolete files)

100.89 KB, patch
: review+
: review+
Details | Diff | Splinter Review
1.01 KB, patch
: review+
Details | Diff | Splinter Review
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
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
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

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.

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

Attached patch product and component ids (obsolete) — Splinter Review
Blocks: 43940
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.
Assignee: tara → cyeh
*** 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.
Target Milestone: --- → Bugzilla 2.16
Keywords: patch
Summary: Duplicate field names in bugzilla tables → Duplicate field names in bugzilla tables (component IDs etc.)
Taking all of cyeh's Bugzilla bugs.
Assignee: Chris.Yeh → justdave
Blocks: emailprefs
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 :)
Keywords: patch
Attachment #10578 - Attachment is obsolete: true
Unless Mat comes forward with a new patch, I'll start working on this.
Assignee: justdave → jake
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Whiteboard: [schema]
Version: other → unspecified
Attachment #10701 - Flags: review-
Attached patch partial patch (obsolete) — Splinter Review
Comment on attachment 48817 [details] [diff] [review]
partial patch

This partial patch has the 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.
Attachment #48817 - Flags: review-
Attachment #10701 - Attachment is obsolete: true
Attachment #48817 - Attachment is obsolete: true
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.
Attachment #49062 - Flags: review-
> product_id meduimint not null,


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 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.
Attachment #49062 - Attachment is obsolete: true
Attached patch same thing, but as diff -u (obsolete) — Splinter Review
Attachment #49405 - Attachment is obsolete: true
The latest patch is now available on landfill for testing...
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.
Attachment #49406 - Attachment is obsolete: true
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #52129 - Flags: review-
Attached patch patch v6: Ready for review (obsolete) — Splinter Review
OK, v6 fixes all known issues and (I believe) incorporates everyone's
suggestions that have been mentioned so far.

Ready for the infamous review :)
Keywords: patch, review
Attachment #52129 - Attachment is obsolete: true
Blocks: 104690
Blocks: bz-postgres
Comment on attachment 54007 [details] [diff] [review]
patch v6: Ready for review

Needs work - still contains "meduimint".
Attachment #54007 - Flags: review-
Keywords: patch, review
Attached patch Patch v7: Use smallint (obsolete) — Splinter Review
Attachment #54007 - Attachment is obsolete: true
Keywords: patch, review
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Attached patch patch v8: More love (obsolete) — Splinter Review
Another revision... fixes some errors I found and updates to current cvs.

Patch applied to
Attachment #55483 - Attachment is obsolete: true
This bug blocks a "we would like for 2.16 bug" so retargeting... it very well
may get bumped back to 2.18 again.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
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 portion:

There some more uses in 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


Btw- This patch could also use a refresh to the tip.
Attachment #59004 - Flags: review-
> 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 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 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 :)
Attachment #59004 - Attachment is obsolete: true
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.
Priority: P3 → P1
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Comment on attachment 67206 [details] [diff] [review]
Patch v9: Minor checksetup changes

Walkthrough, this probably doesn't apply any more, blah, blah, etc ;)

>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.86
>diff -u -r1.86
>@@ -75,6 +75,8 @@
>         delta_ts,
>         sum(votes.count)
> from bugs left join votes using(bug_id)
>+left join products on bugs.product_id =
>+left join components on bugs.component_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 ;)

>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.115
>diff -u -r1.115
>---	30 Jan 2002 14:14:11 -0000	1.115
>+++	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
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-??
>+# Use integer ID's for products and components.
>+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

<skipping edit* for the moment>

>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.134
>diff -u -r1.134
>---	30 Jan 2002 14:14:12 -0000	1.134
>+++	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, FROM versions " .
>+            "LEFT JOIN products ON = versions.product_id " .
>+            "ORDER BY versions.value");

again, why the left join? Versions w/o products are invalid, and there should
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, FROM components " .
>+            "LEFT JOIN products ON = components.product_id " .
>+            "ORDER BY");


>@@ -582,7 +586,9 @@
>     if ($dotargetmilestone) {
>         # reading target milestones in from the database -
>-        SendSQL("SELECT value, product FROM milestones ORDER BY sortkey, value");
>+        SendSQL("SELECT milestones.value, FROM milestones " .
>+                "LEFT JOIN products ON = milestones.product_id " .
>+                "ORDER BY milestones.sortkey, value");

I'll stop commenting on these now... Basically, you don't need left joins

>@@ -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 = 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

>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".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
Attachment #67206 - Flags: review-
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

The ones that I pulled and didn't replace are for fields that will be dropped
during this running of  It seems silly to change the field type
only to turn around and drop the field.

Regarding: Dealing w/table errors in

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

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.
Attachment #67206 - Attachment is obsolete: true
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,

I'll look at the rest later.
*** Bug 137504 has been marked as a duplicate of this bug. ***
Depends on: 153540
-> 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 for when this happens. I could use a LEFT JOIN
for 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.
Assignee: jake → bbaetz
Attached patch v11 (obsolete) — Splinter Review
See previous comment for details.
Attachment #72611 - Attachment is obsolete: true
Attached patch v12 (obsolete) — Splinter Review
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.
Attachment #88766 - Attachment is obsolete: true
Attached patch v13 (obsolete) — Splinter Review
Fixes cvs conflict with current trunk
Attachment #90441 - Attachment is obsolete: true
Attached patch v14 (obsolete) — Splinter Review
... and updated for cvs conflicts again....
Attachment #91449 - Attachment is obsolete: true
Blocks: 157756
Comment on attachment 93287 [details] [diff] [review]

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;
Attachment #93287 - Flags: review-
Attached patch v15 (obsolete) — Splinter Review
Fixes milestones/versions
Attachment #93287 - Attachment is obsolete: true
Comment on attachment 93376 [details] [diff] [review]

Needs second review.

Should also test on site with large number of bugs assigned to versions and
miletones to convert.
Attachment #93376 - Flags: review+
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]

oops - Missing an "AS" in buglis.cgi breaks email search.  Line 555 fix is
Attachment #93376 - Flags: review-
Attached patch v16 (obsolete) — Splinter Review
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)
Attachment #93376 - Attachment is obsolete: true
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 " .
+            "FROM components, products " .
+            "WHERE = components.product_id " .
+            " AND = " . SqlQuote($prod) .
+            " AND = " . 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.  

Attached patch v17 (obsolete) — Splinter Review
OK, this no longer does a die. The trick_taint stuff is there for processmail;
see bug 161402
Attachment #93840 - Attachment is obsolete: true
Tried v17

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

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

Except that they're broken for some of this stuff. Hmm...
Attached patch v18 (obsolete) — Splinter Review
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.
Attachment #94315 - Attachment is obsolete: true
Attached patch v19 (obsolete) — Splinter Review
OK, try this. Re the buglist search stuff, can I just say 'yuck'.
Attachment #94610 - Attachment is obsolete: true
Comment on attachment 94721 [details] [diff] [review]


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>


>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.164
>diff -u -r1.164

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

>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.20
>diff -u -r1.20
>---	6 Jun 2001 21:17:40 -0000	1.20
>+++	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-');

>RCS file: /cvsroot/mozilla/webtools/bugzilla/,v
>retrieving revision 1.183
>diff -u -r1.183

>+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.
Attachment #94721 - Flags: review+
Attached patch v20 (obsolete) — Splinter Review
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
Attachment #94721 - Attachment is obsolete: true
Comment on attachment 94819 [details] [diff] [review]

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.

Attachment #94819 - Flags: review+
Attached patch v21 - this is itSplinter Review
This is it - I mean it :)
Attachment #94819 - Attachment is obsolete: true
Comment on attachment 94829 [details] [diff] [review]
v21 - this is it

Carrying forward r=jaypee, and r=joel
Attachment #94829 - Flags: review+
Checked in!

Please file any regression bugs on me.
Closed: 22 years ago
Resolution: --- → FIXED
No longer depends on: 153540
(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.)
Attachment #214082 - Flags: review?(documentation) → review+
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:; previous revision:

This part of the documentation has been removed on newer branches.
Flags: documentation2.18+
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.