284529 - Make bz_get_field_def cross-db compatible

Status: RESOLVED WONTFIX

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

bz_get_field_def is the first function that checksetup tries to use after it
creates the table. It's also the most commonly-used schema-info function.

So, I'm going to make it cross-db compatible.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

OK, here's the first version. As you can see, there are little bits of other
patches inside of here -- sorry; my patches are getting so intertwined that I
can't separated them. :-)

This code does indeed make bz_get_field_def cross-db compatible. I decided to
re-work bz_get_field_def to work in a more sensible fashion, so that also
involved other bits of patching.

Comment 2

[Max Kanat-Alexander]

Oh, by the way, I have tested it on MySQL using my checksetup regression script,
and it upgrades 2.10, 2.14, and 2.16 flawlessly, at the least.

PostgreSQL now stops running on bz_get_index_def instead of bz_get_field_def, so
I'm assuming that indicates success. :-)

Comment 3

[Max Kanat-Alexander]

Oh, I thought I should clarify something: I know it upgrades flawlessly because
I used mysqldump to dump the schema between the tip and the upgraded databases,
and they worked perfectly.

Comment 4

[Max Kanat-Alexander]

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

OK, I've tested it, and I confirm that it works. The only change that I need to
make is that:

+	 $type .= " auto_increment" if
$ref_sth->{mysql_is_auto_increment}->[0];

should be:
+	 $type .= " auto_increment" if $ref_sth->{mysql_is_auto_increment}[0];

Although I'm not sure if that's a huge difference.

Comment 5

[Tomas Kopal]

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

>Index: Bugzilla/DB.pm
>===================================================================
>@@ -321,27 +346,63 @@
>+    if ($ref->{COLUMN_NAME} ne $newname) {

Shouldn't this be BZ_TYPE_NAME?

>+        $type .= " auto_increment" if $ref_sth->{mysql_is_auto_increment}->[0];

This is MySQL specific, but that's probably intentional, isn't it?

>+#####################################################################
>+# Schema Information
>+#####################################################################
>+
>+sub _bz_schema {
>+    my ($self) = @_;
>+    eval("require $self->{private_schema_class}")
>+        or die $self->{private_schema_class} . " is not a valid "
>+               . " Bugzilla::Schema:\n  $@";
>+    # Can't use ||= on private_ DBI vars. See DBI docs.
>+    $self->{private_bz_schema} = $self->{private_schema_class}->new
>+        unless $self->{private_bz_schema};
>+    return $self->{private_bz_schema};
>+}

This is again part of different patch :-).

>+    # Append the BZ_TYPE_NAME field.

Append? I understand the code you are initializing the hash member value?

>+    elsif ($field_data->{TYPE_NAME} =~ /text/i) {
>+        return 1;
>+    }
>+
>+   if (!$type_info) {
>+       warn "Type Info Null for this Field: " . Data::Dumper->Dump([$field_data]);
>+   }
>+
>+    return ($field_data->{COLUMN_SIZE} == $type_info->{COLUMN_SIZE});

Nit: Indentation size.

>@@ -753,15 +876,25 @@
>  Description: Returns information about a column in a table in the database.
>  Params:      $table = name of table containing the column (scalar)
>               $column = column you want info about (scalar)
>- Returns:     An reference to an array containing information about the
>-              field, with the following information in each array place:
>-              0 = column name
>-              1 = column type
>-              2 = 'YES' if the column can be NULL, empty string otherwise
>-              3 = The type of key, either 'MUL', 'UNI', 'PRI, or ''.
>-              4 = The default value
>-              5 = An "extra" column, per MySQL docs. Don't use it.
>-              If the column does not exist, the function returns undef.
>+ Returns:     An reference to a hash containing information about the
>+              field, in the format specified in DBI::column_info.
>+              We only guarantee that the following hash keys will contain
>+              accurate data:
>+              C<TABLE_NAME>
>+              C<COLUMN_NAME>
>+              C<NULLABLE>
>+              C<COLUMN_DEF>
>+              C<TYPE_NAME>
>+              C<COLUMN_SIZE>
>+              C<DATA_TYPE>
>+              C<SQL_DATA_TYPE>
>+              C<NUM_PREC_RADIX> will sometimes be defined, for integers.
>+              C<BZ_TYPE_NAME> is a custom field, which contains the 
>+                              lowercase TYPE_NAME with COLUMN_SIZE
>+                              appended if necessary. Examples: "varchar(20)"
>+                              "integer".
>+
>+              If the column does not exist, this method returns undef.

I don't like the term NULLABLE, but I can't come up with anything better :-).
Also, it would be great to have somu description of the constants. The names
are definitely much better than numbers, but they are still not too obvious.

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.357
>diff -u -r1.357 checksetup.pl
>--- checksetup.pl	1 Mar 2005 21:19:19 -0000	1.357
>+++ checksetup.pl	3 Mar 2005 06:56:15 -0000
>@@ -294,11 +294,11 @@
>     }, 
>     { 
>         name => 'DBI', 
>-        version => '1.36' 
>+        version => '1.38' 
>     }, 
>     { 
>         name => 'DBD::mysql', 
>-        version => '2.1010' 
>+        version => '2.9002' 
>     }, 
>     { 
>         name => 'File::Spec', 

Pretty sure this is again part of different patch.


Man, this is dense :-). I will need few more reads to get my head around it,
and a lot of DBI doco reading too. So take this more as a surface scan than in
depth review :-).

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> >+    if ($ref->{COLUMN_NAME} ne $newname) {
> 
> Shouldn't this be BZ_TYPE_NAME?

  Yes. :-)

> >+        $type .= " auto_increment" if $ref_sth->{mysql_is_auto_increment}->[0];
> 
> This is MySQL specific, but that's probably intentional, isn't it?

  Yes. I'll get around to that when I work on the other functions.
 
> This is again part of different patch :-).

  Yep. :-)

> >+    # Append the BZ_TYPE_NAME field.
> 
> Append? I understand the code you are initializing the hash member value?

  No. The hash member values come from DBI::column_info. This one's being added
by me, though. I suppose I could say "add."

> Nit: Indentation size.

  Oh, good catch.

> I don't like the term NULLABLE, but I can't come up with anything better :-).

  Don't worry, you don't have to like it. It's the standard field returned by
DBI::column_info.

> Also, it would be great to have somu description of the constants.

  Quote:

> >+ Returns:     An reference to a hash containing information about the
> >+              field, in the format specified in DBI::column_info.

  I suppose I could make that clearer, though.

> Pretty sure this is again part of different patch.

  The DBI part is. The DBD part is part of this patch. That's why it was hard to
take one out of the other.

> Man, this is dense :-). I will need few more reads to get my head around it,
> and a lot of DBI doco reading too. So take this more as a surface scan than in
> depth review :-).

  Hahahaha. Believe me, *I* needed a lot of work to be able to *write* it. :-) I
might work on it more, too, to make it return something more in the format that
DB::Schema uses. I'm starting to think that that might be more generally useful.

Comment 7

[Max Kanat-Alexander]

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

I think I'm going to do all of this a different way.

Comment 8

[Max Kanat-Alexander]

WONTFIXed in favor of the methods that are part of bug 285111.