From d43d7a46dd822c20ff5226924c51545d92a81a25 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Wed, 9 Sep 2026 13:49:20 +0800 Subject: [PATCH] =?UTF-8?q?fix=F0=9F=90=9B:=20make=20the=20seed=20natural-?= =?UTF-8?q?key=20indexes=20buildable=20on=20SQL=20Server?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1786700008000 could not be applied to any SQL Server database. Not an old one with awkward data - any of them, including an empty one: Msg 1505 ... duplicate key ... The duplicate key value is (, , 0). MySQL, PostgreSQL and SQLite treat two NULLs in a unique index as different values, so any number of rows missing a seed_code coexist under uk_sys_menu_app_seed_code_del. SQL Server treats them as equal and permits exactly one. 1786700001000 seeds five menus and none of them has a seed_code, so the second one already collides with the first. sys_api's index has the same shape over two nullable columns, path and action. On SQL Server the index is now filtered to the rows that carry a value, which is what the other three engines do by not comparing their NULLs. The filter is not added elsewhere: MySQL has no filtered index at all, and on PostgreSQL and SQLite it would only restate what those engines already do. Nothing that has applied this migration is affected, and no SQL Server database can have. Verified against SQL Server 2022. The migration completes; the filtered index still rejects a second (order, dir) and still lets another app reuse "dir", so filtering removed the NULL rows from the index rather than the index's teeth. Two degradations turn that red: dropping the filter, and naming only path in sys_api's - the second one needed a fixture row with a path and no action, because rows missing both are excluded either way and the first attempt at that degradation came out green. There is also a control test asserting the unfiltered statement still fails on this engine, so the first test is passing because of the fix rather than because SQL Server turned out not to mind. --- .../1786700008000_seed_natural_keys.go | 46 +++++- ...008000_seed_natural_keys_sqlserver_test.go | 148 ++++++++++++++++++ .../1786700008000_unique_index_test.go | 51 ++++++ 3 files changed, 241 insertions(+), 4 deletions(-) create mode 100644 cmd/migrate/migration/version/1786700008000_seed_natural_keys_sqlserver_test.go create mode 100644 cmd/migrate/migration/version/1786700008000_unique_index_test.go diff --git a/cmd/migrate/migration/version/1786700008000_seed_natural_keys.go b/cmd/migrate/migration/version/1786700008000_seed_natural_keys.go index b07dbe14..3ff1b0bf 100644 --- a/cmd/migrate/migration/version/1786700008000_seed_natural_keys.go +++ b/cmd/migrate/migration/version/1786700008000_seed_natural_keys.go @@ -3,6 +3,7 @@ package version import ( "fmt" "runtime" + "strings" "gorm.io/gorm" @@ -47,8 +48,9 @@ func seedNaturalKeys(db *gorm.DB) error { } } if !m.HasIndex(&adminmodels.SysMenu{}, "uk_sys_menu_app_seed_code_del") { - if err := db.Exec( - "CREATE UNIQUE INDEX uk_sys_menu_app_seed_code_del ON sys_menu (app_code, seed_code, deleted_at)", + if err := db.Exec(uniqueIndexOverNullable(db.Dialector.Name(), + "uk_sys_menu_app_seed_code_del", "sys_menu", + "app_code, seed_code, deleted_at", "seed_code"), ).Error; err != nil { return err } @@ -63,8 +65,9 @@ func seedNaturalKeys(db *gorm.DB) error { return err } if !m.HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") { - if err := db.Exec( - "CREATE UNIQUE INDEX uk_sys_api_app_path_action_del ON sys_api (app_code, path, action, deleted_at)", + if err := db.Exec(uniqueIndexOverNullable(db.Dialector.Name(), + "uk_sys_api_app_path_action_del", "sys_api", + "app_code, path, action, deleted_at", "path", "action"), ).Error; err != nil { return err } @@ -73,6 +76,41 @@ func seedNaturalKeys(db *gorm.DB) error { return nil } +// uniqueIndexOverNullable builds a CREATE UNIQUE INDEX whose key includes +// columns that can be NULL, and makes it mean the same thing on all four +// drivers this repository registers. +// +// Three of them treat two NULLs as different values, so any number of rows +// missing one of these columns coexist under the index. SQL Server does not: +// its unique index treats NULLs as equal and permits exactly one. The +// unfiltered statement therefore fails there on any database with two rows +// lacking a seed_code - which is every database, including a brand-new one, +// because 1786700001000 seeds five menus and none of them has one: +// +// Msg 1505 ... duplicate key ... The duplicate key value is (, , 0). +// +// Adding the filter on SQL Server takes the rows that carry no value out of +// the index, which is what the other three do by not comparing their NULLs. +// It is not added elsewhere: MySQL has no filtered index at all, and on +// PostgreSQL and SQLite it would only restate what those engines already do. +// +// Only databases that have not applied this migration are affected, and no +// SQL Server database can have: it could not get past this statement. +// +// Takes the dialect by name rather than the connection, so the statement it +// builds for every driver can be checked without one of each running. +func uniqueIndexOverNullable(dialect, name, table, columns string, nullable ...string) string { + stmt := fmt.Sprintf("CREATE UNIQUE INDEX %s ON %s (%s)", name, table, columns) + if dialect != "sqlserver" || len(nullable) == 0 { + return stmt + } + preds := make([]string, 0, len(nullable)) + for _, c := range nullable { + preds = append(preds, c+" IS NOT NULL") + } + return stmt + " WHERE " + strings.Join(preds, " AND ") +} + // refuseOnDuplicateApis reports the (app_code, path, action) values that // would make the unique index impossible, rather than the index failing to // build and saying only that it did. Only live rows count: a soft-deleted diff --git a/cmd/migrate/migration/version/1786700008000_seed_natural_keys_sqlserver_test.go b/cmd/migrate/migration/version/1786700008000_seed_natural_keys_sqlserver_test.go new file mode 100644 index 00000000..a8f58cf5 --- /dev/null +++ b/cmd/migrate/migration/version/1786700008000_seed_natural_keys_sqlserver_test.go @@ -0,0 +1,148 @@ +package version + +import ( + "os" + "testing" + + "gorm.io/driver/sqlserver" + "gorm.io/gorm" + + adminmodels "go-admin/app/admin/models" +) + +// sqlserverDSNEnv points these tests at a database. They skip without it, so +// a developer with no SQL Server running still gets a green run. +// +// This file exists for the same reason the PostgreSQL one does, one driver +// further along. The rest of the package runs on SQLite, where the defect it +// covers cannot happen: SQLite, MySQL and PostgreSQL all treat two NULLs in a +// unique index as different values, and SQL Server treats them as equal and +// permits one. A suite that never pointed at SQL Server reported success for +// a migration that could not be applied to any SQL Server database at all, +// new or old. +const sqlserverDSNEnv = "GO_ADMIN_TEST_SQLSERVER_DSN" + +func sqlserverDB(t *testing.T) *gorm.DB { + t.Helper() + + dsn := os.Getenv(sqlserverDSNEnv) + if dsn == "" { + // Skipping locally is the point; skipping in CI is the failure this + // file exists to prevent. + if os.Getenv("CI") != "" { + t.Fatalf("%s is not set while CI is: the SQL Server migration tests must not skip here", sqlserverDSNEnv) + } + t.Skipf("%s is not set; skipping the SQL Server migration tests", sqlserverDSNEnv) + } + + db, err := gorm.Open(sqlserver.Open(dsn), &gorm.Config{}) + if err != nil { + t.Fatalf("connecting to %s: %v", sqlserverDSNEnv, err) + } + return db +} + +// freshSQLServerTables drops and rebuilds the two tables this migration +// touches, so a rerun does not inherit the previous run's index. +func freshSQLServerTables(t *testing.T, db *gorm.DB) { + t.Helper() + for _, m := range []any{&adminmodels.SysMenu{}, &adminmodels.SysApi{}} { + if db.Migrator().HasTable(m) { + if err := db.Migrator().DropTable(m); err != nil { + t.Fatalf("dropping: %v", err) + } + } + } + if err := db.AutoMigrate(&adminmodels.SysMenu{}, &adminmodels.SysApi{}); err != nil { + t.Fatalf("automigrate: %v", err) + } +} + +// The migration completes on SQL Server. +// +// It did not. Five menus with no seed_code is what 1786700001000 leaves on +// every database, and the unfiltered index rejects the second of them: +// +// Msg 1505 ... duplicate key ... The duplicate key value is (, , 0). +func TestSeedNaturalKeysOnSQLServer(t *testing.T) { + db := sqlserverDB(t) + freshSQLServerTables(t, db) + + // Three rows in the state 1786700006000 leaves behind: an app_code that + // defaulted to empty, no seed_code, and live. + for _, name := range []string{"one", "two", "three"} { + if err := db.Exec( + "INSERT INTO sys_menu (menu_name, app_code, deleted_at) VALUES (?, '', 0)", name, + ).Error; err != nil { + t.Fatalf("seeding %s: %v", name, err) + } + } + // sys_api's key has two nullable columns and either one is enough to + // collide, so both shapes are here. Two rows missing both, and two more + // that have a path and no action: a filter naming only path would let + // that second pair back into the index, where their equal NULLs collide. + for i := 0; i < 2; i++ { + if err := db.Exec("INSERT INTO sys_api (app_code, deleted_at) VALUES ('', 0)").Error; err != nil { + t.Fatalf("seeding sys_api: %v", err) + } + if err := db.Exec( + "INSERT INTO sys_api (app_code, path, deleted_at) VALUES ('', '/api/v1/half', 0)", + ).Error; err != nil { + t.Fatalf("seeding a sys_api row with no action: %v", err) + } + } + + if err := seedNaturalKeys(db); err != nil { + t.Fatalf("seedNaturalKeys on SQL Server: %v", err) + } + for _, name := range []string{"uk_sys_menu_app_seed_code_del", "uk_sys_api_app_path_action_del"} { + var model any = &adminmodels.SysMenu{} + if name == "uk_sys_api_app_path_action_del" { + model = &adminmodels.SysApi{} + } + if !db.Migrator().HasIndex(model, name) { + t.Errorf("%s was not created", name) + } + } + + // Rows that do carry a seed code still cannot collide - the filter takes + // the ones with no value out of the index, it does not turn the index off. + code := "dir" + first := adminmodels.SysMenu{MenuName: "d1", AppCode: "order", SeedCode: &code} + if err := db.Create(&first).Error; err != nil { + t.Fatalf("first seeded menu: %v", err) + } + second := adminmodels.SysMenu{MenuName: "d2", AppCode: "order", SeedCode: &code} + if err := db.Create(&second).Error; err == nil { + t.Error("a duplicate (app_code, seed_code) was accepted; the filtered index is not enforcing anything") + } + // A different app may reuse the same seed code, which is why the key is + // composite in the first place. + other := adminmodels.SysMenu{MenuName: "d3", AppCode: "crm", SeedCode: &code} + if err := db.Create(&other).Error; err != nil { + t.Errorf("another app could not reuse the seed code: %v", err) + } +} + +// The control. Without the filter the statement fails on this engine, so the +// test above is passing because of the fix rather than because SQL Server +// turned out not to mind. +func TestSQLServerRejectsTheUnfilteredIndex(t *testing.T) { + db := sqlserverDB(t) + freshSQLServerTables(t, db) + + for _, name := range []string{"one", "two"} { + if err := db.Exec( + "INSERT INTO sys_menu (menu_name, app_code, deleted_at) VALUES (?, '', 0)", name, + ).Error; err != nil { + t.Fatalf("seeding %s: %v", name, err) + } + } + + err := db.Exec(uniqueIndexOverNullable("postgres", + "uk_unfiltered_probe", "sys_menu", "app_code, seed_code, deleted_at", "seed_code")).Error + if err == nil { + t.Fatal("SQL Server accepted two NULLs in a unique index; the filter this migration adds is not needed") + } + t.Logf("as expected: %v", err) +} diff --git a/cmd/migrate/migration/version/1786700008000_unique_index_test.go b/cmd/migrate/migration/version/1786700008000_unique_index_test.go new file mode 100644 index 00000000..e03d58d9 --- /dev/null +++ b/cmd/migrate/migration/version/1786700008000_unique_index_test.go @@ -0,0 +1,51 @@ +package version + +import ( + "strings" + "testing" +) + +// The index has to mean the same thing on every driver this repository +// registers, and the drivers do not agree about NULL. +// +// MySQL, PostgreSQL and SQLite treat two NULLs as different values, so any +// number of rows missing one of these columns coexist under the index. SQL +// Server treats them as equal and permits exactly one, so the unfiltered +// statement fails there on any database with two rows lacking a seed_code - +// which is every database, a brand-new one included, because 1786700001000 +// seeds five menus and none of them carries one. +func TestUniqueIndexOverNullableFiltersOnlyWhereItHasTo(t *testing.T) { + const plain = "CREATE UNIQUE INDEX uk ON sys_menu (app_code, seed_code, deleted_at)" + + for _, dialect := range []string{"mysql", "postgres", "sqlite"} { + got := uniqueIndexOverNullable(dialect, "uk", "sys_menu", "app_code, seed_code, deleted_at", "seed_code") + if got != plain { + t.Errorf("%s: %q\n want %q", dialect, got, plain) + } + } + + got := uniqueIndexOverNullable("sqlserver", "uk", "sys_menu", "app_code, seed_code, deleted_at", "seed_code") + want := plain + " WHERE seed_code IS NOT NULL" + if got != want { + t.Errorf("sqlserver: %q\n want %q", got, want) + } +} + +// sys_api's key has two nullable columns, and either one being NULL is enough +// to collide on SQL Server. +func TestUniqueIndexOverNullableCoversEveryNullableColumn(t *testing.T) { + got := uniqueIndexOverNullable("sqlserver", "uk", "sys_api", + "app_code, path, action, deleted_at", "path", "action") + if !strings.HasSuffix(got, " WHERE path IS NOT NULL AND action IS NOT NULL") { + t.Errorf("got %q", got) + } +} + +// A key with nothing nullable in it needs no filter anywhere, or SQL Server +// would get a WHERE clause naming no column. +func TestUniqueIndexOverNullableWithoutNullableColumns(t *testing.T) { + got := uniqueIndexOverNullable("sqlserver", "uk", "sys_menu", "app_code, deleted_at") + if strings.Contains(got, "WHERE") { + t.Errorf("got %q", got) + } +}