287483 - Sanitycheck screams about "Bad value 0 found in components.initialqacontact"

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect, P1)

Description

[Teemu Mannermaa (:wicked)]

After I updated to CVS tip moments ago sanitycheck.cgi started to yell "Bad
value 0 found in components.initialqacontact" for every component there is. This
installation doesn't use QA contact field.

Looks like this is due to changes done in bug 285534. Among other things it told
sanitycheck to not allow zero for initialqacontact in components table and added
following block of migration code to checksetup.pl line 3877:

# 2005-03-09 qa_contact should be NULL instead of 0, bug 285534
if (!$dbh->bz_get_field_def('bugs', 'qa_contact')->[2]) { # if it's NOT NULL
    $dbh->bz_change_field_type('bugs', 'qa_contact', 'mediumint');
    $dbh->do("UPDATE bugs SET qa_contact = NULL WHERE qa_contact = 0");
    $dbh->do("UPDATE components SET initialqacontact = NULL
               WHERE initialqacontact = 0");
}

At first glance it would seem that it changes all initialqacontact fields to be
NULL instead of zero BUT unfortunately schema has not been changed to allow this
field to be NULL. Only change was to allow qa_contact in bugs table to be NULL.

Doesn't $dbh->do() make sure the SQL runs succesfully? When I try the latter
UPDATE in MySQL command line it doesn't change anything but simply states that
no changes were dones and there were warnings. (It didn't tell me what the
warning were, dunno how I can get it to say them.)

Mkanat, what were you trying to do? Should initialqacontact in components too
allow NULLs or not? Either checksetup/schema or sanitycheck should be fixed to
follow your intent.

Comment 1

[Max Kanat-Alexander]

My initialqacontact already does allow NULL:

initialqacontact => {TYPE => 'INT3'},

Yes, $dbh->do will die hard if the SQL fails.

Are you sure that your schema doesn't allow null for initialqacontact? That's
very strange... how might it have gotten that way?

Comment 2

[Teemu Mannermaa (:wicked)]

My CVS tip test install does have the line you quoted in Bugzilla/DB/Schema.pm.
But even after running checksetup.pl the actual DB doesn't allow nulls for
initialqacontact. Shouldn't there be migration code that runs

 $dbh->bz_change_field_type('component', 'initialqacontact', 'mediumint');

to actually change the DB? Just like qa_contact field in bugs table is changed.

This is what components table has:

| Field            | Type         | Null | Key | Default | Extra          |
| initialqacontact | mediumint(9) |      |     | 0       |                |

For reference, this is what bugs table has:

| Field               | Type         | Null | Key | Default             | Extra 
| qa_contact          | mediumint(9) | YES  | MUL | NULL                |       

Also, $dbh->do() seems to just ignore any warnings the UPDATE produces..

Comment 3

[Max Kanat-Alexander]

Ahhhh. This was a regression caused by checking in the Schema module. I must
have missed that one; intialqacontact used to be NOT NULL, and we silently
changed it to NULL. I tried to catch all such accidental changes before we
checked in the Schema, but I guess I missed that one.

OK, so that needs to be handled.

Comment 4

[Teemu Mannermaa (:wicked)]

Attachment: Schema and editcomponents fix, V1

This patch adds a check to checksetup.pl to detect initialqacontact field of
components table that does not allow NULLs. If found, converts the field to
mediumint and updates any zeros (previous default) to NULLs (current default).

I tested this and it converted schema and data correctly. Also sanitycheck is
happy after this has run. Did I forgot something?

Additionally, this fixes editcomponents to use NULL instead of zero for marking
components that have no QA contact defined. Previously sanitycheck started
complaining if QA contact was removed because initialqacontact reverted back to
zero.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 178734 [details] [diff] [review]
Schema and editcomponents fix, V1

>+# 2005-03-27 initialqacontact should be NULL instead of 0, bug 287483
>+if (!$dbh->bz_get_field_def('components',
>+                            'initialqacontact')->[2]) { # if it's NOT NULL
>+    $dbh->bz_change_field_type('components', 'initialqacontact', 'mediumint');
>+    $dbh->do("UPDATE components SET initialqacontact = NULL " .
>+             "WHERE initialqacontact = 0");

  I remember being concerned about doing this this way, but now that I think
about it I can't come up with any problem with it. :-) So I guess it's OK! :-)

>@@ -611,9 +613,11 @@
>             ThrowUserError('component_need_valid_initialqacontact',
>                            {'name' => $componentold});
>         }
>+        my $initialqacontactsql =
>+              $initialqacontact ne '' ? SqlQuote($initialqacontactid) : 'NULL';

  Has $initialqacontact already been trimmed, here? I would assume that it has,
so the '' check will actually work...?

  If you renamed the variable to initial_qa_contact_sql, I'd be happier. :-)

  Otherwise, this looks correct on inspection.

Comment 6

[Teemu Mannermaa (:wicked)]

(In reply to comment #5)
> >+        my $initialqacontactsql =
> >+              $initialqacontact ne '' ? SqlQuote($initialqacontactid) :
>   Has $initialqacontact already been trimmed, here? I would assume that it
> has, so the '' check will actually work...?

Yeah, it has been trimmed and there is already such check in the old code. In
both places too.

>   If you renamed the variable to initial_qa_contact_sql, I'd be happier. :-)

I just followed existing variable naming of that file. :)

Comment 7

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.382; previous revision: 1.381
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.52; previous revision: 1.51
done