Gorm: Migrator().ColumnTypes() has only partial information for some drivers

[asmeikal]

GORM Playground Link

https://github.com/go-gorm/playground/pull/183

The playground PR highlights that some column information is missing even when I expect it to be available.

Description

I was using the AutoMigrate feature and I found out that removing a not null constraint from a column in postgres is never really applied to the DB.

// changin this:
type Entity struct {
  Name string `gorm:"not null"`
}

// to this:
type Entity struct {
  Name string `gorm:"nullable"`
}
// has no effect when using AutoMigrate

The cause for this is the fact that sql.ColumnType has different information available based on the underlying driver.

Nullable reports whether the column may be null. If a driver does not support this property ok will be false.

Source

When using pq as a driver, their implementation of ColumnType does not implement the RowsColumnTypeNullable interface from sql, see here for the pq implementation.

A possible solution would be removing all references to sql.ColumnType from the migrator interface, and replace them with an interface compatible with sql.ColumnType:

type ColumnType interface {
    Name() string
    DatabaseTypeName() string
    Length() (length int64, ok bool)
    DecimalSize() (precision int64, scale int64, ok bool)
    Nullable() (nullable bool, ok bool)
}

type Migrator interface {
        // unchanged
    MigrateColumn(dst interface{}, field *schema.Field, columnType ColumnType) error
        // unchanged
    ColumnTypes(dst interface{}) ([]ColumnType, error)
        // unchanged
}

Each driver could then implement its own ColumnTypes version, e.g., using the INFORMATION_SCHEMA tables for postgres, to have all column information available.

[jinzhu]

Each database driver implemented the function by itself, maybe open an issue for the driver if it doesn't match your requirements.

[asmeikal]

The problem cannot be solved by the database drivers repo.

sql.ColumnType is based on the raw response from the database. For postgres, the column information is taken from the RowDescription message (here is the driver implementation), which does not contain any information about column nullability.

The go driver cannot solve this problem: the database does not give this type of information when running a SELECT. The solution on their side would be to run an additional query for each SELECT just to get nullability information, which is highly inefficient. The correct solution would be for the Migrator for postgres to look at information_schema.columns, which is already done in other cases.

I don't think you should close this issue: this problem causes two bugs in AutoMigrate and MigrateColumn on postgres and mysql:

• For postgres, changing not null to nullable (or nullable to not null) is not detected by AutoMigrate and MigrateColumn.
• For mysql, changing the size of a field is not detected by AutoMigrate and MigrateColumn.

This problem also affects gormigrate and anyone else that tries to use the Migrator to generate migrations.

If you want, I can try and put together a few PRs for postgres and mysql, by changing the interface as described in the first message.

[maeglindeveloper]

hi, I have the same issue on my side.
I post an issue there but it seems to be related https://github.com/go-gorm/gorm/issues/3645

[jinzhu]

@asmeikal It would be great if you can submit a PR to implement the ColumnTypes method the mysql, postgres driver

[asmeikal]

@jinzhu I opened #3647 to update the ColumnTypes interfaces. I will open two PRs on the mysql and postgres driver once this is merged.

[asmeikal]

@jinzhu I opened this PR on the postgres driver repo, and this PR on the mysql driver repo. I will need to update the gorm version in go.mod to 1.20.5 once a new tag is added to this repo.

I updated the original PR highlighting the issue to use the two forks of the drivers, and the tests for mysql and postgres are passing.

[maeglindeveloper]

@asmeikal , can you confirm that if the type of a column change, lets say from varchar(36) to a SET for instance, it will work as well with this PR ?
Thanks a lot for all your work @asmeikal @jinzhu <3 !

[asmeikal]

@maeglindeveloper I'm not familiar with mysql and the SET data type, but the migration should happen. The database may reject the statement (e.g., changing a varchar to integer blindly), but data type migrations were already handled.

[asmeikal]

@jinzhu I mixed up the information_schema.columns structure of mysql and postgres, and unintentionally added a bug to the postgres implementation of ColumnTypes. I opened this PR to fix it as soon as I realized it. Sorry for the mistake.

[jinzhu]

@jinzhu I mixed up the information_schema.columns structure of mysql and postgres, and unintentionally added a bug to the postgres implementation of ColumnTypes. I opened this PR to fix it as soon as I realized it. Sorry for the mistake.

Merged, thank you for your awesome work.