store: fix PRAGMA foreign_keys scoping for table-rebuild migrations
The previous implementation issued PRAGMA foreign_keys toggles inside transactions (after tx.Begin()), but SQLite ignores pragmas once a transaction is open with modernc.org/sqlite, making those calls no-ops. This fix relocates the toggle to autocommit mode (db.Exec, not tx.Exec) and limits it to only the three migrations that need it (0023/0024/0025), which rebuild tables with incoming foreign keys (kind_assignments/ workout_type_paces reference workout_kinds; activities/sync_runs reference sync_state). The connection string now correctly restores ?_pragma=foreign_keys(1), ensuring FK enforcement is ON by default for normal runtime operation and during migrations that don't need the special handling. Each affected migration now: 1. Disables FK in autocommit mode before starting its transaction 2. Runs the migration's CREATE/INSERT/DROP/RENAME sequence 3. Re-enables FK in autocommit mode after the transaction commits All other migrations run normally with FK enforcement active throughout, protecting against cascading deletions silently failing if a future connection change causes FK enforcement to inadvertently remain off. Added TestForeignKeyEnforcementPostMigration to verify FK is correctly enforced after all migrations complete: valid FK references are accepted, and invalid ones are rejected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -24,10 +24,7 @@ type DB struct {
|
||||
// Open opens (creating if needed) the SQLite database at path and applies
|
||||
// any migrations that haven't run yet.
|
||||
func Open(path string) (*DB, error) {
|
||||
// Don't enable foreign keys in connection string; instead, enable them
|
||||
// after migrations complete. This allows migrations to disable FK checks
|
||||
// as needed for table rebuilds.
|
||||
sqlDB, err := sql.Open("sqlite", path)
|
||||
sqlDB, err := sql.Open("sqlite", path+"?_pragma=foreign_keys(1)")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open sqlite database: %w", err)
|
||||
}
|
||||
@@ -81,38 +78,62 @@ func (db *DB) migrate() error {
|
||||
if err != nil {
|
||||
return fmt.Errorf("read migration %s: %w", name, err)
|
||||
}
|
||||
|
||||
// Migrations 0023, 0024, and 0025 rebuild tables that have incoming
|
||||
// foreign keys (kind_assignments/workout_type_paces reference
|
||||
// workout_kinds; sync_state is referenced by activities/sync_runs).
|
||||
// These require temporary FK disable in autocommit mode (before the
|
||||
// transaction begins) so the DROP TABLE succeeds; a mid-transaction
|
||||
// PRAGMA is a no-op with modernc.org/sqlite.
|
||||
tableRebuildMigrations := map[string]bool{
|
||||
"0023_profile_user_scoped.sql": true,
|
||||
"0024_workout_kinds_user_scoped.sql": true,
|
||||
"0025_sync_state_user_scoped.sql": true,
|
||||
}
|
||||
needsFKToggle := tableRebuildMigrations[name]
|
||||
|
||||
if needsFKToggle {
|
||||
if _, err := db.Exec(`PRAGMA foreign_keys = OFF`); err != nil {
|
||||
return fmt.Errorf("disable foreign keys before migration %s: %w", name, err)
|
||||
}
|
||||
}
|
||||
|
||||
tx, err := db.Begin()
|
||||
if err != nil {
|
||||
if needsFKToggle {
|
||||
db.Exec(`PRAGMA foreign_keys = ON`)
|
||||
}
|
||||
return fmt.Errorf("begin migration tx for %s: %w", name, err)
|
||||
}
|
||||
// Disable foreign key enforcement for the migration so that table
|
||||
// rebuilds (e.g. dropping a table with dependent foreign keys) can
|
||||
// succeed. The migrations themselves use PRAGMA defer_foreign_keys
|
||||
// when possible; this is a fallback for modernc.org/sqlite's
|
||||
// limitations with that pragma in transactions.
|
||||
if _, err := tx.Exec(`PRAGMA foreign_keys = OFF`); err != nil {
|
||||
tx.Rollback()
|
||||
return fmt.Errorf("disable foreign keys for migration %s: %w", name, err)
|
||||
}
|
||||
|
||||
if _, err := tx.Exec(string(content)); err != nil {
|
||||
tx.Rollback()
|
||||
if needsFKToggle {
|
||||
db.Exec(`PRAGMA foreign_keys = ON`)
|
||||
}
|
||||
return fmt.Errorf("apply migration %s: %w", name, err)
|
||||
}
|
||||
if _, err := tx.Exec(`PRAGMA foreign_keys = ON`); err != nil {
|
||||
tx.Rollback()
|
||||
return fmt.Errorf("enable foreign keys after migration %s: %w", name, err)
|
||||
}
|
||||
|
||||
if _, err := tx.Exec(`INSERT INTO schema_migrations (filename) VALUES (?)`, name); err != nil {
|
||||
tx.Rollback()
|
||||
if needsFKToggle {
|
||||
db.Exec(`PRAGMA foreign_keys = ON`)
|
||||
}
|
||||
return fmt.Errorf("record migration %s: %w", name, err)
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
if needsFKToggle {
|
||||
db.Exec(`PRAGMA foreign_keys = ON`)
|
||||
}
|
||||
return fmt.Errorf("commit migration %s: %w", name, err)
|
||||
}
|
||||
}
|
||||
// Enable foreign key enforcement after all migrations are complete.
|
||||
if _, err := db.Exec(`PRAGMA foreign_keys = ON`); err != nil {
|
||||
return fmt.Errorf("enable foreign keys after migrations: %w", err)
|
||||
|
||||
if needsFKToggle {
|
||||
if _, err := db.Exec(`PRAGMA foreign_keys = ON`); err != nil {
|
||||
return fmt.Errorf("enable foreign keys after migration %s: %w", name, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -3,6 +3,7 @@ package store
|
||||
import (
|
||||
"context"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -231,3 +232,71 @@ func TestUsersAndOwnershipSchema_AppliesCleanly(t *testing.T) {
|
||||
t.Fatal("expected a second profile row for the same user_id to violate UNIQUE(user_id)")
|
||||
}
|
||||
}
|
||||
|
||||
func TestForeignKeyEnforcementPostMigration(t *testing.T) {
|
||||
db := openTestDB(t)
|
||||
ctx := context.Background()
|
||||
|
||||
// Verify that FK enforcement is correctly active after migrations complete.
|
||||
// This tests that the PRAGMA foreign_keys toggle in db.migrate() (for
|
||||
// migrations 0023/0024/0025) is properly scoped and re-enabled after each
|
||||
// table rebuild.
|
||||
|
||||
// Insert an activity that we can reference.
|
||||
activityID, err := db.UpsertActivity(ctx, Activity{
|
||||
GarminActivityID: 1001,
|
||||
StartTimeUTC: "2026-07-11 05:00:00",
|
||||
RawJSON: "{}",
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("UpsertActivity: %v", err)
|
||||
}
|
||||
|
||||
// Get the ID of one of the 8 migration-seeded workout_kinds.
|
||||
var seedKindID int64
|
||||
if err := db.QueryRowContext(ctx, `SELECT id FROM workout_kinds LIMIT 1`).Scan(&seedKindID); err != nil {
|
||||
t.Fatalf("query seeded workout_kind: %v", err)
|
||||
}
|
||||
|
||||
// Inserting a kind_assignment with a valid FK reference should succeed.
|
||||
assignmentID, err := db.InsertKindAssignment(ctx, KindAssignment{
|
||||
ActivityID: activityID,
|
||||
WorkoutKindID: &seedKindID,
|
||||
AssignmentSource: AssignmentSourceManual,
|
||||
Status: AssignmentStatusAssigned,
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("InsertKindAssignment with valid FK: %v", err)
|
||||
}
|
||||
if assignmentID == 0 {
|
||||
t.Fatal("expected non-zero assignment ID")
|
||||
}
|
||||
|
||||
// Inserting a kind_assignment with a nonexistent workout_kind_id should
|
||||
// be rejected by the FK constraint, proving FK enforcement is ON.
|
||||
nonexistentKindID := int64(999999)
|
||||
_, err = db.InsertKindAssignment(ctx, KindAssignment{
|
||||
ActivityID: activityID,
|
||||
WorkoutKindID: &nonexistentKindID,
|
||||
AssignmentSource: AssignmentSourceManual,
|
||||
Status: AssignmentStatusAssigned,
|
||||
})
|
||||
if err == nil {
|
||||
t.Fatal("expected FK constraint violation for nonexistent workout_kind_id, but insert succeeded")
|
||||
}
|
||||
// The error message should mention FOREIGN KEY or similar constraint issue.
|
||||
if !contains(err.Error(), "FOREIGN KEY", "constraint", "UNIQUE") {
|
||||
t.Logf("FK error message: %v", err)
|
||||
// Still pass, but log it; the exact error text varies by driver/context.
|
||||
}
|
||||
}
|
||||
|
||||
func contains(s string, substrs ...string) bool {
|
||||
lower := strings.ToLower(s)
|
||||
for _, substr := range substrs {
|
||||
if strings.Contains(lower, strings.ToLower(substr)) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user