373869 - Custom field names must be all lowercase or buglist.cgi sorting throws an error

Status: RESOLVED FIXED

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

Description

[prabir]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: 3.0 rc1

Add a custom field.Search some result. Through change column link add that custom field to the search result.Click on the custom field for sorting the bugs. It is displaying an error screen with message "Bugzilla has suffered an internal error. Please save this page and send it to [e-mail] with details of what you were doing at the time this message appeared. 

URL: http://localhost/buglist.cgi?bug_id=5621%2C5693%2C5694%2C5795%2C5962%2C6564&field-1-0-0=bug_id&query_format=advanced&type-1-0-0=anyexact&value-1-0-0=5621%2C5693%2C5694%2C5795%2C5962%2C6564&order=bugs.cf_Priority%2Cbugs.bug_id&query_based_on=

"
"The custom sort order specified in your form submission contains an invalid column name bugs.cf_Priority. "
But when we add the custom field it create one table called "cf_priority". it has columns id,value,sortkey and isactive.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[prabir]

Attachment: screen shot of the bug

Comment 2

[Max Kanat-Alexander]

This only happens if you make a field name that has uppercase characters.

We should convert all field names to lower case before inserting them into the DB. This should be pretty easy.

Comment 3

[Max Kanat-Alexander]

Attachment: v1

This fixes it, and also fixes it historically for anybody who's already made mixed-case names. I tested the upgrade code on landfill and it works. (I also tested the normal code, and it works.)

Comment 4

[prabir]

But how to change already existing custom fields since many bugs have already
been entered in these fields.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> But how to change already existing custom fields since many bugs have already
> been entered in these fields.

  The patch handles that.

Comment 6

[prabir]

Can we use the same patch in version 2.23.3?

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> Can we use the same patch in version 2.23.3?

  Maybe. It's unlikely. This is a bug, so it's just something that has to be fixed in a future verison. If you're good at perl, you might be able to adapt the patch to your installation, but otherwise you'll have to wait for 3.0 Final to be released. 2.23.3 isn't a stable version anyway, so of course we don't guarantee it to work perfectly in production.

Comment 8

[Frédéric Buclin]

Comment on attachment 258510 [details] [diff] [review]
v1

No, this fix is incorrect. The problem comes from buglist.cgi which uses lc(), see http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/buglist.cgi#850

The right fix is to remove lc() from this line.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 258510 [details] [diff] [review]
v1

