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 same type comparing #261

Merged
merged 2 commits into from Mar 19, 2024
Merged

Fix same type comparing #261

merged 2 commits into from Mar 19, 2024

Conversation

0marq
Copy link
Contributor

@0marq 0marq commented Mar 15, 2024

fieldColumnType.DatabaseTypeName() returns non capitalized string, while fileType.SQL returns capitalized string. This commit aims to fix this and avoid detecting a type change when the type did not change.

  • [ x ] Do only one thing
  • [ x ] Non breaking API changes
  • [ x ] Tested

What did this pull request do?

Fix same type comparing when running auto-migrate

User Case Description

When running auto migrate, we detected we ended up attempting to execute the statement.

ALTER TABLE "myTable" ALTER COLUMN "myColumn" TYPE JSONB USING "myColumn"::JSONB

This resulted in an error since our database (CockroachDB) does not allow type alter statements on JSONB columns.

After reviewing the code and debugging, I noticed that this behaviour was due to this comparison.
fieldColumnType.DatabaseTypeName() returns non capitalised string, while fileType.SQL returns capitalised string.
After using strings.ToUpper on fieldColumnType.DatabaseTypeName() before comparing it to fileType.SQL, this type change is no longer detected and auto-migrate succeeds.

This PR aims to fix this and avoid detecting a type change when the type did not change.

`fieldColumnType.DatabaseTypeName()` returns non capitalized string, while `fileType.SQL` returns capitalized string.
This commit aims to fix this and avoid detecting a type change when the type did not change.
@0marq
Copy link
Contributor Author

0marq commented Mar 15, 2024

Maybe I could call strings.ToUpper on both expressions, instead of only on fieldColumnType.DatabaseTypeName. I'll wait for your opinion before. :)

migrator.go Outdated
@@ -312,7 +312,7 @@ func (m Migrator) AlterColumn(value interface{}, field string) error {
fileType := clause.Expr{SQL: m.DataTypeOf(field)}
// check for typeName and SQL name
isSameType := true
if fieldColumnType.DatabaseTypeName() != fileType.SQL {
if strings.ToUpper(fieldColumnType.DatabaseTypeName()) != fileType.SQL {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, nice catch ! Updated

@jinzhu jinzhu merged commit dba70db into go-gorm:master Mar 19, 2024
1 check passed
@0marq 0marq mentioned this pull request Mar 19, 2024
3 tasks
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.

None yet

2 participants