Skip to content

Commit f78884b

Browse files
PttCodingManclaude
andcommitted
fix: require base_version on page content edits; add migration ledger.
Optimistic lock previously accepted updates with no base_version as a "legacy client" escape hatch — which meant the check could be silently bypassed. Now required whenever the payload touches content_md or title; metadata-only edits (is_public toggle, parent/sort moves) still bypass. Migrations move from ad-hoc ALTER-on-startup to a versioned ledger (schema_migrations) with legacy-DB backfill by column inference. Partial indexes move out of migration bodies into post-run invariants so fresh DBs — where all migrations skip as "already applied" — still get them. Bump VERSION to 0.0.3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bca9246 commit f78884b

11 files changed

Lines changed: 278 additions & 51 deletions

VERSION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
0.0.2
1+
0.0.3

backend/app/database.py

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1089,37 +1089,12 @@ async def init_db():
10891089
await db.execute(statement)
10901090
await db.commit()
10911091

1092-
# Migrate: add new user columns if missing
1093-
cols = await db.execute_fetchall("PRAGMA table_info(users)")
1094-
col_names = {c["name"] for c in cols}
1095-
if "display_name" not in col_names:
1096-
await db.execute("ALTER TABLE users ADD COLUMN display_name TEXT DEFAULT ''")
1097-
if "email" not in col_names:
1098-
await db.execute("ALTER TABLE users ADD COLUMN email TEXT DEFAULT ''")
1099-
if "deleted_at" not in col_names:
1100-
await db.execute("ALTER TABLE users ADD COLUMN deleted_at TIMESTAMP")
1101-
if "original_username" not in col_names:
1102-
await db.execute("ALTER TABLE users ADD COLUMN original_username TEXT")
1103-
await db.execute("CREATE INDEX IF NOT EXISTS idx_users_deleted ON users(deleted_at)")
1104-
await db.commit()
1105-
1106-
# Migrate: add new page columns if missing
1107-
page_cols = await db.execute_fetchall("PRAGMA table_info(pages)")
1108-
page_col_names = {c["name"] for c in page_cols}
1109-
if "version" not in page_col_names:
1110-
await db.execute("ALTER TABLE pages ADD COLUMN version INTEGER NOT NULL DEFAULT 1")
1111-
if "deleted_at" not in page_col_names:
1112-
await db.execute("ALTER TABLE pages ADD COLUMN deleted_at TIMESTAMP")
1113-
await db.execute("CREATE INDEX IF NOT EXISTS idx_pages_deleted ON pages(deleted_at)")
1114-
if "is_public" not in page_col_names:
1115-
await db.execute("ALTER TABLE pages ADD COLUMN is_public INTEGER NOT NULL DEFAULT 0")
1116-
# Create the partial index regardless — fresh DBs also need it, and
1117-
# we can't put it in SCHEMA_SQL because upgrading DBs run SCHEMA_SQL
1118-
# before the ALTER TABLE above.
1119-
await db.execute(
1120-
"CREATE INDEX IF NOT EXISTS idx_pages_public ON pages(slug) WHERE is_public = 1"
1121-
)
1122-
await db.commit()
1092+
# Versioned schema migrations. Handles both fresh DBs (nothing to do,
1093+
# SCHEMA_SQL already created everything) and upgrades from pre-migration
1094+
# deployments (backfills the ledger from observed schema). See
1095+
# app/migrations.py for the list and the rules for adding new ones.
1096+
from app.migrations import run_migrations
1097+
await run_migrations(db)
11231098

11241099
# Ensure FTS5 index exists with the best available tokenizer.
11251100
# If the tokenizer changed (e.g. unicode61 → trigram), the table is

