diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 3b9659f31..52087af95 100644 --- a/Bugzilla/DB.pm +++ b/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 + +=over + +=item B + +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. + +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 + +=over + +=item C<$fixes> + +An arrayref of hashrefs. Each hashref supports: + +=over + +=item C + +The table containing the column to alter. + +=item C + +The column to alter. + +=item C + +The target abstract column definition (same format used by +C). + +This must be the B desired definition of the column, not just +the attributes that are changing: C replaces the whole +column definition with what you pass, so any omitted attribute (such as +C, C, or C) will be dropped from the +column. Foreign keys are the exception -- they are preserved by +C 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 to trigger a change. + +=item C (optional) + +If specified, the fix is only applied when the current column C +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 in at least one of the attributes listed there, so calling +this method repeatedly (for example across re-runs of C) +is safe and idempotent. + +=back + +=item B (nothing) + +=back + =item C =over diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index 0265b3350..3284fba3c 100644 --- a/Bugzilla/Install/DB.pm +++ b/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) = (@_); diff --git a/t/db-fk-safe-alter.t b/t/db-fk-safe-alter.t new file mode 100644 index 000000000..7b2bcd8d8 --- /dev/null +++ b/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;