mirror of
https://github.com/screentinker/screentinker.git
synced 2026-06-14 18:22:46 -06:00
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.
This commit is contained in:
ScreenTinker
parent
ec44cb785a
commit
66c95bb331
|
|
@ -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(`
|
||||
|
|
|
|||
103
server/lib/tenant-cascade-migration.js
Normal file
103
server/lib/tenant-cascade-migration.js
Normal file
|
|
@ -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 };
|
||||
98
server/test/tenant-cascade-migration.test.js
Normal file
98
server/test/tenant-cascade-migration.test.js
Normal file
|
|
@ -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();
|
||||
});
|
||||
Loading…
Reference in a new issue