diff --git a/backend/internal/store/db.go b/backend/internal/store/db.go index 5083b19..406a276 100644 --- a/backend/internal/store/db.go +++ b/backend/internal/store/db.go @@ -24,7 +24,10 @@ 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) { - sqlDB, err := sql.Open("sqlite", path+"?_pragma=foreign_keys(1)") + // 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) if err != nil { return nil, fmt.Errorf("open sqlite database: %w", err) } @@ -82,10 +85,23 @@ func (db *DB) migrate() error { if err != nil { 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() 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() return fmt.Errorf("record migration %s: %w", name, err) @@ -94,5 +110,9 @@ func (db *DB) migrate() error { 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) + } return nil } diff --git a/backend/internal/store/migrations/0021_users_table.sql b/backend/internal/store/migrations/0021_users_table.sql new file mode 100644 index 0000000..47c00ee --- /dev/null +++ b/backend/internal/store/migrations/0021_users_table.sql @@ -0,0 +1,6 @@ +CREATE TABLE users ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + oidc_sub TEXT NOT NULL UNIQUE, + display_name TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')) +); diff --git a/backend/internal/store/migrations/0022_activities_and_syncruns_user_id.sql b/backend/internal/store/migrations/0022_activities_and_syncruns_user_id.sql new file mode 100644 index 0000000..a7abe08 --- /dev/null +++ b/backend/internal/store/migrations/0022_activities_and_syncruns_user_id.sql @@ -0,0 +1,15 @@ +-- user_id is nullable here even though every row will eventually need one: +-- migrations can't take runtime parameters, so the actual owner isn't known +-- yet. A one-time Go bootstrap step (store.ClaimLegacyOwner, run at +-- geniusrund startup) backfills every existing row to one user once given +-- that user's OIDC subject; from then on every store method requires a +-- non-nil userID and this column is never NULL again in practice. +ALTER TABLE activities ADD COLUMN user_id INTEGER REFERENCES users(id); +ALTER TABLE sync_runs ADD COLUMN user_id INTEGER REFERENCES users(id); + +-- Used by UpsertActivity's ON CONFLICT target going forward. The original +-- UNIQUE(garmin_activity_id) constraint (migration 0001) stays in place too +-- -- Garmin's own activity ids are already globally unique in practice, so +-- the stricter constraint is harmless, and SQLite can't drop a column-level +-- constraint without a full table rebuild, which isn't worth the risk here. +CREATE UNIQUE INDEX ux_activities_user_garmin_id ON activities(user_id, garmin_activity_id); diff --git a/backend/internal/store/migrations/0023_profile_user_scoped.sql b/backend/internal/store/migrations/0023_profile_user_scoped.sql new file mode 100644 index 0000000..fb6ec01 --- /dev/null +++ b/backend/internal/store/migrations/0023_profile_user_scoped.sql @@ -0,0 +1,76 @@ +-- profile.id's CHECK(id = 1) singleton constraint (migration 0003) can't be +-- dropped via ALTER TABLE -- SQLite has no DROP CONSTRAINT -- so this +-- rebuilds the table via SQLite's documented rename/recreate/copy/drop +-- pattern instead. Always create the replacement under a different name and +-- RENAME it into place at the end (never rename the live table away first) +-- -- verified directly against SQLite that this ordering is what keeps +-- other tables' foreign keys intact when they exist (not the case for +-- profile, but kept consistent with migrations 0024/0025 for the same +-- pattern). user_id is nullable for the same not-yet-known-owner reason as +-- migration 0022 -- see its comment. +CREATE TABLE profile_new ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id INTEGER REFERENCES users(id), + name TEXT NOT NULL DEFAULT 'Default', + garmin_email TEXT NOT NULL DEFAULT '', + garmin_password TEXT NOT NULL DEFAULT '', + rolling_window_days INTEGER NOT NULL DEFAULT 90, + backfill_horizon_days INTEGER NOT NULL DEFAULT 1095, + max_heart_rate REAL, + resting_heart_rate REAL, + hr_zone1_min_pct REAL NOT NULL DEFAULT 50, + hr_zone1_max_pct REAL NOT NULL DEFAULT 60, + hr_zone2_min_pct REAL NOT NULL DEFAULT 60, + hr_zone2_max_pct REAL NOT NULL DEFAULT 70, + hr_zone3_min_pct REAL NOT NULL DEFAULT 70, + hr_zone3_max_pct REAL NOT NULL DEFAULT 80, + hr_zone4_min_pct REAL NOT NULL DEFAULT 80, + hr_zone4_max_pct REAL NOT NULL DEFAULT 90, + hr_zone5_min_pct REAL NOT NULL DEFAULT 90, + hr_zone5_max_pct REAL NOT NULL DEFAULT 100, + warmup_minutes REAL NOT NULL DEFAULT 10, + cooldown_minutes REAL NOT NULL DEFAULT 5, + min_representative_pace_sec_per_km REAL NOT NULL DEFAULT 720, + min_representative_time_seconds REAL NOT NULL DEFAULT 3, + pace_color TEXT NOT NULL DEFAULT '#3b82f6', + heart_rate_color TEXT NOT NULL DEFAULT '#ef4444', + warmup_color TEXT NOT NULL DEFAULT '#c2410c', + effort_color TEXT NOT NULL DEFAULT '#7c3aed', + recovery_color TEXT NOT NULL DEFAULT '#15803d', + cooldown_color TEXT NOT NULL DEFAULT '#fb923c', + main_line_tint_pct REAL NOT NULL DEFAULT 20, + background_darken_pct REAL NOT NULL DEFAULT 35, + target_brighten_pct REAL NOT NULL DEFAULT 20, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + updated_at TEXT NOT NULL DEFAULT (datetime('now')), + UNIQUE(user_id) +); + +INSERT INTO profile_new ( + id, user_id, name, garmin_email, garmin_password, rolling_window_days, backfill_horizon_days, + max_heart_rate, resting_heart_rate, + hr_zone1_min_pct, hr_zone1_max_pct, hr_zone2_min_pct, hr_zone2_max_pct, + hr_zone3_min_pct, hr_zone3_max_pct, hr_zone4_min_pct, hr_zone4_max_pct, + hr_zone5_min_pct, hr_zone5_max_pct, + warmup_minutes, cooldown_minutes, + min_representative_pace_sec_per_km, min_representative_time_seconds, + pace_color, heart_rate_color, warmup_color, effort_color, recovery_color, cooldown_color, + main_line_tint_pct, background_darken_pct, target_brighten_pct, + created_at, updated_at +) +SELECT + id, NULL, name, garmin_email, garmin_password, rolling_window_days, backfill_horizon_days, + max_heart_rate, resting_heart_rate, + hr_zone1_min_pct, hr_zone1_max_pct, hr_zone2_min_pct, hr_zone2_max_pct, + hr_zone3_min_pct, hr_zone3_max_pct, hr_zone4_min_pct, hr_zone4_max_pct, + hr_zone5_min_pct, hr_zone5_max_pct, + warmup_minutes, cooldown_minutes, + min_representative_pace_sec_per_km, min_representative_time_seconds, + pace_color, heart_rate_color, warmup_color, effort_color, recovery_color, cooldown_color, + main_line_tint_pct, background_darken_pct, target_brighten_pct, + created_at, updated_at +FROM profile; + +DROP TABLE profile; + +ALTER TABLE profile_new RENAME TO profile; diff --git a/backend/internal/store/migrations/0024_workout_kinds_user_scoped.sql b/backend/internal/store/migrations/0024_workout_kinds_user_scoped.sql new file mode 100644 index 0000000..e54e54f --- /dev/null +++ b/backend/internal/store/migrations/0024_workout_kinds_user_scoped.sql @@ -0,0 +1,31 @@ +-- workout_kinds.name's UNIQUE(name) constraint (migration 0001) must become +-- per-user (UNIQUE(user_id, name)) now that every user gets their own copy +-- of the same 8-kind taxonomy -- otherwise a second user could never be +-- provisioned (inserting the same seeded name would collide). kind_assignments +-- and workout_type_paces hold foreign keys into this table +-- (workout_kind_id REFERENCES workout_kinds(id)) -- the create-new-name/ +-- copy/drop-old/rename-into-place order below (verified against a real +-- SQLite database) leaves those foreign keys' schema text untouched +-- throughout, so they resolve correctly again the instant the final +-- `ALTER TABLE workout_kinds_new RENAME TO workout_kinds` completes. +CREATE TABLE workout_kinds_new ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id INTEGER REFERENCES users(id), + name TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + color TEXT NOT NULL DEFAULT '', + rule_json TEXT NOT NULL, + priority INTEGER NOT NULL DEFAULT 0, + is_active INTEGER NOT NULL DEFAULT 1, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + updated_at TEXT NOT NULL DEFAULT (datetime('now')), + UNIQUE(user_id, name) +); + +INSERT INTO workout_kinds_new (id, user_id, name, description, color, rule_json, priority, is_active, created_at, updated_at) +SELECT id, NULL, name, description, color, rule_json, priority, is_active, created_at, updated_at +FROM workout_kinds; + +DROP TABLE workout_kinds; + +ALTER TABLE workout_kinds_new RENAME TO workout_kinds; diff --git a/backend/internal/store/migrations/0025_sync_state_user_scoped.sql b/backend/internal/store/migrations/0025_sync_state_user_scoped.sql new file mode 100644 index 0000000..421ecd9 --- /dev/null +++ b/backend/internal/store/migrations/0025_sync_state_user_scoped.sql @@ -0,0 +1,16 @@ +-- Same CHECK(id = 1) rebuild reasoning as migration 0023's profile rebuild. +CREATE TABLE sync_state_new ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + user_id INTEGER REFERENCES users(id), + earliest_synced_date TEXT, + backfill_complete INTEGER NOT NULL DEFAULT 0, + UNIQUE(user_id) +); + +INSERT INTO sync_state_new (id, user_id, earliest_synced_date, backfill_complete) +SELECT id, NULL, earliest_synced_date, backfill_complete +FROM sync_state; + +DROP TABLE sync_state; + +ALTER TABLE sync_state_new RENAME TO sync_state; diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index db166ca..5222469 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -195,3 +195,39 @@ func TestReplaceLapsIsIdempotent(t *testing.T) { t.Fatalf("expected 1 lap after replace, got %d", len(got)) } } + +func TestUsersAndOwnershipSchema_AppliesCleanly(t *testing.T) { + db := openTestDB(t) + ctx := context.Background() + + // A fresh DB has no legacy singleton rows, so every user_id column + // should already be backfilled to nothing (fresh install, no rows at + // all yet in these tables besides the migration-seeded workout_kinds -- + // which do have NULL user_id until a real user is provisioned). + var nullableCount int + if err := db.QueryRowContext(ctx, `SELECT COUNT(*) FROM workout_kinds WHERE user_id IS NULL`).Scan(&nullableCount); err != nil { + t.Fatalf("count workout_kinds: %v", err) + } + if nullableCount != 8 { + t.Fatalf("expected 8 migration-seeded workout_kinds with NULL user_id on a fresh DB, got %d", nullableCount) + } + + // UNIQUE(user_id, name) allows the same name across two different users. + if _, err := db.ExecContext(ctx, `INSERT INTO users (oidc_sub, display_name) VALUES ('sub-a', 'A'), ('sub-b', 'B')`); err != nil { + t.Fatalf("insert users: %v", err) + } + if _, err := db.ExecContext(ctx, ` + INSERT INTO workout_kinds (user_id, name, rule_json) + VALUES ((SELECT id FROM users WHERE oidc_sub='sub-a'), 'Custom Kind', '{}'), + ((SELECT id FROM users WHERE oidc_sub='sub-b'), 'Custom Kind', '{}')`); err != nil { + t.Fatalf("expected same workout_kind name to be allowed across two different users, got: %v", err) + } + + // profile/sync_state UNIQUE(user_id) still rejects a genuine duplicate. + if _, err := db.ExecContext(ctx, `INSERT INTO profile (user_id) VALUES ((SELECT id FROM users WHERE oidc_sub='sub-a'))`); err != nil { + t.Fatalf("insert profile for sub-a: %v", err) + } + if _, err := db.ExecContext(ctx, `INSERT INTO profile (user_id) VALUES ((SELECT id FROM users WHERE oidc_sub='sub-a'))`); err == nil { + t.Fatal("expected a second profile row for the same user_id to violate UNIQUE(user_id)") + } +}