From 9ab503f180a876db4a1b60b4fd36d01e15983eee Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Thu, 3 Apr 2025 21:35:07 -0400 Subject: [PATCH 01/10] Implement new test When object has 'renamed_from' property, but the previous version does not exists, the warning should be issued and for this object CREATE XXX query should be generated. --- t/30sqlt-new-diff-pgsql.t | 23 +++++++++++++++++++++++ t/data/diff/pgsql/create2.yml | 18 ++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t index 3d269ae9d..119e78265 100644 --- a/t/30sqlt-new-diff-pgsql.t +++ b/t/30sqlt-new-diff-pgsql.t @@ -15,6 +15,12 @@ use Storable 'dclone'; plan tests => 4; +my @warns; +local $SIG{__WARN__} = sub { + push @warns, $_[0] =~ s/\s+$//r; +}; + + use_ok('SQL::Translator::Diff') or die "Cannot continue\n"; my $tr = SQL::Translator->new; @@ -55,6 +61,10 @@ CREATE TABLE "added" ( "id" bigint ); +CREATE TABLE "fake_rename" ( + "fake_rename" integer +); + ALTER TABLE "employee" DROP CONSTRAINT "FK5302D47D93FE702E"; ALTER TABLE "employee" DROP COLUMN "job_title"; @@ -64,6 +74,8 @@ ALTER TABLE "employee" ADD CONSTRAINT "FK5302D47D93FE702E_diff" FOREIGN KEY ("em ALTER TABLE "old_name" RENAME TO "new_name"; +ALTER TABLE "new_name" ADD COLUMN "fake_rename" integer; + ALTER TABLE "new_name" ADD COLUMN "new_field" integer; ALTER TABLE "person" DROP CONSTRAINT "UC_age_name"; @@ -124,10 +136,16 @@ CREATE TABLE added ( id bigint ); +CREATE TABLE fake_rename ( + fake_rename integer +); + ALTER TABLE employee DROP COLUMN job_title; ALTER TABLE old_name RENAME TO new_name; +ALTER TABLE new_name ADD COLUMN fake_rename integer; + ALTER TABLE new_name ADD COLUMN new_field integer; ALTER TABLE person DROP CONSTRAINT UC_age_name; @@ -170,3 +188,8 @@ eq_or_diff($out, <<'## END OF DIFF', "No differences found"); -- No differences found; ## END OF DIFF + +is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed table can't find old table "not_exists" for renamed table!, + 'Warning: old table not found'; +is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed column can't find old column "old_name.not_exists" for renamed column!, + 'Warning: old column not found'; diff --git a/t/data/diff/pgsql/create2.yml b/t/data/diff/pgsql/create2.yml index b58fdee8d..45b435703 100644 --- a/t/data/diff/pgsql/create2.yml +++ b/t/data/diff/pgsql/create2.yml @@ -92,8 +92,26 @@ schema: other: data_type: int name: new_field + order: 3 + fake_rename: + data_type: int + name: fake_rename + extra: + renamed_from: not_exists order: 2 order: 4 + fake_rename: + name: fake_rename + extra: + renamed_from: not_exists + fields: + fake_rename: + data_type: int + name: fake_rename + extra: + renamed_from: not_exists + order: 2 + order: 7 person: constraints: - deferrable: 1 From cd839d9d0ffe87858d8a05944334f8fdc6650f4e Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 18:15:59 -0400 Subject: [PATCH 02/10] Remove excess code Fixes: 185 GitHub: https://github.com/dbsrgits/sql-translator/issues/185 --- t/30sqlt-new-diff-pgsql.t | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t index 119e78265..99fc4372b 100644 --- a/t/30sqlt-new-diff-pgsql.t +++ b/t/30sqlt-new-diff-pgsql.t @@ -22,15 +22,12 @@ local $SIG{__WARN__} = sub { use_ok('SQL::Translator::Diff') or die "Cannot continue\n"; - -my $tr = SQL::Translator->new; - my ($source_schema, $target_schema) = map { my $t = SQL::Translator->new; $t->parser('YAML') - or die $tr->error; + or die $t->error; my $out = $t->translate(catfile($Bin, qw/data diff pgsql/, $_)) - or die $tr->error; + or die $t->error; my $schema = $t->schema; unless ($schema->name) { From 38bee5fe92f6b9e6f09a62d87388031e47e37547 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 18:18:27 -0400 Subject: [PATCH 03/10] Move the code loading a scheme into subroutine --- t/30sqlt-new-diff-pgsql.t | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t index 99fc4372b..371c89996 100644 --- a/t/30sqlt-new-diff-pgsql.t +++ b/t/30sqlt-new-diff-pgsql.t @@ -20,21 +20,25 @@ local $SIG{__WARN__} = sub { push @warns, $_[0] =~ s/\s+$//r; }; +sub load { + return map { + my $t = SQL::Translator->new; + $t->parser('YAML') + or die $t->error; + my $out = $t->translate(catfile($Bin, qw/data diff pgsql/, $_)) + or die $t->error; + + my $schema = $t->schema; + unless ($schema->name) { + $schema->name($_); + } + ($schema); + } @_; +} + use_ok('SQL::Translator::Diff') or die "Cannot continue\n"; -my ($source_schema, $target_schema) = map { - my $t = SQL::Translator->new; - $t->parser('YAML') - or die $t->error; - my $out = $t->translate(catfile($Bin, qw/data diff pgsql/, $_)) - or die $t->error; - - my $schema = $t->schema; - unless ($schema->name) { - $schema->name($_); - } - ($schema); -} (qw( create1.yml create2.yml )); +my ($source_schema, $target_schema) = load( 'create1.yml', 'create2.yml' ); # Test for differences my $out = SQL::Translator::Diff::schema_diff( From 79b17e74c21046383980e3e112752ace68f7b874 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 20:05:30 -0400 Subject: [PATCH 04/10] Implemented new tests for column renames --- t/30sqlt-new-diff-pgsql.t | 60 ++++++++++++++++++++++++++++++++++- t/data/diff/pgsql/rename1.yml | 54 +++++++++++++++++++++++++++++++ t/data/diff/pgsql/rename2.yml | 56 ++++++++++++++++++++++++++++++++ t/data/diff/pgsql/rename3.yml | 56 ++++++++++++++++++++++++++++++++ t/data/diff/pgsql/rename4.yml | 60 +++++++++++++++++++++++++++++++++++ t/data/diff/pgsql/rename5.yml | 56 ++++++++++++++++++++++++++++++++ 6 files changed, 341 insertions(+), 1 deletion(-) create mode 100644 t/data/diff/pgsql/rename1.yml create mode 100644 t/data/diff/pgsql/rename2.yml create mode 100644 t/data/diff/pgsql/rename3.yml create mode 100644 t/data/diff/pgsql/rename4.yml create mode 100644 t/data/diff/pgsql/rename5.yml diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t index 371c89996..7835bd249 100644 --- a/t/30sqlt-new-diff-pgsql.t +++ b/t/30sqlt-new-diff-pgsql.t @@ -13,7 +13,7 @@ use Test::SQL::Translator qw(maybe_plan); use SQL::Translator::Schema::Constants; use Storable 'dclone'; -plan tests => 4; +plan tests => 10; my @warns; local $SIG{__WARN__} = sub { @@ -194,3 +194,61 @@ is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed table can't fin 'Warning: old table not found'; is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed column can't find old column "old_name.not_exists" for renamed column!, 'Warning: old column not found'; + + +sub diff_it { + my( $src, $dst ) = @_; + + my $out = SQL::Translator::Diff::schema_diff( + $src, + 'PostgreSQL', + $dst, + 'PostgreSQL', { + ignore_index_names => 1, + ignore_constraint_names => 1, + sqlt_args => { + quote_identifiers => 0, + } + } + ); + + return $out =~ s/^.*?BEGIN;\n+(.*)COMMIT.*?$/$1/sr =~ s/\n+$//gmr ."\n"; +} + +< v2 +$out = diff_it load qw/ rename1.yml rename2.yml /; + +eq_or_diff $out, < v3 +$out = diff_it load qw/ rename2.yml rename3.yml /; + +eq_or_diff $out, < v4 +$out = diff_it load qw/ rename3.yml rename4.yml /; + +eq_or_diff $out, < v5 +$out = diff_it load qw/ rename4.yml rename5.yml /; + +eq_or_diff $out, < Date: Thu, 3 Apr 2025 21:37:41 -0400 Subject: [PATCH 05/10] Rework changes detection --- lib/SQL/Translator/Diff.pm | 225 +++++++++++++++++++++---------------- 1 file changed, 127 insertions(+), 98 deletions(-) diff --git a/lib/SQL/Translator/Diff.pm b/lib/SQL/Translator/Diff.pm index e3d8b0f86..ea093acbf 100644 --- a/lib/SQL/Translator/Diff.pm +++ b/lib/SQL/Translator/Diff.pm @@ -99,6 +99,76 @@ sub BUILD { } } +# This function detects changes between two versions. Four scenarios are possible: +# v1 | v2 +# X | Y:RX | It was renamed to Y from X - rename +# X | X* | Something changed inside - alter +# - | X | No field in the old version - create +# X | - | No field in the new version - drop + +# For each of this scenario corresponding callback is fired: rename, alter, drop, create. +# Additionally 'next' callback if fired for every comparison. It could be used to prepare +# data structures where to fill the comparison/diff result. +# The callbacks are called with the next parameters: +# rename( $src_version, $dst_version ) +# alter ( $src_version, $dst_version ) +# create( $dst_version ) +# drop ( $src_version ) +# next ( $dst_name, $dst_version ) +# Where: +# $dst_name - the name of a destination object +# $src_version - a source object we want to migrate from +# $dst_version - a destination object we want to migrate to + +sub _detect_changes { + my( $changes, $src, $dst, $is_renamed, $get_name, $has_previous ) = @_; + + # Hash of renames: { old_name => SomeClass::Obj new_name } + # Where the key is the name of target object + # and the value is the source object + my $renames = {}; + # Find renamed destination objects and store corresponding source object there. Eg. + # if X object was renamed to 'y', then hash will be { y => X }. + for my $xsource ( @$dst ) { + my $name = $is_renamed->( $xsource ) or next; + my( $dst_name, $src_version ) = ( $get_name->( $xsource ), $has_previous->( $name ) ); + + $renames->{ $dst_name } = $src_version; + } + + my $src_used = {}; + # For each destination object trigger corresponding callback. + for my $dst_version ( @$dst ) { + my $dst_name = $get_name->( $dst_version ); + $changes->{ next } and $changes->{ next }( $dst_name, $dst_version ); + + my $src_version = $renames->{ $dst_name }; + if( $src_version ) { + $changes->{ rename }( $src_version, $dst_version ); + } + # Notice, when 'rename' happened we should call 'alter' which will check changes + # inside objects between source and destination. + if( $src_version //= $has_previous->( $get_name->( $dst_version ) ) ) { + $changes->{ alter }( $src_version, $dst_version ); + $src_used->{ $get_name->( $src_version ) } = 1; + next; + } + + # We are here when there is no SRC version + $changes->{ create }( $dst_version ); + } + + # Drop each SRC object which does not have corresponding DST object. + for my $src_version ( @$src ) { + next if $src_used->{ $get_name->( $src_version ) }; + + $changes->{ drop }( $src_version ); + } + + + return $changes; +} + sub compute_differences { my ($self) = @_; @@ -114,57 +184,33 @@ sub compute_differences { $preprocess->($target_schema); } - my %src_tables_checked = (); - my @tar_tables = sort { $a->name cmp $b->name } $target_schema->get_tables; - ## do original/source tables exist in target? - for my $tar_table (@tar_tables) { - my $tar_table_name = $tar_table->name; - - my $src_table; - - $self->table_diff_hash->{$tar_table_name} = { map { $_ => [] } @diff_hash_keys }; - - if (my $old_name = $tar_table->extra('renamed_from')) { - $src_table = $source_schema->get_table($old_name, $self->case_insensitive); - if ($src_table) { - $self->table_diff_hash->{$tar_table_name}{table_renamed_from} = [ [ $src_table, $tar_table ] ]; - } else { - delete $tar_table->extra->{renamed_from}; - carp qq#Renamed table can't find old table "$old_name" for renamed table\n#; - } - } else { - $src_table = $source_schema->get_table($tar_table_name, $self->case_insensitive); - } - - unless ($src_table) { - ## table is new - ## add table(s) later. - push @{ $self->tables_to_create }, $tar_table; - next; - } - - my $src_table_name = $src_table->name; - $src_table_name = lc $src_table_name if $self->case_insensitive; - $src_tables_checked{$src_table_name} = 1; - - $self->diff_table_options($src_table, $tar_table); - - ## Compare fields, their types, defaults, sizes etc etc - $self->diff_table_fields($src_table, $tar_table); - - $self->diff_table_indexes($src_table, $tar_table); - $self->diff_table_constraints($src_table, $tar_table); - - } # end of target_schema->get_tables loop - - for my $src_table ($source_schema->get_tables) { - my $src_table_name = $src_table->name; + my $changes = { + next => sub{ + my( $name ) = @_; + $self->table_diff_hash->{ $name } = { map { $_ => [] } @diff_hash_keys }; + }, + create => sub{ push @{ $self->tables_to_create }, shift }, + drop => sub{ push @{ $self->tables_to_drop }, shift }, + rename => sub{ $self->table_diff_hash->{ $_[1]->name }{ table_renamed_from } = [ [ @_ ] ] }, + alter => sub{ + $self->diff_table_options( @_ ); + + ## Compare fields, their types, defaults, sizes etc etc + $self->diff_table_fields( @_ ); + + $self->diff_table_indexes( @_ ); + $self->diff_table_constraints( @_ ); + }, + }; - $src_table_name = lc $src_table_name if $self->case_insensitive; + my( $src, $dst ) = ($source_schema, $target_schema); + _detect_changes( $changes, + scalar $src->get_tables, scalar $dst->get_tables, + sub{ shift->extra( 'renamed_from' ) }, + sub{ shift->name }, + sub{ $src->get_table( shift, $self->case_insensitive ) }, + ); - push @{ $self->tables_to_drop }, $src_table - unless $src_tables_checked{$src_table_name}; - } return $self; } @@ -366,57 +412,40 @@ CONSTRAINT_DROP: sub diff_table_fields { my ($self, $src_table, $tar_table) = @_; - # List of ones we've renamed from so we don't drop them - my %renamed_source_fields; - - for my $tar_table_field ($tar_table->get_fields) { - my $f_tar_name = $tar_table_field->name; - - if (my $old_name = $tar_table_field->extra->{renamed_from}) { - my $src_table_field = $src_table->get_field($old_name, $self->case_insensitive); - unless ($src_table_field) { - carp qq#Renamed column can't find old column "@{[$src_table->name]}.$old_name" for renamed column\n#; - delete $tar_table_field->extra->{renamed_from}; - } else { - push @{ $self->table_diff_hash->{$tar_table}{fields_to_rename} }, [ $src_table_field, $tar_table_field ]; - $renamed_source_fields{$old_name} = 1; - next; + my $skip; + my $diff_hash = $self->table_diff_hash->{$tar_table}; + my $changes = { + create => sub{ push @{ $diff_hash->{fields_to_create} }, shift }, + drop => sub{ push @{ $diff_hash->{fields_to_drop} }, shift }, + rename => sub{ push @{ $diff_hash->{fields_to_rename} }, [ @_ ]; $skip = 1; }, + alter => sub{ + my( $src, $dst ) = @_; + + # XXX: rename_xxx should rename, alter_xxx should alter, but ::Producers automatically + # calls 'alter_xxx' from theirs 'rename_xxx'. Workaround that here: + if( $skip ) { $skip = 0; return; } + + # field exists, something changed. This is a bit complex. Parsers can + # normalize types, but only some of them do, so compare the normalized and + # parsed types for each field to each other + if ( !$dst->equals($src, $self->case_insensitive) + && !$dst->equals($src->parsed_field, $self->case_insensitive) + && !$dst->parsed_field->equals($src, $self->case_insensitive) + && !$dst->parsed_field->equals($src->parsed_field, $self->case_insensitive) + ) { + # Some producers might need src field to diff against + push @{ $diff_hash->{fields_to_alter} }, [ $src, $dst ]; } - } - - my $src_table_field = $src_table->get_field($f_tar_name, $self->case_insensitive); - - unless ($src_table_field) { - push @{ $self->table_diff_hash->{$tar_table}{fields_to_create} }, $tar_table_field; - next; - } - - # field exists, something changed. This is a bit complex. Parsers can - # normalize types, but only some of them do, so compare the normalized and - # parsed types for each field to each other - if ( !$tar_table_field->equals($src_table_field, $self->case_insensitive) - && !$tar_table_field->equals($src_table_field->parsed_field, $self->case_insensitive) - && !$tar_table_field->parsed_field->equals($src_table_field, $self->case_insensitive) - && !$tar_table_field->parsed_field->equals($src_table_field->parsed_field, $self->case_insensitive)) { - - # Some producers might need src field to diff against - push @{ $self->table_diff_hash->{$tar_table}{fields_to_alter} }, [ $src_table_field, $tar_table_field ]; - next; - } - } - - # Now check to see if any fields from src_table need to be dropped - for my $src_table_field ($src_table->get_fields) { - my $f_src_name = $src_table_field->name; - next if $renamed_source_fields{$f_src_name}; - - my $tar_table_field = $tar_table->get_field($f_src_name, $self->case_insensitive); + }, + }; - unless ($tar_table_field) { - push @{ $self->table_diff_hash->{$tar_table}{fields_to_drop} }, $src_table_field; - next; - } - } + my( $src, $dst ) = ( $src_table, $tar_table ); + _detect_changes( $changes, + scalar $src->get_fields, scalar $dst->get_fields, + sub{ shift->extra->{renamed_from} }, + sub{ shift->name }, + sub{ $src->get_field( shift, $self->case_insensitive ) }, + ); } sub diff_table_options { From b4d619f16dd3be5f42af0ca6d75a5f96f87238d1 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 21:48:13 -0400 Subject: [PATCH 06/10] Do not skip creating objects whose names equal to newly renamed Eg. SRC:x, DST:y:rx, DST:x --- lib/SQL/Translator/Diff.pm | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/SQL/Translator/Diff.pm b/lib/SQL/Translator/Diff.pm index ea093acbf..1e0119a34 100644 --- a/lib/SQL/Translator/Diff.pm +++ b/lib/SQL/Translator/Diff.pm @@ -123,17 +123,26 @@ sub BUILD { sub _detect_changes { my( $changes, $src, $dst, $is_renamed, $get_name, $has_previous ) = @_; - # Hash of renames: { old_name => SomeClass::Obj new_name } + # Hash of renamed_to: { new_name => SomeClass::Obj old_name } # Where the key is the name of target object # and the value is the source object - my $renames = {}; + my $renamed_to = {}; + + # Hash of renamed_from { old_name => 1 } + # Where the key is the name of object which was renamed to something different. + # Required to exclude previous versions. Eg. if SRC has x and DST has x it does not + # mean that something was changed inside x. It could be possible that DST:x was + # renamed from SRC:y, thus SRC:x should not be counted as previous version of DST:x. + my $renamed_from = {}; + # Find renamed destination objects and store corresponding source object there. Eg. # if X object was renamed to 'y', then hash will be { y => X }. for my $xsource ( @$dst ) { my $name = $is_renamed->( $xsource ) or next; my( $dst_name, $src_version ) = ( $get_name->( $xsource ), $has_previous->( $name ) ); - $renames->{ $dst_name } = $src_version; + $renamed_from->{ $name } = 1; + $renamed_to->{ $dst_name } = $src_version; } my $src_used = {}; @@ -142,13 +151,13 @@ sub _detect_changes { my $dst_name = $get_name->( $dst_version ); $changes->{ next } and $changes->{ next }( $dst_name, $dst_version ); - my $src_version = $renames->{ $dst_name }; + my $src_version = $renamed_to->{ $dst_name }; if( $src_version ) { $changes->{ rename }( $src_version, $dst_version ); } # Notice, when 'rename' happened we should call 'alter' which will check changes # inside objects between source and destination. - if( $src_version //= $has_previous->( $get_name->( $dst_version ) ) ) { + if( $src_version //= !$renamed_from->{ $dst_name } && $has_previous->( $dst_name ) ) { $changes->{ alter }( $src_version, $dst_version ); $src_used->{ $get_name->( $src_version ) } = 1; next; From 740fa227360ad93fd8460e186341ca5bf385a673 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 22:25:37 -0400 Subject: [PATCH 07/10] Prioritize renames before create and alter Renames should go first, otherwise it could not be possible to add a new column with the name which was just renamed to something. Eg. SRC:x->DST:y, Create DST:x. Otherwise we can not run 'Create DST:x', because schema still have 'x' column. --- lib/SQL/Translator/Diff.pm | 6 +++++- lib/SQL/Translator/Producer/PostgreSQL.pm | 2 +- lib/SQL/Translator/Utils.pm | 2 +- t/30sqlt-new-diff-mysql.t | 8 ++++---- t/30sqlt-new-diff-pgsql.t | 12 ++++++------ t/30sqlt-new-diff-sqlite.t | 4 ++-- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/lib/SQL/Translator/Diff.pm b/lib/SQL/Translator/Diff.pm index 1e0119a34..694b45c99 100644 --- a/lib/SQL/Translator/Diff.pm +++ b/lib/SQL/Translator/Diff.pm @@ -292,13 +292,17 @@ sub produce_diff_sql { () } + # Renames should go first, otherwise it could not be possible to add a new column with + # the name which was just renamed to something. Eg. SRC:x->DST:y, Create DST:x. + # Otherwise we can not run 'Create DST:x', because schema still have 'x' column. + } qw/rename_table alter_drop_constraint alter_drop_index drop_field + rename_field add_field alter_field - rename_field alter_create_index alter_create_constraint alter_table/), diff --git a/lib/SQL/Translator/Producer/PostgreSQL.pm b/lib/SQL/Translator/Producer/PostgreSQL.pm index a0d4ac99b..ac611227c 100644 --- a/lib/SQL/Translator/Producer/PostgreSQL.pm +++ b/lib/SQL/Translator/Producer/PostgreSQL.pm @@ -1188,9 +1188,9 @@ sub batch_alter_table { # now add everything else push @sql, batch_alter_table_statements( $diff_hash, $options, qw( + rename_field add_field alter_field - rename_field alter_create_index alter_create_constraint alter_table diff --git a/lib/SQL/Translator/Utils.pm b/lib/SQL/Translator/Utils.pm index 30e29463a..91173e374 100644 --- a/lib/SQL/Translator/Utils.pm +++ b/lib/SQL/Translator/Utils.pm @@ -394,9 +394,9 @@ sub batch_alter_table_statements { alter_drop_constraint alter_drop_index drop_field + rename_field add_field alter_field - rename_field alter_create_index alter_create_constraint alter_table diff --git a/t/30sqlt-new-diff-mysql.t b/t/30sqlt-new-diff-mysql.t index 9151a84d9..8766439c4 100644 --- a/t/30sqlt-new-diff-mysql.t +++ b/t/30sqlt-new-diff-mysql.t @@ -72,6 +72,8 @@ ALTER TABLE person DROP INDEX u_name; ALTER TABLE employee DROP COLUMN job_title; +ALTER TABLE person CHANGE COLUMN description physical_description text NULL; + ALTER TABLE new_name ADD COLUMN new_field integer NULL; ALTER TABLE person ADD COLUMN is_rock_star tinyint(4) NULL DEFAULT 1; @@ -84,8 +86,6 @@ ALTER TABLE person CHANGE COLUMN age age integer(11) NULL DEFAULT 18; ALTER TABLE person CHANGE COLUMN iq iq integer(11) NULL DEFAULT 0; -ALTER TABLE person CHANGE COLUMN description physical_description text NULL; - ALTER TABLE person ADD UNIQUE INDEX unique_name (name); ALTER TABLE employee ADD CONSTRAINT FK5302D47D93FE702E_diff FOREIGN KEY (employee_id) REFERENCES person (person_id); @@ -136,12 +136,12 @@ ALTER TABLE old_name RENAME TO new_name, ADD COLUMN new_field integer NULL; ALTER TABLE person DROP CONSTRAINT UC_age_name, + CHANGE COLUMN description physical_description text NULL, ADD COLUMN is_rock_star tinyint(4) NULL DEFAULT 1, CHANGE COLUMN person_id person_id integer(11) NOT NULL auto_increment, CHANGE COLUMN name name varchar(20) NOT NULL, CHANGE COLUMN age age integer(11) NULL DEFAULT 18, CHANGE COLUMN iq iq integer(11) NULL DEFAULT 0, - CHANGE COLUMN description physical_description text NULL, ADD UNIQUE UC_person_id (person_id), ADD UNIQUE UC_age_name (age, name), ENGINE=InnoDB; @@ -207,13 +207,13 @@ ALTER TABLE employee DROP FOREIGN KEY FK5302D47D93FE702E, ALTER TABLE person DROP CONSTRAINT UC_age_name, DROP INDEX u_name, + CHANGE COLUMN description physical_description text NULL, ADD COLUMN is_rock_star tinyint(4) NULL DEFAULT 1, ADD COLUMN value double(8, 2) NULL DEFAULT 0.00, CHANGE COLUMN person_id person_id integer(11) NOT NULL auto_increment, CHANGE COLUMN name name varchar(20) NOT NULL, CHANGE COLUMN age age integer(11) NULL DEFAULT 18, CHANGE COLUMN iq iq integer(11) NULL DEFAULT 0, - CHANGE COLUMN description physical_description text NULL, ADD UNIQUE INDEX unique_name (name), ADD UNIQUE UC_person_id (person_id), ADD UNIQUE UC_age_name (age, name), diff --git a/t/30sqlt-new-diff-pgsql.t b/t/30sqlt-new-diff-pgsql.t index 7835bd249..87f183e67 100644 --- a/t/30sqlt-new-diff-pgsql.t +++ b/t/30sqlt-new-diff-pgsql.t @@ -83,6 +83,8 @@ ALTER TABLE "person" DROP CONSTRAINT "UC_age_name"; DROP INDEX "u_name"; +ALTER TABLE "person" RENAME COLUMN "description" TO "physical_description"; + ALTER TABLE "person" ADD COLUMN "is_rock_star" smallint DEFAULT 1; ALTER TABLE "person" ALTER COLUMN "person_id" TYPE serial; @@ -99,8 +101,6 @@ ALTER TABLE "person" ALTER COLUMN "nickname" SET NOT NULL; ALTER TABLE "person" ALTER COLUMN "nickname" TYPE character varying(24); -ALTER TABLE "person" RENAME COLUMN "description" TO "physical_description"; - ALTER TABLE "person" ADD CONSTRAINT "unique_name" UNIQUE ("name"); ALTER TABLE "person" ADD CONSTRAINT "UC_person_id" UNIQUE ("person_id"); @@ -151,6 +151,8 @@ ALTER TABLE new_name ADD COLUMN new_field integer; ALTER TABLE person DROP CONSTRAINT UC_age_name; +ALTER TABLE person RENAME COLUMN description TO physical_description; + ALTER TABLE person ADD COLUMN is_rock_star smallint DEFAULT 1; ALTER TABLE person ALTER COLUMN person_id TYPE serial; @@ -167,8 +169,6 @@ ALTER TABLE person ALTER COLUMN nickname SET NOT NULL; ALTER TABLE person ALTER COLUMN nickname TYPE character varying(24); -ALTER TABLE person RENAME COLUMN description TO physical_description; - ALTER TABLE person ADD CONSTRAINT UC_person_id UNIQUE (person_id); ALTER TABLE person ADD CONSTRAINT UC_age_name UNIQUE (age, name); @@ -190,10 +190,10 @@ eq_or_diff($out, <<'## END OF DIFF', "No differences found"); ## END OF DIFF -is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed table can't find old table "not_exists" for renamed table!, - 'Warning: old table not found'; is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed column can't find old column "old_name.not_exists" for renamed column!, 'Warning: old column not found'; +is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed table can't find old table "not_exists" for renamed table!, + 'Warning: old table not found'; sub diff_it { diff --git a/t/30sqlt-new-diff-sqlite.t b/t/30sqlt-new-diff-sqlite.t index 0a79df19e..12912ece5 100644 --- a/t/30sqlt-new-diff-sqlite.t +++ b/t/30sqlt-new-diff-sqlite.t @@ -63,14 +63,14 @@ DROP INDEX u_name; -- SQL::Translator::Producer::SQLite cant drop_field; +-- SQL::Translator::Producer::SQLite cant rename_field; + ALTER TABLE new_name ADD COLUMN new_field int; ALTER TABLE person ADD COLUMN is_rock_star tinyint(4) DEFAULT 1; -- SQL::Translator::Producer::SQLite cant alter_field; --- SQL::Translator::Producer::SQLite cant rename_field; - CREATE UNIQUE INDEX unique_name ON person (name); CREATE UNIQUE INDEX UC_person_id ON person (person_id); From fd58e134bbc663096eaaff4bc07539eb51e0af12 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Fri, 4 Apr 2025 23:04:18 -0400 Subject: [PATCH 08/10] Revive warnings when there is no previous version for renamed object --- lib/SQL/Translator/Diff.pm | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/SQL/Translator/Diff.pm b/lib/SQL/Translator/Diff.pm index 694b45c99..5defa538d 100644 --- a/lib/SQL/Translator/Diff.pm +++ b/lib/SQL/Translator/Diff.pm @@ -152,7 +152,10 @@ sub _detect_changes { $changes->{ next } and $changes->{ next }( $dst_name, $dst_version ); my $src_version = $renamed_to->{ $dst_name }; - if( $src_version ) { + # Corner case: sometimes field is marked as renamed, but does not have previous + # version. This happens when user forgot to remove this mark for the next migration. + # Eg. v1 x; v2 y:rx; v3 y:rx + if( exists $renamed_to->{ $dst_name } ) { $changes->{ rename }( $src_version, $dst_version ); } # Notice, when 'rename' happened we should call 'alter' which will check changes @@ -200,7 +203,16 @@ sub compute_differences { }, create => sub{ push @{ $self->tables_to_create }, shift }, drop => sub{ push @{ $self->tables_to_drop }, shift }, - rename => sub{ $self->table_diff_hash->{ $_[1]->name }{ table_renamed_from } = [ [ @_ ] ] }, + rename => sub{ + my( $src, $dst ) = @_; + if( $src ) { + $self->table_diff_hash->{ $_[1]->name }{ table_renamed_from } = [ [ $src, $dst ] ] + } + else { + my $old_name = delete $dst->extra->{ renamed_from }; + carp qq#Renamed table can't find old table "$old_name" for renamed table\n#; + } + }, alter => sub{ $self->diff_table_options( @_ ); @@ -430,7 +442,16 @@ sub diff_table_fields { my $changes = { create => sub{ push @{ $diff_hash->{fields_to_create} }, shift }, drop => sub{ push @{ $diff_hash->{fields_to_drop} }, shift }, - rename => sub{ push @{ $diff_hash->{fields_to_rename} }, [ @_ ]; $skip = 1; }, + rename => sub{ + my( $src, $dst ) = @_; + if( $src ) { + push @{ $diff_hash->{fields_to_rename} }, [ $src, $dst ]; $skip = 1; + } + else { + my $old_name = delete $dst->extra->{renamed_from}; + carp qq#Renamed column can't find old column "@{[$src_table->name]}.$old_name" for renamed column\n#; + } + }, alter => sub{ my( $src, $dst ) = @_; From 88a26289a379d6476f1ecc52897fff2fa91e9976 Mon Sep 17 00:00:00 2001 From: Eugen Konkov Date: Sun, 8 Jun 2025 13:19:01 -0400 Subject: [PATCH 09/10] Move tests for renames into its own file --- t/30sqlt-new-diff-pgsql-renames.t | 100 ++++++++++++++++++++++++++++++ t/30sqlt-new-diff-pgsql.t | 60 +----------------- 2 files changed, 101 insertions(+), 59 deletions(-) create mode 100644 t/30sqlt-new-diff-pgsql-renames.t diff --git a/t/30sqlt-new-diff-pgsql-renames.t b/t/30sqlt-new-diff-pgsql-renames.t new file mode 100644 index 000000000..e1f56370d --- /dev/null +++ b/t/30sqlt-new-diff-pgsql-renames.t @@ -0,0 +1,100 @@ +#!/usr/bin/perl +# vim: set ft=perl: + +use strict; +use warnings; +use SQL::Translator; + +use File::Spec::Functions qw(catfile updir tmpdir); +use FindBin qw($Bin); +use Test::More; +use Test::Differences; +use Test::SQL::Translator qw(maybe_plan); +use SQL::Translator::Schema::Constants; +use Storable 'dclone'; + +plan tests => 5; + +use_ok('SQL::Translator::Diff') or die "Cannot continue\n"; + +my @warns; +local $SIG{__WARN__} = sub { + push @warns, $_[0] =~ s/\s+$//r; +}; + + +sub load { + return map { + my $t = SQL::Translator->new; + $t->parser('YAML') + or die $t->error; + my $out = $t->translate(catfile($Bin, qw/data diff pgsql/, $_)) + or die $t->error; + + my $schema = $t->schema; + unless ($schema->name) { + $schema->name($_); + } + ($schema); + } @_; +} + + +sub diff_it { + my( $src, $dst ) = @_; + + my $out = SQL::Translator::Diff::schema_diff( + $src, + 'PostgreSQL', + $dst, + 'PostgreSQL', { + ignore_index_names => 1, + ignore_constraint_names => 1, + sqlt_args => { + quote_identifiers => 0, + } + } + ); + + return $out =~ s/^.*?BEGIN;\n+(.*)COMMIT.*?$/$1/sr =~ s/\n+$//gmr ."\n"; +} + +< v2 +$out = diff_it load qw/ rename1.yml rename2.yml /; + +eq_or_diff $out, < v3 +$out = diff_it load qw/ rename2.yml rename3.yml /; + +eq_or_diff $out, < v4 +$out = diff_it load qw/ rename3.yml rename4.yml /; + +eq_or_diff $out, < v5 +$out = diff_it load qw/ rename4.yml rename5.yml /; + +eq_or_diff $out, < 10; +plan tests => 6; my @warns; local $SIG{__WARN__} = sub { @@ -194,61 +194,3 @@ is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed column can't fi 'Warning: old column not found'; is shift @warns, q!SQL::Translator::Diff::schema_diff(): Renamed table can't find old table "not_exists" for renamed table!, 'Warning: old table not found'; - - -sub diff_it { - my( $src, $dst ) = @_; - - my $out = SQL::Translator::Diff::schema_diff( - $src, - 'PostgreSQL', - $dst, - 'PostgreSQL', { - ignore_index_names => 1, - ignore_constraint_names => 1, - sqlt_args => { - quote_identifiers => 0, - } - } - ); - - return $out =~ s/^.*?BEGIN;\n+(.*)COMMIT.*?$/$1/sr =~ s/\n+$//gmr ."\n"; -} - -< v2 -$out = diff_it load qw/ rename1.yml rename2.yml /; - -eq_or_diff $out, < v3 -$out = diff_it load qw/ rename2.yml rename3.yml /; - -eq_or_diff $out, < v4 -$out = diff_it load qw/ rename3.yml rename4.yml /; - -eq_or_diff $out, < v5 -$out = diff_it load qw/ rename4.yml rename5.yml /; - -eq_or_diff $out, < Date: Sun, 8 Jun 2025 13:21:30 -0400 Subject: [PATCH 10/10] Rename callback: next -> init --- lib/SQL/Translator/Diff.pm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/SQL/Translator/Diff.pm b/lib/SQL/Translator/Diff.pm index 5defa538d..f4f7017a0 100644 --- a/lib/SQL/Translator/Diff.pm +++ b/lib/SQL/Translator/Diff.pm @@ -149,7 +149,7 @@ sub _detect_changes { # For each destination object trigger corresponding callback. for my $dst_version ( @$dst ) { my $dst_name = $get_name->( $dst_version ); - $changes->{ next } and $changes->{ next }( $dst_name, $dst_version ); + $changes->{ init } and $changes->{ init }( $dst_name, $dst_version ); my $src_version = $renamed_to->{ $dst_name }; # Corner case: sometimes field is marked as renamed, but does not have previous @@ -197,7 +197,7 @@ sub compute_differences { } my $changes = { - next => sub{ + init => sub{ my( $name ) = @_; $self->table_diff_hash->{ $name } = { map { $_ => [] } @diff_hash_keys }; },