17453 - (bz-enums) Enumerators in Bugzilla are not cross-DB compatible

Status: RESOLVED FIXED

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

Description

[Andre-John Mas]

Although this has not been an issue yet it might become one. The operating
system types and platforms are define as values of enumerators in the bugs
database. If there is a new OS that comes out, either by name or version
I would have to modify the value of the definition of the enumerator. This
is not very clean, in the sense that attributes should be set once at the
creation of the database, any other value should really be in a table where
it is easy to update. Sure a table with one column is not very elegant, but
it does provide the flexibility that time would require.

Comment 1

[randall.whitman]

One issue with changing enum columns to text columns (am assuming that
the proposed new table would have some type of text column) will be
to make sure that the values continue to sort in proper order.
Especially if fields such as Status and Resolution are changed,
as well as Platform and OS.

Comment 2

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 3

[Chris Yeh]

enums are bad because they are specific to mysql. this will be fixed when we 
re-do the database schema

Comment 4

[Stephan Niemz]

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.

Comment 5

[Dave Miller [:justdave]]

Moving to real milestones...

Comment 6

[Hixie (not reading bugmail)]

Taking all Bugzilla 3.0 bugs -- congratulations to MattyT for the triage, it
really was spot on. Note: I may end up pushing some of these bugs back to 3.2,
we'll see. However, I believe all these bugs should just fall out of the 
redesign. Let's hope I'm right!

(Note: I'm also resetting the priority field to "---" so that I can retriage any
that I consider important or likely to be dropped.)

Comment 7

[Dave Miller [:justdave]]

Stealing this back for Bugzilla 2.16.  I've found numerous comments in other 
places by both cyeh and tara indicating that this was going to be one of the 
first priorities after 2.14 shiped.  The status of the bug about it needs to 
reflect that.

Comment 8

[Dave Miller [:justdave]]

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

Comment 9

[Dave Miller [:justdave]]

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

Comment 10

[Zach Lipton [:zach]]

moving

Comment 11

[David Lawrence [:dkl]]

I have made preliminary changes to do away with enum types in Mysql. I am
uploading a enum patch against 2.14 for review that alleviates enum types
in the database and adds the necessary tables to replace them. Only two files so
far seem to be affected, globals.pl and checksetup.pl. Please let me know what
you think. I am running it on a test bugzilla install with not noticable problems.

Comment 12

[David Lawrence [:dkl]]

Attachment: Patch to alleviate enum data types

Comment 13

[Jacob Steenhagen]

Comment on attachment 48106 [details] [diff] [review]
Patch to alleviate enum data types

> # Contributor(s): Terry Weissman <[e-mail]>
> #                 Dan Mosedale <[e-mail]>
> #                 Jake <[e-mail]>
>+# 				  David Lawrence <[e-mail]>

Why is your name so much farther over than everyone elses?  :)

>+# Set driver to desired database, ex. mysql, Pg, Oracle, etc.
>+$::driver  = "mysql";
>+

If we're gonna have this, it need to be in localconfig, not globals.pl.  I'd personally hope
to aviod having this for the time being.

>+    if ( $::driver eq 'mysql' ) {
>+        SendSQL("show columns from $table");
>+        while ( my @row = FetchSQLData() ) {
>+            my ($name,$type) = (@row);
>+            $a{"$name,type"} = $type;
>+            push (@list, $name);
>+        }
>+    } elsif ( $::driver eq 'Pg' ) {
>+        my $ref = $::db->func($table, "table_attributes");
>+        for my $index ( 0..@{$ref} ) {
>+            next if !$ref->[$index]->{'NAME'};
>+            my $name = $ref->[$index]->{'NAME'};
>+            my $type = $ref->[$index]->{'TYPE'};
>+            $a{"$name,type"} = $type;
>+            push (@list, $name);
>+        }
>+    }   

There almost has to be a good way to make DBD fake this for us.  It can't be that uncommon
to want to know what all columns are in a table.

