Skip to content

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

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

KES777
Copy link
Contributor

@KES777 KES777 commented Mar 27, 2025 •
edited
Loading

Fixes: #183

This PR fixes the issue with 'downgrade' migration where fields are dropped instead of renamed back.

To show the problem better lets describe migration:

  • DB version 1 is generated with field name 'comment'

  • At DB version 2 field 'comment' is renamed to 'new_comment'

  • 'new_comment' field has 'extra'->'renamed_from' property.

  • Version 1 is the OLD version and has 'comment' (this is source)

  • Version 2 is the NEW version and has 'new_comment:renamed_from' (this is target)

When upgrade occurs code checks NEW version and sees 'renamed_from' property thus generates 'RENAME COLUMN' DDL.

But for 'downgrade' migration Version 1 and Version 2 have changed places:

  • Version 2 is the OLD version and has 'new_comment:renamed_from' (this is source)
  • Version 1 is the NEW version and has 'comment' (this is target)

Here the old code does not see 'renamed_from' at target, thus generates DROP/ADD DDLs for 'comment/new_comment' fields.

This process looks like this:

           SRC / OLD                       TAR / NEW
upgrade:   comment                    ->   new_comment:renamed_from
downgrade: new_comment:renamed_from   ->   comment

Solution

To detect renames for downgrades we need to check additionally SRC field for 'renamed_from' property.

Details

For upgrade, the script works well and generates this diff:

  email => {
    constraints_to_create => [],
    constraints_to_drop => [],
    fields_to_alter => [],
    fields_to_create => [],
    fields_to_drop => [],
    fields_to_rename => [
      [
        SQL::Translator::Schema::Field {
          _ERROR => ,
          comments => [],
          data_type => text,
          extra => {},
          is_nullable => 1,
          is_primary_key => 0,
          name => comment,
          order => 5,
          size => [
            0,
          ],
          table => SQL::Translator::Schema::Table email,
        },
        SQL::Translator::Schema::Field {
          _ERROR => ,
          comments => [],
          data_type => text,
          extra => {
            renamed_from => comment,
          },
          is_nullable => 1,
          is_primary_key => 0,
          name => commentx,
          order => 5,
          size => [
            0,
          ],
          table => SQL::Translator::Schema::Table email,
        },
      ],
    ],
    indexes_to_create => [],
    indexes_to_drop => [],
    table_options => [],
    table_renamed_from => [],
  },

But for downgrade I see this:

{
  constraints_to_create => [],
  constraints_to_drop => [],
  fields_to_alter => [],
  fields_to_create => [
    SQL::Translator::Schema::Field {
      _ERROR => ,
      comments => [],
      data_type => text,
      extra => {},
      is_nullable => 1,
      is_primary_key => 0,
      name => comment,
      order => 5,
      size => [
        0,
      ],
      table => SQL::Translator::Schema::Table email,
    },
  ],
  fields_to_drop => [
    SQL::Translator::Schema::Field {
      _ERROR => ,
      comments => [],
      data_type => text,
      extra => {
        renamed_from => comment,
      },
      is_nullable => 1,
      is_primary_key => 0,
      name => commentx,
      order => 5,
      size => [
        0,
      ],
      table => SQL::Translator::Schema::Table email,
    },
  ],
  fields_to_rename => [],
  indexes_to_create => [],
  indexes_to_drop => [],
  table_options => [],
  table_renamed_from => [],
}

The issue is that the previous version does not have extra->renamed_from part :D

When we are upgrading we can see at Translator::Diff::diff_table_fields target field is:

SQL::Translator::Schema::Field {
  _ERROR => ,
  comments => [],
  data_type => text,
  extra => {
    renamed_from => comment,
  },
  is_nullable => 1,
  is_primary_key => 0,
  name => commentx,
  order => 5,
  size => [
    0,
  ],
  table => SQL::Translator::Schema::Table email,
}

But when downgrading target field is:

SQL::Translator::Schema::Field {
  _ERROR => ,
  comments => [],
  data_type => text,
  extra => {},
  is_auto_increment => 0,
  is_nullable => 1,
  is_primary_key => 0,
  name => comment,
  order => 5,
  size => [
    0,
  ],
  table => SQL::Translator::Schema::Table email,
}

