tableRegexp does not match certain multi-line "CREATE TABLE" statements

[d-Rickyy-b]

GORM Playground Link

I did not provide a playground link, because I was able to pinpoint the issue and will provide a PR to fix it.
This report rather serves as a reference and documentation.

tl;dr

AutoMigrate with gorm did not work for me on my sqlite db. It tried to create a table that already existed. The reason were two regexes in the sqlite connector that did not cover a specific edge case.

Description

I recently upgraded gorm to the latest version in my project and found that it displayed an error message to me:

SQL logic error: table "users" already exists (1)

It tried to create a new table which already existed.

This was weird because of two reasons. First, it worked with an older version of gorm and second, even if the table exists, gorm should just update its fields by creating a temporary table and copying over the contents.

Narrowing down the issue

So I did some investigations and found out that the actual breaking commit was go-gorm/gorm@93986de. Every version released before this commit worked, every version after did not. The problematic lines in the commit were these ones:
https://github.com/go-gorm/gorm/blob/93986de8e43bc9af6864621c9a4855f0f860cde2/migrator/migrator.go#L456-L459

When using AutoMigrate with my table, line 458 got executed. That shows that gorm did not manage to read the default value from the table schema and thought it needs to create a default value for a column in my table. But instead the database already had a default value set.

I assumed that it was due to a parsing issue, so I checked the schema and the parsing code. The schema was pretty weirdly formatted but valid sql.

CREATE TABLE "users"
(
    id         integer
        primary key
        unique,
    created_at datetime,
    updated_at datetime,
    username   text,
    first_name text,
    last_name  text,
    lang_code  text
, `dark_mode` numeric DEFAULT true);

Parsing issue nr. 1

There is indeed a parsing issue right here:

sqlite/ddlmod.go

Lines 31 to 34 in 502ed63

	func parseDDL(strs ...string) (*ddl, error) {
	var result ddl
	for _, str := range strs {
	if sections := tableRegexp.FindStringSubmatch(str); len(sections) > 0 {

tableRegex = (?is)(CREATE TABLE [`|\"|'|\t]?[\\w\\d-]+[`|\"|'|\t]?)(?: \\((.*)\\))?

The tableRegex wasn't able to parse the above sql statement, because of the line break right after "users". That's the reason why the returned sqlDDL did not contain any fields/columns. The part of the tableRegex (?: \\((.*)\\))? expected a space (0x20) after the quotes around the table name. In my case the SQL did not contain a space but a newline.

Side note: The regex isn't very good overall, because it also matches things like CREATE TABLE |users| because | is being used in the character set for "separators".

That regex issue leads to the columnTypes return value being filled with only the rawColumnTypes (SELECTed from the actual table) and hence contain no information about e.g. autoincrement, default values, etc.
https://github.com/go-gorm/gorm/blob/b444011d094db7444f87f442c33860365f55770a/migrator/migrator.go#L116-L118

Then the MigrateColumn code gets executed and for any column that has additional properties (like uniqueness, nullability, default values) we will at some point set the alterColumn variable to True. In my case the dark_mode column caused the issue due to a default value being used.
https://github.com/go-gorm/gorm/blob/b444011d094db7444f87f442c33860365f55770a/migrator/migrator.go#L503-L506

Since alterColumn was set to true, gorm now executed the AlterColumn method of the sqlite module, where we finally call m.recreateTable().
https://github.com/go-gorm/gorm/blob/b444011d094db7444f87f442c33860365f55770a/migrator/migrator.go#L524-L526

AlterColumn Method:

sqlite/migrator.go

Lines 79 to 100 in 502ed63

	func (m Migrator) AlterColumn(value interface{}, name string) error {
	return m.RunWithoutForeignKey(func() error {
	return m.recreateTable(value, nil, func(rawDDL string, stmt *gorm.Statement) (sql string, sqlArgs []interface{}, err error) {
	if field := stmt.Schema.LookUpField(name); field != nil {
	// lookup field from table definition, ddl might looks like `'name' int,` or `'name' int)`
	reg, err := regexp.Compile("(`|'|\"| )" + field.DBName + "(`|'|\"| ) .*?(,|\\)\\s*$)")
	if err != nil {
	return "", nil, err
	}
	createSQL := reg.ReplaceAllString(rawDDL, fmt.Sprintf("`%v` ?$3", field.DBName))
	if createSQL == rawDDL {
	return "", nil, fmt.Errorf("failed to look up field %v from DDL %v", field.DBName, rawDDL)
	}
	return createSQL, []interface{}{m.FullDataTypeOf(field)}, nil
	}
	return "", nil, fmt.Errorf("failed to alter field with name %v", name)
	})
	})
	}

Parsing issue nr. 2

In the recreateTable method, we want to create a temporary table name (e.g. users__temp) in order to copy over all existing fields from the old table to a newly created one, to migrate to a new db schema.

sqlite/migrator.go

Lines 383 to 397 in 502ed63

	newTableName := table + "__temp"
	createSQL, sqlArgs, err := getCreateSQL(rawDDL, stmt)
	if err != nil {
	return err
	}
	if createSQL == "" {
	return nil
	}
	tableReg, err := regexp.Compile(" ('|`|\"| )" + table + "('|`|\"| ) ")
	if err != nil {
	return err
	}
	createSQL = tableReg.ReplaceAllString(createSQL, fmt.Sprintf(" `%v` ", newTableName))

But again we have some regex issue here: " ('|`|\"| )" + table + "('|`|\"| ) " - at least one space before and after the table's name is expected. This does not work when there is a newline right after the table name (which is valid sqlite syntax). That finally lead to the case where the actual createSQL was CREATE TABLE users [...] instead of CREATE TABLE users__temp.

I'll create a PR to fix these two regex parsing issues. While debugging I also came across yet another issue, so stay tuned for another (much shorter) report :)

Best regards,
Rico