> sub SplitEnumType {

Hmmm... I think if we keep the name 'SplitEnumType()' it might not be a bad idea to explain
that it's a legacy name.

>Index: checksetup.pl
...
>+    bug_severity varchar(255) not null,
>+    bug_status varchar(255) not null,
...
>+    op_sys varchar(255) not null,
>+    priority varchar(255) not null,
...
>+    rep_platform varchar(255) not null,
...
>+    resolution varchar(255) not null,
Isn't 255 a bit big for these fields?

>+        if ( $tabname =~ /$enum/ ) {

I know matching the table in a regexp is done just a few lines before this, but this
really isn't a good idea (see bug 95890)

> # Final checks...
>+
>+# 2001-09-03 [e-mail] bug17453
>+# Moved enum types to separate tables so we need change the old enum types to 
>+# standard varchars in the bugs table.
>+ChangeFieldType ('bugs', 'bug_status', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'resolution', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'priority', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'bug_severity', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'rep_platform', 'varchar(255) not null');
>+ChangeFieldType ('bugs', 'op_sys', 'varchar(255) not null');

These really should be done before the # Final checks comment (you should start at line
2530 in the current CVS, not 2542 where this one does.


Also, we'll need to add some checks to sanitycheck.cgi to make sure that any
os/resolution/etc that exists in the bugs table also exists in it's associated 'enum' table.

Comment 14

[Jacob Steenhagen]

-> Patch Writer

Comment 15

[David Lawrence [:dkl]]

Attachment: Second version with Jake suggested changes

Comment 16

[David Lawrence [:dkl]]

I made suggested changes by Jake and also added a section to sanitycheck.cgi
that reports number of bugs that have values not present in the separate enum
tables.

Comment 17

[Matthew Tuck [:CodeMachine]]

I don't really support because it contains schema changes that checksetup.pl
will need to convert later.  This is because I'm intending on doing all this and
more for 2.16, including using IDs for everything, allowing records to be set
inactive, sortkeys, etc.

The start of this will be bug #94534 for resolutions, which I will shortly
upload a patch to.

Comment 18

[Matthew Tuck [:CodeMachine]]

Also, the sanity checks you added are referential checks, and as such, you
should use the CrossCheck subroutine.  See the other uses of CrossCheck in the
file.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Why VARCHAR?  The size of everyone's database will explode depending on how many
bugs they have in there.  Going from 1-2 bytes per field per bug for an enum to
N+1 bytes for varchar?  Why not create a separate table each for OS, platform,
etc.  Have each table's keyID field be whatever is the least common denominator
of an unsigned int.  (tinyint, smallint, whatever).  Then in the bug's table,
store the OS ID or Platform ID or whatever.  We can do a sub-select (or 2nd
query for dbs that don't do subselects) to get what to substitute.  Or since
those don't change much at all, we can do something similar to what we do with
localconfig.js.  But instead of generating a js file, just generate a perl file
with pre-made variables.  Does this make any sense at all?

Comment 20

[Dave Miller [:justdave]]

right in with the rest of the crap in versioncache :)

Comment 21

[Matthew Tuck [:CodeMachine]]

This is likely to change.  We are already moving to IDs for resolutions (bug
#94534) and products/components (bug #43600).  I'll want to move more over as a
part of bug #97706.  I also suggested to dkl we make statuses an integer with
hardcoded meaning and eschew a statuses table until we're ready to do customised
statuses (bug #101179), since it will make that easier.

We may not get all that done for 2.16, but we're unlikely to get all the
PostgreSQL stuff done for 2.16 either.

Comment 22

[Dave Miller [:justdave]]

This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.

Comment 23

[Matthew Tuck [:CodeMachine]]

Moving off, custres at least won't make 2.16.

Comment 24

[Alex Schuilenburg]

Attachment: Script to edit values in the tables that replace enums

Following dkl's patch, here is a script that will allow you to edit/add/delete
the values in the tables used to replace the "enum" data types.  Works with
2.15

Comment 25

[Alex Schuilenburg]

Attachment: Patch to include link to edittable.cgi

Patch to include a link to edittable.cgi for 2.16rc1

Comment 26

[Bradley Baetz (:bbaetz)]

Comment on attachment 48114 [details] [diff] [review]
Second version with Jake suggested changes

This all needs to be done with ids.

For the moment, theres nothing wrong with a CHECK constraint, and stuff can get
moved along  to lookup tabes as customfields/custres/etc happen

Comment 27

[Bradley Baetz (:bbaetz)]

Comment on attachment 84253 [details] [diff] [review]
Patch to include link to edittable.cgi

This is just an addon to the other patch

Comment 28

[Dave Miller [:justdave]]

enhancements without current patches are being pushed to 2.20

Comment 29

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 30

[Max Kanat-Alexander]

Attachment: Un-bitrotted varchar patch

This patch is basically just an un-bit-rotted version of what dkl posted quite
some time ago. I've done some basic testing on it, and it still works just as
well as it did back then.

I talked to justdave a bit about this in IRC.

At first, I wanted to go to using ids and foreign keys and all that. It turns
out, though, that that would involve major and invasive changes to Bugzilla,
because it would change the type of all the affected fields from a string to an
integer, as far as the perl was concerned. That is, if we have to both
eliminate enumerators and convert to a foreign-key/id-based system, we'll never
get this badly-needed patch in.

The nice thing about this patch is that it only involves a very small change
for a very important architectural benefit.

Red Hat uses varchars now; I've been running their code fork for over a year
locally. It doesn't cause a speed problem, and the database doesn't seem
significantly bloated here.

I've given all the tables a primary key that's a smallint, though. The plan is
to:

(1) Move enums to varchars.
(2) Move these varchars to integers.

That breaks up the patch much more nicely.

There ARE some problems with this patch which have to be resolved:

(1) The sort order of statuses and resolutions becomes alphabetical. This can
be fixed by a hack that I have locally. Basically, every time you see something
like "resolution" in the sort order, you replace it with "resolution.id". You
also have to add the appropriate table to the JOIN, and make sure that you also
modify the GROUP BY clause as necessary.
  I have code for this that works; I can clean it up and post it here soon.

(2) The actual resolutions that are allowed to be inserted into the DB becomes
unbounded. This is *nice* because it means that I can delete resolutions, and
clean up afterward if I want. The only place this might cause a problem is if
there's some security hole that we don't know about in the perl side of things,
and then bugs can be made to have invalid statuses or resolutions. Not too big
of a deal, and sanitycheck catches it easily.

Comment 31

[Max Kanat-Alexander]

Attachment: v3.1 (Without the extra patch in it)

Whoops, the previous patch accidentally also had a very old docs patch of mine
inside it.

Comment 32

[Jim C. Nasby]

(In reply to comment #30)
> At first, I wanted to go to using ids and foreign keys and all that. It turns
> out, though, that that would involve major and invasive changes to Bugzilla,
> because it would change the type of all the affected fields from a string to an
> integer, as far as the perl was concerned. That is, if we have to both
> eliminate enumerators and convert to a foreign-key/id-based system, we'll never
> get this badly-needed patch in.
What about using views to hide the IDs?

> (1) The sort order of statuses and resolutions becomes alphabetical. This can
> be fixed by a hack that I have locally. Basically, every time you see something
> like "resolution" in the sort order, you replace it with "resolution.id". You
> also have to add the appropriate table to the JOIN, and make sure that you also
> modify the GROUP BY clause as necessary.
>   I have code for this that works; I can clean it up and post it here soon.
I would suggest using a seperate field for sort order; that would allow for
arbitrary changes without the uglyness of deleting and inserting or changing
your primary key.

> (2) The actual resolutions that are allowed to be inserted into the DB becomes
> unbounded. This is *nice* because it means that I can delete resolutions, and
> clean up afterward if I want. The only place this might cause a problem is if
> there's some security hole that we don't know about in the perl side of things,
> and then bugs can be made to have invalid statuses or resolutions. Not too big
> of a deal, and sanitycheck catches it easily.
Wouldn't referrential integrity be a much better way to ensure there are no
invalid statuses or resolutions?

Comment 33

[Max Kanat-Alexander]

(In reply to comment #32)
> What about using views to hide the IDs?

  Eh? In MySQL 3? Could you explain that a bit more? :-) Maybe I'm just missing
something.

> I would suggest using a seperate field for sort order; that would allow for
> arbitrary changes without the uglyness of deleting and inserting or changing
> your primary key.

  I totally agree, I'll add that.

> Wouldn't referrential integrity be a much better way to ensure there are no
> invalid statuses or resolutions?

  Yes, if we were using/requiring a database that provided referential
integrity. You'll notice that that is almost the entire purpose of sanitycheck
at this point, to assure referential integrity.

  Also, seeing as how dkl hasn't yet commented on this, and I've emailed him and
he hasn't responded, I'm going to take this since I'm actively working on it and
I really care about it. DKL, you can totally feel free to take it back from me,
if you want.

Comment 34

[Jim C. Nasby]

(In reply to comment #33)
> (In reply to comment #32)
> > What about using views to hide the IDs?
> 
>   Eh? In MySQL 3? Could you explain that a bit more? :-) Maybe I'm just missing
> something.

No, guess not (well, except for a real database ;P)

>   Yes, if we were using/requiring a database that provided referential
> integrity. You'll notice that that is almost the entire purpose of sanitycheck
> at this point, to assure referential integrity.

I think it would be worthwhile to look at supporting real RI once real databases
are supported. Faking RI in the application not only performs worse, it's prone
to breaking. Of course that's beyond the scope of this bug.

Comment 35

[Joel Peshkin]

I'm glad to see someone doing this because I need it as does anyone who makes
hardware and doesn't want to have to upadte the schema every time a new product
ships.

I am a little concerned about using a varchar instead of the id value.  I'm torn
between being a purist and being expedient here.  

Comment 36

[Joel Peshkin]

Comment on attachment 163456 [details] [diff] [review]
v3.1 (Without the extra patch in it)

OK, vladd and I debated this in IRC a bit.  Using the Varchar() is not as much
of a shortcut as it would appear to be because you still have to handle renames
properly etc...

So, we should really do this "right" and reference the ID.  I suggest the
following...

Add ONE new table with columns...
ID - primary key, used for the references
field - the fieldid of the former enum field for which this row applies
name - the text
sortkey - the sort order to put these enume types in order.  In case of a tie,
we go alphabetical, so we always sort by sortkey, name
status - either ENUM_NORMAL, ENUM_DEFAULT, or ENUM_DELETED

Then, the code in checksetup becomes strictly migration code and you need a UI
to create, edit, delete, and rename the types.

The added difficulty of the patch is not so bad once you take into account the
way that fixes renames.

It looks like this is already a P1, so I can't raise it any further.  I wish I
could.

Comment 37

[Vlad Dascalu]

(In reply to comment #36)
> status - either ENUM_NORMAL, ENUM_DEFAULT, or ENUM_DELETED

Per comment #3, we should use something else because this is MySQL specific and
maybe we do it right this time from the beginning :)

Comment 38

[Joel Peshkin]

Actually, that is what I meant.  Those would be constants in Constants.pm

I'm open to suggestions on better names.

Comment 39

[Vlad Dascalu]

Some brainstorming:

Maybe we should create a single table, and an additional column called "Field ID"?

We could have 1 for "Hardware" and 2 for "OS". This would make really really
easy to convert future fields to such said system, and would drive us right into
the implementation of custom fields (or maybe ten kilometers closer :) ).

When implementing the said UI, we could have a param, field, that would specify
that field to edit, something like: editfields.cgi?fieldid=1.
Adding/removing/editing values for drop-down fields would have the advantage to
be made using one centralized UI form.


So, even if we never implement custom fields, having only one table with an
additional FIELD_ID column could have its benifits.

Comment 40

[Joel Peshkin]

There is already a table of field ids... fielddefs

Do we really need a new one?

Comment 41

[Vlad Dascalu]

joel: This would be a table with the options that drop-down fields can take. We
only specify the fieldid so that it associates a possible field value with the
said fieldid.

Comment 42

[Vlad Dascalu]

This sounds like your field column:

field - the fieldid of the former enum field for which this row applies

only that I would say:

field - the fieldid where this drop-down option is possible.

Comment 43

[Joel Peshkin]

Yes, I think we are both saying the same thing.

Comment 44

[Joel Peshkin]

I'd also like to make sure that myk agrees with the schema change.  All the
commonly used Search.pm criteria would look up the numeric values before
starting the search.  The boolean charts might just do the extra join.  This
would add a simple join to fetch displayed fields, but that is relatively minor
since it effects only the results returned.  In general, I think this is all
goodness jut like moving product and components to used ids.

Comment 45

[Jim C. Nasby]

(In reply to comment #39)
> Some brainstorming:
> 
> Maybe we should create a single table, and an additional column called "Field ID"?

There is a downside to this approach: it makes doing refferential integrity a
pain. You'd have to emulate it using triggers.

Comment 46

[Max Kanat-Alexander]

I'm cautious about adding too much "it would be cool if" to this bug -- it's
already been open much too long. :-)

When I first took over this bug, I looked at the code changes required to move
to ids, and it's non-trivial. I don't recall the exact details, but when I got
into it, it was a major modification to the behavior of Bugzilla in certain
places. The basic details were that the enum types were considerably different
than the product/component stuff, particularly as the Bugzilla process flow is
centered around them, touching almost everywhere in the code. (Of course, thanks
to SQL, we should be able to hide some complexity by always returning the
values, and not the ids, to perl.)

Actually, even a hack to do the rename would be a smaller code change than
moving to ids. However, if moving to ids is the "right way" of doing it, then
that's what we'll do.

Comment 47

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

Getting rid of enumerators is the right thing to do.

We should use unique tables for each bug attribute, rather than a single table
for all of them, because that models their distinct real-world meanings.  After
all, status and severity may both be attributes of bugs, but they are
fundamentally different attributes (as are version, milestone, and component)
whose values bear no real-world relationship to each other (unlike the reporter
and assignee attributes, whose values are all Bugzilla users and may even be the
same user).

As for IDs versus values in the bugs table, IDs are the way to go in the long
run, but there's nothing inherently unacceptable about using names temporarily
if they move us forward.  There's a development cost in having to write code to
update the names everywhere they're used when they change as well as to fix the
increased number of bugs in such code; but if switching to names first is much
easier, and it doesn't make switching to IDs in the future much harder, then it
may be a logical first step.

In general, several small changes usually gets us there faster and better than
one big one, although I haven't looked at the code enough to determine if that's
the case here.

Comment 48

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

bbaetz' opinion on the schema issues would also be welcome here.

Comment 49

[Joel Peshkin]

OK, then attachment 163456 [details] [diff] [review] is a lot closer to the right thing than I previously
said.

Let's try...

Make sure that we handle renames properly (I think this means that we use an
administrative GUI rather than localconfig)

Make sure that, when we delete a name from the list, we just deactivate it from
being selected rather than forgetting it was ever valid.  Either that or we
require anything using that value to be changed before deleting a name.

I suggest that the tables holding the legal values follow a clear naming
convention like legal_op_sys and legal_priority, etc....

I think that providing attributes (sortkey and status { NORMAL, DEFAULT, DELETED
} ) in the legal_* table is a good way to go.

I have a hidden agenda here (especially in suggesting the strong naming
convention).  The standard bugzilla will use the same mechanism to manage
op_sys, priority, status, etc...  That should make it very easy to hack in a few
more using the same mechanism.

Comment 50

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

> Make sure that, when we delete a name from the list, we just deactivate it
> from being selected rather than forgetting it was ever valid.  Either that
> or we require anything using that value to be changed before deleting a name.

We do both of these with some other attributes, and I think it would make sense
to do so with these attributes as well.

> I suggest that the tables holding the legal values follow a clear naming
> convention like legal_op_sys and legal_priority, etc....

I agree about the clear naming conventien, but I think we should be consistent
with other tables holding bug attributes like the components, milestones, and
versions tables and just name these after their related bug table columns, i.e.
op_syses and priorities.

In addition to being consistent, those names are simpler and don't imply that
all values in the table are legal (which, if some values can be disabled, seems
misleading and may be inconsistent with how we use the concept of legality in
other parts of Bugzilla).

> I think that providing attributes (sortkey and status { NORMAL, DEFAULT, DELETED
> } ) in the legal_* table is a good way to go.

Note that other Bugzilla tables use a boolean "isactive" column to specify
whether or not a given value can now be associated with bugs.  We should
probably do that here as well.

Comment 51

[Jim C. Nasby]

(In reply to comment #47)
> if they move us forward.  There's a development cost in having to write code to
> update the names everywhere they're used when they change as well as to fix the
> increased number of bugs in such code; but if switching to names first is much
> easier, and it doesn't make switching to IDs in the future much harder, then it
> may be a logical first step.

Doesn't mysql support refferential integrity that propagates updates? I know
PostgreSQL, Sybase, and the 'big 3' all do. That would remove the need for any
extra code to handle renames.

Comment 52

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

MySQL's InnoDB table type supports referential integrity, although I don't know
if it automatically propagates updates.  We don't use that table type at the
moment, although we could and probably should.  That's perhaps a larger change,
however, and it might require more work than we want to do at this stage.

Comment 53

[Max Kanat-Alexander]

OK, then here's a summary of the changes that need to be made to attachment 163456 [details] [diff] [review]:

General
-------

+ We want to add an "isactive" column to each table. isactive=0 values will
still be valid if they show up, but they will not be in lists otherwise. That's
actually going to be a bit complex in editbugs. 

+ The table names must change. In one way, I prefer the legal_* option, as that
makes it easier to look at the schema in a GUI tool, and it groups all our
"field value" tables together in a logical fashion. I know that right now we use
"products" and "components", but we could say that's a legacy issue and go to
legal_* from here on out for enumerated types. On the other hand, there's a
chance that at some point these tables may evolve to be complex like the
"products" and "components" table, and then I'd feel sort of silly having called
them legal_*.

  I'd really like a comment from bbaetz or justdave on the above. Without that,
I'll follow Myk's authority and use the "op_syses" method, which I think also
makes a lot of sense.

+ Add a "sortkey" field that's a smallint.

Renames
-------

+ Renaming a value must be handled gracefully. This will require an interface
much like versions has for products/components. As a side effect, we will
finally be able to set their sort value! Hooray! Unfortunately, there's no way
to conceptualize a "rename" in localconfig, so this interface MUST be added. It
should be easy, mostly copy/paste from the Target Milestone interface.

+ The above change has a side-effect: Dealing with the enum types in
checksetup.pl and localconfig will go away. This means that the first time
checksetup.pl is run, it will create the "default" values (the standard BZ
values) for Resolution, Status, Severity, and Priority. Otherwise, the default
values will be:

Platform: --- (isactive=0), PC, Macintosh, Other, All
Op_sys: --- (isactive=0), Windows, Mac OS, Linux, Other, All

  checksetup.pl will not re-create these values later, as after the first
install they are dealt with by the admin.

  As far as I know, defaults for the values don't really matter, and so don't
need to be dealt with at this point.

  This patch will not allow you to *truly* gracefully rename
resolutions/statuses, as that would require major changes to templates, and
would also require ids. However, the patch will deal with everything but the
templates, keeping the functionality that we have now. A future bug can handle
the rest.

Comment 54

[Dave Miller [:justdave]]

(In reply to comment #53)
> On the other hand, there's a
> chance that at some point these tables may evolve to be complex like the
> "products" and "components" table, and then I'd feel sort of silly having
> called them legal_*.
> 
>   I'd really like a comment from bbaetz or justdave on the above. Without
> that, I'll follow Myk's authority and use the "op_syses" method, which I
> think also makes a lot of sense.

Yeah, I think legal_* sounds funny, too.  Go with the basics :)

>   As far as I know, defaults for the values don't really matter, and so don't
> need to be dealt with at this point.

The only potential issue I can think of is with the browser/OS detection in
enter_bug.cgi...  As long as it gracefully deals with missing options, it should
be okay.  We should probably get good documentation of what the detection stuff
looks for, so if people want those detected, they can add the values that it can
find.

Comment 55

[timeless]

iirc it doesn't fail very gracefully, i don't have time to check atm.

Comment 56

[Joel Peshkin]

We need to find a better suffix than "es." A lot of code will need to get from
the field name to the table name so we need to be 100% consistent about it.
op_syses is fine, but then priority becomes priorityes.  How about "_vals" so we
have "op_sys_vals" and "priority_vals".

I'm open to alternatives, but the consistency is very important.

Comment 57

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

> We need to find a better suffix than "es."

This is usually solved by using singular names, so that the tables would be
op_sys and priority.  Along with making the table names consistent (although not
consistent with existing pluralized names), this makes certain logic dealing
with the tables and their values easier to code.

Singular names are a popular and accepted way of naming tables (and OO classes,
which tables in a relational DB are roughly equivalent to).  In fact, using
singular names has been proposed before by Bugzilla developers, and we use it in
module (Bug.pm, Attachment.pm) and template (template/en/default/bug|attachment)
names.  We should do the same here.

Comment 58

[Bradley Baetz (:bbaetz)]

The biggest issue with this is query performance. Mysql won't use more than one
index per table. You'd have to have query.pm do a lookup first, rather than
doing this as a join. (On other dbs, it should instead be a simple subselect)

Comment 59

[Jim C. Nasby]

(In reply to comment #58)
> The biggest issue with this is query performance. Mysql won't use more than one
> index per table. You'd have to have query.pm do a lookup first, rather than
> doing this as a join. (On other dbs, it should instead be a simple subselect)

I'm not sure what comment this is a reply to, but most databases wouldn't use an
index to scan a tiny lookup table because it would actually be more expensive
than just doing a table-scan.

Comment 60

[Max Kanat-Alexander]

(In reply to comment #58)
> The biggest issue with this is query performance. Mysql won't use more than one
> index per table. You'd have to have query.pm do a lookup first, rather than
> doing this as a join. (On other dbs, it should instead be a simple subselect)

I'm also not entirely sure what this is in reply to, since we'll only be using
one index. When we're using varchars, it will be the varchar index, and when
we're using ids, it will be the id index.

Comment 61

[Bradley Baetz (:bbaetz)]

Create a dummy 100,000 row bus table and compare the perf with a query selecting
on platform and os and something else which already does a join.

In theory you're right, but in practice there are issues.

Comment 62

[Joel Peshkin]

Corrct me if I'm wrong, but using a varchar or an ID (assuming we look up the ID
first rather than doing a join) should be no better and no worse
performance-wise than an ENUM.  Am I wrong?

Comment 63

[Max Kanat-Alexander]

OK, here are the other changes, then, and the final decisions:

+ We will use singular names for the tables.
+ Fix browser/OS detection in enter_bug.cgi (which should be done anyway) to
deal with missing options.

- Query Performance should be identical to sorting against target_milestone as
it works currently, and so should not matter.

There are no other joins that are relevant with the varchar method, other than
the above. Since the current column type in the bugs table stays the same, the
only issue is referential integrity, which does not need to be checked in the
Search.pm query, only in sanitycheck.cgi.

Comment 64

[Max Kanat-Alexander]

Update: I've completed the checksetup changes, on to more.

Notes to self:
  + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
re-create it with the default values. The GUI will just have to disallow
deleting the final value, or always make sure that a "---" isactive=0 value is
in there.

  Also, will I need a new permission bit for the "fieldvalues" edit screen, or
should I just use the "admin" bit?

Comment 65

[Joel Peshkin]

I'll vote for using the existing "admin" group.

Comment 66

[Dave Miller [:justdave]]

I'm coming into this bug late as I'm trying to catch up on bugmail... :)

(In reply to comment #64)
>   + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
> re-create it with the default values. The GUI will just have to disallow
> deleting the final value, or always make sure that a "---" isactive=0 value is
> in there.

I'd go with the "never allow the last value to be deleted" option, personally. 
Need to always require at least one enabled option as well.  If they're all
disabled, then the enter_bug page breaks.

>   Also, will I need a new permission bit for the "fieldvalues" edit screen, or
> should I just use the "admin" bit?

It can probably go under editcomponents for now.  That'll get broken apart more
later when we revamp the admin permissions.

Comment 67

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

>   + An enum table cannot be allowed to be emptied. Otherwise, checksetup will
> re-create it with the default values.

Can't checksetup just check for table existence?


>I'd go with the "never allow the last value to be deleted" option, personally. 
>Need to always require at least one enabled option as well.  If they're all
>disabled, then the enter_bug page breaks.

Seems like fixing the bug in enter_bug that causes it to break on empty lists
would be easier, more robust, and make more sense to an installation admin than
forbidding them from deleting the (randomly selected, based on the pattern of
deletions) last remaining item in the list.

Comment 68

[Max Kanat-Alexander]

(In reply to comment #67)
> Can't checksetup just check for table existence?

  That would be nice. :-)

  The structure of checksetup is such that the table exists before I am allowed
to populate it with data. In fact, "create the table" and "populate it with
data" happen in very different places.

  I have to populate it with the default data if "the user has never populated
it." The only way that I can guess that is if checksetup has just created an
empty table. I suppose I could add some sort of method to checksetup that would
note if it just created the table, but I think that's a complexity beyond the
scope of this patch.

  Anyhow, as justdave pointed out, the tables always need to have one value
anyhow, for enter_bug.

> Seems like fixing the bug in enter_bug that causes it to break on empty lists
> would be easier, more robust, and make more sense to an installation admin 
> than forbidding them from deleting the (randomly selected, based on the 
> pattern of deletions) last remaining item in the list.

  I'm not sure it would make sense to have an empty Version or Operating
System... although maybe I could make them NULL.

  Anyhow, if we'd like, I think we can file another bug for that later. I'm
already changing enough things that I'd like to not change enter_bug if I don't
have to. :-)

Comment 69

[Max Kanat-Alexander]

Attachment: v4.0 Work In Progress (Requires bug 179451) (use -p1)

Here's my work in progress. This is actually a patch against my work on bug
179451, since this work requires that work.

This is here to get comments on what I've done so far. This is a working patch,
you can see the installation currently at http://landfill.bugzilla.org/mkanat/

Basically, this patch has everything but the renaming facility and the actual
use of isactive (which are significant bits in and of themselves). Mostly, it's
just LARGE modifications to checksetup.

Feel free to give me any comments.

Comment 70

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

>   I'm not sure it would make sense to have an empty Version or Operating
> System... although maybe I could make them NULL.

I think it probably would, actually, for some uses of Bugzilla.  Take the
mozilla.org product on b.m.o, for example.

>   Anyhow, if we'd like, I think we can file another bug for that later. I'm
> already changing enough things that I'd like to not change enter_bug if I
don't have to. :-)

Ok, sounds good.

Comment 71

[Dave Miller [:justdave]]

Comment on attachment 167706 [details] [diff] [review]
v4.0 Work In Progress (Requires bug 179451) (use -p1)

>+if (LocalVarExists('severities')) {
>+    print "\nThe \@severities setting in your localconfig file ",
>+          "is no longer used.\nThis data is now stored in the ",
>+          "database, instead.\nWe recommend you remove this ",
>+          "setting from localconfig after\nchecksetup.pl is finished ",
>+          "running.\n";
>+}
>+if (LocalVarExists('priorities')) {
   etc
>+if (LocalVarExists('opsys')) {
   etc
>+if (LocalVarExists('platforms')) {
   etc

Would it make sense here to do the print statement once, inside a routine that
checks if any of those exist?  "The following settings in your localconfig file
are no longer used:" then list them or something.  Since all four are going
away at once, that's going to fill up the whole screen if it prints the same
thing 4 times.

>+my @my_severities;
>+@my_severities = @{*{$main::{'severities'}}{ARRAY}} 
>+    if defined($main::{'severities'});

Do we want to use exists instead of defined here?  It is a hash we're looking
at after all, and the reason this whole "examining the symbol table" thing was
concocted was so we could tell if an empty array existed.  Empty arrays are
considered undefined, but they exist.

>+# Set default values for what used to be the enum types.
>+# These values are no longer stored in localconfig.
>+# However, if we are upgrading from a Bugzilla without enums to a 
>+# Bugzilla with enums, we use the localconfig values one more time.

This comment is backwards.  We're upgrading from a Bugzilla with enums to one
without.

>+my @resolutions = ("","FIXED","INVALID","WONTFIX","LATER","REMIND",
>+                   "DUPLICATE","WORKSFORME","MOVED");
>+PopulateEnumTable('resolution', @resolutions);

>+my @states = ("UNCONFIRMED","NEW","ASSIGNED","REOPENED","RESOLVED",
>+              "VERIFIED","CLOSED");
>+PopulateEnumTable('bug_status', @states);

We should put these in variables up with the others just for consistency.  We
should also make the comments above it (and the other enum values, too) crystal
clear that it's defaults only, and they won't affect the database once you have
a running bugzilla, and use the UI to change them.  We just had someone on the
newsgroup in the last day or two who edited checksetup.pl trying to fix his
enums instead of editing localconfig.

We also need to be careful here...  while I'm thinking about it...  I believe
the MOVED resolution was added later, as was the UNCONFIRMED status (i.e. they
didn't always exist).  We need to test on whether we're importing and removing
the other enums from localconfig or not, and if we are moving from localconfig
to the database, and those values are not present for status and resolution, to
add them, in order to ensure smooth upgrades from early Bugzilla versions.  It
would probably be a really good idea to examine the version history of
checksetup.pl to make sure of this.

Comment 72

[Dave Miller [:justdave]]

Since this is adding the schema necessary to make things retirable (but not
actually adding code to deal with it yet), making the bug for retiring things
depend on this.

Comment 73

[Max Kanat-Alexander]

> We need to test on whether we're importing and removing
> the other enums from localconfig or not, and if we are moving from localconfig
> to the database, and those values are not present for status and resolution, to
> add them, in order to ensure smooth upgrades from early Bugzilla versions.

  I can just add an optional parameter to PopulateEnumTable() that says "If
true, all the values must be in the table," so it will behave a bit more like
the old CheckEnumField. But then people wouldn't be able to delete resolutions
or statuses... is that OK? Can't they not delete them now, anyway?

Comment 74

[Max Kanat-Alexander]

> [snip]
> those values are not present for status and resolution, to
> add them, in order to ensure smooth upgrades from early Bugzilla versions.

  Oh wait, I just realized that the current patch actually already does this.
The user can't specify any preference for what he wants status and resolution to
be, they're just always set to that array. I believe the current checksetup
actually also works this way.

Comment 75

[Max Kanat-Alexander]

Status update: I've incorporated Dave's comments, and I'm about 1/2-way through
working on the interface for updating field values. You can see my work in
progress at: http://landfill.bugzilla.org/mkanat/editvalues.cgi

If you're not a landfill admin, just email me directly with your landfill
account name and I'll grant you editcomponents on my installation.

By the way, thinking about it now, even people with editcomponents should *not*
be able to change the global field values, really... at least, this is a
security change from before, where only somebody with shell access to the
machine ("admin") could do it...

Comment 76

[Dave Miller [:justdave]]

(In reply to comment #74)
>   Oh wait, I just realized that the current patch actually already does this.
> The user can't specify any preference for what he wants status and resolution
> to be, they're just always set to that array. I believe the current checksetup
> actually also works this way.

OK, you're right.  Since we know lots of people *have* customized them though,
we need to make sure it's relnoted that they need to modify that line to contain
the ones they customized before running checksetup.pl or they'll get errors in
the database.

(In reply to comment #75)
> By the way, thinking about it now, even people with editcomponents should
> *not* be able to change the global field values, really... at least, this is
> a security change from before, where only somebody with shell access to the
> machine ("admin") could do it...

But it's something that most folks had wanted to let people do, they just
couldn't.  maybe a 'bz_editfieldvalues' priv?

Comment 77

[Max Kanat-Alexander]

Attachment: v5.0 Ready For Review (Requires bug 179451) (use -p1)

OK, I believe that this version is ready for review.

After careful consideration, I decided that this patch does NOT do certain
things. It does NOT:

+ Create retireable enumeration items
+ Fix up old chart data when you rename something
+ Create any new permissions
+ Fix enter_bug.cgi's broken browser detection

This is because Bugzilla ALREADY does NOT do these things. This patch changes
ONLY what it changes, it does not add any other new functionality to Bugzilla
(except it adds sortkeys to the enums, because that was really easy and
required).

The intention of this patch is to keep Bugzilla functionality the SAME as it is
now, basically, but get us the large architectural benefit of moving away from
enums.

A few questions:

(1) Am I doing something silly or practical with those indexes on the tables?
(2) Did I need to make that modification to the admin/table.html.tmpl template?


Note that this patch is a patch AGAINST the patch on bug 179451. That means I
used diff -Nru to get it, not "cvs diff". So use -p1 when you patch.

Comment 78

[Joel Peshkin]

Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

Just took a quick glance...  One thing jumped out.  Do we want to have any
default contents of the former enum fields?  I'm all for getting it out of
localconfig, but should checksetup put in something when it creates a new DB?

Comment 79

[Max Kanat-Alexander]

(In reply to comment #78)
> (From update of attachment 168741 [details] [diff] [review] [edit])
> Just took a quick glance...  One thing jumped out.  Do we want to have any
> default contents of the former enum fields?  I'm all for getting it out of
> localconfig, but should checksetup put in something when it creates a new DB?

  Oh, it does put in defaults. Look around where we call PopulateEnumTable.

Comment 80

[Max Kanat-Alexander]

Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

I hear that Joel's pretty busy nowadays. :-) Joel, if you review this before
Dave does, and you feel confident about your review, you can clear my request
to Dave. Same to Dave, vice-versa.

Comment 81

[David Lawrence [:dkl]]

(In reply to comment #80)
> (From update of attachment 168741 [details] [diff] [review] [edit])
> I hear that Joel's pretty busy nowadays. :-) Joel, if you review this before
> Dave does, and you feel confident about your review, you can clear my request
> to Dave. Same to Dave, vice-versa.
> 

I do want to look at this. We have been very busy here at RH as well. Good news
is that we will be on a mandatory shutdown from Dec 24th to Jan 3rd so I will
try to get a look at it during that time if Joel doesn't beat me to it. Even if
so, I will still look at it since two heads are better sometimes.

Comment 82

[David Lawrence [:dkl]]

(In reply to comment #81)
> 
> I do want to look at this. We have been very busy here at RH as well. Good news
> is that we will be on a mandatory shutdown from Dec 24th to Jan 3rd so I will
> try to get a look at it during that time if Joel doesn't beat me to it. Even if
> so, I will still look at it since two heads are better sometimes.

Ok, I have been working too hard. I think I just realized that you meant the
other  Dave. Oh well, time to sleep ;)

Comment 83

[Max Kanat-Alexander]

Oh, I could mean you, too, dkl! :-) If you'll review it for me, I'd accept your
review. A lot of it's your code, though. :-)

I'm pretty sure that this patch is good, though, except for the few questions I
posted when I posted it.

Comment 84

[Max Kanat-Alexander]

Comment on attachment 168741 [details] [diff] [review]
v5.0 Ready For Review (Requires bug 179451) (use -p1)

Anybody can review the patch.

Comment 85

[Max Kanat-Alexander]

Attachment: v5.1 Ready for Review

I fixed some bitrot, here.

Also, to make review easier, there's an installation with this patch installed
on landfill, where everybody has the right to modify fields [editcomponents]
(so you can see the admin templates):

http://landfill.bugzilla.org/bz17453/

Comment 86

[Frédéric Buclin]

Comment on attachment 172631 [details] [diff] [review]
v5.1 Ready for Review

Note: I went *very* quickly through this patch, so I may misunderstand some
points. Moreover, I did not read any comment before looking at this patch, so
again, I may say exactly the same (or even the opposite) of what has already
been said. Sorry if it is the case. ;)


>+ChangeFieldType ('bugs', 'bug_status', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'resolution', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'priority', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'bug_severity', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'rep_platform', 'varchar(64) not null');
>+ChangeFieldType ('bugs', 'op_sys', 'varchar(64) not null');

Why don't we store the ID of the different fields here instead of there name?
This way, you don't need to change values in the 'bugs' table everytime you
rename one of this field. Moreover, IIRC we generally prefer to point to field
IDs rather than field names.



>+    unless ($field) {
>+        &::ThrowUserError('fieldname_not_specified');
>+        exit;
>+    }

Is &:: really necessary here?? Moreover, ThrowUserError() already calls 'exit'
so you don't need to repeat it here.



>+unless (UserInGroup("editcomponents")) {
>+    ThrowUserError('auth_cant_edit_fieldvalues');
>+    exit;
>+}

This has been "deprecated" in favor of (see bug 265898):

UserInGroup("editcomponents")
  || ThrowUserError("auth_failure", {group  => "editcomponents",
				     action => "edit",
				     object => "field values"});



>+    # Cleanups and validity checks
>+    unless ($value) {
>+        ThrowUserError('fieldvalue_undefined');
>+        exit;
>+    }

Why not using this synthax:

ThrowUserError() unless $value;

Same remarks for the following checks. Remember that you don't need to write
'exit'.



>+        $dbh->do("UPDATE bugs
>+                  SET $field = ?,
>+                  delta_ts = delta_ts
>+                  WHERE $field = ?",
>+                 undef,
>+                 $value,
>+                 $valueold);

Bah... I really don't like the identation. Why not:

$dbh->do("UPDATE bugs SET $field = ?, delta_ts = delta_ts
	  WHERE $field = ?", undef, $value, $valueold);

Personally, I find it much easier to read. Moreover, delta_ts will *very* soon
change its type, see bug 257315. Just for your information. ;)


>+  [% ELSIF error == "auth_cant_edit_fieldvalues" %]
>+    [% title = "Access Denied" %]
>+    Sorry, you aren't a member of the 'editcomponents' group, and so
>+    you aren't allowed to add, modify or delete field values.

As said above, you should use the existing "auth_failure" error message.

Comment 87

[Max Kanat-Alexander]

(In reply to comment #86)
> Why don't we store the ID of the different fields here instead of there name?

  Because that's an ENOURMOUSLY complex change. We decided to do it this way. We
had a long conversation about this, if you look back in the bug.

> >+    unless ($field) {
> >+        &::ThrowUserError('fieldname_not_specified');
> >+        exit;
> >+    }
> 
> Is &:: really necessary here?? Moreover, ThrowUserError() already calls 'exit'
> so you don't need to repeat it here.

  You're probably right about that. I copied this code straight out of
editmilestones.cgi. :-) This was my first real Bugzilla patch. :-)

> This has been "deprecated" in favor of (see bug 265898):
> 
> UserInGroup("editcomponents")
>   || ThrowUserError("auth_failure", {group  => "editcomponents",
> 				     action => "edit",
> 				     object => "field values"});

  Oh, that's much better, anyway! :-) OK, I'll use that. :-)

> Why not using this synthax:
> 
> ThrowUserError() unless $value;

  Also a much better idea. :-) Or even $value || ThrowUserError, like we do
everywhere else.

> Bah... I really don't like the identation. Why not:
> 
> $dbh->do("UPDATE bugs SET $field = ?, delta_ts = delta_ts
> 	  WHERE $field = ?", undef, $value, $valueold);

  Yeah. I also agree the indentation should be better.

Comment 88

[Max Kanat-Alexander]

Comment on attachment 172631 [details] [diff] [review]
v5.1 Ready for Review

OK, I've decided to split off the admin interface into a different bug.

Comment 89

[Max Kanat-Alexander]

Attachment: v6.1 (Only Remove Enums)

OK, this version only removes the enum fields. The admin interface will be
moved to a separate bug.

Comment 90

[Tomas Kopal]

Just some nits by inspection, I will test it during the weekend. Otherwise it
looks pretty good :-).

> # [e-mail] - bug 17453
> # Set default values for what used to be the enum types.
> # These values are no longer stored in localconfig.
> # However, if we are upgrading from a Bugzilla without enums to a 
> # Bugzilla with enums, we use the localconfig values one more time.

"without enums to a BZ with enums" is reversed :-).

> my $isactive = 1;
> $isactive = 0 if ($defaultinactive{$value});

can be simplyfied to "my $isactive = $defaultinactive{$value};".

> # The resolution and bug_status lists are absolute. On an upgrade from
> # a Bugzilla with enums, whatever is in the enum will be replaced with
> # this. However, after the upgrade, they can be changed at will in the DB.

Hmm, I think we still have some of them hardcoded, so you should mention that
changing some of them later can break bugzilla (what will happen if you e.g.
remove FIXED state? ;-) ).

> # 2000-02-12 Added a new state to bugs, UNCONFIRMED.  Added ability to confirm
> # a vote via bugs.  Added user bits to control which users can confirm bugs
> # by themselves, and which users can edit bugs without their names on them.
> # Added a user field which controls which groups a user can put other users
> # into.

This comment does not seem to correspond to the resolutions enum, so you
probably should not move it with it?

>     print "\nThe following settings in your localconfig file",
>           " are no longer used:\n  " . join(", ", @deprecatedvars) .
>           "\nThis data is now controlled through the Bugzilla",
>           " administrative interface.\nWe reccomend you remove",
>           " these settings from localconfig.\n"

Shouldn't you add that they should be removed after successful run of the
upgrade code? (e.g. something like "... you remove these settings once they are
imported to DB" or something in that sense). If someone gets some error later
and thinks it's because of this, they could remove it too early.

> # If the above returned a enum type, take that type and parse it into the
> # list of values.  Assumes that enums don't ever contain an apostrophe!
> # Note: This function no longer works directly on enum types but instead 
> # accesses separate tables containing enum-like values. This was done to
> # accomplish better compatibility with databases that do not support enum
> # types.

You want to rephrase the first two lines. E.g. apostrophes are fine now.

Comment 91

[Max Kanat-Alexander]

Attachment: v7

OK, here is Version 7, based on Tomas's feedback. I also noticed that
SplitEnumTypes was *only* being used in globals.pl anyway, so I renamed it.

Eventually it will get moved out of globals.pl, but that's a part of bug
110503.

Comment 92

[Joel Peshkin]

Comment on attachment 174110 [details] [diff] [review]
v7

sanitycheck reports invalid values for all the fields that were formerly enums.
 It looks like you just forgot to yank the onld enum type checking code.


Aside from that, it seems to work and it looks clean.

This can be reviewed independently of the admin interface, but this must not
land until both are ready to land since it does eliminate the simple way to
update enum types without adding the new one.

Comment 93

[Max Kanat-Alexander]

Attachment: v8 (Fixed sanitycheck)

Ahh, nice catch. :-) I fixed sanitycheck. There should be no other differences
in this patch.

Comment 94

[Joel Peshkin]

Comment on attachment 174179 [details] [diff] [review]
v8 (Fixed sanitycheck)

r=joel with the condition that this not land until its administration interface
does.

Comment 95

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.351; previous revision: 1.350
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.304; previous revision: 1.303
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.82; previous revision: 1.81
done

Comment 96

[Max Kanat-Alexander]

Added to the release notes in bug 286274.

Comment 97

[Max Kanat-Alexander]

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

Comment 98

[Frédéric Buclin]

Documentation about legal values (editvalues.cgi) has already been updated in bug 281876.