diff --git a/backend/db/migrations/0001_baseline_schema.sql b/backend/db/migrations/0001_baseline_schema.sql index 13f687dd..dbf2ccd4 100644 --- a/backend/db/migrations/0001_baseline_schema.sql +++ b/backend/db/migrations/0001_baseline_schema.sql @@ -92,9 +92,12 @@ CREATE TABLE IF NOT EXISTS graph_nodes ( CREATE INDEX IF NOT EXISTS idx_graph_nodes_user_course ON graph_nodes(user_id, course_id); -- Knowledge graph edges +-- graph_edges.user_id carries a hard FK (#179) with no ON DELETE clause, so it +-- defaults to NO ACTION (RESTRICT): a users row cannot be hard-deleted while +-- edges still reference it. This prevents orphaned edges; it does not cascade. CREATE TABLE IF NOT EXISTS graph_edges ( id TEXT PRIMARY KEY, - user_id TEXT NOT NULL, + user_id TEXT NOT NULL REFERENCES users(id), -- #179 source_node_id TEXT NOT NULL REFERENCES graph_nodes(id), target_node_id TEXT NOT NULL REFERENCES graph_nodes(id), strength DOUBLE PRECISION DEFAULT 0.5, @@ -102,6 +105,10 @@ CREATE TABLE IF NOT EXISTS graph_edges ( relationship_type TEXT DEFAULT 'related' ); +-- Index the FK referencing column: Postgres does not auto-index the +-- referencing side of a foreign key, and sibling tables index this path. +CREATE INDEX IF NOT EXISTS idx_graph_edges_user_id ON graph_edges(user_id); + -- Learning sessions CREATE TABLE IF NOT EXISTS sessions ( id TEXT PRIMARY KEY, @@ -487,16 +494,24 @@ CREATE TABLE IF NOT EXISTS user_cosmetics ( -- filters work for tag-based search. last_summary is the cached output of -- the most recent /summarize action; null until the user runs it. -- +-- notes.user_id / notes.course_id carry hard FKs (#180) — they are core user +-- data. The FKs have no ON DELETE clause, so they default to NO ACTION +-- (RESTRICT): a user or course row cannot be hard-deleted while notes still +-- reference it. This guarantees notes never become orphaned, but it does NOT +-- cascade-delete them. Today nothing hard-deletes users/courses +-- (delete_account is a soft delete; delete_course only removes the +-- user_courses enrollment row), so RESTRICT never fires. If cleanup-on-delete +-- is ever wanted, switch these to ON DELETE CASCADE and add a hard-delete path. +-- -- note_concepts is a junction table linking notes <-> graph_nodes. -- ON DELETE CASCADE on note_id ensures deleting a note cleans up its --- links. The graph_node FK is intentionally NOT a hard FK because --- graph_nodes uses TEXT ids managed by application code (no enforced FK --- pattern elsewhere in this codebase — see graph_edges.source_node_id). +-- links. Only the note_concepts.concept_node_id link is intentionally NOT a +-- hard FK, because graph_nodes uses TEXT ids managed by application code. CREATE TABLE notes ( id TEXT PRIMARY KEY, - user_id TEXT NOT NULL, - course_id TEXT NOT NULL, + user_id TEXT NOT NULL REFERENCES users(id), -- #180 + course_id TEXT NOT NULL REFERENCES courses(id), -- #180 title TEXT, body TEXT, tags TEXT[] NOT NULL DEFAULT '{}', diff --git a/backend/db/migrations/0020_fk_integrity.sql b/backend/db/migrations/0020_fk_integrity.sql new file mode 100644 index 00000000..ad4f5b5b --- /dev/null +++ b/backend/db/migrations/0020_fk_integrity.sql @@ -0,0 +1,73 @@ +-- Migration: foreign-key integrity for graph_edges + notes (#179, #180) +-- +-- Backfills, on already-migrated databases, the FK constraints that fresh +-- databases now get inline from 0001_baseline_schema.sql. graph_edges.user_id +-- and notes.user_id / notes.course_id historically shipped as bare TEXT columns +-- with no REFERENCES, inconsistent with every sibling learning table: +-- #179 graph_edges.user_id -> users(id) +-- #180 notes.user_id -> users(id) +-- #180 notes.course_id -> courses(id) +-- +-- migrate.py wraps each migration in a single transaction, so this is plain +-- (non-CONCURRENT) DDL. Each constraint is added behind a pg_constraint guard +-- because Postgres has no ADD CONSTRAINT IF NOT EXISTS, which also makes this +-- migration a no-op on fresh databases that already have the inline FKs from +-- the baseline. Pre-existing orphan rows are deleted first so the ALTER TABLE +-- can validate. +-- +-- ON DELETE semantics: these FKs have no ON DELETE clause, so they default to +-- NO ACTION (RESTRICT). A referenced users/courses row cannot be hard-deleted +-- while a graph_edges/notes row still points at it. This guarantees no orphans +-- but does NOT cascade-delete dependents. Today nothing hard-deletes +-- users/courses (delete_account is a soft delete; delete_course only removes +-- the user_courses enrollment row), so RESTRICT never actually fires. Switch +-- to ON DELETE CASCADE (and add a hard-delete path) if cleanup is ever wanted. + +-- #179 graph_edges.user_id: remove edges whose user_id has no users row, then +-- add the FK other learning tables already enforce. +DELETE FROM graph_edges + WHERE user_id NOT IN (SELECT id FROM users); + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'graph_edges_user_id_fkey' + ) THEN + ALTER TABLE graph_edges + ADD CONSTRAINT graph_edges_user_id_fkey + FOREIGN KEY (user_id) REFERENCES users(id); + END IF; +END $$; + +-- Index the FK referencing column: Postgres does not auto-index the +-- referencing side of a foreign key, and sibling tables index this path. +CREATE INDEX IF NOT EXISTS idx_graph_edges_user_id ON graph_edges(user_id); + +-- #180 notes.user_id / notes.course_id: notes is core user data but both +-- columns are bare TEXT. Remove rows pointing at a non-existent user or course +-- (e.g. notes left dangling after a course delete) before adding the FKs. +DELETE FROM notes + WHERE user_id NOT IN (SELECT id FROM users) + OR course_id NOT IN (SELECT id FROM courses); + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'notes_user_id_fkey' + ) THEN + ALTER TABLE notes + ADD CONSTRAINT notes_user_id_fkey + FOREIGN KEY (user_id) REFERENCES users(id); + END IF; +END $$; + +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 FROM pg_constraint WHERE conname = 'notes_course_id_fkey' + ) THEN + ALTER TABLE notes + ADD CONSTRAINT notes_course_id_fkey + FOREIGN KEY (course_id) REFERENCES courses(id); + END IF; +END $$; diff --git a/backend/tests/test_fk_integrity_migration.py b/backend/tests/test_fk_integrity_migration.py new file mode 100644 index 00000000..e37b7e88 --- /dev/null +++ b/backend/tests/test_fk_integrity_migration.py @@ -0,0 +1,64 @@ +""" +Drift guard for the FK-integrity migration (#179, #180). + +No live Postgres in the unit suite, so we assert the textual invariants that +matter: the numbered migration adds each constraint behind the pg_constraint +guard (re-runnable) and cleans orphans first, and the canonical baseline schema +declares the same REFERENCES inline so a fresh database is born with the FKs. + +After main restructured db/ into ordered migrations applied by migrate.py, the +FK DDL lives in migrations/0020_fk_integrity.sql (for already-migrated DBs) and +the inline REFERENCES live in migrations/0001_baseline_schema.sql (for fresh +DBs). The old flat db/supabase_schema.sql / migration_*.sql files were deleted. +""" +import os + +_MIGRATIONS = os.path.join( + os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "db", "migrations" +) + +FK_MIGRATION = "0020_fk_integrity.sql" +BASELINE = "0001_baseline_schema.sql" + +CONSTRAINTS = ( + "graph_edges_user_id_fkey", + "notes_user_id_fkey", + "notes_course_id_fkey", +) + + +def _read(name: str) -> str: + with open(os.path.join(_MIGRATIONS, name), encoding="utf-8") as fh: + return fh.read() + + +def test_migration_adds_each_constraint_behind_a_guard(): + sql = _read(FK_MIGRATION) + for name in CONSTRAINTS: + assert name in sql, f"{name} missing from migration" + # Every ADD CONSTRAINT must be inside an IF NOT EXISTS pg_constraint guard. + assert sql.count("IF NOT EXISTS") >= len(CONSTRAINTS) + assert "pg_constraint" in sql + + +def test_migration_cleans_orphans_before_altering(): + sql = _read(FK_MIGRATION) + # Orphan deletes must precede the ALTER TABLE that validates the FK. + assert "DELETE FROM graph_edges" in sql + assert "DELETE FROM notes" in sql + assert sql.index("DELETE FROM graph_edges") < sql.index("graph_edges_user_id_fkey") + assert sql.index("DELETE FROM notes") < sql.index("notes_user_id_fkey") + + +def test_migration_indexes_the_referencing_column(): + # graph_edges.user_id needs an index on the FK referencing side (#179). + sql = _read(FK_MIGRATION) + assert "CREATE INDEX IF NOT EXISTS idx_graph_edges_user_id" in sql + + +def test_baseline_declares_inline_references(): + # Fresh databases must be born with the FKs, declared inline in the baseline. + sql = _read(BASELINE) + assert "user_id TEXT NOT NULL REFERENCES users(id)" in sql # graph_edges + assert "user_id TEXT NOT NULL REFERENCES users(id)" in sql # notes + assert "course_id TEXT NOT NULL REFERENCES courses(id)" in sql # notes