[harmony] Bug 2052103: Drop FK references around altering attachment id type

Bugzilla/DB.pm

@@ -1064,6 +1064,39 @@ sub bz_drop_related_fks {
   return $related;
 }
 
+sub bz_fk_safe_alter_columns {
+  my ($self, $fixes) = @_;
+
+  foreach my $fix (@$fixes) {
+    my ($table, $column, $target) = @{$fix}{qw(table column definition)};
+    next if !$table || !$column || !$target;
+
+    my $current = $self->bz_column_info($table, $column);
+    next if !$current;
+
+    my $only_if_type = $fix->{only_if_type};
+    if ($only_if_type) {
+      my @allowed_types = ref($only_if_type) eq 'ARRAY'
+        ? @$only_if_type
+        : ($only_if_type);
+      next if !grep { $current->{TYPE} eq $_ } @allowed_types;
+    }
+
+    my $needs_alter = 0;
+    foreach my $key (keys %$target) {
+      if (!defined $current->{$key} || $current->{$key} ne $target->{$key}) {
+        $needs_alter = 1;
+        last;
+      }
+    }
+    next if !$needs_alter;
+
+    warn "Dropping foreign keys on $table.$column\n";
+    $self->bz_drop_related_fks($table, $column);
+    $self->bz_alter_column($table, $column, $target);
+  }
+}
+
 sub bz_drop_index {
   my ($self, $table, $name) = @_;
 
@@ -2644,6 +2677,84 @@ not already be SQL-quoted.
 
 =back
 
+=item C<bz_fk_safe_alter_columns>
+
+=over
+
+=item B<Description>
+
+Performs one or more column alterations in a foreign-key-safe way.
+
+For each requested fix, this method compares the current column
+definition to the target definition, drops related foreign keys when
+needed, and then alters the column.
+
+This is intended for upgrade paths where a referenced or referencing
+column type changes and foreign keys must be dropped before running
+C<bz_alter_column>.
+
+Note that missing foreign keys are replaced later in the installation
+process, so this method does not attempt to re-add them.
+
+Looking for foreign keys is expensive, so this method should only be
+used when you know that foreign keys exist on the columns being
+altered.
+
+=item B<Params>
+
+=over
+
+=item C<$fixes>
+
+An arrayref of hashrefs. Each hashref supports:
+
+=over
+
+=item C<table>
+
+The table containing the column to alter.
+
+=item C<column>
+
+The column to alter.
+
+=item C<definition>
+
+The target abstract column definition (same format used by
+C<bz_alter_column>).
+
+This must be the B<complete> desired definition of the column, not just
+the attributes that are changing: C<bz_alter_column> replaces the whole
+column definition with what you pass, so any omitted attribute (such as
+C<NOTNULL>, C<DEFAULT>, or C<PRIMARYKEY>) will be dropped from the
+column. Foreign keys are the exception -- they are preserved by
+C<bz_alter_column> and re-added later in the installation process, so
+they should not be listed here.
+
+Only scalar attributes are compared when deciding whether an alteration
+is needed (see below), so avoid relying on reference-valued attributes
+in C<definition> to trigger a change.
+
+=item C<only_if_type> (optional)
+
+If specified, the fix is only applied when the current column C<TYPE>
+matches this value. May be a scalar type name or an arrayref of type
+names. This can be used when a column may have had multiple types in
+the past, but you're only doing a specific phase of the upgrade.
+
+=back
+
+Each fix is only applied when the current column definition differs from
+C<definition> in at least one of the attributes listed there, so calling
+this method repeatedly (for example across re-runs of C<checksetup.pl>)
+is safe and idempotent.
+
+=back
+
+=item B<Returns> (nothing)
+
+=back
+
 =item C<bz_drop_column>
 
 =over

Bugzilla/Install/DB.pm

@@ -484,7 +484,12 @@ sub update_table_definitions {
   $dbh->bz_drop_column('groups',   'last_changed');
 
   # 2019-01-31 dylan@hardison.net - Bug TODO
-  _update_flagtypes_id();
+  $dbh->bz_fk_safe_alter_columns([
+    {table => 'flaginclusions', column => 'type_id', definition => {TYPE => 'INT3', NOTNULL => 1}, only_if_type => 'INT2'},
+    {table => 'flagexclusions', column => 'type_id', definition => {TYPE => 'INT3', NOTNULL => 1}, only_if_type => 'INT2'},
+    {table => 'flags',          column => 'type_id', definition => {TYPE => 'INT3', NOTNULL => 1}, only_if_type => 'INT2'},
+    {table => 'flagtypes',      column => 'id',      definition => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1}}
+  ]);
 
   $dbh->bz_alter_column('keyworddefs', 'id',
     {TYPE => 'SMALLSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
@@ -808,13 +813,14 @@ sub update_table_definitions {
   $dbh->bz_add_column('profiles', 'bounce_count', {TYPE => 'INT1', NOTNULL => 1, DEFAULT => 0});
 
   # Bug 1588221 - dkl@mozilla.com
-  $dbh->bz_alter_column('bugs_activity', 'attach_id', {TYPE => 'INT5'});
-  $dbh->bz_alter_column('attachments', 'attach_id',
-    {TYPE => 'BIGSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
-  $dbh->bz_alter_column('attach_data', 'id',
-    {TYPE => 'INT5', NOTNULL => 1, PRIMARYKEY => 1});
-  $dbh->bz_alter_column('flags',            'attach_id', {TYPE => 'INT5'});
-  $dbh->bz_alter_column('user_request_log', 'attach_id', {TYPE => 'INT5', NOTNULL => 0});
+  $dbh->bz_fk_safe_alter_columns([
+    {table => 'bugs_activity',    column => 'attach_id', definition => {TYPE => 'INT5'}},
+    {table => 'attachments',      column => 'attach_id', definition => {TYPE => 'BIGSERIAL', NOTNULL => 1, PRIMARYKEY => 1}},
+    {table => 'attach_data',      column => 'id',        definition => {TYPE => 'INT5', NOTNULL => 1, PRIMARYKEY => 1}},
+    {table => 'flags',            column => 'attach_id', definition => {TYPE => 'INT5'}},
+    {table => 'user_request_log', column => 'attach_id', definition => {TYPE => 'INT5', NOTNULL => 0}},
+  ]);
+
   _populate_attachment_storage_class();
 
 
@@ -905,30 +911,6 @@ sub _update_product_name_definition {
   }
 }
 
-sub _update_flagtypes_id {
-  my $dbh   = Bugzilla->dbh;
-  my @fixes = (
-    {table => 'flaginclusions', column => 'type_id'},
-    {table => 'flagexclusions', column => 'type_id'},
-    {table => 'flags',          column => 'type_id'},
-  );
-  my $flagtypes_def = $dbh->bz_column_info('flagtypes', 'id');
-  foreach my $fix (@fixes) {
-    my $def = $dbh->bz_column_info($fix->{table}, $fix->{column});
-    if ($def->{TYPE} eq 'INT2') {
-      warn "Dropping foreign keys on $fix->{table}\n";
-      $dbh->bz_drop_related_fks($fix->{table}, $fix->{column});
-      $def->{TYPE} = 'INT3';
-      $dbh->bz_alter_column($fix->{table}, $fix->{column}, $def);
-    }
-  }
-
-  if ($flagtypes_def->{TYPE} ne 'MEDIUMSERIAL') {
-    $flagtypes_def->{TYPE} = 'MEDIUMSERIAL';
-    $dbh->bz_alter_column('flagtypes', 'id', $flagtypes_def);
-  }
-}
-
 # A helper for the function below.
 sub _write_one_longdesc {
   my ($id, $who, $when, $buffer) = (@_);

t/db-fk-safe-alter.t

@@ -0,0 +1,154 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+use 5.10.1;
+use strict;
+use warnings;
+use lib qw( . lib local/lib/perl5 );
+use Test::More;
+use Test2::Tools::Mock qw(mock);
+
+BEGIN {
+  $ENV{LOCALCONFIG_ENV} = 'BMO';
+  $ENV{BMO_db_driver}   = 'sqlite';
+  $ENV{BMO_db_name}     = ':memory:';
+}
+use Bugzilla;
+use Bugzilla::DB;
+
+# bz_fk_safe_alter_columns only calls back into bz_column_info,
+# bz_drop_related_fks and bz_alter_column, so we can exercise it against a
+# bare Bugzilla::DB object with those three methods mocked. This keeps the
+# test database-independent and lets us assert exactly what gets passed to
+# bz_alter_column.
+
+# The (mocked) current state of the database, keyed by "table.column".
+my %current_column;
+
+# Ordered log of drop/alter events, so we can assert both what happened and
+# that foreign keys are dropped *before* the column is altered.
+my @events;
+
+my $mock = mock 'Bugzilla::DB' => (
+  override => [
+    bz_column_info => sub {
+      my ($self, $table, $column) = @_;
+      return $current_column{"$table.$column"};
+    },
+    bz_drop_related_fks => sub {
+      my ($self, $table, $column) = @_;
+      push @events, {action => 'drop', table => $table, column => $column};
+      return [];
+    },
+    bz_alter_column => sub {
+      my ($self, $table, $column, $def) = @_;
+      push @events, {action => 'alter', table => $table, column => $column, def => $def};
+    },
+  ],
+);
+
+my $dbh = bless {}, 'Bugzilla::DB';
+
+# Run a fix set with warnings silenced (the method warns for every drop).
+sub run_fixes {
+  @events = ();
+  local $SIG{__WARN__} = sub { };
+  $dbh->bz_fk_safe_alter_columns(\@_);
+}
+
+# --- The definition is passed to bz_alter_column verbatim ------------------
+# This is the regression guard for bug 2052103: passing only {TYPE => ...}
+# would silently drop NOTNULL. The method must forward the caller's complete
+# definition unchanged.
+%current_column = ('flags.type_id' => {TYPE => 'INT2', NOTNULL => 1});
+run_fixes({
+  table        => 'flags',
+  column       => 'type_id',
+  definition   => {TYPE => 'INT3', NOTNULL => 1},
+  only_if_type => 'INT2',
+});
+is(scalar(@events), 2, 'a needed fix produces a drop and an alter');
+is($events[0]{action}, 'drop',  'foreign keys are dropped first');
+is($events[1]{action}, 'alter', 'column is altered second');
+is_deeply(
+  $events[1]{def},
+  {TYPE => 'INT3', NOTNULL => 1},
+  'the complete definition (including NOTNULL) is forwarded unchanged'
+);
+
+# --- only_if_type gating: scalar mismatch skips the fix --------------------
+%current_column = ('flags.type_id' => {TYPE => 'INT3', NOTNULL => 1});
+run_fixes({
+  table        => 'flags',
+  column       => 'type_id',
+  definition   => {TYPE => 'INT3', NOTNULL => 1},
+  only_if_type => 'INT2',
+});
+is(scalar(@events), 0, 'only_if_type mismatch (scalar) skips the fix entirely');
+
+# --- only_if_type gating: arrayref match applies the fix -------------------
+%current_column = ('flags.type_id' => {TYPE => 'INT4', NOTNULL => 1});
+run_fixes({
+  table        => 'flags',
+  column       => 'type_id',
+  definition   => {TYPE => 'INT3', NOTNULL => 1},
+  only_if_type => ['INT2', 'INT4'],
+});
+is(scalar(@events), 2, 'only_if_type arrayref match applies the fix');
+
+# --- Idempotency: no change needed means no drop and no alter --------------
+%current_column = ('flags.type_id' => {TYPE => 'INT3', NOTNULL => 1});
+run_fixes({
+  table      => 'flags',
+  column     => 'type_id',
+  definition => {TYPE => 'INT3', NOTNULL => 1},
+});
+is(scalar(@events), 0, 'a column already matching the target is left untouched');
+
+# A differing attribute (NOTNULL) still triggers the fix even when TYPE matches.
+%current_column = ('flags.type_id' => {TYPE => 'INT3'});
+run_fixes({
+  table      => 'flags',
+  column     => 'type_id',
+  definition => {TYPE => 'INT3', NOTNULL => 1},
+});
+is(scalar(@events), 2, 'a differing scalar attribute triggers the fix');
+
+# --- Missing column is skipped, not fatal ----------------------------------
+%current_column = ();
+run_fixes({
+  table      => 'no_such_table',
+  column     => 'no_such_column',
+  definition => {TYPE => 'INT3'},
+});
+is(scalar(@events), 0, 'a non-existent column is skipped');
+
+# --- Incomplete fix specs are skipped --------------------------------------
+%current_column = ('flags.type_id' => {TYPE => 'INT2'});
+run_fixes(
+  {column => 'type_id', definition => {TYPE => 'INT3'}},    # no table
+  {table  => 'flags',   definition => {TYPE => 'INT3'}},    # no column
+  {table  => 'flags',   column     => 'type_id'},           # no definition
+);
+is(scalar(@events), 0, 'fixes missing table/column/definition are skipped');
+
+# --- Multiple fixes are processed in order ---------------------------------
+%current_column = (
+  'flags.type_id'    => {TYPE => 'INT2', NOTNULL => 1},
+  'flagtypes.id'     => {TYPE => 'MEDIUMINT', NOTNULL => 1, PRIMARYKEY => 1},
+);
+run_fixes(
+  {table => 'flags',     column => 'type_id', definition => {TYPE => 'INT3', NOTNULL => 1}, only_if_type => 'INT2'},
+  {table => 'flagtypes', column => 'id',      definition => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1}},
+);
+my @altered = map { "$_->{table}.$_->{column}" } grep { $_->{action} eq 'alter' } @events;
+is_deeply(
+  \@altered,
+  ['flags.type_id', 'flagtypes.id'],
+  'multiple fixes are altered in the order given'
+);
+
+done_testing;