If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
P1
major
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: prabir, Assigned: justdave)

Tracking

Bugzilla 3.0
Bug Flags:
approval +
approval3.0 +
blocking3.0 +

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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 prabirranjan@yahoo.com 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.
(Reporter)

Comment 1

11 years ago
Created attachment 258509 [details]
screen shot of the bug

Comment 2

11 years ago
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.
Severity: critical → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking3.0+
OS: Windows 2000 → All
Priority: -- → P1
Hardware: PC → All
Summary: In search result screen sort the bugs by any custom field displays an error screen → Custom field names must be all lowercase or buglist.cgi sorting throws an error
Target Milestone: --- → Bugzilla 3.0

Comment 3

11 years ago
Created attachment 258510 [details] [diff] [review]
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.)
Assignee: general → mkanat
Status: NEW → ASSIGNED
Attachment #258510 - Flags: review?(LpSolit)

Updated

11 years ago
Keywords: relnote
(Reporter)

Comment 4

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

Comment 5

11 years ago
(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.
(Reporter)

Comment 6

11 years ago
Can we use the same patch in version 2.23.3?

Comment 7

11 years ago
(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

11 years ago
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.
Attachment #258510 - Flags: review?(LpSolit) → review-

Comment 9

11 years ago
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.
Attachment #258510 - Flags: review- → review?(LpSolit)

Comment 10

11 years ago
(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

11 years ago
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
Attachment #258510 - Flags: review?(LpSolit) → review-

Comment 12

11 years ago
Created attachment 259484 [details] [diff] [review]
v2

Thanks for catching that.

The patch is now unfortunately larger, but the additions are largely POD. (The actual functions added are simple.)
Attachment #258510 - Attachment is obsolete: true
Attachment #259484 - Flags: review?(LpSolit)

Comment 13

11 years ago
Created attachment 259485 [details] [diff] [review]
v3

I had forgotten to actually rename the "column_rename" error to "db_rename."
Attachment #259484 - Attachment is obsolete: true
Attachment #259485 - Flags: review?(LpSolit)
Attachment #259484 - Flags: review?(LpSolit)

Comment 14

11 years ago
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).
Attachment #259485 - Flags: review?(LpSolit) → review-

Comment 15

11 years ago
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.

Updated

11 years ago
Version: unspecified → 3.0

Comment 16

11 years ago
Created attachment 260076 [details] [diff] [review]
v4

Okay, you're right. :-) PostgreSQL *always* had lowercase column and table names, so we only need to run that code on MySQL.
Attachment #259485 - Attachment is obsolete: true
Attachment #260076 - Flags: review?(LpSolit)

Comment 17

11 years ago
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.
Attachment #260076 - Flags: review?(LpSolit) → review-
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.
Created attachment 261570 [details] [diff] [review]
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.
Assignee: mkanat → justdave
Attachment #260076 - Attachment is obsolete: true
Attachment #261570 - Flags: review?(LpSolit)
Assignee: justdave → justdave
Status: ASSIGNED → NEW
Created attachment 261572 [details] [diff] [review]
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. :)
Attachment #261570 - Attachment is obsolete: true
Attachment #261572 - Flags: review?(LpSolit)
Attachment #261570 - Flags: review?(LpSolit)

Comment 21

11 years ago
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
Attachment #261572 - Flags: review?(mkanat)
Attachment #261572 - Flags: review?(LpSolit)
Attachment #261572 - Flags: review+

Comment 22

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

Okay! Surprising that it lets us rename tables to their own name, but not columns.
Attachment #261572 - Flags: review?(mkanat) → review+

Updated

11 years ago
Flags: approval3.0+
Flags: approval+
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Blocks: 377564

Comment 24

11 years ago
Added to relnotes in bug 379777.
Keywords: relnote

Updated

9 years ago
Duplicate of this bug: 453434
You need to log in before you can comment on or make changes to this bug.