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) + } +}