Bug 286527: Cross-DB bz_rename_column and bz_drop_column (Part of Bug 285111)

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=Tomas.Kopal, a=myk

4 files changed, 213 insertions, 27 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index 2168e6ae4..28eb9eed9 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -360,6 +360,19 @@ sub bz_add_column {
 sub bz_alter_column {
     my ($self, $table, $name, $new_def) = @_;
 
+    # You can't change a column to be NOT NULL if you have no DEFAULT,
+    # if there are any NULL values in that column.
+    if ($new_def->{NOTNULL} && !exists $new_def->{DEFAULT}) {
+        # Check for NULLs
+        my $any_nulls = $self->selectrow_array(
+            "SELECT 1 FROM $table WHERE $name IS NULL");
+        if ($any_nulls) {
+            die "You cannot alter the ${table}.${name} column to be NOT NULL"
+                . " without\nspecifying a default, because there are NULL"
+                . " values currently in it.";
+        }
+    }
+
     my $current_def = $self->bz_column_info($table, $name);
 
     if (!$self->_bz_schema->columns_equal($current_def, $new_def)) {
@@ -434,6 +447,23 @@ sub bz_change_field_type ($$$) {
     }
 }
 
+sub bz_drop_column {
+    my ($self, $table, $column) = @_;
+
+    my $current_def = $self->bz_column_info($table, $column);
+
+    if ($current_def) {
+        my @statements = $self->_bz_real_schema->get_drop_column_ddl(
+            $table, $column);
+        print "Deleting unused column $column from table $table ...\n";
+        foreach my $sql (@statements) {
+            $self->do($sql);
+        }
+        $self->_bz_real_schema->delete_column($table, $column);
+        $self->_bz_store_real_schema;
+    }
+}
+
 # XXX - Need to make this cross-db compatible
 # XXX - This shouldn't print stuff to stdout
 sub bz_drop_field ($$) {
@@ -494,6 +524,28 @@ sub bz_drop_table_indexes ($) {
     }
 }
 
+sub bz_rename_column {
+    my ($self, $table, $old_name, $new_name) = @_;
+
+    my $old_col_exists  = $self->bz_column_info($table, $old_name);
+
+    if ($old_col_exists) {
+        my $already_renamed = $self->bz_column_info($table, $new_name);
+        die "Name conflict: Cannot rename ${table}.${old_name} to"
+            . " ${table}.${new_name},\nbecause ${table}.${new_name}"
+            . " already exists." if $already_renamed;
+        my @statements = $self->_bz_real_schema->get_rename_column_ddl(
+            $table, $old_name, $new_name);
+        print "Changing column $old_name in table $table to"
+              . " be named $new_name...\n";
+        foreach my $sql (@statements) {
+            $self->do($sql);
+        }
+        $self->_bz_real_schema->rename_column($table, $old_name, $new_name);
+        $self->_bz_store_real_schema;
+    }
+}
+
 # XXX - Needs to be made cross-db compatible
 sub bz_rename_field ($$$) {
     my ($self, $table, $field, $newname) = @_;
@@ -814,6 +866,9 @@ Bugzilla::DB - Database access routines, using L<DBI>
   $dbh->bz_add_column($table, $name, \%definition);
   $dbh->bz_add_index($table, $name, $definition);
   $dbh->bz_drop_index($table, $name);
+  $dbh->bz_alter_column($table, $name, \%new_def);
+  $dbh->bz_drop_column($table, $column);
+  $dbh->bz_rename_column($table, $old_name, $new_name);
 
   # Schema Modification (DEPRECATED)
   $dbh->bz_add_field($table, $column, $definition);
@@ -1153,6 +1208,40 @@ C<Bugzilla::DB::Schema::ABSTRACT_SCHEMA>.
               $name  - The name of the index that you want to drop.
  Returns:     nothing
 
+=item C<bz_alter_column($table, $name, \%new_def)>
+
+ Description: Changes the data type of a column in a table. Prints out
+              the changes being made to stdout. If the new type is the
+              same as the old type, the function returns without changing
+              anything.
+ Params:      $table   = the table where the column is
+              $name    = the name of the column you want to change
+              $new_def = An abstract column definition for the new 
+                         data type of the columm
+ Returns:     nothing
+
+=item C<bz_drop_column($table, $column)>
+
+ Description: Removes a column from a database table. If the column
+              doesn't exist, we return without doing anything. If we do
+              anything, we print a short message to stdout about the change.
+ Params:      $table  = The table where the column is
+              $column = The name of the column you want to drop
+ Returns:     none
+
+=item C<bz_rename_column($table, $old_name, $new_name)>
+
+ Description: Renames a column in a database table. If the C<$old_name>
+              column doesn't exist, we return without doing anything.
+              If C<$old_name> and C<$new_name> both already exist in the
+              table specified, we fail.
+ Params:      $table    = The table containing the column 
+                          that you want to rename
+              $old_name = The current name of the column that 
+                          you want to rename
+              $new_name = The new name of the column
+ Returns:     nothing
+
 =back
 
 
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 564d8c7a0..054080772 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -1189,32 +1189,29 @@ sub get_type_ddl {
 
 } #eosub--get_type_ddl
 #--------------------------------------------------------------------------
-sub get_column_ddl {
+sub get_column {
+=item C<get_column($table, $column)>
 
-=item C<get_column_ddl>
-
- Description: Public method to generate a DDL segment of a "create table"
-              SQL statement for a given table and field.
+ Description: Public method to get the abstract definition of a column.
  Parameters:  $table - the table name
               $column - a column in the table
- Returns:     a hash containing information about the column including its
+ Returns:     a hashref containing information about the column, including its
               type (C<TYPE>), whether or not it can be null (C<NOTNULL>),
-              its default value if it has one (C<DEFAULT), whether it is
-              a etc. The hash will be empty if either the specified
-              table or column does not exist in the database schema.
+              its default value if it has one (C<DEFAULT), etc.
+              Returns undef if the table or column does not exist.
 
 =cut
 
     my($self, $table, $column) = @_;
 
-    my $thash = $self->{schema}{$table};
-    return() unless ($thash);
-
-    my %fields = @{ $thash->{FIELDS} };
-    return() unless ($fields{$column});
-    return %{ $fields{$column} };
-
-} #eosub--get_column_ddl
+    # Prevent a possible dereferencing of an undef hash, if the
+    # table doesn't exist.
+    if (exists $self->{schema}->{$table}) {
+        my %fields = (@{ $self->{schema}{$table}{FIELDS} });
+        return $fields{$column};
+    }
+    return undef;
+} #eosub--get_column
 #--------------------------------------------------------------------------
 sub get_table_list {
 
@@ -1363,7 +1360,7 @@ sub _get_create_index_ddl {
 
 sub get_add_column_ddl {
 
-=item C<get_alter_ddl($table, $column, \%definition)>
+=item C<get_add_column_ddl($table, $column, \%definition)>
 
  Description: Generate SQL to add a column to a table.
  Params:      $table - The table containing the column.
@@ -1417,7 +1414,7 @@ sub get_add_index_ddl {
 
 sub get_alter_column_ddl {
 
-=item C<get_alter_ddl($table, $column, \%definition)>
+=item C<get_alter_column_ddl($table, $column, \%definition)>
 
  Description: Generate SQL to alter a column in a table.
               The column that you are altering must exist,
@@ -1482,8 +1479,13 @@ sub get_alter_column_ddl {
     # OR if we changed the type and we are NOT NULL
     if ( (!$old_def->{NOTNULL} && $new_def->{NOTNULL}) ||
          ($typechange && $new_def->{NOTNULL}) ) {
+        if (exists $new_def->{DEFAULT}) {
+            # Handle any fields that were NULL before, if we have a default.
+            push(@statements, "UPDATE $table SET $column = $default"
+                            . "  WHERE $column IS NULL");
+        }
         push(@statements, "ALTER TABLE $table ALTER COLUMN $column"
-                          . " SET NOT NULL");
+                        . " SET NOT NULL");
     }
     # If we went from NOT NULL to NULL
     elsif ($old_def->{NOTNULL} && !$new_def->{NOTNULL}) {
@@ -1522,6 +1524,40 @@ sub get_drop_index_ddl {
     return ("DROP INDEX $name");
 }
 
+sub get_drop_column_ddl {
+
+=item C<get_drop_column_ddl($table, $column)>
+
+ Description: Generate SQL to drop a column from a table.
+ Params:      $table - The table containing the column.
+              $column - The name of the column being dropped.
+ Returns:     An array of SQL statements.
+
+=cut
+
+    my ($self, $table, $column) = @_;
+    return ("ALTER TABLE $table DROP COLUMN $column");
+}
+
+sub get_rename_column_ddl {
+
+=item C<get_rename_column_ddl($table, $old_name, $new_name)>
+
+ Description: Generate SQL to change the name of a column in a table.
+              NOTE: ANSI SQL contains no simple way to rename a column,
+                    so this function is ABSTRACT and must be implemented
+                    by subclasses.
+ Params:      $table - The table containing the column to be renamed.
+              $old_name - The name of the column being renamed.
+              $new_name - The name the column is changing to.
+ Returns:     An array of SQL statements.
+
+=cut
+
+    die "ANSI SQL has no way to rename a column, and your database driver\n"
+        . " has not implemented a method.";
+}
+
 sub get_column_abstract {
 
 =item C<get_column_abstract($table, $column)>
@@ -1572,6 +1608,51 @@ sub get_index_abstract {
     return undef;
 }
 
+sub delete_column {
+
+=item C<delete_column($table, $column)>
+
+ Description: Deletes a column from this Schema object.
+ Params:      $table - Name of the table that the column is in.
+                       The table must exist, or we will fail.
+              $column  - Name of the column to delete.
+ Returns:     nothing
+
+=cut
+
+    my ($self, $table, $column) = @_;
+
+    my $abstract_fields = $self->{abstract_schema}{$table}{FIELDS};
+    my $name_position = lsearch($abstract_fields, $column);
+    die "Attempted to delete nonexistent column ${table}.${column}" 
+        if $name_position == -1;
+    # Delete the key/value pair from the array.
+    splice(@$abstract_fields, $name_position, 2);
+
+    $self->{schema} = dclone($self->{abstract_schema});
+    $self->_adjust_schema();
+}
+
+sub rename_column {
+
+=item C<rename_column($table, $old_name, $new_name>
+
+ Description: Renames a column on a table in the Schema object.
+              The column that you are renaming must exist.
+ Params:      $table - The table the column is on.
+              $old_name - The current name of the column.
+              $new_name - The new name of hte column.
+ Returns:     nothing
+
+=cut
+
+    my ($self, $table, $old_name, $new_name) = @_;
+    my $def = $self->get_column_abstract($table, $old_name);
+    die "Renaming a column that doesn't exist" if !$def;
+    $self->delete_column($table, $old_name);
+    $self->set_column($table, $new_name, $def);
+}
+
 sub set_column {
 
 =item C<set_column($table, $column, \%new_def)>
@@ -1592,7 +1673,7 @@ sub set_column {
 
     my ($self, $table, $column, $new_def) = @_;
 
-    my $fields = \@{ $self->{abstract_schema}{$table}{FIELDS} };
+    my $fields = $self->{abstract_schema}{$table}{FIELDS};
     $self->_set_object($table, $column, $new_def, $fields);
 }
 
@@ -1616,7 +1697,7 @@ sub set_index {
 
     my ($self, $table, $name, $definition) = @_;
 
-    my $indexes = \@{ $self->{abstract_schema}{$table}{INDEXES} };
+    my $indexes = $self->{abstract_schema}{$table}{INDEXES};
     $self->_set_object($table, $name, $definition, $indexes);
 }
 
diff --git a/Bugzilla/DB/Schema/Mysql.pm b/Bugzilla/DB/Schema/Mysql.pm
index 6340998b7..07d7036e4 100644
--- a/Bugzilla/DB/Schema/Mysql.pm
+++ b/Bugzilla/DB/Schema/Mysql.pm
@@ -100,11 +100,8 @@ sub _get_create_index_ddl {
 
 # MySQL has a simpler ALTER TABLE syntax than ANSI.
 sub get_alter_column_ddl {
-
     my ($self, $table, $column, $new_def) = @_;
-
     my $new_ddl = $self->get_type_ddl($new_def);
-
     return (("ALTER TABLE $table CHANGE COLUMN $column $column $new_ddl"));
 }
 
@@ -113,4 +110,10 @@ sub get_drop_index_ddl {
     return ("DROP INDEX $name ON $table");
 }
 
+sub get_rename_column_ddl {
+    my ($self, $table, $old_name, $new_name) = @_;
+    my $def = $self->get_type_ddl($self->get_column($table, $old_name));
+    return ("ALTER TABLE $table CHANGE COLUMN $old_name $new_name $def");
+}
+
 1;
diff --git a/Bugzilla/DB/Schema/Pg.pm b/Bugzilla/DB/Schema/Pg.pm
index 59072e79d..d88bc520c 100644
--- a/Bugzilla/DB/Schema/Pg.pm
+++ b/Bugzilla/DB/Schema/Pg.pm
@@ -101,15 +101,22 @@ sub get_add_column_ddl {
     push(@statements, "ALTER TABLE $table ADD COLUMN $column $type");
 
     my $default = $definition->{DEFAULT};
-    # Replace any abstract default value (such as 'TRUE' or 'FALSE')
-    # with its database-specific implementation.
     if (defined $default) {
+        # Replace any abstract default value (such as 'TRUE' or 'FALSE')
+        # with its database-specific implementation.
         $default = $specific->{$default} if exists $specific->{$default};
         push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
                          . " SET DEFAULT $default");
     }
 
     if ($definition->{NOTNULL}) {
+        # Handle rows that were NULL when we added the column.
+        # We *must* have a DEFAULT. This check is usually handled
+        # at a higher level than this code, but I figure it can't
+        # hurt to have it here.
+        die "NOT NULL columns must have a DEFAULT" 
+            unless exists $definition->{DEFAULT};
+        push(@statements, "UPDATE $table SET $column = $default");
         push(@statements, "ALTER TABLE $table ALTER COLUMN $column "
                          . " SET NOT NULL");
     }
@@ -122,4 +129,10 @@ sub get_add_column_ddl {
     return @statements;
 }
 
+sub get_rename_column_ddl {
+    my ($self, $table, $old_name, $new_name) = @_;
+
+    return ("ALTER TABLE $table RENAME COLUMN $old_name TO $new_name");
+}
+
 1;