backend/app/migrations.py

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
"""Versioned schema migrations for JustWiki.
2+
3+
Why not Alembic: JustWiki's core pitch is "single SQLite file, no external
4+
deps." Alembic would add a CLI, a versions/ directory, an alembic.ini, and a
5+
separate `alembic upgrade head` step in every deploy — extra weight that buys
6+
us very little on a schema this small. Instead we keep the work in-process:
7+
migrations are plain async functions in this module, identified by a
8+
monotonically increasing integer version, recorded in `schema_migrations`,
9+
and run once on startup by init_db().
10+
11+
Ground rules:
12+
* Append-only. Never renumber or rewrite a shipped migration — existing
13+
deployments have already recorded the version.
14+
* Each migration must be idempotent at the SQL level (IF NOT EXISTS, column
15+
probes, etc.) so partial re-runs are safe if a crash happens mid-run.
16+
* Use `run_migrations` as the single entry point. It returns the list of
17+
versions applied in this invocation; callers can use that signal to
18+
decide whether expensive follow-up work (full-text rebuild etc.) is
19+
needed.
20+
"""
21+
import logging
22+
from typing import Awaitable, Callable
23+
24+
import aiosqlite
25+
26+
logger = logging.getLogger(__name__)
27+
28+
MigrationFn = Callable[[aiosqlite.Connection], Awaitable[None]]
29+
Migration = tuple[int, str, MigrationFn]
30+
31+
32+
async def _column_exists(db: aiosqlite.Connection, table: str, col: str) -> bool:
33+
rows = await db.execute_fetchall(f"PRAGMA table_info({table})")
34+
return any(r["name"] == col for r in rows)
35+
36+
37+
# ── Migration functions ────────────────────────────────────────────────────
38+
# New migrations go at the bottom. Never renumber or edit a shipped migration.
39+
40+
41+
async def _m001_user_profile_columns(db: aiosqlite.Connection) -> None:
42+
if not await _column_exists(db, "users", "display_name"):
43+
await db.execute("ALTER TABLE users ADD COLUMN display_name TEXT DEFAULT ''")
44+
if not await _column_exists(db, "users", "email"):
45+
await db.execute("ALTER TABLE users ADD COLUMN email TEXT DEFAULT ''")
46+
47+
48+
async def _m002_user_soft_delete(db: aiosqlite.Connection) -> None:
49+
if not await _column_exists(db, "users", "deleted_at"):
50+
await db.execute("ALTER TABLE users ADD COLUMN deleted_at TIMESTAMP")
51+
if not await _column_exists(db, "users", "original_username"):
52+
await db.execute("ALTER TABLE users ADD COLUMN original_username TEXT")
53+
54+
55+
async def _m003_page_version_counter(db: aiosqlite.Connection) -> None:
56+
if not await _column_exists(db, "pages", "version"):
57+
await db.execute(
58+
"ALTER TABLE pages ADD COLUMN version INTEGER NOT NULL DEFAULT 1"
59+
)
60+
61+
62+
async def _m004_page_soft_delete(db: aiosqlite.Connection) -> None:
63+
if not await _column_exists(db, "pages", "deleted_at"):
64+
await db.execute("ALTER TABLE pages ADD COLUMN deleted_at TIMESTAMP")
65+
66+
67+
async def _m005_page_is_public(db: aiosqlite.Connection) -> None:
68+
if not await _column_exists(db, "pages", "is_public"):
69+
await db.execute(
70+
"ALTER TABLE pages ADD COLUMN is_public INTEGER NOT NULL DEFAULT 0"
71+
)
72+
73+
74+
MIGRATIONS: list[Migration] = [
75+
(1, "user_profile_columns", _m001_user_profile_columns),
76+
(2, "user_soft_delete", _m002_user_soft_delete),
77+
(3, "page_version_counter", _m003_page_version_counter),
78+
(4, "page_soft_delete", _m004_page_soft_delete),
79+
(5, "page_is_public", _m005_page_is_public),
80+
]
81+
82+
83+
# ── Post-migration index invariants ────────────────────────────────────────
84+
# These indexes reference columns added by migrations, so they can't live in
85+
# SCHEMA_SQL (which runs first and would hit "no such column" on an upgrade).
86+
# They can't live in the migration bodies either: when a fresh DB boots,
87+
# every migration is detected as pre-applied and skipped, so an index baked
88+
# into a migration body would never run. Treating them as always-ensure
89+
# invariants after migrations is idempotent and covers both paths.
90+
_INDEX_INVARIANTS = (
91+
"CREATE INDEX IF NOT EXISTS idx_users_deleted ON users(deleted_at)",
92+
"CREATE INDEX IF NOT EXISTS idx_pages_deleted ON pages(deleted_at)",
93+
"CREATE INDEX IF NOT EXISTS idx_pages_public ON pages(slug) WHERE is_public = 1",
94+
)
95+
96+
97+
async def _ensure_indexes(db: aiosqlite.Connection) -> None:
98+
for stmt in _INDEX_INVARIANTS:
99+
await db.execute(stmt)
100+
await db.commit()
101+
102+
103+
# ── Runner ─────────────────────────────────────────────────────────────────
104+
105+
106+
async def _ensure_ledger(db: aiosqlite.Connection) -> None:
107+
await db.execute(
108+
"""
109+
CREATE TABLE IF NOT EXISTS schema_migrations (
110+
version INTEGER PRIMARY KEY,
111+
name TEXT NOT NULL,
112+
applied_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP
113+
)
114+
"""
115+
)
116+
await db.commit()
117+
118+
119+
async def _applied_versions(db: aiosqlite.Connection) -> set[int]:
120+
rows = await db.execute_fetchall("SELECT version FROM schema_migrations")
121+
return {r["version"] for r in rows}
122+
123+
124+
async def _detect_preexisting(db: aiosqlite.Connection) -> set[int]:
125+
"""Infer which shipped migrations are already effectively applied.
126+
127+
Needed for databases created before this module existed: the schema may
128+
already carry columns added by earlier in-place ALTERs. Backfilling the
129+
ledger here keeps the upgrade silent — no re-running of idempotent DDL,
130+
no spurious log lines about "applying migration v3".
131+
132+
Only probes for artifacts the migration actually creates; anything more
133+
ambitious (row counts, index options) gets fragile fast.
134+
"""
135+
applied: set[int] = set()
136+
if await _column_exists(db, "users", "display_name") and await _column_exists(
137+
db, "users", "email"
138+
):
139+
applied.add(1)
140+
if await _column_exists(db, "users", "deleted_at") and await _column_exists(
141+
db, "users", "original_username"
142+
):
143+
applied.add(2)
144+
if await _column_exists(db, "pages", "version"):
145+
applied.add(3)
146+
if await _column_exists(db, "pages", "deleted_at"):
147+
applied.add(4)
148+
if await _column_exists(db, "pages", "is_public"):
149+
applied.add(5)
150+
return applied
151+
152+
153+
async def run_migrations(db: aiosqlite.Connection) -> list[int]:
154+
"""Apply any pending migrations. Returns the versions applied this run.
155+
156+
Also ensures schema-invariant indexes (see `_INDEX_INVARIANTS`) regardless
157+
of whether any migration ran, so fresh DBs get them too.
158+
"""
159+
await _ensure_ledger(db)
160+
161+
applied = await _applied_versions(db)
162+
# First run against a pre-existing DB: backfill the ledger from what's
163+
# observable in the schema, so we don't re-announce "applying v1…v5".
164+
if not applied:
165+
inferred = await _detect_preexisting(db)
166+
for v in sorted(inferred):
167+
name = next((n for (ver, n, _) in MIGRATIONS if ver == v), f"legacy_{v}")
168+
await db.execute(
169+
"INSERT OR IGNORE INTO schema_migrations (version, name) VALUES (?, ?)",
170+
(v, name),
171+
)
172+
if inferred:
173+
await db.commit()
174+
applied = inferred
175+
176+
just_applied: list[int] = []
177+
for version, name, fn in MIGRATIONS:
178+
if version in applied:
179+
continue
180+
logger.info("Applying schema migration %03d: %s", version, name)
181+
try:
182+
await fn(db)
183+
await db.execute(
184+
"INSERT INTO schema_migrations (version, name) VALUES (?, ?)",
185+
(version, name),
186+
)
187+
await db.commit()
188+
except Exception:
189+
# Leave the half-applied state on disk so an operator can inspect
190+
# it. The next startup will retry from this same migration.
191+
logger.exception("Schema migration %03d (%s) failed", version, name)
192+
raise
193+
just_applied.append(version)
194+
195+
await _ensure_indexes(db)
196+
return just_applied

