Rework changes detection; Implement tests for renames by KES777 · Pull Request #188 · dbsrgits/sql-translator

KES777 · 2025-04-05T03:15:24Z

In this PR we:

Add more tests for renames workflows.
Simplify code to detect renames.
Now function could be easily reused for different types of objects
Document code
Fix the issue when field is created before renames happened.
This happens when we want to drop the old object and give the name of dropped object to another one.

This changes are related to #184

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.

Fixes: 185 GitHub: dbsrgits#185

KES777 · 2025-04-06T17:30:25Z

Also fixes this: jjn1056/DBIx-Class-Migration#129

Eg. SRC:x, DST:y:rx, DST:x

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.

rabbiveesh

apologies on the slow turnaround; out of perl now and just started a new $job, but i'll see if i can squeeze this in.
Trying to understand the full intent + then i can dig into more

rabbiveesh · 2025-06-05T20:53:04Z

the tests here have nothing to do with the rest of the producer behaviors, could you just put this in a new diff test which produces yaml?

I'll do that.

Btw it was done few month ago.

rabbiveesh · 2025-06-05T20:57:56Z

+# alter ( $src_version, $dst_version )
+# create( $dst_version )
+# drop  ( $src_version )
+# next  ( $dst_name, $dst_version )


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

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.

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.

okay i finally got a full undestanding here;
here's my take re naming -
$changes gets renamed to actions, and all of its fields get a name prefixed with on_ b/c these are actions - so on_create, on_drop, on_rename and so on

the $is_renamed should be called $check_renamed cuz it sounds like a boolean rather than a callback
honestly, all 3 of those params can get put together under one hash called checks or something, and also have names that more clearly say that they're callbacks

i'm ready to approve once the naming changes are in and also the comments are adjusted (right now they're out of sync, code uses init and the comment says next)

KES777 · 2025-06-08T19:07:48Z

Intention in this PR is to fix bugs, generalize code and extend tests.

matthewpersico · 2025-09-11T16:08:03Z

  # now add everything else
  push @sql, batch_alter_table_statements(
    $diff_hash, $options, qw(
+      rename_field


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.

@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.

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

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

matthewpersico

So sorry this fell off the table.

rabbiveesh · 2026-09-07T21:17:08Z

+    rename =>  sub{
+      my( $src, $dst ) =  @_;
+      if( $src ) {
+        push @{ $diff_hash->{fields_to_rename} }, [ $src, $dst ]; $skip = 1;


the $skip state tracking here is subtle and easy to misunderstand; i'd recommend something else:

in your engine above, you should have rename NOT fire alter also, and then the rename callback can itself call the alter callback in the table case.

what do you think about that?

rabbiveesh

I've settled on names, and then i have comments about the $skip bit in the table field diff

Eugen Konkov added 3 commits April 3, 2025 22:44

Implement new test

9ab503f

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.

Remove excess code

cd839d9

Fixes: 185 GitHub: dbsrgits#185

Move the code loading a scheme into subroutine

38bee5f

KES777 force-pushed the implement_tests_for_renames branch from 0575de1 to 9ddf601 Compare April 5, 2025 03:16

KES777 mentioned this pull request Apr 11, 2025

Do not drop renamed field at 'downgrade' migration script #184

Open

KES777 force-pushed the implement_tests_for_renames branch from 63d26ac to f9e50ba Compare May 4, 2025 21:00

Eugen Konkov added 5 commits May 4, 2025 17:03

Implemented new tests for column renames

79b17e7

Rework changes detection

e6fd680

Do not skip creating objects whose names equal to newly renamed

b4d619f

Eg. SRC:x, DST:y:rx, DST:x

Prioritize renames before create and alter

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.

Revive warnings when there is no previous version for renamed object

fd58e13

KES777 force-pushed the implement_tests_for_renames branch from f9e50ba to fd58e13 Compare May 4, 2025 21:03

rabbiveesh reviewed Jun 5, 2025

View reviewed changes

Eugen Konkov added 2 commits June 8, 2025 13:19

Move tests for renames into its own file

88a2628

Rename callback: next -> init

7dc2124

KES777 requested a review from rabbiveesh June 8, 2025 19:07

matthewpersico reviewed Sep 11, 2025

View reviewed changes

KES777 requested a review from matthewpersico October 7, 2025 00:11

matthewpersico approved these changes Sep 7, 2026

View reviewed changes

rabbiveesh reviewed Sep 7, 2026

View reviewed changes

rabbiveesh requested changes Sep 7, 2026

View reviewed changes

Uh oh!

Conversation

KES777 commented Apr 5, 2025

Uh oh!

KES777 commented Apr 6, 2025

Uh oh!

rabbiveesh left a comment

Choose a reason for hiding this comment

Uh oh!

rabbiveesh Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

KES777 Jun 7, 2025

Choose a reason for hiding this comment

Uh oh!

KES777 Sep 6, 2025

Choose a reason for hiding this comment

Uh oh!

rabbiveesh Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

KES777 Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rabbiveesh Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

KES777 commented Jun 8, 2025

Uh oh!

matthewpersico Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

KES777 Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KES777 Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewpersico left a comment

Choose a reason for hiding this comment

Uh oh!

rabbiveesh Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

rabbiveesh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KES777 Jun 7, 2025 •

edited

Loading

KES777 Sep 11, 2025 •

edited

Loading

KES777 Sep 11, 2025 •

edited

Loading