@rabbiveesh
Copy link
Contributor

Can i bother you to add a test for this? otherwise i'll write one myself, but it won't be right away.
The solution seems good and i went over some stickier cases + it looks to be that it'll do the right thing; but tests are best

@KES777
Copy link
Contributor Author

KES777 commented Mar 27, 2025

Yes, I would like to write, but I do not see a proper way how to do that. I need more insights on this, eg. how to setup extra->{renamed_from} via SCHEMA diff? It looks like the test should involve DBICM module... (

I do not want it to stale for a while again. Please provide more thoughts, if you are Ok we can even arrange a call.

@rabbiveesh
Copy link
Contributor

well, since it's just logic, you can set up a test where you provide 2 incredibly minimal yaml schema, and then test that the conversion is generated sensibly in both directions;
i can do it, i'm just on the busier side recently

@rabbiveesh
Copy link
Contributor

you can always shoot me a message on irc, i'm veesh on perl + libera

@KES777
Copy link
Contributor Author

KES777 commented Mar 27, 2025

Ah! I got it! I was thinking about SQL schema, but yaml will resolve the problem.

Thanks for the hint!!! 🚀

@KES777
Copy link
Contributor Author

KES777 commented Mar 31, 2025 •
edited
Loading

hm... this discovers another bug: renamed tables are dropped/added during downgrades =(
I suppose the algorithm should be improved.

@rabbiveesh
Copy link
Contributor

rabbiveesh commented Mar 31, 2025 via email

@KES777
Copy link
Contributor Author

KES777 commented Mar 31, 2025 •
edited
Loading

I did deeper analysis. It could not be fixed easily. We need to pass additionally the direction of migration: downgrade or upgrade. This is the bigger change and need to be negotiated.

Why direction of migration is required

Imagine this steps:

V1 V2 V3
X Y:RX X

Where X is column name, Y:RX is column with Y name and reanmed_from property set to X name.

What happens when we generate migration scripts

Migration Diff result [src, tar] Description
v1 -> v2 X, Y:RX This works well. The X was renamed to Y.
v2 -> v3 Y:RX, X This is also expected. The Y was dropped and X was created.
v2 -> v1 Y:RX, X For downgrade script it is not expected that Y will be dropped and X will be created from scratch. During migration v1->v2 it was renamed and now durint v2->v1 we expect it should be renamed back. But this drop happens because it works as designed for upgrade direction. But this scenario it is downgrade. Unfortunately We can not detect this automatically: the order of [src, dst] is the same and the set of extra properties are the same.

(For now I am skipping more complex scenarios when v3 migration has columns with created and/or renamed names similar to v2 version and its consequences on v1)

Proposition

From the above to get the expected result during downgrades we need to pass the flag that the downgrade migration should be generated.

@rabbiveesh
Copy link
Contributor

rabbiveesh commented Apr 1, 2025 via email

@KES777
Copy link
Contributor Author

KES777 commented Apr 1, 2025

Ok

@KES777
Copy link
Contributor Author

KES777 commented Apr 3, 2025

I asked to pass direction to SQLT from DBIC:DH frioux/DBIx-Class-DeploymentHandler#81
If merged then it will resolve the generation of wrong downgrade migration. I also reworked how diffs are detected. Those makes code much clear/maintainable. I'll create another PR a bit later.

@rabbiveesh
Copy link
Contributor

I already said that i don't think that 'direction' is in the scope of this tool.
If he's going to make changes there, then he should simply handle putting the renamed flag in the correct place.
I will not accept a PR that implements direction

@KES777
Copy link
Contributor Author

KES777 commented Apr 3, 2025

then he should simply handle putting the renamed flag in the correct place.

What do you mean?

@KES777 KES777 mentioned this pull request Apr 5, 2025
Open
@KES777
Copy link
Contributor Author

KES777 commented Apr 11, 2025

Ignore these changes for a moment. Please let's proceed with #188 first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Downgrade script is wrong. Data was lost.
2 participants
@KES777 @rabbiveesh