From 565f72740bf9800c95ccd6377883ea11f6517964 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 21 Sep 2026 09:17:17 +0200 Subject: [PATCH] test(db): createTestOrm binds an ORM to a suite's own SQLite handle --- package-lock.json | 2 + server/package.json | 2 + server/src/db/orm-driver.ts | 97 ++++++++++++++++++--------- server/tests/helpers/test-orm.ts | 59 ++++++++++++++++ server/tests/unit/db/test-orm.test.ts | 38 +++++++++++ 5 files changed, 168 insertions(+), 30 deletions(-) create mode 100644 server/tests/helpers/test-orm.ts create mode 100644 server/tests/unit/db/test-orm.test.ts diff --git a/package-lock.json b/package-lock.json index 2e250c756..523add7ae 100644 --- a/package-lock.json +++ b/package-lock.json @@ -22652,6 +22652,7 @@ "@mikro-orm/nestjs": "^7.1.0", "@mikro-orm/postgresql": "^7.2.1", "@mikro-orm/seeder": "^7.2.1", + "@mikro-orm/sql": "^7.2.1", "@modelcontextprotocol/sdk": "^1.30.0", "@nestjs/common": "^11.2.5", "@nestjs/config": "^4.0.4", @@ -22678,6 +22679,7 @@ "jimp": "^1.6.1", "json5": "^2.2.3", "jsonwebtoken": "^9.0.3", + "kysely": "^0.29.6", "nestjs-zod": "^5.5.0", "nodemailer": "^9.1.1", "otplib": "^12.0.1", diff --git a/server/package.json b/server/package.json index 274a7e012..3219ffe0f 100644 --- a/server/package.json +++ b/server/package.json @@ -38,6 +38,7 @@ "@mikro-orm/nestjs": "^7.1.0", "@mikro-orm/postgresql": "^7.2.1", "@mikro-orm/seeder": "^7.2.1", + "@mikro-orm/sql": "^7.2.1", "@modelcontextprotocol/sdk": "^1.30.0", "@nestjs/common": "^11.2.5", "@nestjs/config": "^4.0.4", @@ -64,6 +65,7 @@ "jimp": "^1.6.1", "json5": "^2.2.3", "jsonwebtoken": "^9.0.3", + "kysely": "^0.29.6", "nestjs-zod": "^5.5.0", "nodemailer": "^9.1.1", "otplib": "^12.0.1", diff --git a/server/src/db/orm-driver.ts b/server/src/db/orm-driver.ts index a1ebf729f..63288cab5 100644 --- a/server/src/db/orm-driver.ts +++ b/server/src/db/orm-driver.ts @@ -2,41 +2,78 @@ import { getRawConnection } from './database'; import type { Configuration } from '@mikro-orm/core'; import { SqliteConnection, SqliteDriver } from '@mikro-orm/sqlite'; -import { SqliteDialect } from 'kysely'; +import type Database from 'better-sqlite3'; +import { SqliteDialect, SqliteDriver as KyselySqliteDriver, type Dialect, type SqliteDialectConfig } from 'kysely'; /** - * A SQLite driver that reuses the connection `db/database.ts` already owns - * instead of opening a second one. - * - * This is not a preference — it is forced by the test harness. `database.ts` - * hands each vitest worker `:memory:`, and in better-sqlite3 an in-memory - * database belongs to its *connection*: two connections to `:memory:` are two - * unrelated databases. MikroORM's stock `SqliteConnection.createKyselyDialect()` - * always does `new Database(dbName)`, so it would migrate a database nothing - * else in the process can see. - * - * Sharing it also keeps the single-writer property the app has always had: one - * handle, one WAL, no two-writers-on-one-file question in production. + * The stock Kysely SQLite driver's `destroy()` unconditionally does + * `db.close()`. MikroORM's own `close()` (`AbstractSqlConnection.close()`) + * always calls the Kysely client's `destroy()` too, regardless of the `force` + * flag it was given, and it caches that client behind a private field — so a + * `SqliteConnection` subclass has no hook to intercept just that one call. + * The fix has to live at the Kysely layer: hand MikroORM a driver identical to + * the stock one except `destroy()` is a no-op, so closing the ORM never closes + * a handle somebody else owns. */ -class SharedSqliteConnection extends SqliteConnection { - /** - * Resolved lazily, on every (re)connect rather than once at import: a backup - * restore closes the handle and opens a new one, and Kysely caches whatever it - * is given for the life of its client. `reinitialize()` closes this connection - * so that the next `connect()` lands here again and picks up the new handle. - */ - override createKyselyDialect(): SqliteDialect { - return new SqliteDialect({ database: getRawConnection() }); +class NonClosingSqliteDriver extends KyselySqliteDriver { + override async destroy(): Promise { + // The handle's owner (whoever `getHandle` below reads from) closes it. } } -export class SharedSqliteDriver extends SqliteDriver { - constructor(config: Configuration) { - super(config); - // The base constructor already built a stock SqliteConnection; replace it - // before anything connects. `connection` is protected on AbstractSqlDriver, - // and there is no supported hook for supplying the class from a subclass of - // SqliteDriver (its own constructor hardcodes SqliteConnection). - (this as unknown as { connection: SqliteConnection }).connection = new SharedSqliteConnection(config); +/** + * A SQLite driver bound to a better-sqlite3 handle somebody else owns. + * + * Forced by the test harness: `database.ts` hands each vitest worker + * `:memory:`, and in better-sqlite3 an in-memory database belongs to its + * *connection* — two connections to `:memory:` are two unrelated databases. + * MikroORM's stock `SqliteConnection.createKyselyDialect()` always does + * `new Database(dbName)`, so it would migrate a database nothing else in the + * process can see. Sharing the handle also keeps the single-writer property the + * app has always had: one handle, one WAL. + * + * `getHandle` is called on every (re)connect rather than once: a backup restore + * closes the handle and opens a new one, and Kysely caches whatever it is given + * for the life of its client, so `reinitialize()` closes the connection and the + * next `connect()` lands here again with the new handle. + */ +export function createBoundSqliteDriver(getHandle: () => Database.Database): typeof SqliteDriver { + class BoundSqliteConnection extends SqliteConnection { + override createKyselyDialect(): Dialect { + const config: SqliteDialectConfig = { database: getHandle() }; + // Delegate the compiler/adapter/introspector to a throwaway stock dialect + // (stateless factories); only `createDriver()` needs the non-closing swap. + const stock = new SqliteDialect(config); + return { + createDriver: () => new NonClosingSqliteDriver(config), + createQueryCompiler: () => stock.createQueryCompiler(), + createAdapter: () => stock.createAdapter(), + createIntrospector: (db) => stock.createIntrospector(db), + }; + } } + + return class BoundSqliteDriver extends SqliteDriver { + constructor(config: Configuration) { + super(config); + // The base constructor already built a stock SqliteConnection; replace it + // before anything connects. `connection` is protected on AbstractSqlDriver + // and SqliteDriver's constructor hardcodes the class, so there is no + // supported hook for supplying it. + (this as unknown as { connection: SqliteConnection }).connection = new BoundSqliteConnection(config); + } + }; } + +/** + * The production driver: bound to the connection `db/database.ts` owns. + * + * Wrapped in a closure rather than passed as `getRawConnection` directly: + * `createBoundSqliteDriver` doesn't call its argument until something actually + * connects, but merely *reading* the `getRawConnection` binding here would — + * this module is evaluated at import time, and a test that mocks + * `db/database` with a partial replacement (most do, via `buildDbMock` or a + * hand-rolled object) throws on that read even when the test never touches + * the database at all. + */ +export const SharedSqliteDriver = createBoundSqliteDriver(() => getRawConnection()); diff --git a/server/tests/helpers/test-orm.ts b/server/tests/helpers/test-orm.ts new file mode 100644 index 000000000..896d19c02 --- /dev/null +++ b/server/tests/helpers/test-orm.ts @@ -0,0 +1,59 @@ +import type { EntityClass, EntityRepository, GetRepository } from '@mikro-orm/core'; +import { MikroORM, type EntityManager } from '@mikro-orm/sqlite'; +import type Database from 'better-sqlite3'; +import { ALL_ENTITIES } from '../../src/db/entities'; +import { createBoundSqliteDriver } from '../../src/db/orm-driver'; + +export interface TestOrm { + orm: MikroORM; + /** A fork of the global EntityManager; `clear()` empties its identity map. */ + em: EntityManager; + repo(entity: EntityClass): GetRepository>; + clear(): void; + /** Closes the ORM only — the better-sqlite3 handle stays open for the caller. */ + close(): Promise; +} + +/** + * An ORM over a test database the caller already opened (usually + * `createTestDb()`), so factories can keep inserting with raw SQL through the + * same handle and the ORM sees those rows. + * + * `allowGlobalContext` defaults to true here and only here: a test builds a + * service with `t.repo(X)` and calls it without an HTTP request around it. + * Production keeps the default (false), which is what `withRequestContext` in + * `src/nest/database/request-context.ts` is for. + * + * `close()` on the underlying MikroORM connection ignores the `force` flag and + * always tears down its Kysely client — what actually keeps the handle open + * is `NonClosingSqliteDriver` in `../../src/db/orm-driver.ts`, which + * `createBoundSqliteDriver` hands MikroORM in place of Kysely's stock + * `SqliteDriver`; its `destroy()` is a no-op, so this `close()` never reaches + * the handle at all. + */ +export async function createTestOrm( + db: Database.Database, + options: { allowGlobalContext?: boolean } = {}, +): Promise { + const orm = await MikroORM.init({ + entities: [...ALL_ENTITIES], + driver: createBoundSqliteDriver(() => db), + dbName: ':memory:', + allowGlobalContext: options.allowGlobalContext ?? true, + discovery: { warnWhenNoEntities: false }, + }); + const em = orm.em.fork(); + return { + orm, + em, + repo: (entity) => em.getRepository(entity), + clear: () => em.clear(), + close: async () => { + // The `force` flag is irrelevant here — MikroORM's connection.close() + // always destroys its Kysely client either way. It's the driver's + // `destroy()` being a no-op (see the docstring above) that keeps the + // handle open, not this call site. + await orm.close(false); + }, + }; +} diff --git a/server/tests/unit/db/test-orm.test.ts b/server/tests/unit/db/test-orm.test.ts new file mode 100644 index 000000000..ca44e522f --- /dev/null +++ b/server/tests/unit/db/test-orm.test.ts @@ -0,0 +1,38 @@ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createTestDb, resetTestDb } from '../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { createUser } from '../../helpers/factories'; +import { Users } from '../../../src/db/entities/Users.entity'; + +const testDb = createTestDb(); +let t: TestOrm; + +beforeAll(async () => { t = await createTestOrm(testDb); }); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('createTestOrm', () => { + it('ORM-001: reads through the very handle it was given', async () => { + const { user } = createUser(testDb, { username: 'alice' }); + const found = await t.repo(Users).findOne({ id: user.id }); + expect(found?.username).toBe('alice'); + }); + + it('ORM-002: clear() drops the identity map so a raw update is visible', async () => { + const { user } = createUser(testDb, { username: 'bob' }); + await t.repo(Users).findOne({ id: user.id }); + testDb.prepare('UPDATE users SET username = ? WHERE id = ?').run('robert', user.id); + t.clear(); + const again = await t.repo(Users).findOne({ id: user.id }); + expect(again?.username).toBe('robert'); + }); + + it('ORM-003: closing the ORM leaves the handle usable', async () => { + const local = await createTestOrm(testDb); + // Force a real connection first — MikroORM connects lazily, so closing an + // untouched ORM would prove nothing about NonClosingSqliteDriver. + await local.repo(Users).findOne({ id: 1 }); + await local.close(); + expect(testDb.prepare('SELECT 1 AS one').get()).toEqual({ one: 1 }); + }); +});