Fix #6557: Avoid redundant column migrations when using type aliases#7652
Fix #6557: Avoid redundant column migrations when using type aliases#7652DasJott wants to merge 14 commits intogo-gorm:masterfrom
Conversation
|
Prevent redundant migrations by improving type alias comparison and adding migration tests This PR updates column type comparison logic in Additionally, test infrastructure changes update Key Changes• Reworked type comparison in Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
propel-code-bot
bot
changed the title
|
Hi @DasJott Can you add some tests？ |
propel-code-bot
bot
changed the title
propel-code-bot
bot
changed the title
There was a problem hiding this comment.
Suggested guarding type comparisons when database type names are empty to avoid unnecessary ALTERs.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Handle empty database type names to prevent redundant migrations:
migrator/migrator.go
Review Details
📁 6 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
| // found, smart migrate | ||
| fullDataType := strings.TrimSpace(strings.ToLower(m.DB.Migrator().FullDataTypeOf(field).SQL)) | ||
| realDataType := strings.ToLower(columnType.DatabaseTypeName()) | ||
| dbDataType := strings.ToLower(columnType.DatabaseTypeName()) // from db |
There was a problem hiding this comment.
[Logic] If columnType.DatabaseTypeName() returns an empty string (drivers can legally return "" for unsupported types), baseDbType becomes "" and aliasMatch stays false, which sets alterColumn = true and can trigger unnecessary ALTERs on every migration. The previous logic treated empty realDataType as a prefix match and didn’t force an ALTER just because the driver lacked a type name. Can you add a guard so an empty dbDataType doesn’t force alterColumn (e.g., short‑circuit type comparison when dbDataType == "" or treat it as isSameBaseType = true)?
Context for Agents
If `columnType.DatabaseTypeName()` returns an empty string (drivers can legally return "" for unsupported types), `baseDbType` becomes "" and `aliasMatch` stays false, which sets `alterColumn = true` and can trigger unnecessary ALTERs on every migration. The previous logic treated empty `realDataType` as a prefix match and didn’t force an ALTER just because the driver lacked a type name. Can you add a guard so an empty `dbDataType` doesn’t force `alterColumn` (e.g., short‑circuit type comparison when `dbDataType == ""` or treat it as `isSameBaseType = true`)?
File: migrator/migrator.go
Line: 473fix(migrate_test): update test structure for array types and generic types fix(migrate_test): ensure no ALTER statements are generated during migrations
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Found a logic gap in type comparison that could miss array-vs-scalar schema mismatches.
Status: Changes Suggested | Risk: Medium
Issues Identified & Suggestions
- Detect array-vs-scalar differences before base-type stripping:
migrator/migrator.go
Review Details
📁 4 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
|
|
||
| // strip length/precision specifiers for base-type comparison | ||
| baseDbType := dbDataType | ||
| if p := strings.IndexAny(dbDataType, "(["); p >= 0 { |
There was a problem hiding this comment.
[Logic] Array vs scalar mismatch can be missed after stripping "[" in the base-type comparison. If dbDataType includes array notation (e.g., varchar[]) but the struct type is scalar (varchar), baseDbType/baseStructType both become varchar, isSameBaseType becomes true, and alterColumn never flips—leaving the schema incorrect.
Fix: before stripping, detect array-ness and force alterColumn when one side is array and the other isn't, e.g. compare strings.Contains(dbDataType, "[]") with strings.Contains(m.DataTypeOf(field), "[]") and set alterColumn = true (or treat isSameBaseType as false) when they differ.
Context for Agents
Array vs scalar mismatch can be missed after stripping "[" in the base-type comparison. If `dbDataType` includes array notation (e.g., `varchar[]`) but the struct type is scalar (`varchar`), `baseDbType`/`baseStructType` both become `varchar`, `isSameBaseType` becomes true, and `alterColumn` never flips—leaving the schema incorrect.
Fix: before stripping, detect array-ness and force `alterColumn` when one side is array and the other isn't, e.g. compare `strings.Contains(dbDataType, "[]")` with `strings.Contains(m.DataTypeOf(field), "[]")` and set `alterColumn = true` (or treat `isSameBaseType` as false) when they differ.
File: migrator/migrator.go
Line: 4873 participants
What did this pull request do?
Improves the mapping of types to aliases.
The mapping keys do not (and CAN not) include any additional information behind the actual type name.
Therefore to check for aliases in a certain mapping we need to strip that info off, like:
varchar(255)[] => varchar.To really be sure for the mapping to work reliably the types are mapped best in both ways, like:
varchar => character varying,character varying => varchar.For at least postgres this was not the case for exactly that case.
For not having to touch all the other driver packages, this code simply tries to map the other way around when the first attempt failed.
User Case Description
We had a data struct containing a field
pq.StringArraywithgorm:"type:varchar[]". That type was returned by postgres ascharacter varying[]. The current code could not map that correctly and therefore on every service restart an unnecessary migration was started (which took very long on a table containing millions of entries).This is fixed with this PR.