From de599e1f6aadc3dd3085530d489e5941deaac8c6 Mon Sep 17 00:00:00 2001 From: Michael Burns Date: Mon, 24 Aug 2026 01:28:19 -0700 Subject: [PATCH] feat(schema): CHECK constraints on the posts/comments flags Migration [114] constrained the leaf tables and skipped these two as the awkward case. [116] finishes it: the boolean flags on posts (locked, edited, is_self, over_18, deleted, stickied, comments_locked, approved, is_question) and on comments (edited, deleted, is_submitter, stickied, approved). The interesting part is the rebuild, not the constraints. `posts` is referenced by six tables and by itself, carries the FTS5 sync triggers, and both tables feed the v_daily_activity view -- so the recipe used for the leaf tables is wrong here in three separate ways: - With foreign keys enabled, ALTER TABLE ... RENAME rewrites the REFERENCES clauses of child tables to follow the rename. Renaming `posts` out of the way would leave `comments` pointing at `posts_old`. rebuild_referenced_table uses SQLite's documented order instead -- build under a temporary name, copy, drop the original, rename into place -- so children keep naming the table they always named. A spec asserts PRAGMA foreign_key_list(comments) still says posts, and that it says neither posts_old nor posts_rebuild. - Foreign keys must be off for the drop, via a PRAGMA that is only effective outside a transaction. Lapis runs migrations without one unless asked, so this works, but it is a dependency on that default and the docs now say so. PRAGMA foreign_key_check runs afterwards and the migration asserts on orphans. - SQLite re-parses the whole schema during the rename, so a view still pointing at the dropped table fails that parse. The three FTS triggers and v_daily_activity are dropped up front and recreated after. The FTS index itself is never touched, so its contents still match the copied rows. Specs cover both halves of that: an existing post is still findable by search after a rebuild, and a post created afterwards is indexed by the reattached AFTER INSERT trigger. Rows are copied as-is rather than normalized. Every writer sets these from a Lua boolean, so a violation would mean data this schema should never have held -- same policy as [114]'s text enums. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0128hUpuk1spKzk4UHburdki --- CHANGELOG.md | 20 +++ app/migrations.lua | 229 ++++++++++++++++++++++++++++ app/spec/posts_constraints_spec.lua | 182 ++++++++++++++++++++++ app/spec/schema_helper.lua | 1 + docs/sqlite-features.md | 30 +++- 5 files changed, 457 insertions(+), 5 deletions(-) create mode 100644 app/spec/posts_constraints_spec.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 68c6fb8..fd6b12d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ Format loosely follows [Keep a Changelog](https://keepachangelog.com/). This run took the PoC from a rough, non-booting prototype to a running, test-covered Reddit clone. Highlights, newest first: +### CHECK constraints reach posts and comments +- Migration `[116]` constrains the boolean flags on both tables — `locked`, + `edited`, `is_self`, `over_18`, `deleted`, `stickied`, `comments_locked`, + `approved`, `is_question` on posts; `edited`, `deleted`, `is_submitter`, + `stickied`, `approved` on comments. `[114]` had skipped them as the awkward + case. +- **A referenced table cannot be rebuilt the way a leaf can.** With foreign keys + on, `ALTER TABLE ... RENAME` rewrites child tables' `REFERENCES` clauses to + follow the rename, so moving `posts` aside would have left `comments` pointing + at `posts_old`. `rebuild_referenced_table` uses SQLite's documented order + instead: build under a temporary name, copy, drop the original, rename into + place. A spec asserts `PRAGMA foreign_key_list(comments)` still names `posts`. +- The three FTS5 sync triggers and `v_daily_activity` are dropped and recreated + around the swap, because SQLite re-parses the schema during the rename and a + view pointing at the dropped table fails it. The FTS index itself is untouched; + specs check both that existing rows are still searchable and that new writes + are indexed by the reattached triggers. +- `PRAGMA foreign_key_check` runs afterwards and the migration asserts on any + orphaned row. + ### Exact cursors for the `new` sort - **`new` no longer pages by window.** Its key, `(created_at, id)`, never moves once a post is written, so the database seeks straight to the cursor row with a diff --git a/app/migrations.lua b/app/migrations.lua index c653d8f..63bc42f 100644 --- a/app/migrations.lua +++ b/app/migrations.lua @@ -51,6 +51,44 @@ local opts = {} opts["strict"] = true opts["if_not_exists"] = true +--- Rebuild a table that other tables point at. +-- +-- `rebuild_table` renames the original out of the way first, which is fine for a +-- leaf but wrong here: with foreign keys enabled, `ALTER TABLE ... RENAME` +-- **rewrites the REFERENCES clauses of child tables** to follow the rename, so +-- `comments` would end up pointing at `posts_old`. SQLite's documented order +-- avoids that -- build the replacement under a temporary name, copy, drop the +-- original, then rename the replacement into its place, so the children's +-- clauses keep naming the table they always named. +-- +-- The caller must disable foreign keys around this (a `PRAGMA` that only takes +-- effect outside a transaction) and re-check them afterwards. Triggers and views +-- attached to the table have to be dropped first and recreated after: SQLite +-- re-parses the whole schema during the rename, and a view left pointing at the +-- dropped table fails that parse. +-- +-- @tparam string name table to rebuild +-- @tparam table columns the new `schema.create_table` definition +-- @tparam table copy `{ into = {col, ...}, from = {expr, ...} }` +-- @tparam table indexes CREATE INDEX statements to reapply +local function rebuild_referenced_table(name, columns, copy, indexes) + local temp = name .. "_rebuild" + schema.create_table(temp, columns, opts) + db.query( + ("INSERT INTO %s (%s) SELECT %s FROM %s"):format( + temp, + table.concat(copy.into, ", "), + table.concat(copy.from, ", "), + name + ) + ) + db.query("DROP TABLE " .. name) + db.query("ALTER TABLE " .. temp .. " RENAME TO " .. name) + for _, sql in ipairs(indexes) do + db.query(sql) + end +end + --- Rebuild a table in place, so it can gain constraints SQLite cannot ALTER in. -- -- Neither `CHECK` nor a foreign key's `ON DELETE` action can be added to an @@ -1290,6 +1328,197 @@ return { end end, + -- CHECK constraints on the boolean flags of `posts` and `comments`. + -- + -- Migration [114] constrained the leaf tables and deliberately skipped these + -- two, because they are the awkward ones: `posts` is referenced by six other + -- tables and by itself, it carries the FTS5 sync triggers, and both tables + -- feed the `v_daily_activity` view. Doing it properly means SQLite's + -- documented order rather than the rename-first shortcut -- see + -- `rebuild_referenced_table`. + -- + -- Rows are copied as-is. Every writer sets these from a Lua boolean, so a + -- violation would mean data this schema should never have held, and failing + -- loudly beats quietly rewriting someone's posts. Same policy as [114]'s + -- text enums. + [116] = function() + -- Only effective outside a transaction; lapis runs migrations without one + -- unless asked (`transaction = "global" | "individual"`). + db.query("PRAGMA foreign_keys = OFF") + + -- Dropped now, recreated below: the schema is re-parsed during the + -- rename, and a view pointing at the dropped table fails that parse. + for _, trigger in ipairs({ "posts_fts_ai", "posts_fts_ad", "posts_fts_au" }) do + db.query("DROP TRIGGER IF EXISTS " .. trigger) + end + db.query("DROP VIEW IF EXISTS v_daily_activity") + + local post_columns = { + "id", + "user_id", + "sub_id", + "title", + "url", + "created_at", + "updated_at", + "locked", + "edited", + "is_self", + "over_18", + "body", + "thumbnail", + "crosspost_parent_id", + "link_flair", + "deleted", + "stickied", + "comments_locked", + "external_guid", + "approved", + "is_question", + "accepted_comment_id", + "domain", + "public_id", + } + rebuild_referenced_table("posts", { + { "id", types.integer({ unique = true, primary_key = true }) }, + { "user_id", types.integer }, + { "sub_id", types.integer }, + { "title", types.text }, + { "url", types.text({ null = true }) }, + { "created_at", types.text }, + { "updated_at", types.text }, + { "locked", types.integer({ default = false }) }, + { "edited", types.integer({ default = false }) }, + { "is_self", types.integer({ default = false }) }, + { "over_18", types.integer({ default = false }) }, + { "body", types.text({ null = true }) }, + { "thumbnail", types.text({ null = true }) }, + { "crosspost_parent_id", types.integer({ null = true }) }, + { "link_flair", types.text({ null = true }) }, + { "deleted", types.integer({ default = false }) }, + { "stickied", types.integer({ default = false }) }, + { "comments_locked", types.integer({ default = false }) }, + { "external_guid", types.text({ null = true }) }, + { "approved", types.integer({ default = 1 }) }, + { "is_question", types.integer({ default = false }) }, + { "accepted_comment_id", types.integer({ null = true }) }, + { "domain", types.text({ null = true }) }, + { "public_id", types.text({ null = true }) }, + "FOREIGN KEY(sub_id) REFERENCES forum(id)", + "FOREIGN KEY(user_id) REFERENCES users(id)", + "FOREIGN KEY(crosspost_parent_id) REFERENCES posts(id)", + "CHECK (locked IN (0, 1))", + "CHECK (edited IN (0, 1))", + "CHECK (is_self IN (0, 1))", + "CHECK (over_18 IN (0, 1))", + "CHECK (deleted IN (0, 1))", + "CHECK (stickied IN (0, 1))", + "CHECK (comments_locked IN (0, 1))", + "CHECK (approved IN (0, 1))", + "CHECK (is_question IN (0, 1))", + }, { into = post_columns, from = post_columns }, { + [[CREATE INDEX posts_sub_id_idx ON posts (sub_id)]], + [[CREATE INDEX posts_user_id_idx ON posts (user_id)]], + [[CREATE INDEX posts_created_at_idx ON posts (created_at)]], + [[CREATE INDEX posts_deleted_idx ON posts (deleted)]], + [[CREATE INDEX posts_sub_id_created_at_idx ON posts (sub_id, created_at) + WHERE deleted = 0 AND locked = 0]], + [[CREATE INDEX posts_sub_id_stickied_idx ON posts (sub_id, stickied)]], + [[CREATE INDEX posts_external_guid_idx ON posts (external_guid)]], + [[CREATE INDEX posts_sub_id_approved_idx ON posts (sub_id, approved)]], + [[CREATE UNIQUE INDEX posts_public_id_idx ON posts (public_id)]], + }) + + local comment_columns = { + "id", + "post_id", + "user_id", + "parent_comment_id", + "body", + "created_at", + "updated_at", + "edited", + "deleted", + "is_submitter", + "stickied", + "approved", + "public_id", + } + rebuild_referenced_table("comments", { + { "id", types.integer({ unique = true, primary_key = true }) }, + { "post_id", types.integer }, + { "user_id", types.integer }, + { "parent_comment_id", types.integer({ null = true }) }, + { "body", types.text }, + { "created_at", types.text }, + { "updated_at", types.text }, + { "edited", types.integer({ default = false }) }, + { "deleted", types.integer({ default = false }) }, + { "is_submitter", types.integer({ default = false }) }, + { "stickied", types.integer({ default = false }) }, + { "approved", types.integer({ default = 1 }) }, + { "public_id", types.text({ null = true }) }, + "FOREIGN KEY(user_id) REFERENCES users(id)", + "FOREIGN KEY(post_id) REFERENCES posts(id)", + "UNIQUE(user_id, post_id, parent_comment_id)", + "CHECK (edited IN (0, 1))", + "CHECK (deleted IN (0, 1))", + "CHECK (is_submitter IN (0, 1))", + "CHECK (stickied IN (0, 1))", + "CHECK (approved IN (0, 1))", + }, { into = comment_columns, from = comment_columns }, { + [[CREATE INDEX comments_post_id_idx ON comments (post_id)]], + [[CREATE INDEX comments_parent_comment_id_idx ON comments (parent_comment_id)]], + [[CREATE INDEX comments_user_id_idx ON comments (user_id)]], + [[CREATE INDEX comments_post_id_parent_comment_id_idx + ON comments (post_id, parent_comment_id)]], + [[CREATE INDEX comments_post_id_approved_idx ON comments (post_id, approved)]], + [[CREATE UNIQUE INDEX comments_public_id_idx ON comments (public_id)]], + }) + + -- Reattach what was dropped. The FTS index itself was never touched, so + -- its contents still match the rows that were copied across. + db.query([[ + CREATE TRIGGER IF NOT EXISTS posts_fts_ai AFTER INSERT ON posts BEGIN + INSERT INTO posts_fts(rowid, title, body) VALUES (new.id, new.title, new.body); + END]]) + db.query([[ + CREATE TRIGGER IF NOT EXISTS posts_fts_ad AFTER DELETE ON posts BEGIN + INSERT INTO posts_fts(posts_fts, rowid, title, body) + VALUES ('delete', old.id, old.title, old.body); + END]]) + db.query([[ + CREATE TRIGGER IF NOT EXISTS posts_fts_au AFTER UPDATE ON posts BEGIN + INSERT INTO posts_fts(posts_fts, rowid, title, body) + VALUES ('delete', old.id, old.title, old.body); + INSERT INTO posts_fts(rowid, title, body) VALUES (new.id, new.title, new.body); + END]]) + db.query([[ + CREATE VIEW IF NOT EXISTS v_daily_activity AS + WITH activity(day, kind) AS ( + SELECT date(created_at), 'post' FROM posts WHERE deleted = 0 + UNION ALL + SELECT date(created_at), 'comment' FROM comments WHERE deleted = 0 + UNION ALL + SELECT date(created_at), 'signup' FROM users + ) + SELECT day, + SUM(CASE WHEN kind = 'post' THEN 1 ELSE 0 END) AS posts, + SUM(CASE WHEN kind = 'comment' THEN 1 ELSE 0 END) AS comments, + SUM(CASE WHEN kind = 'signup' THEN 1 ELSE 0 END) AS signups + FROM activity + GROUP BY day + ]]) + + -- Nothing should have been orphaned; say so loudly if it was. + local orphans = db.query("PRAGMA foreign_key_check") + assert( + not orphans or #orphans == 0, + "foreign_key_check found " .. tostring(orphans and #orphans) .. " orphaned row(s)" + ) + db.query("PRAGMA foreign_keys = ON") + end, + -- classify text : https://github.com/leafo/lapis-bayes [1439944992] = require("lapis.bayes.schema").run_migrations, } diff --git a/app/spec/posts_constraints_spec.lua b/app/spec/posts_constraints_spec.lua new file mode 100644 index 0000000..9440b85 --- /dev/null +++ b/app/spec/posts_constraints_spec.lua @@ -0,0 +1,182 @@ +--- CHECK constraints on posts/comments (migration [116]). +-- +-- These two are the awkward tables: `posts` is referenced by six others and by +-- itself, carries the FTS5 sync triggers, and both feed the `v_daily_activity` +-- view. So this covers the rebuild as much as the constraints -- re-running the +-- migration against real rows and checking that nothing came unstuck. + +local use_test_env = require("lapis.spec").use_test_env +local db = require("lapis.db") + +describe("posts/comments constraints", function() + use_test_env() + + local Users = require("models.users") + local Forum = require("src.models.forum") + local Posts = require("src.models.posts") + local Comments = require("models.comments") + local Votes = require("src.models.votes") + local migrations = require("migrations") + + local author, sub, post, comment + + setup(function() + require("spec.schema_helper")() + author = Users:create({ + user_name = "flagger", + user_pass = "password", + user_email = "f@e.com", + }) + sub = Forum:create({ name = "flagsub", creator_id = author.id }) + post = Posts:create({ + user_id = author.id, + sub_id = sub.id, + title = "findable zebra", + body = "a searchable body", + url = "https://e.example", + }) + comment = Comments:create({ post_id = post.id, user_id = author.id, body = "hello" }) + Votes:set(author.id, post.id, nil, 1) + end) + + local function insert_fails(sql, ...) + return not (pcall(db.query, sql, ...)) + end + + it("rejects a non-boolean post flag", function() + local now = db.format_date() + assert.is_true( + insert_fails( + [[INSERT INTO posts (user_id, sub_id, title, created_at, updated_at, deleted) + VALUES (?, ?, 'bad', ?, ?, 7)]], + author.id, + sub.id, + now, + now + ) + ) + end) + + it("rejects a non-boolean comment flag", function() + local now = db.format_date() + assert.is_true( + insert_fails( + [[INSERT INTO comments (post_id, user_id, body, created_at, updated_at, approved) + VALUES (?, ?, 'bad', ?, ?, 5)]], + post.id, + author.id, + now, + now + ) + ) + end) + + it("still accepts the flags the app writes", function() + local p = Posts:create({ + user_id = author.id, + sub_id = sub.id, + title = "ok", + url = "https://e.example/ok", + }) + p:update({ stickied = 1, comments_locked = 1, is_question = 1 }) + assert.same(1, tonumber(Posts:find(p.id).stickied)) + local c = Comments:create({ post_id = p.id, user_id = author.id, body = "c" }) + c:update({ edited = 1, deleted = 1 }) + assert.same(1, tonumber(Comments:find(c.id).deleted)) + end) + + describe("the rebuild", function() + it("keeps child foreign keys pointing at the rebuilt table", function() + -- The failure this guards against: renaming the *original* out of the + -- way (as the leaf-table rebuild does) makes SQLite rewrite child + -- REFERENCES clauses to follow it, so `comments` would end up naming + -- `posts_old`. Migration [116] builds under a temp name instead. + local targets = {} + for _, fk in ipairs(db.query("PRAGMA foreign_key_list(comments)") or {}) do + targets[fk.table] = true + end + assert.is_true(targets["posts"] == true, "comments should still reference posts") + for name in pairs(targets) do + assert.are_not.same("posts_rebuild", name) + assert.are_not.same("posts_old", name) + end + end) + + it("preserves rows, the FTS index and the view when re-run", function() + local posts_before = tonumber(db.select("count(*) AS n FROM posts")[1].n) + local comments_before = tonumber(db.select("count(*) AS n FROM comments")[1].n) + assert.is_true(posts_before > 0 and comments_before > 0) + + migrations[116]() + + assert.same(posts_before, tonumber(db.select("count(*) AS n FROM posts")[1].n)) + assert.same(comments_before, tonumber(db.select("count(*) AS n FROM comments")[1].n)) + assert.same("findable zebra", Posts:find(post.id).title) + assert.same("hello", Comments:find(comment.id).body) + + -- Full-text search still finds the row: the FTS index was left alone + -- and its sync triggers were reattached. + local hits = Posts:search("zebra") + local found = false + for _, row in ipairs(hits) do + if tonumber(row.id) == tonumber(post.id) then + found = true + end + end + assert.is_true(found, "FTS should still match the seeded post") + + -- The view was dropped and recreated around the swap. + assert.is_true(#db.select("day FROM v_daily_activity") >= 1) + + -- And the vote still resolves to its post. + assert.same(1, Votes:post_score(post.id)) + end) + + it("keeps the FTS triggers live for new writes after a rebuild", function() + local fresh = Posts:create({ + user_id = author.id, + sub_id = sub.id, + title = "postrebuild quokka", + url = "https://e.example/q", + }) + local found = false + for _, row in ipairs(Posts:search("quokka")) do + if tonumber(row.id) == tonumber(fresh.id) then + found = true + end + end + assert.is_true(found, "the reattached AFTER INSERT trigger should have indexed it") + end) + + it("leaves the expected indexes in place", function() + local names = {} + for _, r in ipairs(db.select("name FROM sqlite_master WHERE type = 'index'")) do + names[r.name] = true + end + for _, expected in ipairs({ + "posts_sub_id_idx", + "posts_user_id_idx", + "posts_created_at_idx", + "posts_deleted_idx", + "posts_sub_id_created_at_idx", + "posts_sub_id_stickied_idx", + "posts_external_guid_idx", + "posts_sub_id_approved_idx", + "posts_public_id_idx", + "comments_post_id_idx", + "comments_parent_comment_id_idx", + "comments_user_id_idx", + "comments_post_id_parent_comment_id_idx", + "comments_post_id_approved_idx", + "comments_public_id_idx", + }) do + assert.is_true(names[expected] == true, expected .. " is missing") + end + end) + + it("leaves no orphaned rows", function() + local orphans = db.query("PRAGMA foreign_key_check") + assert.same(0, orphans and #orphans or 0) + end) + end) +end) diff --git a/app/spec/schema_helper.lua b/app/spec/schema_helper.lua index ac0c2c5..aa0427f 100644 --- a/app/spec/schema_helper.lua +++ b/app/spec/schema_helper.lua @@ -44,6 +44,7 @@ return function() 113, -- drops the superseded moderators / user_profiles tables 114, -- CHECK constraints on the enum-ish columns 115, -- ON DELETE CASCADE for the personal tables + 116, -- CHECK constraints on the posts/comments flags }) do if migrations[k] then migrations[k]() diff --git a/docs/sqlite-features.md b/docs/sqlite-features.md index 5b0dd45..2e2cc61 100644 --- a/docs/sqlite-features.md +++ b/docs/sqlite-features.md @@ -95,11 +95,31 @@ Two consequences worth remembering next time: - **Only leaf tables are cheap to rebuild.** All four here are leaves — no foreign key points *at* them — so the rebuild cannot orphan a reference. -`posts` and `comments` were left alone deliberately: they carry the FTS5 sync -triggers, so a rebuild has to reattach those too, and their remaining -unconstrained columns are boolean flags where a stray value is cosmetic rather -than score-changing. Prefer declaring `CHECK` on **new** tables, where it is -free. +`posts` and `comments` followed in `[116]`, and they are the case that shows why +the leaf-table recipe is not general: + +- **A referenced table must not be renamed out of the way.** With foreign keys + enabled, `ALTER TABLE ... RENAME` **rewrites the `REFERENCES` clauses of child + tables** to follow the rename — so renaming `posts` aside would leave + `comments` pointing at `posts_old`. SQLite's documented order avoids it: build + the replacement under a temporary name, copy, drop the original, then rename + the replacement into place, so the children keep naming the table they always + named. That is `rebuild_referenced_table`, and a spec asserts + `PRAGMA foreign_key_list(comments)` still says `posts` afterwards. +- **Foreign keys have to be off** for the drop, via a `PRAGMA` that is only + effective *outside* a transaction. Lapis runs migrations without one unless + asked (`transaction = "global" | "individual"`), so this works — but it is a + dependency on that default worth knowing about. `PRAGMA foreign_key_check` + runs afterwards and the migration asserts on any orphan. +- **Triggers and views attached to the table must be dropped and recreated.** + SQLite re-parses the whole schema during the rename, and a view still pointing + at the dropped table fails that parse. `[116]` drops the three FTS5 sync + triggers and `v_daily_activity` up front and puts them back afterwards. The + FTS index itself is never touched, so its contents still match the copied + rows — specs cover both that old rows are still findable and that new writes + are still indexed by the reattached triggers. + +Prefer declaring `CHECK` on **new** tables, where it is free. ## Ranking and paging — adopted in SQL for the web listings