Skip to content
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.

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

fix: migrator run with nil schema (#5795) #6303

Closed
wants to merge 5 commits into from

Conversation

black-06
Copy link
Contributor

@black-06 black-06 commented May 9, 2023

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

fix DropColumn RenameColumn HasColumn with string table param.
Close #5795

User Case Description

It should work instead of panic

migrator := DB.Migrator()
migrator.RenameColumn("column_structs", "new_name", "new_new_name")
migrator.HasColumn("column_structs", "new_new_name")
migrator.DropColumn("column_structs", "new_new_name")

@black-06 black-06 requested a review from a631807682 May 9, 2023 01:38
@black-06 black-06 self-assigned this May 9, 2023
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

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

This is a breaking change, because we use if stmt.Schema == nil in many places to judge

@black-06
Copy link
Contributor Author

black-06 commented May 9, 2023

This is a breaking change, because we use if stmt.Schema == nil in many places to judge

Okay, I'll modify it

@black-06 black-06 changed the title fix: drop column with string table (#5795) fix: migrator run with nil schema (#5795) May 10, 2023
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

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

Test cases need to wait for drivers to be merged.
Can tests cover changes? For example, AlterColumn/AddColumn needs to provide datatype, so it does not support specifying by table name, and an error needs to be returned at this time.

@black-06
Copy link
Contributor Author

Test cases need to wait for drivers to be merged. Can tests cover changes? For example, AlterColumn/AddColumn needs to provide datatype, so it does not support specifying by table name, and an error needs to be returned at this time.

modified~

@a631807682
Copy link
Member

The driver changes have been merged, I re-triggered the ci but the test still fails, is it related to this change?

@black-06
Copy link
Contributor Author

The driver changes have been merged, I re-triggered the ci but the test still fails, is it related to this change?

I will check it.

@black-06
Copy link
Contributor Author

When RenameIndex(&IndexStruct{}, "idx_index_structs_name", "idx_users_name_1") in sqlite, it actually exec

CREATE INDEX `idx_users_name_1` ON `index_structs`(`name`)

This is a bug, right? Because idx_index_structs_name still exist.
We need to fix it first

@a631807682
Copy link
Member

When RenameIndex(&IndexStruct{}, "idx_index_structs_name", "idx_users_name_1") in sqlite, it actually exec

CREATE INDEX `idx_users_name_1` ON `index_structs`(`name`)

This is a bug, right? Because idx_index_structs_name still exist. We need to fix it first

yes

@black-06 black-06 closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DropColumn with string table panics with a nil dereference
3 participants