Skip to content

Rework changes detection; Implement tests for renames #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Rework changes detection; Implement tests for renames #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9ab503f
Implement new test
Apr 4, 2025
cd839d9
Remove excess code
Apr 4, 2025
38bee5f
Move the code loading a scheme into subroutine
Apr 4, 2025
79b17e7
Implemented new tests for column renames
Apr 5, 2025
e6fd680
Rework changes detection
Apr 4, 2025
b4d619f
Do not skip creating objects whose names equal to newly renamed
Apr 5, 2025
740fa22
Prioritize renames before create and alter
Apr 5, 2025
fd58e13
Revive warnings when there is no previous version for renamed object
Apr 5, 2025
88a2628
Move tests for renames into its own file
Jun 8, 2025
7dc2124
Rename callback: next -> init
Jun 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
251 changes: 157 additions & 94 deletions lib/SQL/Translator/Diff.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,88 @@ 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 )
Copy link
Contributor

@rabbiveesh rabbiveesh Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not understanding what the next callback is meant to do; would you mind expounding on that?

Copy link
Contributor Author

@KES777 KES777 Jun 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next call back is fired for every dst column. This is to run some code for each dst name/column/table.

At the moment this call back is used to prepare/initialize required data structures which will be filled/used later.
image

I agree that the name is not the best. Probably it could be better if we name it every or init. Just suggest better name you like and I'll change it.

# 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 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 $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 ) );

$renamed_from->{ $name } = 1;
$renamed_to->{ $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->{ 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
# 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
# inside objects between source and destination.
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;
}

# 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) = @_;

Expand All @@ -114,57 +196,42 @@ 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};
my $changes = {
init => 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{
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#;
}
} else {
$src_table = $source_schema->get_table($tar_table_name, $self->case_insensitive);
}
},
alter => sub{
$self->diff_table_options( @_ );

unless ($src_table) {
## table is new
## add table(s) later.
push @{ $self->tables_to_create }, $tar_table;
next;
}
## Compare fields, their types, defaults, sizes etc etc
$self->diff_table_fields( @_ );

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;
$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;
}
Expand Down Expand Up @@ -237,13 +304,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/),
Expand Down Expand Up @@ -366,57 +437,49 @@ 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) {
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{
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#;
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 $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);
},
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 ];
}
},
};

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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/SQL/Translator/Producer/PostgreSQL.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1188,9 +1188,9 @@ sub batch_alter_table {
# now add everything else
push @sql, batch_alter_table_statements(
$diff_hash, $options, qw(
rename_field
Copy link

@matthewpersico matthewpersico Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this change corresponds to the change at https://github.com/dbsrgits/sql-translator/pull/188/files#diff-d1a0629b4bf9c95c1131cc9eaadd08c23943af65ad4b679de4b8190ee019a87bR315-R317, but I don't see why there needs to be a reorder of these entries.

Copy link
Contributor Author

@KES777 KES777 Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthewpersico Thank you for doing review.

It is documented in the commit: 740fa22

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.

Compare:
SRC:x->DST:y, Create DST:x. This works because x was renamed, then we can create field x.
VS
Create DST:x, SRC:x->DST:y. This does not work. We can not create x because schema already has x. Though it will be renamed a few seconds later.

Almost the same logic as for the case when we should drop before add.

Copy link
Contributor Author

@KES777 KES777 Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated statements should be in the same order. Imagine if here
image

a column is not renamed to something else (1), then ADD COLUMN statement (2) will fail.

add_field
alter_field
rename_field
alter_create_index
alter_create_constraint
alter_table
Expand Down
2 changes: 1 addition & 1 deletion lib/SQL/Translator/Utils.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions t/30sqlt-new-diff-mysql.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down
Loading
Loading