diff --git a/backend/internal/database/captain_skill_migration_test.go b/backend/internal/database/captain_skill_migration_test.go index 806bf04c..b07c534e 100644 --- a/backend/internal/database/captain_skill_migration_test.go +++ b/backend/internal/database/captain_skill_migration_test.go @@ -1,6 +1,8 @@ package database import ( + "fmt" + "net/url" "os" "path/filepath" "testing" @@ -12,69 +14,155 @@ import ( "gorm.io/driver/sqlite" "gorm.io/gorm" "gorm.io/gorm/logger" - - "github.com/gochat/gochat/internal/model" ) -func TestCaptainSkillSchemaSQLiteRoundTrip(t *testing.T) { - db, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory&cache=shared"), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) - require.NoError(t, err) - require.NoError(t, db.AutoMigrate(&model.CaptainAssistant{}, &model.CaptainSkill{}, &model.CaptainSkillReference{}, &model.CaptainAssistantSkill{})) - for _, table := range []string{"captain_skills", "captain_skill_references", "captain_assistant_skills"} { - assert.True(t, db.Migrator().HasTable(table), table) - require.NoError(t, db.Migrator().DropTable(table)) - assert.False(t, db.Migrator().HasTable(table), table) +const captainSkillMigrationBaseline = ` +CREATE TABLE accounts (id INTEGER PRIMARY KEY); +CREATE TABLE captain_assistants (id INTEGER PRIMARY KEY, account_id INTEGER NOT NULL); +CREATE TABLE captain_skills (id INTEGER PRIMARY KEY, account_id INTEGER NOT NULL); +CREATE TABLE captain_assistant_skills ( + id INTEGER PRIMARY KEY, + account_id INTEGER NOT NULL REFERENCES accounts(id) ON DELETE CASCADE, + assistant_id INTEGER NOT NULL REFERENCES captain_assistants(id) ON DELETE CASCADE, + skill_id INTEGER NOT NULL REFERENCES captain_skills(id) ON DELETE CASCADE, + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + UNIQUE (assistant_id, skill_id) +); +CREATE INDEX idx_captain_assistant_skills_account ON captain_assistant_skills(account_id); +CREATE INDEX idx_captain_assistant_skills_skill ON captain_assistant_skills(skill_id); +` + +func captainSkillMigrationDir(t *testing.T, dialect string) string { + t.Helper() + dir := t.TempDir() + for _, direction := range []string{"up", "down"} { + require.NoError(t, os.WriteFile(filepath.Join(dir, "000080_baseline."+direction+".sql"), []byte("SELECT 1;"), 0o600)) } + for _, direction := range []string{"up", "down"} { + name := "000081_enforce_captain_skill_tenant_consistency." + direction + ".sql" + parts := []string{"..", "..", "migrations"} + if dialect != "" { + parts = append(parts, dialect) + } + data, err := os.ReadFile(filepath.Join(append(parts, name)...)) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, name), data, 0o600)) + } + return dir } -func openCaptainMigrationPostgres(t *testing.T) *gorm.DB { - t.Helper() - dsn := os.Getenv("GOCHAT_TEST_DB_URL") - if dsn == "" { - dsn = "host=localhost port=5432 user=postgres password=postgres dbname=gochat_test sslmode=disable" - } - db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) +func TestCaptainSkillSQLiteMigrationRoundTrip(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "captain-skill.db") + dbURL := "sqlite3://" + dbPath + "?_foreign_keys=on" + db, err := gorm.Open(sqlite.Open(dbPath+"?_foreign_keys=on"), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) require.NoError(t, err) sqlDB, err := db.DB() require.NoError(t, err) sqlDB.SetMaxOpenConns(1) t.Cleanup(func() { _ = sqlDB.Close() }) - schema := "captain_skill_migration_" + time.Now().Format("20060102150405000000000") - require.NoError(t, db.Exec("CREATE SCHEMA "+schema).Error) - t.Cleanup(func() { _ = db.Exec("DROP SCHEMA " + schema + " CASCADE").Error }) - require.NoError(t, db.Exec("SET search_path TO "+schema).Error) - return db + require.NoError(t, db.Exec(captainSkillMigrationBaseline).Error) + require.NoError(t, db.Exec("INSERT INTO accounts VALUES (1), (2); INSERT INTO captain_assistants VALUES (1, 1), (2, 2); INSERT INTO captain_skills VALUES (1, 1), (2, 2)").Error) + + migrations := captainSkillMigrationDir(t, "sqlite") + require.NoError(t, ForceVersion(dbURL, migrations, 80)) + require.NoError(t, MigrateSteps(dbURL, migrations, 1)) + version, dirty, err := CurrentVersion(dbURL, migrations) + require.NoError(t, err) + assert.Equal(t, uint(81), version) + assert.False(t, dirty) + + require.NoError(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (1, 1, 1, 1)").Error) + assert.Error(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (2, 1, 2, 1)").Error) + assert.Error(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (3, 1, 1, 2)").Error) + + require.NoError(t, MigrateSteps(dbURL, migrations, -1)) + version, dirty, err = CurrentVersion(dbURL, migrations) + require.NoError(t, err) + assert.Equal(t, uint(80), version) + assert.False(t, dirty) + require.NoError(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (2, 1, 2, 1), (3, 1, 1, 2)").Error) + assert.False(t, db.Migrator().HasIndex("captain_assistants", "idx_captain_assistants_account_id_id")) + assert.False(t, db.Migrator().HasIndex("captain_skills", "idx_captain_skills_account_id_id")) +} + +func openCaptainMigrationPostgres(t *testing.T) (*gorm.DB, string) { + t.Helper() + dsn := os.Getenv("GOCHAT_TEST_DB_URL") + if dsn == "" { + dsn = "postgres://postgres:postgres@localhost:5432/gochat_test?sslmode=disable" + } + admin, err := gorm.Open(postgres.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + adminDB, err := admin.DB() + require.NoError(t, err) + t.Cleanup(func() { _ = adminDB.Close() }) + + schema := fmt.Sprintf("captain_skill_migration_%d", time.Now().UnixNano()) + require.NoError(t, admin.Exec("CREATE SCHEMA "+schema).Error) + t.Cleanup(func() { _ = admin.Exec("DROP SCHEMA " + schema + " CASCADE").Error }) + migrationURL, err := url.Parse(dsn) + require.NoError(t, err) + require.NotEmpty(t, migrationURL.Scheme, "GOCHAT_TEST_DB_URL must be a PostgreSQL URL") + query := migrationURL.Query() + query.Set("search_path", schema) + migrationURL.RawQuery = query.Encode() + + db, err := gorm.Open(postgres.Open(migrationURL.String()), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) + require.NoError(t, err) + sqlDB, err := db.DB() + require.NoError(t, err) + sqlDB.SetMaxOpenConns(1) + t.Cleanup(func() { _ = sqlDB.Close() }) + require.NoError(t, db.Exec(captainSkillMigrationBaseline).Error) + return db, migrationURL.String() +} + +func runCaptainSkillMigration(t *testing.T, fn func() error) { + t.Helper() + require.NoError(t, withSanitizedPQEnvironment(fn)) } func TestCaptainSkillPostgresMigrationRoundTrip(t *testing.T) { if os.Getenv("GOCHAT_TEST_DB") == "sqlite" { t.Skip("PostgreSQL migration test") } - db := openCaptainMigrationPostgres(t) - require.NoError(t, db.Exec("CREATE TABLE accounts (id SERIAL PRIMARY KEY); CREATE TABLE captain_assistants (id SERIAL PRIMARY KEY, account_id INTEGER NOT NULL);").Error) + db, dbURL := openCaptainMigrationPostgres(t) + require.NoError(t, db.Exec("INSERT INTO accounts VALUES (1), (2); INSERT INTO captain_assistants VALUES (1, 1), (2, 2); INSERT INTO captain_skills VALUES (1, 1), (2, 2)").Error) + migrations := captainSkillMigrationDir(t, "") + runCaptainSkillMigration(t, func() error { return ForceVersion(dbURL, migrations, 80) }) + runCaptainSkillMigration(t, func() error { return MigrateSteps(dbURL, migrations, 1) }) - up, err := os.ReadFile(filepath.Join("..", "..", "migrations", "000080_add_captain_skills.up.sql")) - require.NoError(t, err) - require.NoError(t, db.Exec(string(up)).Error) - for _, table := range []string{"captain_skills", "captain_skill_references", "captain_assistant_skills"} { - assert.True(t, db.Migrator().HasTable(table), table) - } - require.NoError(t, db.Exec("INSERT INTO accounts(id) VALUES (1); INSERT INTO captain_assistants(id, account_id) VALUES (1, 1);").Error) - require.NoError(t, db.Exec("INSERT INTO captain_skills(id, account_id, name, description, instructions_md, status) VALUES (1, 1, 'policy', 'Policy', 'Use it', 'active')").Error) - require.NoError(t, db.Exec("INSERT INTO captain_skill_references(skill_id, reference_key, content_md, position) VALUES (1, 'standard', 'Terms', 0)").Error) - require.NoError(t, db.Exec("INSERT INTO captain_assistant_skills(account_id, assistant_id, skill_id) VALUES (1, 1, 1)").Error) - assert.Error(t, db.Exec("INSERT INTO captain_skill_references(skill_id, reference_key, content_md, position) VALUES (1, 'standard', 'Duplicate', 1)").Error) - require.NoError(t, db.Exec("DELETE FROM captain_skills WHERE id = 1").Error) - for _, table := range []string{"captain_skill_references", "captain_assistant_skills"} { - var count int64 - require.NoError(t, db.Table(table).Count(&count).Error) - assert.Zero(t, count, table) - } + require.NoError(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (1, 1, 1, 1)").Error) + assert.Error(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (2, 1, 2, 1)").Error) + assert.Error(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (3, 1, 1, 2)").Error) - down, err := os.ReadFile(filepath.Join("..", "..", "migrations", "000080_add_captain_skills.down.sql")) - require.NoError(t, err) - require.NoError(t, db.Exec(string(down)).Error) - for _, table := range []string{"captain_skills", "captain_skill_references", "captain_assistant_skills"} { - assert.False(t, db.Migrator().HasTable(table), table) + runCaptainSkillMigration(t, func() error { return MigrateSteps(dbURL, migrations, -1) }) + require.NoError(t, db.Exec("INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (2, 1, 2, 1), (3, 1, 1, 2)").Error) + var count int64 + require.NoError(t, db.Raw("SELECT count(*) FROM pg_indexes WHERE schemaname = current_schema() AND indexname IN ('idx_captain_assistants_account_id_id', 'idx_captain_skills_account_id_id')").Scan(&count).Error) + assert.Zero(t, count) +} + +func TestCaptainSkillPostgresMigrationRejectsDirtyBindingsAtomically(t *testing.T) { + if os.Getenv("GOCHAT_TEST_DB") == "sqlite" { + t.Skip("PostgreSQL migration test") } + db, dbURL := openCaptainMigrationPostgres(t) + require.NoError(t, db.Exec("INSERT INTO accounts VALUES (1), (2); INSERT INTO captain_assistants VALUES (1, 1), (2, 2); INSERT INTO captain_skills VALUES (1, 1), (2, 2); INSERT INTO captain_assistant_skills(id, account_id, assistant_id, skill_id) VALUES (1, 1, 2, 1)").Error) + migrations := captainSkillMigrationDir(t, "") + runCaptainSkillMigration(t, func() error { return ForceVersion(dbURL, migrations, 80) }) + err := withSanitizedPQEnvironment(func() error { return MigrateSteps(dbURL, migrations, 1) }) + require.Error(t, err) + + var count int64 + require.NoError(t, db.Table("captain_assistant_skills").Count(&count).Error) + assert.Equal(t, int64(1), count) + require.NoError(t, db.Raw("SELECT count(*) FROM pg_indexes WHERE schemaname = current_schema() AND indexname IN ('idx_captain_assistants_account_id_id', 'idx_captain_skills_account_id_id')").Scan(&count).Error) + assert.Zero(t, count) + require.NoError(t, db.Raw("SELECT count(*) FROM pg_constraint WHERE connamespace = current_schema()::regnamespace AND conname IN ('fk_captain_assistant_skills_assistant_tenant', 'fk_captain_assistant_skills_skill_tenant')").Scan(&count).Error) + assert.Zero(t, count) + version, dirty, versionErr := CurrentVersion(dbURL, migrations) + require.NoError(t, versionErr) + assert.Equal(t, uint(81), version) + assert.True(t, dirty) } diff --git a/backend/migrations/000081_enforce_captain_skill_tenant_consistency.down.sql b/backend/migrations/000081_enforce_captain_skill_tenant_consistency.down.sql new file mode 100644 index 00000000..fb328d93 --- /dev/null +++ b/backend/migrations/000081_enforce_captain_skill_tenant_consistency.down.sql @@ -0,0 +1,6 @@ +ALTER TABLE captain_assistant_skills + DROP CONSTRAINT IF EXISTS fk_captain_assistant_skills_assistant_tenant, + DROP CONSTRAINT IF EXISTS fk_captain_assistant_skills_skill_tenant; + +DROP INDEX IF EXISTS idx_captain_assistants_account_id_id; +DROP INDEX IF EXISTS idx_captain_skills_account_id_id; diff --git a/backend/migrations/000081_enforce_captain_skill_tenant_consistency.up.sql b/backend/migrations/000081_enforce_captain_skill_tenant_consistency.up.sql new file mode 100644 index 00000000..0bcef8d6 --- /dev/null +++ b/backend/migrations/000081_enforce_captain_skill_tenant_consistency.up.sql @@ -0,0 +1,12 @@ +CREATE UNIQUE INDEX idx_captain_assistants_account_id_id + ON captain_assistants(account_id, id); +CREATE UNIQUE INDEX idx_captain_skills_account_id_id + ON captain_skills(account_id, id); + +ALTER TABLE captain_assistant_skills + ADD CONSTRAINT fk_captain_assistant_skills_assistant_tenant + FOREIGN KEY (account_id, assistant_id) + REFERENCES captain_assistants(account_id, id) ON DELETE CASCADE, + ADD CONSTRAINT fk_captain_assistant_skills_skill_tenant + FOREIGN KEY (account_id, skill_id) + REFERENCES captain_skills(account_id, id) ON DELETE CASCADE; diff --git a/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.down.sql b/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.down.sql new file mode 100644 index 00000000..d5d22e75 --- /dev/null +++ b/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.down.sql @@ -0,0 +1,21 @@ +CREATE TABLE captain_assistant_skills_000081 ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + account_id INTEGER NOT NULL REFERENCES accounts(id) ON DELETE CASCADE, + assistant_id INTEGER NOT NULL REFERENCES captain_assistants(id) ON DELETE CASCADE, + skill_id INTEGER NOT NULL REFERENCES captain_skills(id) ON DELETE CASCADE, + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + UNIQUE (assistant_id, skill_id) +); + +INSERT INTO captain_assistant_skills_000081 + (id, account_id, assistant_id, skill_id, created_at) +SELECT id, account_id, assistant_id, skill_id, created_at +FROM captain_assistant_skills; + +DROP TABLE captain_assistant_skills; +ALTER TABLE captain_assistant_skills_000081 RENAME TO captain_assistant_skills; +CREATE INDEX idx_captain_assistant_skills_account ON captain_assistant_skills(account_id); +CREATE INDEX idx_captain_assistant_skills_skill ON captain_assistant_skills(skill_id); + +DROP INDEX idx_captain_assistants_account_id_id; +DROP INDEX idx_captain_skills_account_id_id; diff --git a/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.up.sql b/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.up.sql new file mode 100644 index 00000000..7a44152b --- /dev/null +++ b/backend/migrations/sqlite/000081_enforce_captain_skill_tenant_consistency.up.sql @@ -0,0 +1,29 @@ +CREATE UNIQUE INDEX idx_captain_assistants_account_id_id + ON captain_assistants(account_id, id); +CREATE UNIQUE INDEX idx_captain_skills_account_id_id + ON captain_skills(account_id, id); + +CREATE TABLE captain_assistant_skills_000081 ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + account_id INTEGER NOT NULL REFERENCES accounts(id) ON DELETE CASCADE, + assistant_id INTEGER NOT NULL, + skill_id INTEGER NOT NULL, + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + UNIQUE (assistant_id, skill_id), + CONSTRAINT fk_captain_assistant_skills_assistant_tenant + FOREIGN KEY (account_id, assistant_id) + REFERENCES captain_assistants(account_id, id) ON DELETE CASCADE, + CONSTRAINT fk_captain_assistant_skills_skill_tenant + FOREIGN KEY (account_id, skill_id) + REFERENCES captain_skills(account_id, id) ON DELETE CASCADE +); + +INSERT INTO captain_assistant_skills_000081 + (id, account_id, assistant_id, skill_id, created_at) +SELECT id, account_id, assistant_id, skill_id, created_at +FROM captain_assistant_skills; + +DROP TABLE captain_assistant_skills; +ALTER TABLE captain_assistant_skills_000081 RENAME TO captain_assistant_skills; +CREATE INDEX idx_captain_assistant_skills_account ON captain_assistant_skills(account_id); +CREATE INDEX idx_captain_assistant_skills_skill ON captain_assistant_skills(skill_id);