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

[CockroachDB] Fixing how it handle fields DEFAULT values and column types #143

Merged
merged 3 commits into from Jan 1, 2023

Conversation

rwrz
Copy link
Contributor

@rwrz rwrz commented Nov 2, 2022

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

What did this pull request do?

Currently, CockroachDB returns :::int8 together with their default values. We have a regex to remove it, that solves issues with PostgreSQL values, but it doesn't solve cockroachdb ones. So, I'm adding another regex to fix it.
Also, column field types has aliases, and the migrator check them to decide if we need to ALTER a column or not. But the driver is not checking it, so, I'm adding this alias check as well.

User Case Description

You can test this this model:

type Example struct {
	Code   int    `gorm:"default:0"`
}

It would trigger the ALTER COLUMN every time since int returns int8 and wasn't considering the alias to bigint. Also, it would have issues with the default:0, since it would check against 0:::int8 and 0.

migrator.go Outdated
@@ -424,6 +439,8 @@ func (m Migrator) ColumnTypes(value interface{}) (columnTypes []gorm.ColumnType,

if column.DefaultValueValue.Valid {
column.DefaultValueValue.String = regexp.MustCompile(`'(.*)'::[\w]+$`).ReplaceAllString(column.DefaultValueValue.String, "$1")
// cockroachdb, removing :::type
column.DefaultValueValue.String = regexp.MustCompile(`(.*):::[\w]+$`).ReplaceAllString(column.DefaultValueValue.String, "$1")
Copy link
Member

Choose a reason for hiding this comment

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

The following regexp can be merged with the above one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to '?(.*)\b'?:+[\w\s]+$, this should allow an optional QUOTE and matching any number of : between the field name and the field type.
Testing against the strings below will always get the word field:

'field'::character varying
'field':::character varying
field::character varying
field:::character varying

In cockroachdb, the default values returns something like `field:::TIMESTAMPZ`, and the validation here is only by the default value, without its type, this regex is required.
… cases:

`'field'::character varying`
`'field':::character varying`
`field::character varying`
`field:::character varying`
@rwrz rwrz force-pushed the rwrz-cockroachdb-default-fixes branch from 02a1ea8 to 68156b5 Compare December 27, 2022 14:02
@rwrz
Copy link
Contributor Author

rwrz commented Dec 27, 2022

I've merged the regex, but forgot to remove another fix that I'm using here. This fix is to consider the "field type aliases" when checking if the column type has changed.
Mostly because we rely on those aliases for cockroachdb compatibility, but I believe it is also important for PostgreSQL.

if fieldColumnType.DatabaseTypeName() != fileType.SQL {
isSameType = false
// if different, also check for aliases
aliases := m.GetTypeAliases(fieldColumnType.DatabaseTypeName())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix is to consider the "field type aliases" when checking if the column type has changed.

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