From 14721e96582d4752d32fa759bab86e6341e07845 Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 00:56:52 +0200 Subject: [PATCH] =?UTF-8?q?feat(server):=20plan=203i=20groundwork=20?= =?UTF-8?q?=E2=80=94=20AuditLogRepository's=20reserved=20methods,=20the=20?= =?UTF-8?q?demo=20connection-swap=20spike,=20repository-fitness=20confirma?= =?UTF-8?q?tions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../db/repositories/AuditLog.repository.ts | 68 ++++++- .../demo-reset-connection-swap.spike.test.ts | 170 ++++++++++++++++++ .../repositories/AuditLog.repository.test.ts | 59 ++++++ 3 files changed, 293 insertions(+), 4 deletions(-) create mode 100644 server/tests/integration/demo-reset-connection-swap.spike.test.ts diff --git a/server/src/db/repositories/AuditLog.repository.ts b/server/src/db/repositories/AuditLog.repository.ts index 5a31b4bb8..2ed85642c 100644 --- a/server/src/db/repositories/AuditLog.repository.ts +++ b/server/src/db/repositories/AuditLog.repository.ts @@ -22,10 +22,36 @@ const _auditLogRowKeys: AssertRowKeys = true; /** The column set `insertEntry` writes; `id`/`created_at` are generated. */ export type NewAuditLogRow = Omit; -// Reserved for Plan 3i (`admin.service.ts`'s paginated read of the same -// table): `listPage(limit, offset): Promise` (a `qb()` -// join to `Users` for `username`/`email`) and `count(): Promise`. -// Not implemented here — see plan3a-sql-inventory.md §1. +/** + * AD22's joined projection (`admin.service.ts#getAuditLog`): `a.id, + * a.created_at, a.user_id, u.username, u.email as user_email, a.action, + * a.resource, a.details, a.ip` from `audit_log a LEFT JOIN users u ON + * u.id = a.user_id`. Distinct from `AuditLogRow` (the bare-table shape + * `insertEntry` writes) — a deleted/never-existing user leaves `username`/ + * `user_email` both null (the `LEFT JOIN`, not an `INNER JOIN`), the row + * itself is still returned. The `created_at` ISO-suffix normalization and + * the `details` JSON-parse-with-fallback stay in `AdminService` — this + * repository returns the raw joined row untouched, same boundary the + * class-level docs elsewhere in this program draw between repository and + * service. + */ +export interface AuditLogPageRow { + id: number; + created_at: string | null; + user_id: number | null; + username: string | null; + user_email: string | null; + action: string; + resource: string | null; + details: string | null; + ip: string | null; +} + +/** Kysely shape for the `listPage` join — only the columns it actually selects. */ +interface AuditLogKyselyDB { + audit_log: AuditLogRow; + users: { id: number; username: string; email: string }; +} export class AuditLogRepository extends TrekRepository { /** @@ -65,4 +91,38 @@ export class AuditLogRepository extends TrekRepository { ip: entry.ip, }); } + + /** + * AD22 (`admin.service.ts#getAuditLog`) — `SELECT a.id, a.created_at, + * a.user_id, u.username, u.email as user_email, a.action, a.resource, + * a.details, a.ip FROM audit_log a LEFT JOIN users u ON u.id = a.user_id + * ORDER BY a.id DESC LIMIT ? OFFSET ?`. Kysely, not the ORM's own + * QueryBuilder — the same `CollabMessagesRepository#joinedQuery` shape + * every other flat-projection join in this program uses: `LEFT JOIN` + * keeps a deleted-user's row (both `username`/`user_email` come back + * null rather than dropping the row), and the projection is nine flat + * scalars, not a hydrated `Users` entity. + */ + async listPage(limit: number, offset: number): Promise { + return await this.kysely() + .selectFrom('audit_log as a') + .leftJoin('users as u', 'u.id', 'a.user_id') + .select(['a.id', 'a.created_at', 'a.user_id', 'u.username', 'u.email as user_email', 'a.action', 'a.resource', 'a.details', 'a.ip']) + .orderBy('a.id', 'desc') + .limit(limit) + .offset(offset) + .execute(); + } + + /** + * AD23 (`admin.service.ts#getAuditLog`) — `SELECT COUNT(*) as c FROM + * audit_log`. `TrekRepository#count` (no filter) already renders the + * equivalent `COUNT(*)` over the whole table; named explicitly here (a + * thin `super.count()` pass-through) so the reserved AD23 call site has + * its own documented anchor rather than admin's service code calling the + * inherited method by its generic name. + */ + override async count(): Promise { + return await super.count(); + } } diff --git a/server/tests/integration/demo-reset-connection-swap.spike.test.ts b/server/tests/integration/demo-reset-connection-swap.spike.test.ts new file mode 100644 index 000000000..61ab5c4d1 --- /dev/null +++ b/server/tests/integration/demo-reset-connection-swap.spike.test.ts @@ -0,0 +1,170 @@ +/** + * Plan 3i Task 0 — R3's spike. + * + * `demo-reset.ts#resetDemoUser` runs inside a cron tick that + * `CronRegistrarService.register()` already wraps in a `withRequestContext` + * fork (`cron-registrar.service.ts:87-92`, `wrappedTick`). INSIDE that one + * fork, the function reads credentials, calls `closeDb()` then + * `await reinitialize()` (which closes the process's one better-sqlite3 + * handle, opens a new one, and — via `orm.ts#attachOrm`'s + * `registerReinitializeHook` — closes and reconnects the ORM's own + * Kysely-bound `Connection`, then re-runs `runSchemaBootstrap`), and only + * THEN writes the preserved credentials back through a freshly re-required + * `db` reference. + * + * The open question (plan3i-inputs.md correction #2): does a REPOSITORY + * call issued through the SAME `EntityManager` fork the tick started with — + * i.e. still inside the one `withRequestContext(orm, async () => { ... })` + * call — still resolve correctly against the driver's connection after that + * connection object has been closed and rebound to a new raw handle + * mid-callback? This file measures it against a real `buildApp()` boot, not + * a mock, exactly as `demo-seed-request-context.test.ts` (its closest + * neighbour) measures `runDemoSeed`'s own, simpler wrap. + * + * Mechanism (for the report, not asserted directly here — this is the + * reasoning the measurement below either confirms or refutes): MikroORM + * forks an `EntityManager` per `RequestContext.create` call, but a fork does + * NOT fork the `Connection`/`Driver` — those are singletons owned by the ORM + * instance's `Configuration`, shared by every fork. `orm-driver.ts`'s + * `BoundSqliteConnection.createKyselyDialect()` reads `getRawConnection()` + * (the CURRENT `db/database.ts` handle) on every `connect()`, and + * `attachOrm`'s hook calls `connection.close(true)` then + * `connection.connect()` on that ONE shared `Connection` object — it never + * replaces the object itself, only rebinds its internal Kysely client. So + * any EM fork — pre-swap or fresh — that resolves a query calls + * `this.driver.getConnection()` fresh each time, landing on the SAME, + * now-rebound `Connection` object. The theoretical expectation is "safe"; + * this file is what turns that into a measured answer with real evidence + * rather than an assumption. + * + * Test-mode nuance: `NODE_ENV=test` makes `db/database.ts#initDb()` reopen a + * PRISTINE copy of the migrated schema snapshot on every call (a fresh + * `better-sqlite3` instance deserialised from a fixed buffer), not the same + * on-disk file demo-reset.ts restores in production — so a row inserted + * before the swap is genuinely gone after `reinitialize()` resolves. That is + * expected data loss (the same thing a real demo-reset baseline restore + * does) and is NOT what this file is measuring; what it measures is whether + * the post-swap call reaches the new, live connection at all (succeeds, + * even returning null/creating a fresh row) versus throwing, hanging, or + * silently targeting a stale/closed handle. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import type { INestApplication } from '@nestjs/common'; +import { MikroORM } from '@mikro-orm/core'; +import { getRepositoryToken } from '@mikro-orm/nestjs'; +import { buildApp } from '../../src/bootstrap'; +import { withRequestContext } from '../../src/nest/database/request-context'; +import { closeDb, reinitialize } from '../../src/db/database'; +import { Users } from '../../src/db/entities/Users.entity'; +import type { UsersRepository } from '../../src/db/repositories/Users.repository'; + +type Outcome = { ok: true; value: unknown } | { ok: false; error: string }; + +async function capture(fn: () => Promise): Promise { + try { + return { ok: true, value: await fn() }; + } catch (e) { + return { ok: false, error: e instanceof Error ? e.message : String(e) }; + } +} + +describe('Plan 3i Task 0 spike: demo-reset connection swap', () => { + let app: INestApplication | undefined; + + afterEach(async () => { + await app?.close(); + app = undefined; + }); + + it('SPIKE-001: a repository read AND a repository write, issued through the pre-swap EntityManager fork after closeDb()+reinitialize(), inside the SAME withRequestContext call demo-reset.job.ts\'s wrappedTick uses', async () => { + app = await buildApp(); + const orm = app.get(MikroORM); + const users = app.get(getRepositoryToken(Users)); + + let preSwapId: number | undefined; + let readAfterSwap: Outcome | undefined; + let writeAfterSwap: Outcome | undefined; + + // Exactly the shape of cron-registrar.service.ts's wrappedTick: one + // withRequestContext call wrapping the whole tick, read -> swap -> write. + await withRequestContext(orm, async () => { + // Phase 1 — pre-close, proving the fork works before the swap at all + // (mirrors demo-reset.ts's DMR1/DMR2 credential reads). + const inserted = await users.insertUser({ + username: 'spike-pre-swap', + email: 'spike-pre-swap@example.test', + password_hash: 'x', + role: 'user', + first_seen_version: '0.0.0-spike', + }); + preSwapId = inserted.id; + const before = await users.findById(preSwapId); + expect(before?.email).toBe('spike-pre-swap@example.test'); + + // The swap — demo-reset.ts's own `closeDb(); await reinitialize();`, + // called from inside the still-open fork, exactly where resetDemoUser + // calls it between its pre-close reads and its post-reopen writes. + closeDb(); + await reinitialize(); + + // Phase 2 — SAME fork, AFTER the swap. A read first (mirrors nothing + // demo-reset.ts does post-swap, but isolates read-only behaviour), + // then a write (mirrors DMR5/DMR6, the credential/API-key restore). + readAfterSwap = await capture(() => users.findById(preSwapId as number)); + writeAfterSwap = await capture(() => + users.insertUser({ + username: 'spike-post-swap', + email: 'spike-post-swap@example.test', + password_hash: 'x', + role: 'user', + first_seen_version: '0.0.0-spike', + }), + ); + }); + + console.log('[SPIKE-001] readAfterSwap:', JSON.stringify(readAfterSwap)); + console.log('[SPIKE-001] writeAfterSwap:', JSON.stringify(writeAfterSwap)); + + // The pre-swap fork's post-swap READ must not throw (test-mode reopens a + // pristine snapshot, so the pre-swap row is gone — a defined `null` is + // the CORRECT answer here, not a failure). + expect(readAfterSwap?.ok).toBe(true); + if (readAfterSwap?.ok) expect(readAfterSwap.value).toBeNull(); + + // The pre-swap fork's post-swap WRITE must not throw either. + expect(writeAfterSwap?.ok).toBe(true); + + // The decisive check: did that write actually land on the NEW, live + // connection, or silently no-op / target a stale handle? Verify through + // a completely FRESH request context (a new fork), reading the row back + // by a value only the post-swap write could have created. + let writeVisibleOnNewConnection = false; + await withRequestContext(orm, async () => { + const id = await users.findIdByEmailOrUsernameCI('spike-post-swap@example.test', 'spike-post-swap'); + writeVisibleOnNewConnection = id !== null; + }); + console.log('[SPIKE-001] writeVisibleOnNewConnection:', writeVisibleOnNewConnection); + expect(writeVisibleOnNewConnection).toBe(true); + }); + + it('SPIKE-002: a FRESH withRequestContext opened AFTER reinitialize() resolves works correctly for a post-swap write (R3\'s fallback-shape requirement)', async () => { + app = await buildApp(); + const orm = app.get(MikroORM); + const users = app.get(getRepositoryToken(Users)); + + closeDb(); + await reinitialize(); + + await withRequestContext(orm, async () => { + const inserted = await users.insertUser({ + username: 'spike-fresh-fork', + email: 'spike-fresh-fork@example.test', + password_hash: 'x', + role: 'user', + first_seen_version: '0.0.0-spike', + }); + const row = await users.findById(inserted.id); + expect(row?.email).toBe('spike-fresh-fork@example.test'); + }); + }); +}); diff --git a/server/tests/unit/db/repositories/AuditLog.repository.test.ts b/server/tests/unit/db/repositories/AuditLog.repository.test.ts index e0640d5cb..47355e755 100644 --- a/server/tests/unit/db/repositories/AuditLog.repository.test.ts +++ b/server/tests/unit/db/repositories/AuditLog.repository.test.ts @@ -75,4 +75,63 @@ describe('AuditLogRepository', () => { const auditRow = testDb.prepare('SELECT action FROM audit_log').get() as { action: string }; expect(auditRow.action).toBe('trip.create'); }); + + // ── Plan 3i Task 0 — listPage/count (AD22/AD23) ──────────────────────────── + + /** Byte-for-byte AD22 (`admin.service.ts#getAuditLog`). */ + const LEGACY_PAGE_SQL = ` + SELECT a.id, a.created_at, a.user_id, u.username, u.email as user_email, a.action, a.resource, a.details, a.ip + FROM audit_log a + LEFT JOIN users u ON u.id = a.user_id + ORDER BY a.id DESC + LIMIT ? OFFSET ? + `; + + it('AUDITREPO-007: listPage matches the legacy joined SELECT exactly for a known user, a deleted (FK SET NULL) user, and a null user_id row', async () => { + const { user: alice } = createUser(testDb, { username: 'alice', email: 'alice@test.example.com' }); + const { user: bob } = createUser(testDb, { username: 'bob', email: 'bob@test.example.com' }); + + await auditLog.insertEntry({ user_id: alice.id, action: 'trip.create', resource: 'trip', details: '{"title":"Rome"}', ip: '1.1.1.1' }); + await auditLog.insertEntry({ user_id: bob.id, action: 'trip.update', resource: 'trip', details: null, ip: '2.2.2.2' }); + await auditLog.insertEntry({ user_id: null, action: 'user.login', resource: null, details: null, ip: null }); + + // bob is deleted after the fact; `audit_log.user_id REFERENCES users(id) + // ON DELETE SET NULL` fires — this row's own user_id becomes NULL, the + // same shape a NULL-user_id row has, reached a different way. + testDb.prepare('DELETE FROM users WHERE id = ?').run(bob.id); + + const legacy = testDb.prepare(LEGACY_PAGE_SQL).all(10, 0); + const result = await auditLog.listPage(10, 0); + expect(result).toEqual(legacy); + // The deleted user's row and the originally-null row both keep their + // place — a LEFT JOIN never drops the row. + expect(result).toHaveLength(3); + expect(result.filter((r) => r.username === null && r.user_email === null)).toHaveLength(2); + // The known user's row still carries its username/email. + const aliceRow = result.find((r) => r.user_id === alice.id); + expect(aliceRow?.username).toBe('alice'); + expect(aliceRow?.user_email).toBe('alice@test.example.com'); + }); + + it('AUDITREPO-008: listPage honors LIMIT/OFFSET and ORDER BY a.id DESC, matching the legacy statement', async () => { + const { user } = createUser(testDb); + for (let i = 0; i < 5; i++) { + await auditLog.insertEntry({ user_id: user.id, action: `action.${i}`, resource: null, details: null, ip: null }); + } + const legacyPage = testDb.prepare(LEGACY_PAGE_SQL).all(2, 1); + const result = await auditLog.listPage(2, 1); + expect(result).toEqual(legacyPage); + expect(result).toHaveLength(2); + }); + + it('AUDITREPO-009: count matches the legacy COUNT(*) over the whole table', async () => { + const { user } = createUser(testDb); + await auditLog.insertEntry({ user_id: user.id, action: 'a', resource: null, details: null, ip: null }); + await auditLog.insertEntry({ user_id: null, action: 'b', resource: null, details: null, ip: null }); + + const legacyCount = (testDb.prepare('SELECT COUNT(*) as c FROM audit_log').get() as { c: number }).c; + const result = await auditLog.count(); + expect(result).toBe(legacyCount); + expect(result).toBe(2); + }); });