From f4fe27a3f8cc8b225d82b4f723d7e666e413c560 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 21 Sep 2026 18:12:07 +0200 Subject: [PATCH] fix(db): dialect helpers dispatch on the platform; lint gate for the repository layer --- server/eslint.config.mjs | 129 ++++++++++++++++++ server/src/db/dialect/sql-functions.ts | 41 ++++-- server/src/db/orm.ts | 4 +- server/src/db/types/db-timestamp.type.ts | 28 ++-- server/src/nest/app.module.ts | 4 +- server/src/nest/database/orm.module.ts | 5 +- .../unit/db/dialect/sql-functions.test.ts | 32 +++-- server/vitest.config.ts | 2 +- 8 files changed, 197 insertions(+), 48 deletions(-) diff --git a/server/eslint.config.mjs b/server/eslint.config.mjs index 4684c3ad6..d29ea58ad 100644 --- a/server/eslint.config.mjs +++ b/server/eslint.config.mjs @@ -106,6 +106,135 @@ export default tseslint.config( ], }, }, + { + // D4: the driver is an implementation detail of src/db. Nothing else in + // src/ names `better-sqlite3` — a domain module that imports it is coupled + // to the engine the ORM exists to abstract, and (for the value import) + // reaches a connection the DI graph never handed it. + // + // `no-restricted-imports` is repeated rather than added to the src/services + // block above because ESLint flat config REPLACES a rule's options with the + // last matching config object's — a block that set only the driver path + // would silently drop the services wall for every file it matches. + files: ['src/**/*.ts'], + ignores: [ + // The connection and the ORM's bound driver: this is where the engine lives. + 'src/db/database.ts', + 'src/db/orm-driver.ts', + 'src/db/durability.ts', + // The plugin sandbox opens its own per-plugin database files, deliberately + // outside the app's ORM and connection. + 'src/nest/plugins/host/plugin-data.service.ts', + // Backup/restore operates on database FILES, not on rows. + 'src/nest/backup/backup.impl.ts', + // Demo-mode seeding, and the legacy schema/migration/seed scripts the test + // suite still builds its throwaway databases from (guarded by + // tests/unit/db/schema-parity.test.ts). + 'src/demo/**', + 'src/db/schema.ts', + 'src/db/migrations.ts', + 'src/db/seeds.ts', + // TODO(plan 4): pre-existing importers, to be migrated onto the ORM / + // DatabaseService rather than by loosening this rule. All type-only except + // reseat-booked-nights.ts, which opens its own handle. + 'src/db/reseat-booked-nights.ts', + 'src/db/document-provider-seed.ts', + 'src/nest/database/database.service.ts', + 'src/nest/plugins/contributions/plugin-route-normalize.ts', + 'src/nest/plugins/host/plugin-host-state.ts', + 'src/nest/plugins/install/discovery.ts', + 'src/nest/plugins/settings-defaults.ts', + 'src/nest/plugins/signature-status.ts', + ], + rules: { + 'no-restricted-imports': [ + 'error', + { + paths: [ + { + name: 'better-sqlite3', + message: + 'The SQLite driver is an implementation detail of src/db. Go through the ORM (src/db/entities + src/db/repositories) or the injected DatabaseService; the handle itself is owned by src/db/database.ts.', + }, + ], + patterns: [ + { + group: ['**/services/*', '**/services/**/*'], + message: + 'src/services/ is deleted. New backend code goes to src/nest// (service + controller + module, registered in app.module.ts). See src/nest/README.md.', + }, + ], + }, + ], + }, + }, + { + // D4/D10: a repository is dialect-neutral. It talks to the ORM through + // @mikro-orm/core and @mikro-orm/sql — never to a concrete driver package, + // never to better-sqlite3 (the driver is wired once, in src/db/orm-driver.ts) + // — and it never spells raw SQL inline: every database function goes through + // src/db/dialect/sql-functions.ts, which dispatches on the live platform. + // + // Both rules restate the blocks above for the same flat-config reason: the + // last matching config object's options REPLACE, not extend, the earlier + // ones, so dropping either restatement would open a hole under + // src/db/repositories/ only. + files: ['src/db/repositories/**/*.ts'], + rules: { + 'no-restricted-imports': [ + 'error', + { + paths: [ + { + name: '@mikro-orm/sqlite', + message: + 'Repositories are dialect-neutral: import from @mikro-orm/sql / @mikro-orm/core. The driver lives in src/db/orm-driver.ts.', + }, + { + name: '@mikro-orm/postgresql', + message: + 'Repositories are dialect-neutral: import from @mikro-orm/sql / @mikro-orm/core. The driver lives in src/db/orm-driver.ts.', + }, + { + name: 'better-sqlite3', + message: + 'Repositories are dialect-neutral: import from @mikro-orm/sql / @mikro-orm/core. The driver lives in src/db/orm-driver.ts.', + }, + ], + patterns: [ + { + group: ['**/services/*', '**/services/**/*'], + message: + 'src/services/ is deleted. New backend code goes to src/nest// (service + controller + module, registered in app.module.ts). See src/nest/README.md.', + }, + ], + }, + ], + 'no-restricted-syntax': [ + 'error', + { + selector: "MemberExpression[object.name='process'][property.name='env']", + message: + 'Read configuration via src/app-config (readEnv()/derive/tokens), not process.env. Exemptions: eslint.config.mjs + src/app-config/README.md.', + }, + { + selector: "MemberExpression[object.name='process'][property.value='env']", + message: + 'Read configuration via src/app-config (readEnv()/derive/tokens), not process.env. Exemptions: eslint.config.mjs + src/app-config/README.md.', + }, + { + selector: "CallExpression[callee.name='raw']", + message: + 'Dialect SQL goes through src/db/dialect/sql-functions.ts, which dispatches on the live MikroORM platform. A repository must not spell raw SQL.', + }, + { + selector: "TaggedTemplateExpression[tag.name='sql']", + message: + 'Dialect SQL goes through src/db/dialect/sql-functions.ts, which dispatches on the live MikroORM platform. A repository must not spell raw SQL.', + }, + ], + }, + }, { // The plugin RPC decorator kit is written to stay extractable into its own // package, so its dependencies are pinned to Nest plus the two host modules it diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index f3bf0b5c1..558980794 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -1,13 +1,19 @@ -import { raw, type RawQueryFragment } from '@mikro-orm/core'; +import { raw, type Platform, type RawQueryFragment } from '@mikro-orm/core'; +import { SqlitePlatform } from '@mikro-orm/sql'; /** * The only place a repository may spell a database function. * - * Every helper takes an already-qualified column reference (`'t.created_at'`) - * and returns a fragment for the *current* dialect. The ORM exists so a - * Postgres driver can follow; when it does, this file gets a second - * implementation and no repository changes. Values are never interpolated — - * the day count in `dateAdd` is checked to be an integer before it is spelled. + * Every helper takes the MikroORM `Platform` the caller's EntityManager is + * running on (`em.getPlatform()`) plus an already-qualified column reference + * (`'t.created_at'`), and dispatches on that platform at runtime. TREK ships + * one binary and the database is chosen by configuration, so the dialect is a + * runtime property of the live connection, not a build-time swap: when the + * Postgres driver arrives this file grows a second branch and no repository + * changes. An unknown platform fails closed rather than guessing a spelling. + * + * Values are never interpolated — the day count in `dateAdd` is checked to be + * an integer before it is spelled. */ function column(ref: string): string { @@ -17,19 +23,28 @@ function column(ref: string): string { return ref; } +function unsupported(platform: Platform): never { + throw new Error(`sql-functions: no implementation for platform ${platform.constructor.name}`); +} + /** The calendar date (`YYYY-MM-DD`) of a timestamp column. */ -export function dateOf(ref: string): RawQueryFragment { - return raw(`date(${column(ref)})`); +export function dateOf(platform: Platform, ref: string): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw(`date(${column(ref)})`); + return unsupported(platform); } /** The timestamp column shifted by whole days, as a calendar date. */ -export function dateAdd(ref: string, days: number): RawQueryFragment { +export function dateAdd(platform: Platform, ref: string, days: number): RawQueryFragment { if (!Number.isInteger(days)) throw new Error(`sql-functions: dateAdd needs an integer day count, got ${days}`); - const sign = days < 0 ? '-' : '+'; - return raw(`date(${column(ref)}, '${sign}${Math.abs(days)} days')`); + if (platform instanceof SqlitePlatform) { + const sign = days < 0 ? '-' : '+'; + return raw(`date(${column(ref)}, '${sign}${Math.abs(days)} days')`); + } + return unsupported(platform); } /** The database clock, in the same text the column defaults produce. */ -export function currentTimestamp(): RawQueryFragment { - return raw('CURRENT_TIMESTAMP'); +export function currentTimestamp(platform: Platform): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw('CURRENT_TIMESTAMP'); + return unsupported(platform); } diff --git a/server/src/db/orm.ts b/server/src/db/orm.ts index 07b55682c..f115b41f8 100644 --- a/server/src/db/orm.ts +++ b/server/src/db/orm.ts @@ -56,8 +56,8 @@ export async function runSchemaBootstrap(orm: AnyOrm): Promise { export function attachOrm(orm: AnyOrm): void { registerReinitializeHook(async () => { const connection = orm.em.getConnection(); - // The old handle is already closed by this point; better-sqlite3's close() - // is idempotent, so letting Kysely destroy it again is harmless. + // The handle is owned and closed by `database.ts`; the bound driver's + // `destroy()` is a no-op (see `orm-driver.ts`), so this close never reaches it. await connection.close(true); await connection.connect(); await runSchemaBootstrap(orm); diff --git a/server/src/db/types/db-timestamp.type.ts b/server/src/db/types/db-timestamp.type.ts index a3b41356f..505e4cd5c 100644 --- a/server/src/db/types/db-timestamp.type.ts +++ b/server/src/db/types/db-timestamp.type.ts @@ -1,4 +1,4 @@ -import { Type, type EntityProperty, type Platform, type TransformContext } from '@mikro-orm/core'; +import { Type, type EntityProperty, type Platform } from '@mikro-orm/core'; /** The text every DATETIME column holds and every API response emits. */ export const DB_TIMESTAMP_RE = /^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/; @@ -25,7 +25,9 @@ type TimestampValue = string | null | undefined; * text on the way in, and text is written as text on the way out. */ export class DbTimestampType extends Type { - override convertToDatabaseValue(value: TimestampValue, platform: Platform, context?: TransformContext): TimestampValue { + // Both converters drop the base class's `platform`/`context` parameters: the + // wire format is the same text on every dialect, so neither is consulted. + override convertToDatabaseValue(value: TimestampValue): TimestampValue { if (value == null) { return value; } @@ -33,14 +35,10 @@ export class DbTimestampType extends Type { if ((value as unknown) instanceof Date) { return dbNow(value as unknown as Date); } - if (typeof value === 'string') { - return value; - } - // Default: convert to string - return String(value); + return value; } - override convertToJSValue(value: TimestampValue, platform: Platform, context?: TransformContext): TimestampValue { + override convertToJSValue(value: TimestampValue): TimestampValue { if (value == null) { return value; } @@ -48,16 +46,12 @@ export class DbTimestampType extends Type { if ((value as unknown) instanceof Date) { return dbNow(value as unknown as Date); } - // Handle millisecond integers (legacy format) - if (typeof value === 'number' || (value as unknown) instanceof Number) { - const numValue = typeof value === 'number' ? value : (value as unknown as number); - return dbNow(new Date(numValue)); + // Handle millisecond integers (legacy format) — a Date written by the stock + // DateTimeType reads back as a number. + if (typeof value === 'number') { + return dbNow(new Date(value as unknown as number)); } - if (typeof value === 'string') { - return value; - } - // Default: convert to string - return String(value); + return value; } override getColumnType(prop: EntityProperty, platform: Platform): string { diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 743026b51..172d14bbc 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -152,8 +152,8 @@ 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. + // Not part of DatabaseModule: the e2e suites that compose `DatabaseModule` + // without the ORM would fail on an EntityManager-dependent provider there. OrmModule, ], providers: [ diff --git a/server/src/nest/database/orm.module.ts b/server/src/nest/database/orm.module.ts index b91f64f57..76d1f7003 100644 --- a/server/src/nest/database/orm.module.ts +++ b/server/src/nest/database/orm.module.ts @@ -5,8 +5,9 @@ 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, ...] })`) + * Deliberately separate from `DatabaseModule`, which the e2e suites that + * compose `DatabaseModule` without the ORM build 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 diff --git a/server/tests/unit/db/dialect/sql-functions.test.ts b/server/tests/unit/db/dialect/sql-functions.test.ts index 7d2f8535c..bbf85052d 100644 --- a/server/tests/unit/db/dialect/sql-functions.test.ts +++ b/server/tests/unit/db/dialect/sql-functions.test.ts @@ -1,11 +1,13 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; -import { createTestDb, resetTestDb } from '../../../helpers/test-db'; +import { Platform } from '@mikro-orm/core'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { 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 { currentTimestamp, dateAdd, dateOf } from '../../../../src/db/dialect/sql-functions'; -const testDb = createTestDb(); +const testDb = createSnapshotTestDb(); let t: TestOrm; beforeAll(async () => { t = await createTestOrm(testDb); }); @@ -17,7 +19,7 @@ describe('sql-functions (sqlite)', () => { const { user } = createUser(testDb); testDb.prepare("UPDATE users SET created_at = '2026-09-21 13:05:09' WHERE id = ?").run(user.id); const row = await t.em.createQueryBuilder(Users, 'u') - .select([dateOf('u.created_at').as('d')]) + .select([dateOf(t.em.getPlatform(), 'u.created_at').as('d')]) .where({ id: user.id }) .execute('get', false); expect(row).toEqual({ d: '2026-09-21' }); @@ -27,7 +29,7 @@ describe('sql-functions (sqlite)', () => { const { user } = createUser(testDb); testDb.prepare("UPDATE users SET created_at = '2026-09-21 13:05:09' WHERE id = ?").run(user.id); const row = await t.em.createQueryBuilder(Users, 'u') - .select([dateAdd('u.created_at', 10).as('d')]) + .select([dateAdd(t.em.getPlatform(), 'u.created_at', 10).as('d')]) .where({ id: user.id }) .execute('get', false); expect(row).toEqual({ d: '2026-10-01' }); @@ -36,30 +38,38 @@ describe('sql-functions (sqlite)', () => { it('SQLF-003: currentTimestamp is the DB clock in the wire format', async () => { createUser(testDb); const row = await t.em.createQueryBuilder(Users, 'u') - .select([currentTimestamp().as('now')]) + .select([currentTimestamp(t.em.getPlatform()).as('now')]) .limit(1) .execute('get', false); expect(String((row as { now: string }).now)).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); }); it('SQLF-004: dateOf rejects anything that is not a plain column reference', () => { - expect(() => dateOf('u.created_at); DROP TABLE users; --')).toThrow(/not a column reference/); - expect(() => dateOf('u.created_at.extra')).toThrow(/not a column reference/); - expect(() => dateOf('')).toThrow(/not a column reference/); + expect(() => dateOf(t.em.getPlatform(), 'u.created_at); DROP TABLE users; --')).toThrow(/not a column reference/); + expect(() => dateOf(t.em.getPlatform(), 'u.created_at.extra')).toThrow(/not a column reference/); + expect(() => dateOf(t.em.getPlatform(), '')).toThrow(/not a column reference/); }); it('SQLF-005: dateAdd rejects a non-integer day count', () => { - expect(() => dateAdd('u.created_at', 1.5)).toThrow(/integer day count/); - expect(() => dateAdd('u.created_at', Number.NaN)).toThrow(/integer day count/); + expect(() => dateAdd(t.em.getPlatform(), 'u.created_at', 1.5)).toThrow(/integer day count/); + expect(() => dateAdd(t.em.getPlatform(), 'u.created_at', Number.NaN)).toThrow(/integer day count/); }); it('SQLF-006: dateAdd shifts backwards for a negative day count', async () => { const { user } = createUser(testDb); testDb.prepare("UPDATE users SET created_at = '2026-09-21 13:05:09' WHERE id = ?").run(user.id); const row = await t.em.createQueryBuilder(Users, 'u') - .select([dateAdd('u.created_at', -10).as('d')]) + .select([dateAdd(t.em.getPlatform(), 'u.created_at', -10).as('d')]) .where({ id: user.id }) .execute('get', false); expect(row).toEqual({ d: '2026-09-11' }); }); + + it('SQLF-007: an unknown platform fails closed instead of guessing a spelling', () => { + class FakePlatform extends Platform {} + const foreign = new FakePlatform(); + expect(() => dateOf(foreign, 'u.created_at')).toThrow(/no implementation for platform FakePlatform/); + expect(() => dateAdd(foreign, 'u.created_at', 1)).toThrow(/no implementation for platform FakePlatform/); + expect(() => currentTimestamp(foreign)).toThrow(/no implementation for platform FakePlatform/); + }); }); diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 40f201755..a34aac4eb 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -170,7 +170,7 @@ export default defineConfig({ 'src/db/**/*.ts': { statements: 73, branches: 38, functions: 59, lines: 80 }, 'src/db/dialect/**/*.ts': { statements: 99, branches: 99, functions: 99, lines: 99 }, 'src/db/repositories/**/*.ts': { statements: 99, branches: 99, functions: 99, lines: 99 }, - 'src/db/types/**/*.ts': { statements: 89, branches: 83, functions: 99, lines: 89 }, + 'src/db/types/**/*.ts': { statements: 93, branches: 99, functions: 99, lines: 93 }, 'src/mcp/**/*.ts': { statements: 58, branches: 43, functions: 63, lines: 60 }, 'src/middleware/**/*.ts': { statements: 91, branches: 89, functions: 87, lines: 94 }, // The folded-in nest-mcp decorator/registry layer keeps the 80% floor