Bug: Composite Primary Keys Not Fully Recognized

[Shion1305]

Summary

In go-gorm/sqlite, composite primary keys (i.e., tables with more than one primary key) are not detected correctly. The library recognizes only the first key, failing to mark the other columns as primary keys. This affects table migration and causes incorrect schema interpretation.

GORM Playground Link Recreation with GitHub Actions

I was not able to effectively integrate the gorm playground, so I create my own repository and recreated by GitHub Actions.
Refer This Repository and this GitHub Actions Run

Description

This SQL below creates a table with multiple primary keys.

CREATE TABLE something
(
    column1 INT,
    column2 INT,
    column3 INT,
    PRIMARY KEY (column1, column2)
);

In gorm, we can use ColumnTypes method in Migrator interface to get primary keys in a table.
Calling ColumnTypes after running the SQL should tell us that column1 and column2 are primary keys, however, go-gorm/sqlite says only column1 is a primary key.
Refer this test case below.

func TestColumnTypes(t *testing.T) {
	tc := []struct {
		name              string
		query             string
		expectedIsPrimary map[string]bool
	}{
		{
			name:  "What should be expected",
			query: "CREATE TABLE something\n(\n    column1 INT,\n    column2 INT,\n    column3 INT,\n    PRIMARY KEY (column1, column2)\n);",
			expectedIsPrimary: map[string]bool{
				"column1": true,
				"column2": true,
				"column3": false,
			},
		},
	}
	for _, tt := range tc {
		t.Run(tt.name, func(t *testing.T) {
			dbName := fmt.Sprintf("db_%s.sqlite3", uuid.NewString())
			sqliteConn := sqlite.Open(dbName)
			db, err := gorm.Open(sqliteConn, &gorm.Config{})
			if err != nil {
				t.Errorf("Failed to open new db: %s", err)
			}
			db.Exec(tt.query)
			types, err := db.Migrator().ColumnTypes("something")
			if err != nil {
				t.Errorf("Failed to get column types: %s", err)
			}
			for _, ty := range types {
				pr, valid := ty.PrimaryKey()
				exp := tt.expectedIsPrimary[ty.Name()]
				if pr && valid && exp || !pr && !exp {
					continue
				}
				t.Errorf("Expected %s to be primary key: %t, got: isPrimaryKey: %t, isValid: %t",
					ty.Name(), exp, pr, valid)
			}
		})
	}
}

In this testcase, we call .ColumnTypes after creating a table with 2 primary keys. As column1 and column2 are primary keys, this testcase should pass, but this fails.

Problem: a flaw in getAllColumns() in ddlmod.go

parseDDL() in ddlmod.go loads DDL line by line, if a line start with PRIMARY KEY, it will be passed to getAllColumns() and primary key columns are extracted.

