Bug 578739: Instead of removing REFERENCES from _bz_real_schema and then

populating FKs from _bz_schema at the end of checksetup, store REFERENCES
in _bz_real_schema with a special "created => 0" key that tells us that
we still need to create the FK.
r=mkanat, a=mkanat (module owner)

6 files changed, 145 insertions, 62 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index eeeff2280..31051dd8e 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -421,7 +421,10 @@ sub bz_setup_database {
     # If we haven't ever stored a serialized schema,
     # set up the bz_schema table and store it.
     $self->_bz_init_schema_storage();
-    
+   
+    # We don't use bz_table_list here, because that uses _bz_real_schema.
+    # We actually want the table list from the ABSTRACT_SCHEMA in
+    # Bugzilla::DB::Schema.
     my @desired_tables = $self->_bz_schema->get_table_list();
     my $bugs_exists = $self->bz_table_info('bugs');
     if (!$bugs_exists) {
@@ -460,23 +463,38 @@ sub bz_setup_foreign_keys {
     # so if it doesn't have them, then we're setting up FKs
     # for the first time, and should be quieter about it.
     my $activity_fk = $self->bz_fk_info('profiles_activity', 'userid');
-    if (!$activity_fk) {
+    my $any_fks = $activity_fk && $activity_fk->{created};
+    if (!$any_fks) {
         print get_text('install_fk_setup'), "\n";
     }
 
-    # We use _bz_schema because bz_add_table has removed all REFERENCES
-    # items from _bz_real_schema.
-    my @tables = $self->_bz_schema->get_table_list();
+    my @tables = $self->bz_table_list();
     foreach my $table (@tables) {
-        my @columns = $self->_bz_schema->get_table_columns($table);
+        my @columns = $self->bz_table_columns($table);
         my %add_fks;
         foreach my $column (@columns) {
-            my $def = $self->_bz_schema->get_column_abstract($table, $column);
-            if ($def->{REFERENCES}) {
-                $add_fks{$column} = $def->{REFERENCES};
+            # First we check for any FKs that have created => 0,
+            # in the _bz_real_schema. This also picks up FKs with
+            # created => 1, but bz_add_fks will ignore those.
+            my $fk = $self->bz_fk_info($table, $column);
+            # Then we check the abstract schema to see if there
+            # should be an FK on this column, but one wasn't set in the
+            # _bz_real_schema for some reason. We do this to handle
+            # various problems caused by upgrading from versions
+            # prior to 4.2, and also to handle problems caused
+            # by enabling an extension pre-4.2, disabling it for
+            # the 4.2 upgrade, and then re-enabling it later.
+            if (!$fk) {
+                my $standard_def = 
+                    $self->_bz_schema->get_column_abstract($table, $column);
+                if (exists $standard_def->{REFERENCES}) {
+                    $fk = dclone($standard_def->{REFERENCES});
+                }
             }
+
+            $add_fks{$column} = $fk if $fk;
         }
-        $self->bz_add_fks($table, \%add_fks, { silently => !$activity_fk });
+        $self->bz_add_fks($table, \%add_fks, { silently => !$any_fks });
     }
 }
 
@@ -484,9 +502,9 @@ sub bz_setup_foreign_keys {
 sub bz_drop_foreign_keys {
     my ($self) = @_;
 
-    my @tables = $self->_bz_real_schema->get_table_list();
+    my @tables = $self->bz_table_list();
     foreach my $table (@tables) {
-        my @columns = $self->_bz_real_schema->get_table_columns($table);
+        my @columns = $self->bz_table_columns($table);
         foreach my $column (@columns) {
             $self->bz_drop_fk($table, $column);
         }
@@ -523,6 +541,20 @@ sub bz_add_column {
         foreach my $sql (@statements) {
             $self->do($sql);
         }
+
+        # To make things easier for callers, if they don't specify
+        # a REFERENCES item, we pull it from the _bz_schema if the
+        # column exists there and has a REFERENCES item.
+        # bz_setup_foreign_keys will then add this FK at the end of
+        # Install::DB.
+        my $col_abstract = 
+            $self->_bz_schema->get_column_abstract($table, $name);
+        if (exists $col_abstract->{REFERENCES}) {
+            my $new_fk = dclone($col_abstract->{REFERENCES});
+            $new_fk->{created} = 0;
+            $new_def->{REFERENCES} = $new_fk;
+        }
+        
         $self->_bz_real_schema->set_column($table, $name, $new_def);
         $self->_bz_store_real_schema;
     }
@@ -538,17 +570,17 @@ sub bz_add_fks {
 
     my %add_these;
     foreach my $column (keys %$column_fks) {
-        my $col_def = $self->bz_column_info($table, $column);
-        next if $col_def->{REFERENCES};
-        my $fk = $column_fks->{$column};
-        $self->_check_references($table, $column, $fk);
-        $add_these{$column} = $fk;
+        my $current_fk = $self->bz_fk_info($table, $column);
+        next if ($current_fk and $current_fk->{created});
+        my $new_fk = $column_fks->{$column};
+        $self->_check_references($table, $column, $new_fk);
+        $add_these{$column} = $new_fk;
         if (Bugzilla->usage_mode == USAGE_MODE_CMDLINE 
             and !$options->{silently}) 
         {
             print get_text('install_fk_add',
-                           { table => $table, column => $column, fk => $fk }),
-                  "\n";
+                           { table => $table, column => $column, 
+                             fk    => $new_fk }), "\n";
         }
     }
 
@@ -558,9 +590,9 @@ sub bz_add_fks {
     $self->do($_) foreach @sql;
 
     foreach my $column (keys %add_these) {
-        my $col_def = $self->bz_column_info($table, $column);
-        $col_def->{REFERENCES} = $add_these{$column};
-        $self->_bz_real_schema->set_column($table, $column, $col_def);
+        my $fk_def = $add_these{$column};
+        $fk_def->{created} = 1;
+        $self->_bz_real_schema->set_fk($table, $column, $fk_def);
     }
 
     $self->_bz_store_real_schema();
@@ -586,10 +618,8 @@ sub bz_alter_column {
         }
         # Preserve foreign key definitions in the Schema object when altering
         # types.
-        if (defined $current_def->{REFERENCES}) {
-            # Make sure we don't modify the caller's $new_def.
-            $new_def = dclone($new_def);
-            $new_def->{REFERENCES} = $current_def->{REFERENCES};
+        if (my $fk = $self->bz_fk_info($table, $name)) {
+            $new_def->{REFERENCES} = $fk;
         }
         $self->bz_alter_column_raw($table, $name, $new_def, $current_def,
                                    $set_nulls_to);
@@ -635,6 +665,15 @@ sub bz_alter_column_raw {
     $self->do($_) foreach (@statements);
 }
 
+sub bz_alter_fk {
+    my ($self, $table, $column, $fk_def) = @_;
+    my $current_fk = $self->bz_fk_info($table, $column);
+    ThrowCodeError('column_alter_nonexistent_fk',
+                   { table => $table, column => $column }) if !$current_fk;
+    $self->bz_drop_fk($table, $column);
+    $self->bz_add_fk($table, $column, $fk_def);
+}
+
 sub bz_add_index {
     my ($self, $table, $name, $definition) = @_;
 
@@ -686,7 +725,9 @@ sub bz_add_table {
             # initial table creation, because column names have changed
             # over history and it's impossible to keep track of that info
             # in ABSTRACT_SCHEMA.
-            delete $fields{$col}->{REFERENCES};
+            if (exists $fields{$col}->{REFERENCES}) {
+                $fields{$col}->{REFERENCES}->{created} = 0;
+            }
         }
         $self->_bz_real_schema->add_table($name, $table_def);
         $self->_bz_store_real_schema;
@@ -790,22 +831,25 @@ sub bz_drop_column {
 sub bz_drop_fk {
     my ($self, $table, $column) = @_;
 
-    my $col_def = $self->bz_column_info($table, $column);
-    if ($col_def && exists $col_def->{REFERENCES}) {
-        my $def = $col_def->{REFERENCES};
+    my $fk_def = $self->bz_fk_info($table, $column);
+    if ($fk_def and $fk_def->{created}) {
         print get_text('install_fk_drop',
-                       { table => $table, column => $column, fk => $def })
+                       { table => $table, column => $column, fk => $fk_def })
             . "\n" if Bugzilla->usage_mode == USAGE_MODE_CMDLINE;
         my @statements = 
-            $self->_bz_real_schema->get_drop_fk_sql($table,$column,$def);
+            $self->_bz_real_schema->get_drop_fk_sql($table, $column, $fk_def);
         foreach my $sql (@statements) {
             # Because this is a deletion, we don't want to die hard if
             # we fail because of some local customization. If something
             # is already gone, that's fine with us!
             eval { $self->do($sql); } or warn "Failed SQL: [$sql] Error: $@";
         }
-        delete $col_def->{REFERENCES};
-        $self->_bz_real_schema->set_column($table, $column, $col_def);
+        # Under normal circumstances, we don't permanently drop the fk--
+        # we want checksetup to re-create it again later. The only
+        # time that FKs get permanently dropped is if the column gets
+        # dropped.
+        $fk_def->{created} = 0;
+        $self->_bz_real_schema->set_fk($table, $column, $fk_def);
         $self->_bz_store_real_schema;
     }
 
@@ -818,8 +862,7 @@ sub bz_get_related_fks {
     foreach my $check_table (@tables) {
         my @columns = $self->bz_table_columns($check_table);
         foreach my $check_column (@columns) {
-            my $def = $self->bz_column_info($check_table, $check_column);
-            my $fk = $def->{REFERENCES};
+            my $fk = $self->bz_fk_info($check_table, $check_column);
             if ($fk 
                 and (($fk->{TABLE} eq $table and $fk->{COLUMN} eq $column)
                      or ($check_column eq $column and $check_table eq $table)))
@@ -1031,6 +1074,11 @@ sub bz_table_indexes {
     return \%return_indexes;
 }
 
+sub bz_table_list {
+    my ($self) = @_;
+    return $self->_bz_real_schema->get_table_list();
+}
+
 #####################################################################
 # Protected "Real Database" Schema Information Methods
 #####################################################################
diff --git a/Bugzilla/DB/Mysql.pm b/Bugzilla/DB/Mysql.pm
index 66a261c75..699fcfdf6 100644
--- a/Bugzilla/DB/Mysql.pm
+++ b/Bugzilla/DB/Mysql.pm
@@ -721,7 +721,6 @@ EOT
 
         print "Converting table storage format to UTF-8. This may take a",
               " while.\n";
-        my @dropped_fks;
         foreach my $table ($self->bz_table_list_real) {
             my $info_sth = $self->prepare("SHOW FULL COLUMNS FROM $table");
             $info_sth->execute();
@@ -740,8 +739,9 @@ EOT
 
                     print "$table.$name needs to be converted to UTF-8...\n";
 
-                    my $dropped = $self->bz_drop_related_fks($table, $name);
-                    push(@dropped_fks, @$dropped);
+                    # These will be automatically re-created at the end
+                    # of checksetup.
+                    $self->bz_drop_related_fks($table, $name);
 
                     my $col_info =
                         $self->bz_column_info_real($table, $name);
@@ -792,10 +792,6 @@ EOT
             }
 
         } # foreach my $table (@tables)
-
-        foreach my $fk_args (@dropped_fks) {
-            $self->bz_add_fk(@$fk_args);
-        }
     }
 
     # Sometimes you can have a situation where all the tables are utf8,
diff --git a/Bugzilla/DB/Oracle.pm b/Bugzilla/DB/Oracle.pm
index 0819bd19a..a7ac6e93e 100644
--- a/Bugzilla/DB/Oracle.pm
+++ b/Bugzilla/DB/Oracle.pm
@@ -247,7 +247,7 @@ sub bz_drop_table {
 # Dropping all FKs for a specified table. 
 sub _bz_drop_fks {
     my ($self, $table) = @_;
-    my @columns = $self->_bz_real_schema->get_table_columns($table);
+    my @columns = $self->bz_table_columns($table);
     foreach my $column (@columns) {
         $self->bz_drop_fk($table, $column);
     }
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 4837ccc5f..a00a7b9d9 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -44,12 +44,12 @@ use Bugzilla::Constants;
 use Carp qw(confess);
 use Digest::MD5 qw(md5_hex);
 use Hash::Util qw(lock_value unlock_hash lock_keys unlock_keys);
-use List::MoreUtils qw(firstidx);
+use List::MoreUtils qw(firstidx natatime);
 use Safe;
 # Historical, needed for SCHEMA_VERSION = '1.00'
 use Storable qw(dclone freeze thaw);
 
-# New SCHEMA_VERSION (2.00) use this
+# New SCHEMA_VERSIONs (2+) use this
 use Data::Dumper;
 
 =head1 NAME
@@ -208,7 +208,7 @@ update this column in this table."
 
 =cut
 
-use constant SCHEMA_VERSION  => '2.00';
+use constant SCHEMA_VERSION  => 3;
 use constant ADD_COLUMN      => 'ADD COLUMN';
 # Multiple FKs can be added using ALTER TABLE ADD CONSTRAINT in one
 # SQL statement. This isn't true for all databases.
@@ -2545,6 +2545,28 @@ sub set_column {
     $self->_set_object($table, $column, $new_def, $fields);
 }
 
+=item C<set_fk($table, $column \%fk_def)>
+
+Sets the C<REFERENCES> item on the specified column.
+
+=cut
+
+sub set_fk {
+    my ($self, $table, $column, $fk_def) = @_;
+    # Don't want to modify the source def before we explicitly set it below.
+    # This is just us being extra-cautious.
+    my $column_def = dclone($self->get_column_abstract($table, $column));
+    die "Tried to set an fk on $table.$column, but that column doesn't exist"
+        if !$column_def;
+    if ($fk_def) {
+        $column_def->{REFERENCES} = $fk_def;
+    }
+    else {
+        delete $column_def->{REFERENCES};
+    }
+    $self->set_column($table, $column, $column_def);
+}
+
 sub set_index {
 
 =item C<set_index($table, $name, $definition)>
@@ -2697,7 +2719,7 @@ sub serialize_abstract {
  Description: Used for when you've read a serialized Schema off the disk,
               and you want a Schema object that represents that data.
  Params:      $serialized - scalar. The serialized data.
-              $version - A number in the format X.YZ. The "version"
+              $version - A number. The "version"
                   of the Schema that did the serialization.
                   See the docs for C<SCHEMA_VERSION> for more details.
  Returns:     A Schema object. It will have the methods of (and work 
@@ -2710,7 +2732,7 @@ sub deserialize_abstract {
     my ($class, $serialized, $version) = @_;
 
     my $thawed_hash;
-    if (int($version) < 2) {
+    if ($version < 2) {
         $thawed_hash = thaw($serialized);
     }
     else {
@@ -2720,6 +2742,22 @@ sub deserialize_abstract {
         $thawed_hash = ${$cpt->varglob('VAR1')};
     }
 
+    # Version 2 didn't have the "created" key for REFERENCES items.
+    if ($version < 3) {
+        my $standard = $class->new()->{abstract_schema};
+        foreach my $table_name (keys %$thawed_hash) {
+            my %standard_fields = 
+                @{ $standard->{$table_name}->{FIELDS} || [] };
+            my $table = $thawed_hash->{$table_name};
+            my %fields = @{ $table->{FIELDS} || [] };
+            while (my ($field, $def) = each %fields) {
+                if (exists $def->{REFERENCES}) {
+                    $def->{REFERENCES}->{created} = 1;
+                }
+            }
+        }
+    }
+
     return $class->new(undef, $thawed_hash);
 }
 
diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm
index 26e36f7df..225e862de 100644
--- a/Bugzilla/Install/DB.pm
+++ b/Bugzilla/Install/DB.pm
@@ -3155,13 +3155,11 @@ sub _add_foreign_keys_to_multiselects {
           WHERE type = ' . FIELD_TYPE_MULTI_SELECT);
 
     foreach my $name (@$names) {
-        $dbh->bz_add_fk("bug_$name", "bug_id", {TABLE  => 'bugs',
-                                                COLUMN => 'bug_id',
-                                                DELETE => 'CASCADE',});
+        $dbh->bz_add_fk("bug_$name", "bug_id", 
+            {TABLE => 'bugs', COLUMN => 'bug_id', DELETE => 'CASCADE'});
                                                 
-        $dbh->bz_add_fk("bug_$name", "value", {TABLE  => $name,
-                                               COLUMN => 'value',
-                                               DELETE => 'RESTRICT',});
+        $dbh->bz_add_fk("bug_$name", "value",
+            {TABLE  => $name, COLUMN => 'value', DELETE => 'RESTRICT'});
     }
 }
 
@@ -3380,9 +3378,8 @@ sub _convert_flagtypes_fks_to_set_null {
     foreach my $column (qw(request_group_id grant_group_id)) {
         my $fk = $dbh->bz_fk_info('flagtypes', $column);
         if ($fk and !defined $fk->{DELETE}) {
-            # checksetup will re-create the FK with the appropriate definition
-            # at the end of its table upgrades, so we just drop it here.
-            $dbh->bz_drop_fk('flagtypes', $column);
+            $fk->{DELETE} = 'SET NULL';
+            $dbh->bz_alter_fk('flagtypes', $column, $fk);
         }
     }
 }
@@ -3398,10 +3395,9 @@ sub _fix_decimal_types {
 sub _fix_series_creator_fk {
     my $dbh = Bugzilla->dbh;
     my $fk = $dbh->bz_fk_info('series', 'creator');
-    # Change the FK from SET NULL to CASCADE. (It will be re-created
-    # automatically at the end of all DB changes.)
     if ($fk and $fk->{DELETE} eq 'SET NULL') {
-        $dbh->bz_drop_fk('series', 'creator');
+        $fk->{DELETE} = 'CASCADE';
+        $dbh->bz_alter_fk('series', 'creator', $fk);
     }
 }
 
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index 849008adb..e20913a0e 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -95,7 +95,12 @@
   
   [% ELSIF error == "chart_file_open_fail" %]
     Unable to open the chart datafile <tt>[% filename FILTER html %]</tt>.
-  
+ 
+  [% ELSIF error == "column_alter_nonexistent_fk" %]
+    You attempted to modify the foreign key for 
+    [%+ table FILTER html %].[% column FILTER html %], but there is
+    no foreign key on that column.
+ 
   [% ELSIF error == "column_not_null_without_default" %]
     Failed adding the column [% name FILTER html %]:
     You cannot add a NOT NULL column with no default to an existing table