backend/app/routers/pages.py

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -355,18 +355,32 @@ async def update_page(slug: str, body: PageUpdate, user=Depends(get_current_user
355355
detail="You do not have write permission on this page",
356356
)
357357

358-
# Optimistic lock: if the client sent a base_version, it must match.
359-
# Missing base_version is allowed for legacy API clients, but logged.
360-
if body.base_version is not None and body.base_version != current["version"]:
361-
raise HTTPException(
362-
status_code=409,
363-
detail={
364-
"error": "conflict",
365-
"message": "This page was modified by someone else. Reload to see the latest version.",
366-
"current_version": current["version"],
367-
"your_version": body.base_version,
368-
},
369-
)
358+
# Optimistic lock. Required whenever the payload includes a content_md or
359+
# title change — those are the only edits that bump the version counter
360+
# and therefore the only ones that can silently clobber someone else's
361+
# work. Metadata-only edits (is_public toggle, parent/sort moves) don't
362+
# need it: they don't race against an open editor.
363+
touches_content = body.content_md is not None or body.title is not None
364+
if touches_content:
365+
if body.base_version is None:
366+
raise HTTPException(
367+
status_code=400,
368+
detail={
369+
"error": "base_version_required",
370+
"message": "base_version is required on content or title edits.",
371+
"current_version": current["version"],
372+
},
373+
)
374+
if body.base_version != current["version"]:
375+
raise HTTPException(
376+
status_code=409,
377+
detail={
378+
"error": "conflict",
379+
"message": "This page was modified by someone else. Reload to see the latest version.",
380+
"current_version": current["version"],
381+
"your_version": body.base_version,
382+
},
383+
)
370384

371385
title = body.title if body.title is not None else current["title"]
372386
content = body.content_md if body.content_md is not None else current["content_md"]

backend/tests/test_diagrams.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,9 @@ async def test_diagrams_delete_blocked_when_referenced(admin_client):
207207
assert blocked.status_code == 409
208208

209209
# Remove the reference and delete should succeed.
210-
await admin_client.put(f"/api/pages/{slug}", json={"content_md": "no ref"})
210+
await admin_client.put(
211+
f"/api/pages/{slug}", json={"content_md": "no ref", "base_version": 1}
212+
)
211213
ok = await admin_client.delete(f"/api/diagrams/{diagram_id}")
212214
assert ok.status_code == 204
213215

backend/tests/test_media.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ async def test_media_reference_tracking_and_delete(admin_client):
9494
# Remove the reference by updating the page
9595
upd = await admin_client.put(
9696
f"/api/pages/{page['slug']}",
97-
json={"content_md": "No media anymore"},
97+
json={"content_md": "No media anymore", "base_version": 1},
9898
)
9999
assert upd.status_code == 200
100100

backend/tests/test_optimistic_lock.py

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,21 +20,53 @@ async def test_version_bumps_on_content_change(auth_client):
2020
"content_md": "one",
2121
"slug": "lock-bump",
2222
})
23-
# Update with no base_version (legacy client) still works
2423
res = await auth_client.put("/api/pages/lock-bump", json={
2524
"content_md": "two",
25+
"base_version": 1,
2626
})
2727
assert res.status_code == 200
2828
assert res.json()["version"] == 2
2929

