From fb742fc942b311b75d65bd6a9c506fb208d30fe4 Mon Sep 17 00:00:00 2001 From: Dave Miller Date: Thu, 2 Jul 2026 02:34:05 -0400 Subject: [PATCH 1/4] Bug 2052103: Drop FK references around altering attachment id type --- Bugzilla/DB.pm | 92 ++++++++++++++++++++++++++++++++++++++++++ Bugzilla/Install/DB.pm | 46 +++++++-------------- 2 files changed, 106 insertions(+), 32 deletions(-) diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 3b9659f31..421c665b0 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,65 @@ 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 + +=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). + +=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 + +=back + +=item B (nothing) + +=back + =item C =over diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index 0265b3350..cccba46ea 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'}, only_if_type => 'INT2'}, + {table => 'flagexclusions', column => 'type_id', definition => {TYPE => 'INT3'}, only_if_type => 'INT2'}, + {table => 'flags', column => 'type_id', definition => {TYPE => 'INT3'}, 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) = (@_); From 1f712f717a6133675581da7922fa1b7edf6aae64 Mon Sep 17 00:00:00 2001 From: Dave Miller Date: Thu, 2 Jul 2026 05:16:54 -0400 Subject: [PATCH 2/4] Add missing =over in POD --- Bugzilla/DB.pm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 421c665b0..07d84f449 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -2702,6 +2702,8 @@ altered. =item B +=over + =item C<$fixes> An arrayref of hashrefs. Each hashref supports: From 2693b7433cb2696b3692415954bac86246df9b03 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 3 Jul 2026 12:12:16 +0100 Subject: [PATCH 3/4] Bug 2052103: Preserve NOTNULL on flag type_id columns; clarify bz_fk_safe_alter_columns POD The refactor to bz_fk_safe_alter_columns passed only {TYPE => 'INT3'} as the target definition for the flag*.type_id columns. Because bz_alter_column replaces the entire column definition with what it is given, this dropped the NOT NULL constraint that the previous _update_flagtypes_id code preserved (it mutated only TYPE on the full current definition). Add NOTNULL => 1 to the three type_id definitions so the constraint is retained, matching Schema.pm. Also document in the POD that 'definition' must be the complete desired column definition, and that the alteration check only compares scalar attributes. --- Bugzilla/DB.pm | 17 +++++++++++++++++ Bugzilla/Install/DB.pm | 6 +++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm index 07d84f449..52087af95 100644 --- a/Bugzilla/DB.pm +++ b/Bugzilla/DB.pm @@ -2723,6 +2723,18 @@ The column to alter. 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 @@ -2732,6 +2744,11 @@ 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) diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index cccba46ea..3284fba3c 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -485,9 +485,9 @@ sub update_table_definitions { # 2019-01-31 dylan@hardison.net - Bug TODO $dbh->bz_fk_safe_alter_columns([ - {table => 'flaginclusions', column => 'type_id', definition => {TYPE => 'INT3'}, only_if_type => 'INT2'}, - {table => 'flagexclusions', column => 'type_id', definition => {TYPE => 'INT3'}, only_if_type => 'INT2'}, - {table => 'flags', column => 'type_id', definition => {TYPE => 'INT3'}, only_if_type => 'INT2'}, + {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}} ]); From 635788abb7421349f7680cae3ab84191b35474c1 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Fri, 3 Jul 2026 12:14:42 +0100 Subject: [PATCH 4/4] Bug 2052103: Add unit test for bz_fk_safe_alter_columns Exercises the new FK-safe column alteration helper against a bare Bugzilla::DB object with bz_column_info/bz_drop_related_fks/bz_alter_column mocked, keeping the test database-independent. Covers: the complete definition (including NOTNULL) being forwarded to bz_alter_column verbatim (the bug 2052103 regression guard), only_if_type gating for both scalar and arrayref forms, idempotency when no change is needed, drop-before-alter ordering, skipping of missing columns and incomplete fix specs, and ordered processing of multiple fixes. --- t/db-fk-safe-alter.t | 154 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 t/db-fk-safe-alter.t 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;