(In reply to comment #8)
> The right fix is to remove lc() from this line.

  No, that could be there for a good reason.

  The right fix is to make sure column names are always lowercase, which also makes sense for consistency reasons. Then we can assume we're always dealing with lowercase names.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
>   No, that could be there for a good reason.

Read bug 149845 and tell me what the good reason is.

Comment 11

[Frédéric Buclin]

Comment on attachment 258510 [details] [diff] [review]
v1

As I said on IRC, tables are not renamed:

DBD::mysql::db selectall_arrayref failed: Table 'bugs_cvs.cf_test' doesn't exist

it should be bugs_cvs.cf_TesT

Comment 12

[Max Kanat-Alexander]

Attachment: v2

Thanks for catching that.

The patch is now unfortunately larger, but the additions are largely POD. (The actual functions added are simple.)

Comment 13

[Max Kanat-Alexander]

Attachment: v3

I had forgotten to actually rename the "column_rename" error to "db_rename."

Comment 14

[Frédéric Buclin]

Comment on attachment 259485 [details] [diff] [review]
v3

Your patch fails on PostgreSQL 8.1.8:

Renaming column 'bugs.cf_TesT' to 'bugs.cf_test'...
DBD::Pg::db do failed: ERREUR:  la colonne «cf_test» de la relation «bugs» existe déjà
 at Bugzilla/DB.pm line 763
        Bugzilla::DB::bz_rename_column('Bugzilla::DB::Pg=HASH(0x97a5124)', 'bugs', 'cf_TesT', 'cf_test') called at Bugzilla/Install/DB.pm line 2746
        Bugzilla::Install::DB::_fix_uppercase_custom_field_names() called at Bugzilla/Install/DB.pm line 506
        Bugzilla::Install::DB::update_table_definitions() called at ./checksetup.pl line 196


Here is what I can see from psql:

bugs_cvs=> select name from fielddefs;
 cf_TesT

bugs_cvs=> \d
 public | cf_test                     | table    | bugs
 public | cf_test_id_seq              | séquence | bugs

bugs_cvs=> \d bugs
 cf_test             | character varying(64)          | not null default '---'::character varying

So it seems that only the field name is uppercase; the custom column in the 'bugs' table and the custom table themselves seem to be lowercase already.

Note that this problem doesn't occur with MySQL 5.0.24, because the custom column and table both are uppercase, as expected (note sure about MySQL 4.1).

Comment 15

[Frédéric Buclin]

This introduces another problem on PostgreSQL: custom fields having a name being uppercase are not displayed correctly in show_bug, because Bug::fields() contains the uppercase version of the field name (it looks at the fielddefs table), while the bugs table contains the lowercase version of the field name, and so these custom fields are left empty in the UI (or set to their default). So if you click the "Commit" button again, you will override all the current values. If you don't have enough privs, you will get an error telling you that you tried to edit these values.

Comment 16

[Max Kanat-Alexander]

Attachment: v4

Okay, you're right. :-) PostgreSQL *always* had lowercase column and table names, so we only need to run that code on MySQL.

Comment 17

[Frédéric Buclin]

Comment on attachment 260076 [details] [diff] [review]
v4

>Index: Bugzilla/Install/DB.pm

>+sub _fix_uppercase_custom_field_names {

>+            if ($dbh->isa('Bugzilla::DB::Mysql')) {
>+                $dbh->bz_rename_column('bugs', $name, lc($name));
>+                $dbh->bz_rename_table($name, lc($name))
>+                    if $type == FIELD_TYPE_SINGLE_SELECT;
>+            }

There is something wrong with this patch. Bugzilla::Bug::editable_bug_fields() still returns cf_TesT despite the entry in fielddefs is now lowercase and bugs.cf_test is lowercase too. It looks like editable_bug_fields() isn't looking at the bugs table for real but returns some cached data (bz_schema?). Could it be because you don't rename the column in the bugs table as above and bz_schema stored the uppercase version of the column name despite PostgreSQL stored the lowercase one? This would explain the mismatch.

Comment 18

[Dave Miller [:justdave]]

so just to make sure I have all the facts straight...

MySQL has case-sensitive column names (for display purposes).  Our internal schema storage is case-sensitive.  Postgres always stores columns in lower-case no matter what.  If you try to change the case of a column name in postgres, it errors claiming it already has that name...  If you don't change it on postgres, then you wind up with a mismatch between our internal storage and what's in fielddefs.

So what we really need to do is to go ahead and rename the columns...  and special-case the postgres rename to check if the only change being made is a case-change and just skip telling the DB in that case.

Comment 19

[Dave Miller [:justdave]]

Attachment: v5

This is v3 with the addition of overriding Bugzilla::DB::Schema::Pg::get_rename_table_sql() to return nothing if the only change is a case change.

Comment 20

[Dave Miller [:justdave]]

Attachment: v5.1

override the column change thing as well (since that was the original intent).  Leaving table in, too, because mkanat says it has the same behavior so we might as well leave it safe. :)

Comment 21

[Frédéric Buclin]

Comment on attachment 261572 [details] [diff] [review]
v5.1

Tested successfully on both CVS tip and 3.0rc1+, with MySQL 5.0.24 and PostgreSQL 8.1.8. r=LpSolit

Comment 22

[Max Kanat-Alexander]

Comment on attachment 261572 [details] [diff] [review]
v5.1

Okay! Surprising that it lets us rename tables to their own name, but not columns.

Comment 23

[Dave Miller [:justdave]]

3.0 branch:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.93.2.1; previous revision: 1.93
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.24.2.1; previous revision: 1.24
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.80.2.1; previous revision: 1.80
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.13.2.1; previous revision: 1.13
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.26.2.1; previous revision: 1.26
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.92.2.1; previous revision: 1.92
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.49.2.2; previous revision: 1.49.2.1
done

trunk:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.99; previous revision: 1.98
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.84; previous revision: 1.83
done
Checking in Bugzilla/DB/Schema/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema/Pg.pm,v  <--  Pg.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.31; previous revision: 1.30
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.95; previous revision: 1.94
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.55; previous revision: 1.54
done

Comment 24

[Max Kanat-Alexander]

Added to relnotes in bug 379777.