From d7f39916d5b687cfdf2062be237b71498c65b787 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 21 Sep 2026 10:01:36 +0200 Subject: [PATCH] feat(db): request-context and unit-of-work helpers for the ORM, with concurrency tests --- server/src/nest/app.module.ts | 4 + server/src/nest/database/orm.module.ts | 18 ++++ server/src/nest/database/request-context.ts | 16 ++++ server/src/nest/database/unit-of-work.ts | 28 ++++++ server/tests/helpers/test-orm.ts | 18 +++- .../nest/database/request-context.test.ts | 39 ++++++++ .../unit/nest/database/unit-of-work.test.ts | 94 +++++++++++++++++++ 7 files changed, 212 insertions(+), 5 deletions(-) create mode 100644 server/src/nest/database/orm.module.ts create mode 100644 server/src/nest/database/request-context.ts create mode 100644 server/src/nest/database/unit-of-work.ts create mode 100644 server/tests/unit/nest/database/request-context.test.ts create mode 100644 server/tests/unit/nest/database/unit-of-work.test.ts diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 077796112..743026b51 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -26,6 +26,7 @@ import { TrekExceptionFilter } from './common/trek-exception.filter'; import { ZodValidationPipe } from './common/zod-validation.pipe'; import { ConfigModule } from './config/config.module'; import { DatabaseModule } from './database/database.module'; +import { OrmModule } from './database/orm.module'; import { DayNotesModule } from './day-notes/day-notes.module'; import { DaysModule } from './days/days.module'; import { DocSyncModule } from './doc-sync/doc-sync.module'; @@ -151,6 +152,9 @@ import { APP_FILTER, APP_GUARD, APP_INTERCEPTOR, APP_PIPE } from '@nestjs/core'; LlmParseModule, ManagedExtModule, MikroOrmModule.forRoot(mikroOrmConfig), + // Not part of DatabaseModule: 41 e2e suites compose that module without the + // ORM, and an EntityManager-dependent provider there fails their containers. + OrmModule, ], providers: [ // Default-deny: a route is authenticated unless it carries @Public() or diff --git a/server/src/nest/database/orm.module.ts b/server/src/nest/database/orm.module.ts new file mode 100644 index 000000000..b91f64f57 --- /dev/null +++ b/server/src/nest/database/orm.module.ts @@ -0,0 +1,18 @@ +import { Global, Module } from '@nestjs/common'; +import { UnitOfWork } from './unit-of-work'; + +/** + * The ORM-side container module: the home of providers that depend on MikroORM + * itself rather than on the raw better-sqlite3 handle `DatabaseModule` owns. + * + * Deliberately separate from `DatabaseModule`, which 41 e2e suites compose + * standalone (`Test.createTestingModule({ imports: [DatabaseModule, ...] })`) + * with no ORM in the container: a provider there that injects the core + * `EntityManager` fails every one of them at `compile()`, because Nest + * instantiates providers eagerly. Nothing is imported here — the + * `EntityManager` token comes from `MikroOrmModule.forRoot`, whose + * `MikroOrmCoreModule` is itself `@Global()`. + */ +@Global() +@Module({ providers: [UnitOfWork], exports: [UnitOfWork] }) +export class OrmModule {} diff --git a/server/src/nest/database/request-context.ts b/server/src/nest/database/request-context.ts new file mode 100644 index 000000000..9987519a8 --- /dev/null +++ b/server/src/nest/database/request-context.ts @@ -0,0 +1,16 @@ +import { RequestContext, type MikroORM } from '@mikro-orm/core'; + +/** + * A request-scoped EntityManager for work that no HTTP request wraps. + * + * `@mikro-orm/nestjs` forks an EntityManager per HTTP request through its + * middleware; everything else — an MCP tool call, a plugin RPC dispatch, a + * WebSocket message, a cron job, the boot-time seeders, a backup restore — has + * no such wrapper, and with `allowGlobalContext` left at its safe default the + * ORM refuses to run a query there. This is the one helper those entrypoints + * call. It is deliberately a function and not a decorator so a call site can + * pass the ORM it was given rather than reach for a global. + */ +export function withRequestContext(orm: MikroORM, fn: () => T): T { + return RequestContext.create(orm.em, fn); +} diff --git a/server/src/nest/database/unit-of-work.ts b/server/src/nest/database/unit-of-work.ts new file mode 100644 index 000000000..1643690c5 --- /dev/null +++ b/server/src/nest/database/unit-of-work.ts @@ -0,0 +1,28 @@ +import { Injectable } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; + +/** + * The one way a service opens a transaction. + * + * Wraps `em.transactional()` with its default `NESTED` propagation: a call made + * while a transaction is open becomes a savepoint, which is what better-sqlite3's + * nested `transaction()` did and what the services were written against. + * + * Repositories injected by Nest (and the test helper's `t.repo`) resolve the + * transactional fork through MikroORM's TransactionContext, so nothing has to + * be passed down. A manually forked EntityManager does not: `fork()` defaults + * to `useContext: false`, so its `getContext()` returns itself, and a repository + * built on it would write outside the transaction, on a second connection the + * transaction is holding. + * + * Never awaits network I/O inside: the SQLite connection is one Kysely client + * with a mutex, and an open transaction holds it for every other request. + */ +@Injectable() +export class UnitOfWork { + constructor(private readonly em: EntityManager) {} + + transactional(fn: () => Promise): Promise { + return this.em.transactional(() => fn()); + } +} diff --git a/server/tests/helpers/test-orm.ts b/server/tests/helpers/test-orm.ts index 896d19c02..04ede7247 100644 --- a/server/tests/helpers/test-orm.ts +++ b/server/tests/helpers/test-orm.ts @@ -6,7 +6,7 @@ import { createBoundSqliteDriver } from '../../src/db/orm-driver'; export interface TestOrm { orm: MikroORM; - /** A fork of the global EntityManager; `clear()` empties its identity map. */ + /** The global, context-resolving EntityManager; `clear()` empties its identity map. */ em: EntityManager; repo(entity: EntityClass): GetRepository>; clear(): void; @@ -24,6 +24,15 @@ export interface TestOrm { * Production keeps the default (false), which is what `withRequestContext` in * `src/nest/database/request-context.ts` is for. * + * `em` and `repo()` hand out the ORM's global EntityManager rather than a fork + * on purpose: a forked EntityManager has `useContext: false`, so its + * `getContext()` returns itself and ignores `TransactionContext`. A `t.repo(X)` + * built on a fork and used inside `uow.transactional` would write outside the + * open transaction, on a second connection the transaction is holding, and + * deadlock on Kysely's connection mutex. The helper therefore hands out the + * same context-resolving global EM that Nest injects into services, which + * resolves the transactional fork the way production does. + * * `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 @@ -42,12 +51,11 @@ export async function createTestOrm( allowGlobalContext: options.allowGlobalContext ?? true, discovery: { warnWhenNoEntities: false }, }); - const em = orm.em.fork(); return { orm, - em, - repo: (entity) => em.getRepository(entity), - clear: () => em.clear(), + em: orm.em, + repo: (entity) => orm.em.getRepository(entity), + clear: () => orm.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 diff --git a/server/tests/unit/nest/database/request-context.test.ts b/server/tests/unit/nest/database/request-context.test.ts new file mode 100644 index 000000000..55a6bd4cd --- /dev/null +++ b/server/tests/unit/nest/database/request-context.test.ts @@ -0,0 +1,39 @@ +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'; +import { withRequestContext } from '../../../../src/nest/database/request-context'; + +const testDb = createTestDb(); +let t: TestOrm; + +// Global context DISALLOWED here on purpose: this is the production setting. +beforeAll(async () => { t = await createTestOrm(testDb, { allowGlobalContext: false }); }); +beforeEach(() => resetTestDb(testDb)); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('withRequestContext', () => { + it('CTX-001: the global EntityManager refuses a query outside a context', async () => { + createUser(testDb); + await expect(t.orm.em.find(Users, {})).rejects.toThrow(/global EntityManager|global context/i); + }); + + it('CTX-002: inside the helper the same call succeeds on a request-scoped fork', async () => { + const { user } = createUser(testDb); + const found = await withRequestContext(t.orm, () => t.orm.em.findOne(Users, { id: user.id })); + expect(found?.id).toBe(user.id); + }); + + it('CTX-003: two contexts do not share an identity map', async () => { + const { user } = createUser(testDb); + const a = await withRequestContext(t.orm, () => t.orm.em.findOneOrFail(Users, { id: user.id })); + const b = await withRequestContext(t.orm, () => t.orm.em.findOneOrFail(Users, { id: user.id })); + // Compared as a boolean, not with `expect(a).not.toBe(b)`: on a failed + // `Object.is`, vitest's `toBe` still runs a deep `equals()` with + // `iterableEquality` to offer the "replace toBe with toEqual" hint, and + // that iterates these entities' uninitialized `Collection` properties, + // which throw. The assertion below is the same identity check without it. + expect(a === b).toBe(false); + }); +}); diff --git a/server/tests/unit/nest/database/unit-of-work.test.ts b/server/tests/unit/nest/database/unit-of-work.test.ts new file mode 100644 index 000000000..d065826ff --- /dev/null +++ b/server/tests/unit/nest/database/unit-of-work.test.ts @@ -0,0 +1,94 @@ +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 { createTrip, createUser } from '../../../helpers/factories'; +import { Days } from '../../../../src/db/entities/Days.entity'; +import type { DaysRepository } from '../../../../src/db/repositories/Days.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; + +const testDb = createTestDb(); +let t: TestOrm; +let uow: UnitOfWork; +let days: DaysRepository; + +/** + * `t.em` is the ORM's context-resolving global EntityManager and `t.repo()` + * builds on it, which is what makes this suite meaningful: the repository resolves the + * transactional fork out of MikroORM's TransactionContext, exactly as a + * Nest-injected repository does. A manually forked EntityManager would not + * (`fork()` defaults to `useContext: false`) — it would write outside the open + * transaction, on a second connection the transaction is holding, and deadlock + * on Kysely's connection mutex. See the docstring on `createTestOrm`. + */ +beforeAll(async () => { + t = await createTestOrm(testDb); + uow = new UnitOfWork(t.em); + days = t.repo(Days) as DaysRepository; +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +const countDays = (tripId: number) => + (testDb.prepare('SELECT COUNT(*) AS n FROM days WHERE trip_id = ?').get(tripId) as { n: number }).n; + +describe('UnitOfWork.transactional', () => { + it('UOW-001: a throw rolls every statement back', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await expect( + uow.transactional(async () => { + await days.createDay({ trip_id: trip.id, day_number: 1, date: null, notes: null }); + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + expect(countDays(trip.id)).toBe(0); + }); + + it('UOW-002: an inner transaction is a savepoint — its failure leaves the outer writes intact', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + await uow.transactional(async () => { + await days.createDay({ trip_id: trip.id, day_number: 1, date: null, notes: null }); + await uow + .transactional(async () => { + await days.createDay({ trip_id: trip.id, day_number: 2, date: null, notes: null }); + throw new Error('inner'); + }) + .catch(() => undefined); + }); + expect(countDays(trip.id)).toBe(1); + }); + + it('UOW-003: concurrent transactions serialise on the one connection instead of throwing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const order: string[] = []; + const tx = (label: string, dayNumber: number) => + uow.transactional(async () => { + order.push(`${label}:in`); + await days.createDay({ trip_id: trip.id, day_number: dayNumber, date: null, notes: null }); + await new Promise((r) => setTimeout(r, 20)); + order.push(`${label}:out`); + }); + await Promise.all([tx('a', 1), tx('b', 2)]); + expect(countDays(trip.id)).toBe(2); + // The second never enters before the first leaves. + expect(order.indexOf('b:in')).toBeGreaterThan(order.indexOf('a:out')); + }); + + it('UOW-004: a read outside the transaction waits for it and then sees the commit', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const tx = uow.transactional(async () => { + await days.createDay({ trip_id: trip.id, day_number: 1, date: null, notes: null }); + await new Promise((r) => setTimeout(r, 20)); + }); + // A separate fork = a separate acquirer of the connection. It waits on the + // ConnectionMutex instead of deadlocking because nothing inside the + // transaction waits on it: the writer's own timer resolves regardless. + const reader = t.orm.em.fork().getRepository(Days) as DaysRepository; + const seen = await reader.listByTrip(trip.id); + await tx; + expect(seen).toHaveLength(1); + }); +});