Automigration fails for existing table if model has a boolean field.

[ayakut]

• Do only one thing
• Non breaking API changes
• Tested: project's unit tests and on connection to real Cockroach DB.

What did this pull request do?

The PR fixes how the MigrateColumn() method calculates an initial value of the isSameType flag.

User Case Description

Environment:
Go 1.21
DB: Cockroach DB ()
used versions

github.com/jackc/pgx/v5 v5.5.5
gorm.io/driver/postgres v1.5.7
gorm.io/gorm v1.25.8

Issue reproduction:

If you have a model that contains a boolean field, e.g.

Model struct {
  Id int
  TestBool bool
}

And call

gorm.AutoMigrate(&Model{})

then
If the table does not exist in a DB yet - then the auto-migration logic just creates it and it works fine,
BUT
If the table exists in a DB: the auto-migration logic analyzes new vs. existing column configurations. And in this case, it crashes if the model contains boolean fields. Here is except the trace

panic({0xc44060?, 0x1432f00?})

/usr/local/go/src/runtime/panic.go:914 +0x21f

database/sql.(*ColumnType).DecimalSize(...)

/usr/local/go/src/database/sql/sql.go:3175

gorm.io/gorm/migrator.ColumnType.DecimalSize(...)

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator/column_type.go:75

gorm.io/gorm/migrator.Migrator.MigrateColumn({{0x40?, 0xc000328330?, {0xe8e948?, 0xc0002bc900?}}}, {0xd1ea00, 0xc00039c050}, 0xc0004663c0, {0xe8fc88, 0xc0002e72b0})

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator/migrator.go:497 +0x402

gorm.io/driver/postgres.Migrator.MigrateColumn({{{0xa0?, 0xc000328330?, {0xe8e948?, 0xc0002bc900?}}}}, {0xd1ea00, 0xc00039c050}, 0xc0004663c0, {0xe8fc88?, 0xc0002e72b0?})

/home/sio/go/pkg/mod/gorm.io/driver/[email protected]/migrator.go:268 +0x89

gorm.io/gorm/migrator.Migrator.AutoMigrate.func1(0xc00046e8c0)

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator/migrator.go:156 +0x231

gorm.io/gorm/migrator.Migrator.RunWithValue({{0x20?, 0xc000290750?, {0xe8e948?, 0xc0002bc900?}}}, {0xd1ea00?, 0xc00039c050}, 0xc0002afb70)

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator/migrator.go:74 +0x137

gorm.io/gorm/migrator.Migrator.AutoMigrate({{0x0?, 0xc000290750?, {0xe8e948?, 0xc0002bc900?}}}, {0xc0002b4300?, 0x0?, 0x0?})

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator/migrator.go:129 +0x1be

gorm.io/gorm.(*DB).AutoMigrate(0xc000328cf0?, {0xc0002b4300, 0x1, 0x1})

/home/sio/go/pkg/mod/gorm.io/[email protected]/migrator.go:24 +0x42

[ayakut]

By the way,

When I run unit-tests on my local they pass OK 100%

sio@localhost [/home/.../ayakut-gorm/tests] [master]$  go test ./...
ok      gorm.io/gorm/tests 

[a631807682]

We can't seem to assume that the same prefix means the same type because they may have different sizes and precisions.

[ayakut]

Hi, So, what diff in size or precision is expected for type boolean vs bool? The current code crash with nil-panic when it check size and precision of the boolean. Moreover, after isSameType initialization, you do check if not has prefix then let's try aliases. But boolean yes has prefix bool. So the logic goes to checking size and precision. It looks weird... Probably we can extend this logic with explicit special check for boolean type?

…

On Thu, Mar 28, 2024, 09:49 Cr. ***@***.***> wrote: ***@***.**** commented on this pull request. We can't seem to assume that the same prefix means the same type because they may have different sizes and precisions. — Reply to this email directly, view it on GitHub <#6923 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATLB5R5HCPMMCKNSEVOWA3Y2PDQJAVCNFSM6AAAAABFIPQZJGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRVGM4TGOJYGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[a631807682]

Can you provide a replica code in the playground? It seems to have been fixed in go-gorm/postgres#256

[ayakut]

Hi

> It seems to have been fixed in go-gorm/postgres#256

<go-gorm/postgres#256> Yes, they added "bool" as an alias of the "boolean" but as you can check on your own - this alias is not "working" in this function: https://github.com/go-gorm/gorm/blob/1b48aa072d1210c2ba315aeea18b57fddb634875/migrator/migrator.go#L448

> Can you provide a replica code in the playground?

I'm not sure how I can do it as it requires a connection to DBMS... As I've written in the PR description you can try the following steps to reproduce the issue: --- If you have a model that contains a boolean field, e.g. Model struct { Id int TestBool bool } And call: gorm.AutoMigrate(&Model{}) then If the table does not exist in a DB yet - then the auto-migration logic just creates it and it works fine, BUT If the table exists in a DB: the auto-migration logic analyzes new vs. existing column configurations. And in this case, it crashes if the model contains boolean fields.

…

On Thu, Mar 28, 2024 at 11:58 AM Cr. ***@***.***> wrote: Can you provide a replica code in the playground? It seems to have been fixed in go-gorm/postgres#256 <go-gorm/postgres#256> — Reply to this email directly, view it on GitHub <#6923 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AATLB5XMA26XNTOA6PJ5ND3Y2PSTVAVCNFSM6AAAAABFIPQZJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRUHAYTGMJYGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- ----------------------------- Sincerely, Alex Kutko

[ayakut]

Can you provide a replica code in the playground? It seems to have been fixed in go-gorm/postgres#256

Please check - I refactored the solution to minimize the risk of regression (all UTs on my local are passed OK)

[a631807682]

1. Please add some unit tests describing what was fixed.
2. Can you describe why different prefixes must be the same type?