3030
# Another update bumps again
3131
res = await auth_client.put("/api/pages/lock-bump", json={
3232
"content_md": "three",
33+
"base_version": 2,
3334
})
3435
assert res.status_code == 200
3536
assert res.json()["version"] == 3
3637

3738

39+
@pytest.mark.asyncio
40+
async def test_content_edit_requires_base_version(auth_client):
41+
# A client that forgets base_version on a content edit must be rejected
42+
# outright — a silent pass here is exactly the "legacy client loophole"
43+
# that caused this check to be tightened.
44+
await auth_client.post("/api/pages", json={
45+
"title": "Lock Required",
46+
"content_md": "hi",
47+
"slug": "lock-required",
48+
})
49+
res = await auth_client.put("/api/pages/lock-required", json={
50+
"content_md": "bye",
51+
})
52+
assert res.status_code == 400
53+
assert res.json()["detail"]["error"] == "base_version_required"
54+
55+
56+
@pytest.mark.asyncio
57+
async def test_metadata_edit_does_not_require_base_version(auth_client):
58+
# Visibility toggles and sort/parent moves don't bump version and can't
59+
# clobber an editor's draft, so they skip the check by design.
60+
await auth_client.post("/api/pages", json={
61+
"title": "Lock Meta",
62+
"content_md": "body",
63+
"slug": "lock-meta",
64+
})
65+
res = await auth_client.put("/api/pages/lock-meta", json={"is_public": True})
66+
assert res.status_code == 200
67+
assert res.json()["version"] == 1
68+
69+
3870
@pytest.mark.asyncio
3971
async def test_version_does_not_bump_on_noop_update(auth_client):
4072
await auth_client.post("/api/pages", json={

backend/tests/test_pages.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ async def test_update_page(auth_client):
3535

3636
response = await auth_client.put("/api/pages/update-me", json={
3737
"title": "Updated Title",
38-
"content_md": "New content"
38+
"content_md": "New content",
39+
"base_version": 1,
3940
})
4041
assert response.status_code == 200
4142
assert response.json()["title"] == "Updated Title"

backend/tests/test_versions.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ async def test_versions(auth_client):
1212
# Update page to create a version
1313
await auth_client.put("/api/pages/version-page", json={
1414
"title": "Updated Title",
15-
"content_md": "New content"
15+
"content_md": "New content",
16+
"base_version": 1,
1617
})
1718

1819
# List versions

0 commit comments

Comments
 (0)