fix: complete user-delete FK lockstep across PG and SQLite schemas
The prior user-deletion work updated the PG schema and a live-PG migration but left the canonical schema definitions inconsistent, breaking user deletion on fresh PG installs and on all SQLite dev installs. - schema.sql: add ON DELETE SET NULL to context_files.updated_by (was the only user FK missing it; fresh PG installs could not delete an authoring user). - schema_sqlite.sql: bring five user_id FK columns into lockstep with PG (drop NOT NULL, add ON DELETE SET NULL): project_context.updated_by, context_files.updated_by, change_requests.submitted_by, reviews.reviewer_id, audit_log.user_id. - schema_sqlite.sql: remove the audit_log append-only UPDATE/DELETE triggers. ON DELETE SET NULL on audit_log.user_id is an UPDATE the trigger aborted, so deleting any user who had ever logged in failed. This mirrors schema.sql, which dropped the equivalent PG triggers in fc1a2f5; append-only is enforced at the application layer (db.py only INSERTs into audit_log). - db.py: user_delete no longer swallows non-FK exceptions on the SQLite path (Exception masked sqlite3.IntegrityError); only FK violations map to the soft "user_has_references" response, everything else propagates. PG rollback-on-any-error (shared-connection cascade fix) is preserved. - db.py: document that SQLite cannot ALTER FK constraints in place; existing dev DBs must be recreated to pick up these changes. - server.py: the global 409 handler no longer leaks raw psycopg text (index names, column expressions) to API callers; it is logged instead. - migrate_user_fk_set_null.py: use the column from FKS_TO_FIX directly instead of re-deriving it from the constraint name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+15
-2
@@ -76,6 +76,11 @@ def _init_sqlite(cfg: CtxConfig) -> sqlite3.Connection:
|
|||||||
with open(SCHEMA_SQLITE_PATH) as f:
|
with open(SCHEMA_SQLITE_PATH) as f:
|
||||||
conn.executescript(f.read())
|
conn.executescript(f.read())
|
||||||
else:
|
else:
|
||||||
|
# NOTE: SQLite cannot ALTER a FK constraint in place. Existing dev
|
||||||
|
# databases will NOT pick up FK changes in schema_sqlite.sql (e.g.
|
||||||
|
# ON DELETE SET NULL on user_id columns) — delete $CTXD_HOME/ctxd.db
|
||||||
|
# to recreate from the current schema. Only additive ADD COLUMN
|
||||||
|
# migrations are applied here.
|
||||||
# Migration: add metadata_tags column if it doesn't exist
|
# Migration: add metadata_tags column if it doesn't exist
|
||||||
try:
|
try:
|
||||||
conn.execute("ALTER TABLE projects ADD COLUMN metadata_tags TEXT DEFAULT '[]'")
|
conn.execute("ALTER TABLE projects ADD COLUMN metadata_tags TEXT DEFAULT '[]'")
|
||||||
@@ -205,14 +210,22 @@ def user_delete(conn, user_id: str) -> dict:
|
|||||||
try:
|
try:
|
||||||
conn.execute(f"DELETE FROM users WHERE user_id = {ph}", (user_id,))
|
conn.execute(f"DELETE FROM users WHERE user_id = {ph}", (user_id,))
|
||||||
return {"ok": True}
|
return {"ok": True}
|
||||||
except (sqlite3.IntegrityError, Exception) as e:
|
except Exception as e:
|
||||||
if _is_pg(conn):
|
if _is_pg(conn):
|
||||||
import psycopg
|
import psycopg
|
||||||
|
# Roll back unconditionally so a failed DELETE never leaves the
|
||||||
|
# shared connection in an aborted-transaction state (see the 500
|
||||||
|
# cascade fix); only an FK violation maps to the soft response.
|
||||||
conn.rollback()
|
conn.rollback()
|
||||||
if isinstance(e, psycopg.errors.ForeignKeyViolation):
|
if isinstance(e, psycopg.errors.ForeignKeyViolation):
|
||||||
return {"ok": False, "error": "user_has_references", "hint": "Inactivate the user instead of deleting."}
|
return {"ok": False, "error": "user_has_references", "hint": "Inactivate the user instead of deleting."}
|
||||||
raise
|
raise
|
||||||
return {"ok": False, "error": "user_has_references", "hint": "Inactivate the user instead of deleting."}
|
# SQLite: only an FK violation is the expected "still referenced" case.
|
||||||
|
# Anything else (I/O error, programming bug, corruption) must propagate
|
||||||
|
# to the global handler as a 500 rather than be masked as a soft 409.
|
||||||
|
if isinstance(e, sqlite3.IntegrityError):
|
||||||
|
return {"ok": False, "error": "user_has_references", "hint": "Inactivate the user instead of deleting."}
|
||||||
|
raise
|
||||||
|
|
||||||
|
|
||||||
def user_set_password(conn, user_id: str, password: str):
|
def user_set_password(conn, user_id: str, password: str):
|
||||||
|
|||||||
@@ -66,7 +66,8 @@ def main():
|
|||||||
print(f" SKIP {constraint}: already ON DELETE SET NULL")
|
print(f" SKIP {constraint}: already ON DELETE SET NULL")
|
||||||
continue
|
continue
|
||||||
|
|
||||||
col = constraint.replace(f"{table}_", "").replace("_fkey", "")
|
# `col` comes straight from the FKS_TO_FIX tuple — don't re-derive it
|
||||||
|
# from the constraint name, which breaks for manually-named constraints.
|
||||||
print(f" ALTER {table}.{col} ({constraint}): {row[0]} -> SET NULL")
|
print(f" ALTER {table}.{col} ({constraint}): {row[0]} -> SET NULL")
|
||||||
conn.execute(f'ALTER TABLE {table} DROP CONSTRAINT {constraint}')
|
conn.execute(f'ALTER TABLE {table} DROP CONSTRAINT {constraint}')
|
||||||
conn.execute(
|
conn.execute(
|
||||||
|
|||||||
@@ -80,7 +80,7 @@ CREATE TABLE context_files (
|
|||||||
file_path TEXT NOT NULL,
|
file_path TEXT NOT NULL,
|
||||||
content TEXT NOT NULL DEFAULT '',
|
content TEXT NOT NULL DEFAULT '',
|
||||||
version INTEGER NOT NULL DEFAULT 1,
|
version INTEGER NOT NULL DEFAULT 1,
|
||||||
updated_by TEXT REFERENCES users(user_id),
|
updated_by TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
updated_at TEXT NOT NULL DEFAULT to_char(now() at time zone 'utc', 'YYYY-MM-DD"T"HH24:MI:SS"Z"'),
|
updated_at TEXT NOT NULL DEFAULT to_char(now() at time zone 'utc', 'YYYY-MM-DD"T"HH24:MI:SS"Z"'),
|
||||||
UNIQUE(project_id, file_path)
|
UNIQUE(project_id, file_path)
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -68,7 +68,7 @@ CREATE TABLE project_context (
|
|||||||
project_id TEXT PRIMARY KEY REFERENCES projects(project_id) ON DELETE CASCADE,
|
project_id TEXT PRIMARY KEY REFERENCES projects(project_id) ON DELETE CASCADE,
|
||||||
content TEXT NOT NULL DEFAULT '',
|
content TEXT NOT NULL DEFAULT '',
|
||||||
version INTEGER NOT NULL DEFAULT 0,
|
version INTEGER NOT NULL DEFAULT 0,
|
||||||
updated_by TEXT REFERENCES users(user_id),
|
updated_by TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))
|
updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now'))
|
||||||
);
|
);
|
||||||
|
|
||||||
@@ -81,7 +81,7 @@ CREATE TABLE context_files (
|
|||||||
file_path TEXT NOT NULL,
|
file_path TEXT NOT NULL,
|
||||||
content TEXT NOT NULL DEFAULT '',
|
content TEXT NOT NULL DEFAULT '',
|
||||||
version INTEGER NOT NULL DEFAULT 1,
|
version INTEGER NOT NULL DEFAULT 1,
|
||||||
updated_by TEXT REFERENCES users(user_id),
|
updated_by TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')),
|
updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')),
|
||||||
UNIQUE(project_id, file_path)
|
UNIQUE(project_id, file_path)
|
||||||
);
|
);
|
||||||
@@ -122,7 +122,7 @@ CREATE TABLE change_requests (
|
|||||||
request_id TEXT PRIMARY KEY,
|
request_id TEXT PRIMARY KEY,
|
||||||
workspace_id TEXT NOT NULL REFERENCES user_workspaces(workspace_id) ON DELETE CASCADE,
|
workspace_id TEXT NOT NULL REFERENCES user_workspaces(workspace_id) ON DELETE CASCADE,
|
||||||
project_id TEXT NOT NULL REFERENCES projects(project_id),
|
project_id TEXT NOT NULL REFERENCES projects(project_id),
|
||||||
submitted_by TEXT NOT NULL REFERENCES users(user_id),
|
submitted_by TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
status TEXT NOT NULL DEFAULT 'pending'
|
status TEXT NOT NULL DEFAULT 'pending'
|
||||||
CHECK (status IN ('pending', 'approved', 'rejected', 'merged')),
|
CHECK (status IN ('pending', 'approved', 'rejected', 'merged')),
|
||||||
diff_summary TEXT,
|
diff_summary TEXT,
|
||||||
@@ -138,7 +138,7 @@ CREATE TABLE change_requests (
|
|||||||
CREATE TABLE reviews (
|
CREATE TABLE reviews (
|
||||||
review_id INTEGER PRIMARY KEY AUTOINCREMENT,
|
review_id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||||
request_id TEXT NOT NULL REFERENCES change_requests(request_id) ON DELETE CASCADE,
|
request_id TEXT NOT NULL REFERENCES change_requests(request_id) ON DELETE CASCADE,
|
||||||
reviewer_id TEXT NOT NULL REFERENCES users(user_id),
|
reviewer_id TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
decision TEXT NOT NULL CHECK (decision IN ('approved', 'rejected')),
|
decision TEXT NOT NULL CHECK (decision IN ('approved', 'rejected')),
|
||||||
comments TEXT,
|
comments TEXT,
|
||||||
created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')),
|
created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')),
|
||||||
@@ -168,7 +168,7 @@ CREATE INDEX idx_snapshots_cleanup ON snapshots (project_id, user_id, created_at
|
|||||||
-- ============================================================================
|
-- ============================================================================
|
||||||
CREATE TABLE audit_log (
|
CREATE TABLE audit_log (
|
||||||
entry_id INTEGER PRIMARY KEY AUTOINCREMENT,
|
entry_id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||||
user_id TEXT NOT NULL REFERENCES users(user_id),
|
user_id TEXT REFERENCES users(user_id) ON DELETE SET NULL,
|
||||||
agent_id TEXT NOT NULL DEFAULT 'cli',
|
agent_id TEXT NOT NULL DEFAULT 'cli',
|
||||||
session_id TEXT,
|
session_id TEXT,
|
||||||
project_id TEXT REFERENCES projects(project_id),
|
project_id TEXT REFERENCES projects(project_id),
|
||||||
@@ -191,18 +191,11 @@ CREATE INDEX idx_audit_project ON audit_log (project_id, created_at);
|
|||||||
CREATE INDEX idx_audit_agent ON audit_log (agent_id, created_at);
|
CREATE INDEX idx_audit_agent ON audit_log (agent_id, created_at);
|
||||||
CREATE INDEX idx_audit_op ON audit_log (operation, created_at);
|
CREATE INDEX idx_audit_op ON audit_log (operation, created_at);
|
||||||
|
|
||||||
-- Trigger: audit_log is append-only
|
-- Note: audit_log append-only enforcement is handled at the application layer.
|
||||||
CREATE TRIGGER tr_audit_log_no_update
|
-- DB-level BEFORE UPDATE/DELETE triggers conflict with FK ON DELETE SET NULL
|
||||||
BEFORE UPDATE ON audit_log
|
-- (deleting a user must set audit_log.user_id to NULL, which is an UPDATE).
|
||||||
BEGIN
|
-- This mirrors schema.sql, which dropped the equivalent PG triggers for the
|
||||||
SELECT RAISE(ABORT, 'audit_log is append-only — no updates allowed');
|
-- same reason. db.py only ever INSERTs into audit_log.
|
||||||
END;
|
|
||||||
|
|
||||||
CREATE TRIGGER tr_audit_log_no_delete
|
|
||||||
BEFORE DELETE ON audit_log
|
|
||||||
BEGIN
|
|
||||||
SELECT RAISE(ABORT, 'audit_log is append-only — no deletes allowed');
|
|
||||||
END;
|
|
||||||
|
|
||||||
-- ============================================================================
|
-- ============================================================================
|
||||||
-- FULL-TEXT SEARCH (FTS5)
|
-- FULL-TEXT SEARCH (FTS5)
|
||||||
|
|||||||
@@ -821,8 +821,11 @@ class HTTPServer:
|
|||||||
except Exception:
|
except Exception:
|
||||||
logger.exception("Rollback failed after request error")
|
logger.exception("Rollback failed after request error")
|
||||||
if _db.is_integrity_error(e):
|
if _db.is_integrity_error(e):
|
||||||
|
# Don't leak raw psycopg text (index names, column expressions)
|
||||||
|
# to API callers; log it for operators instead.
|
||||||
|
logger.warning("Integrity error on %s %s: %s", method, path, e)
|
||||||
return (409, {"Content-Type": "application/json"},
|
return (409, {"Content-Type": "application/json"},
|
||||||
json.dumps({"error": "conflict", "detail": str(e)}))
|
json.dumps({"error": "conflict"}))
|
||||||
logger.exception("HTTP error")
|
logger.exception("HTTP error")
|
||||||
return (500, {"Content-Type": "text/plain"}, f"Internal error: {e}")
|
return (500, {"Content-Type": "text/plain"}, f"Internal error: {e}")
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user