It tries to match the line with this regex: [(,][`|"|'|\t]?(\w+)[`|"|'|\t]?
This regex can process a line like PRIMARY KEY (column1,column2) , however, it fails to process a line like PRIMARY KEY (column1, column2), as indicated in this screenshot.

Contribution

I would like to contribute to fixing this issue. I'm planning to make pull request for this issue in a few days.

[Shion1305]

@jinzhu Thank you for reviewing the PR and merging it! I want to use the new version in my project, could you release a new version?

[PaienNate]

We found another bug relate to getAllColumns(). When we create a table with comment, sqlite will save the create table sql with its comments like that in sqlite_master:

CREATE TABLE attrs (
    id TEXT PRIMARY KEY,
    data BYTEA,
    attrs_type TEXT,

	-- 坏，Get这个方法太严格了，所有的字段都要有默认值，不然无法反序列化
	binding_sheet_id TEXT default '',

    name TEXT default '',
    owner_id TEXT default '',
    sheet_type TEXT default '',
    is_hidden BOOLEAN default FALSE,

    created_at INTEGER default 0,
    updated_at INTEGER  default 0
)

And the column in Go will become like "--坏，Get这个方法太严格了，所有的字段都要有默认值，不然无法反序列化binding_sheet_id TEXT DEFAULT '',".

We used sqlx to execute the creation SQL previously, so when we use gorm-sqlite with AutoMigrate, it deletes the binding_sheet_id column and creates a new one.

Unfortunately, since this table already exists, the creation SQL will not change, and the binding_sheet_id column will be deleted again and again.

[Shion1305]

@PaienNate Hi, thanks for your comment!
I submitted a PR for this issue and it has been merged.
However, no new version was released, so the problem still remains if you tried to use latest one.
Try using the fixed version by this command

go get -u gorm.io/driver/sqlite@02b8e0623276590f0a11475cbdd4fcb883336978

Hope this helps!

[Shion1305]

@jinzhu Could you release a new tag so that everyone can use the fixed version? I strongly suggest this is a critical bug and needs to be patched ASAP.

[PaienNate]

@PaienNate Hi, thanks for your comment! I submitted a PR for this issue and it has been merged. However, no new version was released, so the problem still remains if you tried to use latest one. Try using the fixed version by this command

go get -u gorm.io/driver/sqlite@02b8e0623276590f0a11475cbdd4fcb883336978

Hope this helps!

I have a try and get this result:

2024/11/14 13:34:17 
[0.000ms] [rows:8235] CREATE TABLE `attrs__temp`  (id TEXT PRIMARY KEY,`data` blob,attrs_type TEXT,-- 坏，Get这个方法太严格了，所有的字段都要有默认值，不然无法反序列化
	binding_sheet_id TEXT default '',name TEXT default '',owner_id TEXT default '',sheet_type TEXT default '',is_hidden BOOLEAN default FALSE,created_at INTEGER default 0,updated_at INTEGER  default 0)

2024/11/14 13:34:17 
[99.342ms] [rows:14673] INSERT INTO `attrs__temp`(`id`,`data`,`attrs_type`,`name`,`owner_id`,`sheet_type`,`is_hidden`,`created_at`,`updated_at`) SELECT `id`,`data`,`attrs_type`,`name`,`owner_id`,`sheet_type`,`is_hidden`,`created_at`,`updated_at` FROM `attrs`

2024/11/14 13:34:17 
[12.000ms] [rows:14673] DROP TABLE `attrs`

2024/11/14 13:34:17 
[1.999ms] [rows:14673] ALTER TABLE `attrs__temp` RENAME TO `attrs`

I tried these code and then automigrate:

var createSql = `
CREATE TABLE attrs__temp (
    id TEXT PRIMARY KEY,
    data BYTEA,
    attrs_type TEXT,
	binding_sheet_id TEXT default '',
    name TEXT default '',
    owner_id TEXT default '',
    sheet_type TEXT default '',
    is_hidden BOOLEAN default FALSE,
    created_at INTEGER default 0,
    updated_at INTEGER  default 0
);
	if strings.Contains(dataDB.Dialector.Name(), "sqlite") {
		tx := dataDB.Begin()
		var count int64
		err = dataDB.Raw("SELECT count(*) FROM `sqlite_master` WHERE tbl_name = 'attrs' AND `sql` LIKE '%这个方法太严格了%'").Count(&count).Error
		if err != nil {
			tx.Rollback()
			return nil, nil, err
		}
		if count > 0 {
			err = tx.Exec(createSql).Error
			if err != nil {
				tx.Rollback()
				return nil, nil, err
			}
			err = tx.Exec("INSERT INTO `attrs__temp` SELECT * FROM `attrs`").Error
			if err != nil {
				tx.Rollback()
				return nil, nil, err
			}
			err = tx.Exec("DROP TABLE `attrs`").Error
			if err != nil {
				tx.Rollback()
				return nil, nil, err
			}
			err = tx.Exec("ALTER TABLE `attrs__temp` RENAME TO `attrs`").Error
			if err != nil {
				tx.Rollback()
				return nil, nil, err
			}
			tx.Commit()
		}
	}

Then result become weird:

2024-11-14T13:09:33.302+0800	DEBUG	GORM	sqlite@v1.5.7-0.20240930031831-02b8e0623276/migrator.go:386	[0.000ms] [rows:-] SELECT sql FROM sqlite_master WHERE type = "table" AND tbl_name = "attrs" AND name = "attrs"
2024-11-14T13:09:33.303+0800	DEBUG	GORM	gorm@v1.25.12/finisher_api.go:651	[0.000ms] [rows:8235] CREATE TABLE `attrs__temp`  (id TEXT PRIMARY KEY,`data` blob,attrs_type TEXT,binding_sheet_id TEXT default '',
    name TEXT default '',
    owner_id TEXT default '',
    sheet_type TEXT default '',
    is_hidden BOOLEAN default FALSE,
    created_at INTEGER default 0,
    updated_at INTEGER  default 0)
2024-11-14T13:09:33.409+0800	WARN	GORM	gorm@v1.25.12/finisher_api.go:651	SLOW SQL >= 100ms
[104.318ms] [rows:14673] INSERT INTO `attrs__temp`(`id`,`data`,`attrs_type`,`binding_sheet_id`) SELECT `id`,`data`,`attrs_type`,`binding_sheet_id` FROM `attrs`
2024-11-14T13:09:33.416+0800	DEBUG	GORM	gorm@v1.25.12/finisher_api.go:651	[7.221ms] [rows:14673] DROP TABLE `attrs`
2024-11-14T13:09:33.418+0800	DEBUG	GORM	gorm@v1.25.12/finisher_api.go:651	[2.088ms] [rows:14673] ALTER TABLE `attrs__temp` RENAME TO `attrs`

seems some line with default will be ignore?

[Shion1305]

@PaienNate Thanks for your report! I'll check into this 🧐

[PaienNate]

@PaienNate Thanks for your report! I'll check into this 🧐

It seems there has some issue with using tabs.

[Shion1305]

@jinzhu Hi, could you please make a new release for go-gorm/sqlite? 🙏 🙏 🙏 🙏

[jinzhu]

@jinzhu Hi, could you please make a new release for go-gorm/sqlite? 🙏 🙏 🙏 🙏

just released v1.5.7, thank you for your contribution.