From 66c95bb331e1dd49878845aef097e8bf31714e64 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Mon, 8 Jun 2026 16:01:52 -0500 Subject: [PATCH] fix(db): cascade tenant resources on workspace/org delete (#18 follow-up) The #18 user-delete bug was the first symptom of a broader gap: 13 tables reference workspaces(id) (and activity_log also organizations(id)) with NO ACTION, so deleting a workspace or organization fails the same FK wall once it holds any content. SQLite can't ALTER an FK action, so this migration rebuilds each table (the create-copy-rename pattern the assignments/schedules migrations already use), changing only the tenant FK clause: workspace_id -> ON DELETE CASCADE (resources belong to the workspace) activity_log.workspace_id / organization_id -> ON DELETE SET NULL (keep audit) user_id FKs are intentionally left as-is - user deletion stays handled app-side by lib/user-deletion.js (the #18 fix). - lib/tenant-cascade-migration.js: pure, idempotent core (table-existence guarded; transforms the stored CREATE text, copies rows verbatim, recreates indexes; fixes activity_log's AUTOINCREMENT sequence; baseline-vs-after foreign_key_check so pre-existing orphan rows don't abort it but a botched rebuild does). - db/database.js: boot wrapper owns the pre-migration snapshot + process.exit on failure, matching the other heavy migrations. Tests (node:test): reproduces the workspace-delete FK failure, applies the migration, verifies FK actions (CASCADE / SET NULL), index recreation, data preserved, and that workspace/org delete now cascades (activity_log preserved). Full suite 27/27. Verified on a copy of a real DB: 13 tables rebuilt, integrity_check ok, workspace delete cascades, no new FK violations. --- server/db/database.js | 38 +++++++ server/lib/tenant-cascade-migration.js | 103 +++++++++++++++++++ server/test/tenant-cascade-migration.test.js | 98 ++++++++++++++++++ 3 files changed, 239 insertions(+) create mode 100644 server/lib/tenant-cascade-migration.js create mode 100644 server/test/tenant-cascade-migration.test.js diff --git a/server/db/database.js b/server/db/database.js index 852c296..d8a7448 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -628,6 +628,44 @@ function backfillPlaylistItemsZoneId() { backfillPlaylistItemsZoneId(); +// Tenant delete-cascade (issue #18 follow-up). Core logic + table list live in +// lib/tenant-cascade-migration.js (so they're unit-testable against an in-memory +// DB). Here we own the boot concerns: a pre-migration snapshot for rollback and +// process.exit on failure, matching the other heavy migrations above. +const { applyTenantDeleteCascade } = require('../lib/tenant-cascade-migration'); +(function migrateTenantDeleteCascadeAtBoot() { + // Cheap guard so we don't snapshot on every boot once applied. + try { + if (db.prepare("SELECT 1 FROM schema_migrations WHERE id = 'phase2_3_tenant_delete_cascade'").get()) return; + } catch { /* schema_migrations may not exist yet */ } + + const ts = new Date().toISOString().replace(/[:.]/g, '-'); + const snapshotPath = path.join(dbDir, `remote_display.pre-tenant-cascade-${ts}.db`); + let snapped = false; + try { + db.pragma('wal_checkpoint(TRUNCATE)'); + fs.copyFileSync(config.dbPath, snapshotPath); + snapped = true; + } catch (e) { + console.error(`[tenant-cascade] Snapshot failed: ${e.message}`); + process.exit(1); + } + + try { + const result = applyTenantDeleteCascade(db); + if (result.status === 'applied') { + console.warn(`[tenant-cascade] workspace/org deletion now cascades (${result.tables.length} tables rebuilt). Snapshot: ${snapshotPath}`); + } else if (snapped) { + // Nothing to do (already applied / no tenancy tables) - drop the snapshot. + try { fs.unlinkSync(snapshotPath); } catch { /* ignore */ } + } + } catch (e) { + console.error(`[tenant-cascade] Migration FAILED: ${e.message}`); + console.error(`[tenant-cascade] Restore with: cp ${snapshotPath} ${config.dbPath}`); + process.exit(1); + } +})(); + // Prune old telemetry (keep last 24h worth at 15s intervals = ~5760, cap at 6000) function pruneTelemetry(deviceId) { db.prepare(` diff --git a/server/lib/tenant-cascade-migration.js b/server/lib/tenant-cascade-migration.js new file mode 100644 index 0000000..f89a6da --- /dev/null +++ b/server/lib/tenant-cascade-migration.js @@ -0,0 +1,103 @@ +'use strict'; + +// Issue #18 follow-up: workspace/org deletion hits the same FK wall the +// user-delete bug did - 13 tables reference workspaces(id) (and activity_log +// also organizations(id)) with NO ACTION. SQLite can't ALTER an FK action, so +// we rebuild each table (create-copy-rename, the pattern the assignments/ +// schedules migrations already use) changing only the tenant FK clause: +// workspace_id -> ON DELETE CASCADE (resources belong to the workspace) +// activity_log.workspace_id / organization_id -> ON DELETE SET NULL (keep audit) +// user_id FKs are intentionally left as-is (user delete is handled app-side by +// lib/user-deletion.js). +// +// Pure/testable: takes a better-sqlite3 db, records itself in schema_migrations, +// idempotent, and does NOT snapshot or exit (the boot caller in db/database.js +// owns the pre-migration snapshot + process.exit-on-failure). + +const MIGRATION_ID = 'phase2_3_tenant_delete_cascade'; + +const WS_CASCADE_TABLES = [ + 'devices', 'content', 'layouts', 'widgets', 'video_walls', 'device_groups', + 'alert_configs', 'white_labels', 'kiosk_pages', 'playlists', 'schedules', 'content_folders', +]; + +function fkOnDeleteAction(db, table, refTable) { + const fk = db.prepare(`PRAGMA foreign_key_list(${table})`).all().find(f => f.table === refTable); + return fk ? fk.on_delete : null; +} + +// Rebuild `table`, changing the ON DELETE action of its FK(s) to the given ref +// table(s). Preserves every column/constraint by transforming the stored CREATE +// text and copying rows verbatim; recreates the table's (non-auto) indexes. +function rebuildTableFkActions(db, table, actions, opts = {}) { + const tmp = `${table}_fkmig_new`; + let sql = db.prepare("SELECT sql FROM sqlite_master WHERE type='table' AND name=?").get(table).sql; + for (const [ref, action] of Object.entries(actions)) { + const re = new RegExp(`REFERENCES\\s+${ref}\\s*\\(\\s*id\\s*\\)(?!\\s+ON\\s+DELETE)`, 'gi'); + sql = sql.replace(re, (m) => `${m} ON DELETE ${action}`); + } + // Rename only the leading `CREATE TABLE [IF NOT EXISTS] ["]table["]` token. + sql = sql.replace(new RegExp(`^CREATE TABLE\\s+(IF NOT EXISTS\\s+)?("?)${table}\\2`, 'i'), `CREATE TABLE "${tmp}"`); + + const indexes = db.prepare("SELECT sql FROM sqlite_master WHERE type='index' AND tbl_name=? AND sql IS NOT NULL").all(table).map(r => r.sql); + db.exec(sql); + db.exec(`INSERT INTO "${tmp}" SELECT * FROM "${table}"`); + db.exec(`DROP TABLE "${table}"`); + db.exec(`ALTER TABLE "${tmp}" RENAME TO "${table}"`); + for (const idx of indexes) db.exec(idx); + // Keep AUTOINCREMENT high-water marks monotonic across the rename (activity_log). + if (opts.autoincrement) { + db.exec(`DELETE FROM sqlite_sequence WHERE name IN ('${table}', '${tmp}')`); + db.exec(`INSERT INTO sqlite_sequence(name, seq) VALUES ('${table}', (SELECT COALESCE(MAX(rowid),0) FROM "${table}"))`); + } +} + +// Returns { status: 'already' | 'no-workspaces' | 'applied', tables?: [...] }. +// Throws (after ROLLBACK) if a rebuild fails; the caller restores from snapshot. +function applyTenantDeleteCascade(db) { + if (db.prepare('SELECT 1 FROM schema_migrations WHERE id = ?').get(MIGRATION_ID)) return { status: 'already' }; + + const have = new Set(db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all().map(r => r.name)); + if (!have.has('workspaces')) { + db.prepare('INSERT OR IGNORE INTO schema_migrations (id) VALUES (?)').run(MIGRATION_ID); + return { status: 'no-workspaces' }; + } + // Idempotency: devices.workspace_id already cascading => treat as applied. + if (have.has('devices') && fkOnDeleteAction(db, 'devices', 'workspaces') === 'CASCADE') { + db.prepare('INSERT OR IGNORE INTO schema_migrations (id) VALUES (?)').run(MIGRATION_ID); + return { status: 'already' }; + } + + const baselineViolations = db.prepare('PRAGMA foreign_key_check').all().length; + const rebuilt = []; + + // foreign_keys must be toggled OUTSIDE a transaction in SQLite. + db.pragma('foreign_keys = OFF'); + try { + db.exec('BEGIN'); + for (const t of WS_CASCADE_TABLES) { + if (!have.has(t)) continue; // partial schema / older DB + if (fkOnDeleteAction(db, t, 'workspaces') === 'CASCADE') continue; // partial re-run safety + rebuildTableFkActions(db, t, { workspaces: 'CASCADE' }); + rebuilt.push(t); + } + if (have.has('activity_log') && fkOnDeleteAction(db, 'activity_log', 'workspaces') !== 'SET NULL') { + rebuildTableFkActions(db, 'activity_log', { workspaces: 'SET NULL', organizations: 'SET NULL' }, { autoincrement: true }); + rebuilt.push('activity_log'); + } + // Rows are copied verbatim, so a rebuild cannot introduce NEW violations; + // abort only if the count grew (catches a botched CREATE transform). + const after = db.prepare('PRAGMA foreign_key_check').all().length; + if (after > baselineViolations) throw new Error(`foreign_key_check violations increased ${baselineViolations} -> ${after}`); + db.prepare('INSERT OR IGNORE INTO schema_migrations (id) VALUES (?)').run(MIGRATION_ID); + db.exec('COMMIT'); + } catch (e) { + try { db.exec('ROLLBACK'); } catch { /* ignore */ } + throw e; + } finally { + db.pragma('foreign_keys = ON'); + } + return { status: 'applied', tables: rebuilt }; +} + +module.exports = { applyTenantDeleteCascade, MIGRATION_ID, WS_CASCADE_TABLES }; diff --git a/server/test/tenant-cascade-migration.test.js b/server/test/tenant-cascade-migration.test.js new file mode 100644 index 0000000..08b6037 --- /dev/null +++ b/server/test/tenant-cascade-migration.test.js @@ -0,0 +1,98 @@ +'use strict'; + +// Issue #18 follow-up: workspace/org deletion must cascade to tenant resources. +// This suite reproduces the FK wall (workspace_id NO ACTION), applies the +// migration, and verifies the rebuilt FKs cascade (and activity_log SET NULLs to +// preserve audit). Pure-function migration tested against an in-memory DB. + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const Database = require('better-sqlite3'); +const { applyTenantDeleteCascade } = require('../lib/tenant-cascade-migration'); + +function freshDb() { + const db = new Database(':memory:'); + db.pragma('foreign_keys = ON'); + // Prod-shaped subset: workspace_id FKs are NO ACTION, like the bug. Includes + // CASCADE child tables (telemetry, playlist_items) to prove deep cascade and a + // workspace index to prove index recreation. activity_log is AUTOINCREMENT. + db.exec(` + CREATE TABLE schema_migrations (id TEXT PRIMARY KEY, applied_at INTEGER DEFAULT (strftime('%s','now'))); + CREATE TABLE organizations (id TEXT PRIMARY KEY, name TEXT); + CREATE TABLE workspaces (id TEXT PRIMARY KEY, organization_id TEXT NOT NULL REFERENCES organizations(id) ON DELETE CASCADE, name TEXT); + CREATE TABLE devices (id TEXT PRIMARY KEY, name TEXT, workspace_id TEXT REFERENCES workspaces(id)); + CREATE TABLE device_telemetry (id INTEGER PRIMARY KEY AUTOINCREMENT, device_id TEXT REFERENCES devices(id) ON DELETE CASCADE, val TEXT); + CREATE TABLE content (id TEXT PRIMARY KEY, workspace_id TEXT REFERENCES workspaces(id)); + CREATE TABLE playlists (id TEXT PRIMARY KEY, workspace_id TEXT REFERENCES workspaces(id)); + CREATE INDEX idx_playlists_workspace ON playlists(workspace_id); + CREATE TABLE playlist_items (id INTEGER PRIMARY KEY AUTOINCREMENT, playlist_id TEXT REFERENCES playlists(id) ON DELETE CASCADE); + CREATE TABLE activity_log (id INTEGER PRIMARY KEY AUTOINCREMENT, action TEXT, + workspace_id TEXT REFERENCES workspaces(id), organization_id TEXT REFERENCES organizations(id)); + `); + db.exec(` + INSERT INTO organizations (id,name) VALUES ('o1','Org 1'); + INSERT INTO workspaces (id,organization_id,name) VALUES ('w1','o1','WS1'),('w2','o1','WS2'); + INSERT INTO devices (id,workspace_id) VALUES ('d1','w1'); + INSERT INTO device_telemetry (device_id,val) VALUES ('d1','x'); + INSERT INTO content (id,workspace_id) VALUES ('c1','w1'); + INSERT INTO playlists (id,workspace_id) VALUES ('p1','w1'),('p2','w2'); + INSERT INTO playlist_items (playlist_id) VALUES ('p1'); + INSERT INTO activity_log (action,workspace_id,organization_id) VALUES ('event','w1','o1'); + `); + return db; +} +const onDelete = (db, t, ref) => (db.prepare(`PRAGMA foreign_key_list(${t})`).all().find(f => f.table === ref) || {}).on_delete; + +test('reproduces the gap: deleting a workspace fails the FK before migration', () => { + const db = freshDb(); + assert.throws(() => db.prepare("DELETE FROM workspaces WHERE id='w1'").run(), /FOREIGN KEY constraint failed/); + db.close(); +}); + +test('migration rewrites the FK actions (CASCADE for tenant tables, SET NULL for activity_log) and keeps indexes', () => { + const db = freshDb(); + const res = applyTenantDeleteCascade(db); + assert.equal(res.status, 'applied'); + for (const t of ['devices', 'content', 'playlists']) assert.equal(onDelete(db, t, 'workspaces'), 'CASCADE', `${t}.workspace_id`); + assert.equal(onDelete(db, 'activity_log', 'workspaces'), 'SET NULL'); + assert.equal(onDelete(db, 'activity_log', 'organizations'), 'SET NULL'); + // index recreated, data intact, foreign_keys restored to ON + assert.ok(db.prepare("SELECT 1 FROM sqlite_master WHERE type='index' AND name='idx_playlists_workspace'").get(), 'index preserved'); + assert.equal(db.prepare('SELECT COUNT(*) c FROM playlists').get().c, 2, 'rows preserved'); + assert.equal(db.pragma('foreign_keys', { simple: true }), 1, 'foreign_keys ON again'); + assert.equal(db.prepare('PRAGMA foreign_key_check').all().length, 0, 'no FK violations'); + db.close(); +}); + +test('after migration: deleting a workspace cascades resources + children; activity_log preserved (SET NULL)', () => { + const db = freshDb(); + applyTenantDeleteCascade(db); + db.prepare("DELETE FROM workspaces WHERE id='w1'").run(); + assert.equal(db.prepare("SELECT COUNT(*) c FROM devices WHERE workspace_id='w1'").get().c, 0, 'devices gone'); + assert.equal(db.prepare("SELECT COUNT(*) c FROM device_telemetry").get().c, 0, 'telemetry cascaded via device'); + assert.equal(db.prepare("SELECT COUNT(*) c FROM content WHERE workspace_id='w1'").get().c, 0, 'content gone'); + assert.equal(db.prepare("SELECT COUNT(*) c FROM playlists WHERE workspace_id='w1'").get().c, 0, 'playlists gone'); + assert.equal(db.prepare("SELECT COUNT(*) c FROM playlist_items").get().c, 0, 'playlist_items cascaded'); + // activity_log row survives, unlinked from the deleted workspace + const a = db.prepare("SELECT workspace_id, organization_id FROM activity_log WHERE action='event'").get(); + assert.equal(a.workspace_id, null, 'activity workspace_id SET NULL'); + assert.equal(a.organization_id, 'o1', 'org link intact (org not deleted)'); + db.close(); +}); + +test('after migration: deleting an organization cascades through its workspaces', () => { + const db = freshDb(); + applyTenantDeleteCascade(db); + db.prepare("DELETE FROM organizations WHERE id='o1'").run(); + assert.equal(db.prepare('SELECT COUNT(*) c FROM workspaces').get().c, 0, 'workspaces cascaded'); + assert.equal(db.prepare('SELECT COUNT(*) c FROM playlists').get().c, 0, 'tenant resources cascaded'); + assert.equal(db.prepare("SELECT organization_id FROM activity_log WHERE action='event'").get().organization_id, null, 'activity org SET NULL'); + db.close(); +}); + +test('idempotent: a second run is a no-op', () => { + const db = freshDb(); + assert.equal(applyTenantDeleteCascade(db).status, 'applied'); + assert.equal(applyTenantDeleteCascade(db).status, 'already'); + db.close(); +});