diff --git a/backend/internal/store/db.go b/backend/internal/store/db.go index 425a306..4862864 100644 --- a/backend/internal/store/db.go +++ b/backend/internal/store/db.go @@ -79,16 +79,19 @@ func (db *DB) migrate() error { 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. + // Migrations 0023, 0024, 0025, and 0026 rebuild tables that have + // incoming foreign keys (kind_assignments/workout_type_paces + // reference workout_kinds; sync_state is referenced by + // activities/sync_runs; laps/activity_samples/kind_assignments + // reference activities). 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, + "0023_profile_user_scoped.sql": true, + "0024_workout_kinds_user_scoped.sql": true, + "0025_sync_state_user_scoped.sql": true, + "0026_activities_unique_constraint.sql": true, } needsFKToggle := tableRebuildMigrations[name] diff --git a/backend/internal/store/migrations/0026_activities_unique_constraint.sql b/backend/internal/store/migrations/0026_activities_unique_constraint.sql index 7379be2..d185a29 100644 --- a/backend/internal/store/migrations/0026_activities_unique_constraint.sql +++ b/backend/internal/store/migrations/0026_activities_unique_constraint.sql @@ -4,13 +4,24 @@ -- constraint alone was intentionally left in place by migration 0022 as a -- belts-and-suspenders safeguard (Garmin IDs are globally unique in practice), -- but it must be removed now to allow Task 6's cross-user test to pass. -PRAGMA foreign_keys = OFF; +-- +-- The FK enforcement toggle needed for this rebuild (DROP TABLE on a table +-- with real inbound foreign keys) happens in Go code around this migration's +-- execution (db.go's tableRebuildMigrations map), in autocommit mode before +-- the transaction begins -- a mid-transaction PRAGMA foreign_keys statement +-- is a documented no-op with modernc.org/sqlite, so it must not appear here. +-- +-- event_type_key keeps the DEFAULT '' that migration 0007 originally gave +-- it (ALTER TABLE ... ADD COLUMN event_type_key TEXT NOT NULL DEFAULT ''); +-- dropping the default here (as an earlier draft of this migration did) +-- broke every INSERT that omits event_type_key and relies on that default, +-- which several existing tests (e.g. TestClaimLegacyOwner_*) do. CREATE TABLE activities_new ( id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER REFERENCES users(id), garmin_activity_id INTEGER NOT NULL, - event_type_key TEXT NOT NULL, + event_type_key TEXT NOT NULL DEFAULT '', workout_id INTEGER, start_time_utc TEXT NOT NULL, duration_seconds REAL NOT NULL, @@ -42,5 +53,3 @@ ALTER TABLE activities_new RENAME TO activities; CREATE INDEX idx_activities_start_time ON activities(start_time_utc); CREATE INDEX idx_activities_user_garmin_id ON activities(user_id, garmin_activity_id); - -PRAGMA foreign_keys = ON; diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index d9ecc1c..7a236a2 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -343,19 +343,27 @@ func TestForeignKeyEnforcementPostMigration(t *testing.T) { // TestRebuildMigrationsPreserveForeignKeyReferences proves the specific // property none of the other migration tests cover: every other test opens -// a fresh DB via store.Open, which runs migrations 0001-0025 in one -// uninterrupted pass over an empty database, so migrations 0023/0024/0025's +// a fresh DB via store.Open, which runs migrations 0001-0026 in one +// uninterrupted pass over an empty database, so migrations 0023/0024/0025/0026's // table-rebuild (create-new/copy/drop-old/rename-into-place) never has any // real pre-existing rows to carry across. A real single-tenant deployment -// upgrading to this schema version has months of synced activities and -// kind_assignments rows referencing real workout_kinds rows by foreign key -// (per this repo's CLAUDE.md: one profile, real Garmin history, a review -// queue with manual overrides already recorded). This test manually applies -// migrations up through 0022, inserts rows simulating that pre-existing -// install, then applies 0023/0024/0025 and verifies the FK-referencing row -// still resolves correctly -- and that FK enforcement is genuinely back on -// afterward -- rather than just checking that migrations apply to an empty -// DB without erroring. +// upgrading to this schema version has months of synced activities, laps, +// activity_samples, and kind_assignments rows referencing real activities/ +// workout_kinds rows by foreign key (per this repo's CLAUDE.md: one profile, +// real Garmin history, a review queue with manual overrides already +// recorded). This test manually applies migrations up through 0022, inserts +// rows simulating that pre-existing install, then applies 0023/0024/0025/0026 +// and verifies every FK-referencing row still resolves correctly -- and that +// FK enforcement is genuinely back on afterward -- rather than just checking +// that migrations apply to an empty DB without erroring. This is the +// regression guard for a real bug: migration 0026 (activities table rebuild) +// originally issued its own mid-transaction `PRAGMA foreign_keys = OFF/ON`, +// which SQLite documents as a no-op once a transaction is open, so +// `DROP TABLE activities` silently cascade-deleted every laps/ +// activity_samples/kind_assignments row for every activity via +// `ON DELETE CASCADE` -- with no error at all. It was masked because every +// other test runs migrations back-to-back on an empty database with no +// pre-existing child rows. func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) { ctx := context.Background() path := filepath.Join(t.TempDir(), "geniusrun_rebuild_test.db") @@ -377,13 +385,14 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) { } rebuildMigrations := map[string]bool{ - "0023_profile_user_scoped.sql": true, - "0024_workout_kinds_user_scoped.sql": true, - "0025_sync_state_user_scoped.sql": true, + "0023_profile_user_scoped.sql": true, + "0024_workout_kinds_user_scoped.sql": true, + "0025_sync_state_user_scoped.sql": true, + "0026_activities_unique_constraint.sql": true, } // Mirrors db.go's migrate() method: FK toggle in autocommit mode around - // the transaction, only for the three table-rebuild migrations. + // the transaction, only for the four table-rebuild migrations. applyMigration := func(name string) { t.Helper() content, err := migrationsFS.ReadFile("migrations/" + name) @@ -470,12 +479,35 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) { t.Fatalf("kind_assignments LastInsertId: %v", err) } - // Now apply the three rebuild migrations that drop/recreate profile, - // workout_kinds, and sync_state. + res, err = sqlDB.ExecContext(ctx, ` + INSERT INTO laps (activity_id, lap_index, raw_json) + VALUES (?, 0, '{}')`, activityID) + if err != nil { + t.Fatalf("insert pre-existing laps row: %v", err) + } + lapID, err := res.LastInsertId() + if err != nil { + t.Fatalf("laps LastInsertId: %v", err) + } + + res, err = sqlDB.ExecContext(ctx, ` + INSERT INTO activity_samples (activity_id, elapsed_seconds, timestamp_ms, heart_rate) + VALUES (?, 60, 1735711260000, 150)`, activityID) + if err != nil { + t.Fatalf("insert pre-existing activity_samples row: %v", err) + } + sampleID, err := res.LastInsertId() + if err != nil { + t.Fatalf("activity_samples LastInsertId: %v", err) + } + + // Now apply the four rebuild migrations that drop/recreate profile, + // workout_kinds, sync_state, and (0026) activities itself. for _, name := range []string{ "0023_profile_user_scoped.sql", "0024_workout_kinds_user_scoped.sql", "0025_sync_state_user_scoped.sql", + "0026_activities_unique_constraint.sql", } { applyMigration(name) } @@ -493,13 +525,47 @@ func TestRebuildMigrationsPreserveForeignKeyReferences(t *testing.T) { t.Fatalf("expected pre-existing kind_assignment to still resolve to 'Pre-Existing Custom Kind' after rebuild, got %q", resolvedName) } + // The pre-existing laps row must still exist and still reference the + // same activity -- this is the exact regression guard for migration + // 0026's DROP TABLE activities silently cascade-deleting laps via + // ON DELETE CASCADE when FK enforcement wasn't actually disabled. + var lapActivityID int64 + if err := sqlDB.QueryRowContext(ctx, ` + SELECT activity_id FROM laps WHERE id = ?`, lapID).Scan(&lapActivityID); err != nil { + t.Fatalf("query pre-existing laps row after rebuild: %v", err) + } + if lapActivityID != activityID { + t.Fatalf("expected pre-existing laps row to still reference activity %d after rebuild, got %d", activityID, lapActivityID) + } + + // Same guard for activity_samples. + var sampleActivityID int64 + if err := sqlDB.QueryRowContext(ctx, ` + SELECT activity_id FROM activity_samples WHERE id = ?`, sampleID).Scan(&sampleActivityID); err != nil { + t.Fatalf("query pre-existing activity_samples row after rebuild: %v", err) + } + if sampleActivityID != activityID { + t.Fatalf("expected pre-existing activity_samples row to still reference activity %d after rebuild, got %d", activityID, sampleActivityID) + } + // FK enforcement must be genuinely active again post-migration: a bogus - // workout_kind_id must be rejected, not silently accepted. + // workout_kind_id, activity_id (laps), and activity_id (activity_samples) + // must all be rejected, not silently accepted. if _, err := sqlDB.ExecContext(ctx, ` INSERT INTO kind_assignments (activity_id, workout_kind_id, assignment_source, status) VALUES (?, 999999, 'rule_engine', 'assigned')`, activityID); err == nil { t.Fatal("expected FK constraint violation for nonexistent workout_kind_id after rebuild, but insert succeeded") } + if _, err := sqlDB.ExecContext(ctx, ` + INSERT INTO laps (activity_id, lap_index, raw_json) + VALUES (999999, 0, '{}')`); err == nil { + t.Fatal("expected FK constraint violation for nonexistent activity_id in laps after rebuild, but insert succeeded") + } + if _, err := sqlDB.ExecContext(ctx, ` + INSERT INTO activity_samples (activity_id, elapsed_seconds, timestamp_ms, heart_rate) + VALUES (999999, 60, 1735711260000, 150)`); err == nil { + t.Fatal("expected FK constraint violation for nonexistent activity_id in activity_samples after rebuild, but insert succeeded") + } } func contains(s string, substrs ...string) bool {