diff --git a/server/scripts/generate-entities.ts b/server/scripts/generate-entities.ts index 8684e5371..9f78d7e09 100644 --- a/server/scripts/generate-entities.ts +++ b/server/scripts/generate-entities.ts @@ -823,6 +823,78 @@ export function RULE10_repositoryTypeMarker(metadata: EntityMetadata[]): Reposit return fixups; } +/** An owning relation whose FK targets a non-PK column on its target entity — Rule 11. */ +export interface ReferencedColumnsFixup { + className: string; + propName: string; + referencedColumnNames: readonly string[]; +} + +/** + * Rule 11 (metadata level: detection only, no mutation; text level: the + * actual insertion — see `injectReferencedColumns` below. This split, and why + * it cannot be a pure metadata rule, mirrors Rule 5's `.joinColumn()` + * injection and Rule 10's repository marker. + * + * `@mikro-orm/sql`'s SQLite introspection (`SqliteSchemaHelper.js`, straight + * from `PRAGMA foreign_key_list`'s `to` column) already sets + * `prop.referencedColumnNames` correctly for every owning to-one relation — + * confirmed empirically with a throwaway `onProcessedMetadata` probe against + * the real migrated schema, BEFORE any rule in this file runs: + * `OauthTokens.client`/`OauthConsents.client` already carry + * `referencedColumnNames: ['client_id']` (the schema's real FK target, + * `Migration20200101012500_oauth_2.ts`'s `REFERENCES oauth_clients(client_id)` + * — a UNIQUE natural key, not `OauthClients`'s PK, `id`), while every other + * owning relation in the whole schema (`OauthTokens.user`, `.parentToken`, + * `SchoolHolidayRegions.countryRef` -> `school_holiday_countries(code)`, + * which IS that table's PK, …) already carries `referencedColumnNames` equal + * to its target's own PK. + * + * The generator's own RENDERER never emits this correct, already-introspected + * fact into the generated text, though — it is not something an earlier rule + * in this file drops. Read directly out of + * `node_modules/@mikro-orm/entity-generator/{EntityGenerator,SourceFile}.js`, + * not assumed: `SourceFile.js#getForeignKeyDecoratorOptions` (shared by every + * output mode, including `defineEntity`, through + * `EntitySchemaSourceFile.getPropertyOptions` -> `DefineEntitySourceFile`) + * only ever emits `.referencedColumnNames([...])` when + * `prop.ownColumns && prop.ownColumns.length !== prop.fieldNames.length` — a + * COMPOSITE-FK partial-ownership column-COUNT mismatch, never a check of + * whether the referenced column differs from the target's PK. `prop.ownColumns` + * itself is only ever set by `EntityGenerator.js#generate()` when + * `targetPrimaryColumns.length !== prop.referencedColumnNames.length` — again + * a length comparison, not a content comparison. For a single-column FK + * against a single-column PK (exactly the `client_id` case here), both + * lengths are 1 either way, so `ownColumns` is never set and the renderer's + * emission check never fires, no matter which column is actually referenced. + * This is a real gap in `@mikro-orm/entity-generator` 7.2.1's own renderer, + * not a defect our earlier text passes introduce or could avoid at the + * metadata level. + * + * Detection compares `prop.referencedColumnNames` (already correct) against + * the target entity's OWN primary-key field names + * (`targetMeta.getPrimaryProps().flatMap(pk => pk.fieldNames)`) — never a + * hard-coded table list, so a future migration adding another FK against a + * non-PK column is caught automatically the next time this generator runs, + * not silently missed. + */ +export function RULE11_referencedColumns(metadata: EntityMetadata[]): ReferencedColumnsFixup[] { + const fixups: ReferencedColumnsFixup[] = []; + for (const meta of metadata) { + for (const prop of meta.relations) { + if (!isOwningToOne(prop)) continue; + const targetMeta = metadata.find((m) => m.className === prop.type); + if (!targetMeta) continue; // unresolved relation target — nothing this rule can compare against + const targetPk = targetMeta.getPrimaryProps().flatMap((pk) => pk.fieldNames); + const referenced = prop.referencedColumnNames ?? []; + const matchesTargetPk = referenced.length === targetPk.length && referenced.every((column, i) => column === targetPk[i]); + if (matchesTargetPk) continue; + fixups.push({ className: meta.className, propName: prop.name, referencedColumnNames: [...referenced] }); + } + } + return fixups; +} + /** A scalar property whose literal default the renderer's own heuristic drops or mis-renders — see below. */ export interface DefaultFixup { className: string; @@ -899,6 +971,8 @@ export interface RuleFixups { retypedScalars: RetypedScalarFixup[]; /** Rule 10's repository-type markers — one per entity class. */ repositoryMarkers: RepositoryTypeMarkerFixup[]; + /** Rule 11's non-PK FK referenced-column fixups. */ + referencedColumns: ReferencedColumnsFixup[]; } /** @@ -932,6 +1006,10 @@ export function applyRules( RULE9_addImplicitUniqueConstraints(metadata, implicitUniques); const defaults = RULE_normalizeLiteralDefaults(metadata); const repositoryMarkers = RULE10_repositoryTypeMarker(metadata); + // Runs after RULE5 so it reads the FINAL (post-rename) owning-relation + // property names — the same names the text pass below must find in the + // rendered `properties: {...}` block. + const referencedColumns = RULE11_referencedColumns(metadata); return { joinColumns, defaults, @@ -939,6 +1017,7 @@ export function applyRules( jsonColumns, retypedScalars: [...retypedByRule1, ...timestamps, ...jsonColumns], repositoryMarkers, + referencedColumns, }; } @@ -1444,6 +1523,43 @@ export function injectRepositoryTypeMarker(source: string, fixups: readonly Repo return result; } +/** + * The text half of Rule 11: appends `.referencedColumnNames(, …)` to a + * FK relation's property line whenever the renderer left it out despite the + * metadata already carrying the real (non-PK) referenced column — see + * `RULE11_referencedColumns`'s doc comment for why the renderer never emits + * this itself for a single-column FK. `p.manyToOne(...)`'s + * `.referencedColumnNames(...)` is a rest-args method (confirmed against the + * installed `node_modules/@mikro-orm/core/entity/defineEntity.d.ts`), so this + * renders bare comma-separated quoted column names, never a `[...]` array + * literal. A property line that already has an explicit + * `.referencedColumnNames(` call is a legitimate no-op (never observed today + * — the renderer's own emission path never fires for this shape, see the + * rule's doc comment — but a future generator upgrade could change that); + * anything else finding no trailing comma to anchor on throws. + */ +export function injectReferencedColumns(source: string, fixups: readonly ReferencedColumnsFixup[]): string { + let result = source; + for (const { className, propName, referencedColumnNames } of fixups) { + const match = findPropertyBuilderLine(result, propName); + if (!match) { + throw new Error(`generate-entities: injectReferencedColumns could not find the property line for "${className}.${propName}".`); + } + const line = match[1]; + if (line.includes('.referencedColumnNames(')) continue; // already explicit — nothing to do + const quoted = referencedColumnNames.map((column) => `'${column}'`).join(', '); + const appended = appendCallBeforeTrailingComma(line, `.referencedColumnNames(${quoted})`); + if (appended === undefined) { + throw new Error( + `generate-entities: injectReferencedColumns found the property line for "${className}.${propName}" but could not append ` + + `.referencedColumnNames(${quoted}) to it — check the renderer shape.`, + ); + } + result = result.slice(0, match.index) + appended + result.slice(match.index + line.length); + } + return result; +} + /** Every text pass, applied in order. */ export function applyTextPasses(source: string, fixups: RuleFixups): string { let result = injectJoinColumns(source, fixups.joinColumns); @@ -1456,6 +1572,7 @@ export function applyTextPasses(source: string, fixups: RuleFixups): string { result = removeUnusedCoreImportIfUnused(result, 'IType'); result = stripHiddenTypeAnnotation(result); result = injectRepositoryTypeMarker(result, fixups.repositoryMarkers); + result = injectReferencedColumns(result, fixups.referencedColumns); return result; } @@ -1550,6 +1667,7 @@ export async function generateEntities(): Promise { jsonColumns: [], retypedScalars: [], repositoryMarkers: [], + referencedColumns: [], }; const rawFiles = await generator.generate({ entityDefinition: 'defineEntity', @@ -1585,6 +1703,7 @@ export async function generateEntities(): Promise { jsonColumns: fixups.jsonColumns.filter((f) => f.className === className), retypedScalars: fixups.retypedScalars.filter((f) => f.className === className), repositoryMarkers: fixups.repositoryMarkers.filter((f) => f.className === className), + referencedColumns: fixups.referencedColumns.filter((f) => f.className === className), }; files.set(`${className}.entity.ts`, applyTextPasses(raw, relevantFixups)); } diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index 4ee2cf65b..983490cd2 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -1,6 +1,5 @@ import { raw, type Platform, type RawQueryFragment } from '@mikro-orm/core'; import { SqlitePlatform } from '@mikro-orm/sql'; -import { sql, type Expression } from 'kysely'; /** * The only place a repository may spell a database function. @@ -101,25 +100,3 @@ export function lower(platform: Platform, ref: string): RawQueryFragment & symbo return unsupported(platform); } -/** - * `CURRENT_TIMESTAMP`, for a query built through `em.getKysely()` — the - * escape hatch for a recursive CTE (`OauthTokensRepository.collectChainIds`) - * or a join the QueryBuilder's relation metadata cannot express correctly - * (`OauthTokensRepository.listActiveByUser`/`listAllActiveWithClientAndUser` - * — `oauth_tokens.client_id` references `oauth_clients.client_id`, not its - * primary key, and the generated entity's `client` relation has no - * `referencedColumnNames` override, so a QueryBuilder `.innerJoin('ot.client', - * …)` silently joins on the wrong column; see those methods' docstrings). - * Kysely has its own expression system, entirely separate from MikroORM's - * `raw()` — `currentTimestamp` above returns a value this file's OTHER - * consumers (the QueryBuilder, `nativeUpdate`) accept, but a Kysely query - * cannot use a `RawQueryFragment` as an operand. This is the one place a - * repository may reach for the Kysely spelling of the same literal, kept - * here rather than in a repository file so `no-restricted-syntax`'s ban on - * spelling raw SQL directly in `src/db/repositories/**` still holds — this - * file is exempt from that rule by design (it IS the one approved place). - */ -export function currentTimestampKysely(platform: Platform): Expression { - if (platform instanceof SqlitePlatform) return sql`CURRENT_TIMESTAMP`; - return unsupported(platform); -} diff --git a/server/src/db/entities/OauthConsents.entity.ts b/server/src/db/entities/OauthConsents.entity.ts index fcfeb3cbf..71d867469 100644 --- a/server/src/db/entities/OauthConsents.entity.ts +++ b/server/src/db/entities/OauthConsents.entity.ts @@ -21,7 +21,7 @@ export const OauthConsentsSchema = defineEntity({ uniques: [{ properties: ['client', 'user'] }], properties: { id: p.integer().primary(), - client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden(), + client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden().referencedColumnNames('client_id'), client_id: p.text().persist(false), user: () => p.manyToOne(Users).ref().deleteRule('cascade').hidden(), user_id: p.integer().persist(false), diff --git a/server/src/db/entities/OauthTokens.entity.ts b/server/src/db/entities/OauthTokens.entity.ts index 357717a6f..89a0bcac1 100644 --- a/server/src/db/entities/OauthTokens.entity.ts +++ b/server/src/db/entities/OauthTokens.entity.ts @@ -33,7 +33,7 @@ export const OauthTokensSchema = defineEntity({ ], properties: { id: p.integer().primary(), - client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden(), + client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden().referencedColumnNames('client_id'), client_id: p.text().persist(false), user: () => p.manyToOne(Users).ref().deleteRule('cascade').hidden().index('idx_oauth_tokens_user'), user_id: p.integer().persist(false).index('idx_oauth_tokens_user'), diff --git a/server/src/db/repositories/OauthTokens.repository.ts b/server/src/db/repositories/OauthTokens.repository.ts index 70cffed8a..c8c970d68 100644 --- a/server/src/db/repositories/OauthTokens.repository.ts +++ b/server/src/db/repositories/OauthTokens.repository.ts @@ -1,14 +1,17 @@ import { OauthTokens } from '../entities/OauthTokens.entity'; import { type AssertRowKeys } from './_shared/rows'; -import { columnRef, currentTimestamp, currentTimestampKysely } from '../dialect/sql-functions'; +import { columnRef, currentTimestamp } from '../dialect/sql-functions'; import { EntityRepository } from '@mikro-orm/sql'; /** - * The Kysely-side table shapes `em.getKysely()` needs for the two escape - * hatches in this repository (the recursive CTE and the `oauth_clients` - * join — see `collectChainIds`/`listActiveByUser`'s docstrings for why the - * QueryBuilder can't express either). Column-shaped, not entity-shaped: this - * is the raw driver's view of the tables, independent of MikroORM metadata. + * The Kysely-side table shape `em.getKysely()` needs for `collectChainIds`'s + * recursive CTE (D4's T6 tier — the QueryBuilder has no recursive-CTE + * support at all, so this one escape hatch stays regardless of the `client` + * relation's referenced column). `oauth_clients`/`users` are no longer part + * of this interface: `listActiveByUser`/`listAllActiveWithClientAndUser` + * (OA26/OA31) moved to the QueryBuilder once the generator learned to emit + * `.referencedColumnNames('client_id')` for `OauthTokens.client` (Plan 3b + * interlude A) — see those methods' docstrings. */ interface OauthTokensKyselyDB { oauth_tokens: { @@ -22,14 +25,6 @@ interface OauthTokensKyselyDB { created_at: string | null; parent_token_id: number | null; }; - oauth_clients: { - client_id: string; - name: string; - }; - users: { - id: number; - username: string; - }; } /** `OauthService.issueTokens`/`issueClientCredentialsToken`'s write (OA13/OA14) — same column set, one method. */ @@ -146,27 +141,25 @@ const _oauthTokenScalarProbe: AssertRowKeys { // --------------------------------------------------------------------- @@ -260,8 +253,9 @@ export class OauthTokensRepository extends EntityRepository { * (unlike `ot.client`) is a NORMAL relation — its column * (`user_id`, default naming, no override) genuinely references * `users.id`, verified empirically — so `.innerJoin('ot.user', 'u')` joins - * correctly and this stays on the QueryBuilder (D4's T5 tier), unlike - * OA26/OA31 below. `columnRef` for both FK scalars in the select list: + * correctly and this stays on the QueryBuilder (D4's T5 tier) — as do + * OA26/OA31 below, since Plan 3b interlude A. `columnRef` for both FK + * scalars in the select list: * `ot.user_id` because the same relation is also joined (the * `McpTokensRepository.listAllWithUsername` finding — selecting a * relation property while also joining it collapses into the join rather @@ -502,30 +496,35 @@ export class OauthTokensRepository extends EntityRepository { * ORDER BY ot.created_at DESC * ``` * - * `em.getKysely()`, NOT the QueryBuilder (this class's docstring): the - * join is `ot.client_id = oc.client_id`, the real FK target per the - * migration, which `.innerJoin('ot.client', 'oc')` cannot express - * correctly (it would join `ot.client_id = oc.id` instead — silently - * wrong, not an error). `currentTimestampKysely` for the same - * `CURRENT_TIMESTAMP` literal `currentTimestamp` spells for the - * QueryBuilder/`nativeUpdate` — Kysely has its own expression system and - * cannot consume a MikroORM `RawQueryFragment` directly (see - * `sql-functions.ts`'s docstring on that helper). Verified empirically - * against a real seeded row before committing to this shape (see the - * task report). + * The QueryBuilder (Plan 3b interlude A — this class's docstring): + * `.innerJoin('ot.client', 'oc')` now joins on the real `oc.client_id` + * column, the generated entity's `client` relation carrying an explicit + * `.referencedColumnNames('client_id')`. `columnRef(platform, + * 'ot.client_id')` in the select list for the same reason OA16's + * `findByAccessTokenHashWithUser` needs it: selecting a relation property + * while ALSO joining that same relation collapses into the join rather + * than emitting the FK scalar column (the `McpTokensRepository + * .listAllWithUsername` finding). `{ $gt: currentTimestamp(platform) }` + * for the `refresh_token_expires_at > CURRENT_TIMESTAMP` predicate — a + * `RawQueryFragment` is a normal filter value here, no Kysely-specific + * spelling needed once the query itself is a QueryBuilder query. */ async listActiveByUser(userId: number): Promise { const platform = this.getEntityManager().getPlatform(); - const rows = await this.getEntityManager() - .getKysely() - .selectFrom('oauth_tokens as ot') - .innerJoin('oauth_clients as oc', 'oc.client_id', 'ot.client_id') - .select(['ot.id', 'ot.client_id', 'oc.name as client_name', 'ot.scopes', 'ot.access_token_expires_at', 'ot.refresh_token_expires_at', 'ot.created_at']) - .where('ot.user_id', '=', userId) - .where('ot.revoked_at', 'is', null) - .where('ot.refresh_token_expires_at', '>', currentTimestampKysely(platform)) - .orderBy('ot.created_at', 'desc') - .execute(); + const rows = await this.qb('ot') + .innerJoin('ot.client', 'oc') + .select([ + 'ot.id', + columnRef(platform, 'ot.client_id'), + 'oc.name as client_name', + 'ot.scopes', + 'ot.access_token_expires_at', + 'ot.refresh_token_expires_at', + 'ot.created_at', + ]) + .where({ 'ot.user_id': userId, 'ot.revoked_at': null, 'ot.refresh_token_expires_at': { $gt: currentTimestamp(platform) } }) + .orderBy({ 'ot.created_at': 'desc' }) + .execute('all', false); return rows.map((row) => ({ id: row.id, client_id: row.client_id, @@ -562,25 +561,37 @@ export class OauthTokensRepository extends EntityRepository { * ORDER BY ot.created_at DESC * ``` * - * `em.getKysely()`, same reason as `listActiveByUser` (OA26) — the - * `oauth_clients` join. `users` joins correctly either way (its relation - * is normally mapped), joined here through Kysely too for one consistent - * query shape rather than mixing a QueryBuilder join into a Kysely query. + * The QueryBuilder, same reason as `listActiveByUser` (OA26) — the + * `client` relation now carries the real referenced column (see this + * class's docstring). `users` joins correctly either way (`ot.user` is a + * normal relation), joined here through the QueryBuilder too for one + * consistent query shape rather than a Kysely query. `columnRef` for both + * `client_id`/`user_id`: both relations are also joined in this same + * query, so a plain `'ot.client_id'`/`'ot.user_id'` select would collapse + * into their respective joins (same finding as `listActiveByUser`/OA16). * Malformed `scopes` JSON degrading to `null` per-row (not a 500) stays * in the SERVICE, unchanged — this repository returns the raw column * text. */ async listAllActiveWithClientAndUser(): Promise { - const rows = await this.getEntityManager() - .getKysely() - .selectFrom('oauth_tokens as ot') - .innerJoin('oauth_clients as oc', 'oc.client_id', 'ot.client_id') - .innerJoin('users as u', 'u.id', 'ot.user_id') - .select(['ot.id', 'ot.client_id', 'oc.name as client_name', 'ot.user_id', 'u.username', 'ot.scopes', 'ot.access_token_expires_at', 'ot.refresh_token_expires_at', 'ot.created_at']) - .where('ot.revoked_at', 'is', null) - .where('ot.refresh_token_expires_at', '>', currentTimestampKysely(this.getEntityManager().getPlatform())) - .orderBy('ot.created_at', 'desc') - .execute(); + const platform = this.getEntityManager().getPlatform(); + const rows = await this.qb('ot') + .innerJoin('ot.client', 'oc') + .innerJoin('ot.user', 'u') + .select([ + 'ot.id', + columnRef(platform, 'ot.client_id'), + 'oc.name as client_name', + columnRef(platform, 'ot.user_id'), + 'u.username', + 'ot.scopes', + 'ot.access_token_expires_at', + 'ot.refresh_token_expires_at', + 'ot.created_at', + ]) + .where({ 'ot.revoked_at': null, 'ot.refresh_token_expires_at': { $gt: currentTimestamp(platform) } }) + .orderBy({ 'ot.created_at': 'desc' }) + .execute('all', false); return rows.map((row) => ({ id: row.id, client_id: row.client_id, diff --git a/server/src/nest/common/row-id.ts b/server/src/nest/common/row-id.ts index 58e07a924..d3bae9205 100644 --- a/server/src/nest/common/row-id.ts +++ b/server/src/nest/common/row-id.ts @@ -3,28 +3,41 @@ * numeric primary key before it reaches a typed repository filter. * * The legacy raw-SQL layer bound a route param straight into `WHERE id = ?` - * and let SQLite's column-affinity rules do the work: a well-formed numeric - * string ('5', ' 5', '5.0') converts and matches, anything else ('abc', '', - * '0x10', '1e3') stays TEXT, can never equal an INTEGER column, and the - * lookup simply misses — answered with the route's ordinary not-found. A - * typed MikroORM filter has no such affinity: `Number('abc')` is `NaN`, and - * the ORM renders that as the bare, unquoted token `NaN` in the generated - * SQL, which SQLite parses as a column reference and throws - * `InvalidFieldNameException: no such column: NaN` — a 500 where the legacy - * route 404'd (Plan 3b Task 2 review, F1). + * and let SQLite's column-affinity rules do the work. Those rules are more + * permissive than they look: SQLite applies NUMERIC affinity to a TEXT + * operand compared against an INTEGER column using the same conversion CAST + * uses, which accepts leading/trailing whitespace, a leading sign, a decimal + * point and an exponent — so `WHERE id = ?` genuinely matches integer row 1 + * for `' 1'`, `'1 '`, `'1.0'` and `'+1'`, and matches row 1000 for `'1e3'` + * (verified empirically against `better-sqlite3`, not assumed — this + * docstring previously claimed the opposite, that `'1e3'` "can never equal + * an INTEGER column"; that claim was false). A typed MikroORM filter has no + * such affinity: `Number('abc')` is `NaN`, and the ORM renders that as the + * bare, unquoted token `NaN` in the generated SQL, which SQLite parses as a + * column reference and throws `InvalidFieldNameException: no such column: + * NaN` — a 500 where the legacy route 404'd (Plan 3b Task 2 review, F1). * * `toRowId` is the guard every converted service calls instead of a bare - * `Number(id)`: it accepts only what the legacy binding actually matched — - * digits only, no sign, no decimal point, no exponent, no hex — and returns - * `null` for everything else, which the caller answers with the route's own - * legacy not-found result BEFORE any repository call (program rule 15, - * `docs/superpowers/plans/2026-09-21-orm-migration-program.md`). - * - * A `/^\d+$/` string match is deliberately stricter than `Number.isSafeInteger` - * alone: `Number('0x10')` is `16` and `Number('1e3')` is `1000`, both safe - * integers a bare `Number()` conversion would have accepted even though - * SQLite's affinity conversion never would have matched either shape against - * an INTEGER column. Requiring the digits-only shape first closes that gap. + * `Number(id)`. It does NOT reproduce every string SQLite's affinity + * conversion would have matched — that would mean accepting `' 1'`, `'1.0'`, + * `'1e3'`, `'+1'`, and there is no realistic route/tool input that spells an + * id that way: every id in this codebase originates as + * `String()` on our own client (a URL path segment or + * an MCP tool argument), which is always a canonical decimal string, digits + * only, no sign, no leading zero beyond what the id itself is, no decimal + * point, no exponent, no surrounding whitespace. `toRowId` accepts exactly + * that canonical shape (`/^\d+$/`, so `'007'` — an id that happens to have + * leading zeros, which SQLite's own affinity conversion also resolves to 7 — + * still matches) and returns `null` for every non-canonical spelling, + * INCLUDING the ones SQLite's affinity would have matched. This is a + * DELIBERATE, ACCEPTED narrowing (program rule 15, + * `docs/superpowers/plans/2026-09-21-orm-migration-program.md`): a + * non-canonical id can only ever reach this guard from something other than + * our own client (a hand-crafted request), and failing closed to the + * route's legacy not-found result for it is the documented parity + * deviation, not a bug — never widen the regex to `/^(0|[1-9]\d*)$/` or any + * other shape that would reject `'007'`, and never widen it to accept the + * divergent shapes above either. */ export function toRowId(value: unknown): number | null { if (typeof value === 'number') { diff --git a/server/tests/unit/db/entity-schema-parity.test.ts b/server/tests/unit/db/entity-schema-parity.test.ts index b25b8d1b7..b9a44dcda 100644 --- a/server/tests/unit/db/entity-schema-parity.test.ts +++ b/server/tests/unit/db/entity-schema-parity.test.ts @@ -303,34 +303,83 @@ describe('entity ↔ migrated-schema parity', () => { }); /** - * FK targets/deleteRule and index names, against `PRAGMA foreign_key_list` - * and `PRAGMA index_list` — the plan's second, allow-listed check - * (Task 0, Step 3). Task 0 found deleteRule disagreeing on 11 of 180 - * owning relations; Task 3's rewrite fixed one of those eleven as a side - * effect (`school_holiday_regions.country` now has a real, explicit - * `joinColumn`, so it reports a genuine FK row) and re-ran this check - * against the other 124 rewritten/added entities, composite-PK relations - * included (`packing_item_contributors`, `vacay_user_settings`, - * `document_connections`, `trip_document_links` — their `.entity.ts` - * source never spells `.deleteRule(...)` on those relations, but MikroORM - * discovery still resolves the correct rule at runtime, confirmed by - * running this check, so they are not drift): the count is now exactly 10, - * unchanged in substance from Task 0's list minus the one now-fixed row - * (`trip_members.invited_by` entity=set null vs db=no action, - * `roadtrip_day_tracks.day_id` entity=set null vs db=cascade, - * `reservation_travelers.reservation_id`/`.user_id` and + * FK targets, against `PRAGMA foreign_key_list` — the plan's second, + * allow-listed check (Task 0, Step 3), the REFERENCED-COLUMN half. Task 0 + * left the whole check (referenced column + deleteRule + index names) + * `it.skip`'d and report-only. This half is un-skipped as of Plan 3b + * interlude A: `RULE11_referencedColumns` (`scripts/generate-entities.ts`) + * closed the one gap that made it fail — `OauthTokens.client`/ + * `OauthConsents.client` targeted `oauth_clients.id` (the generator's + * default assumption for an owning to-one relation) instead of the schema's + * real FK target, the UNIQUE natural key `oauth_clients.client_id` + * (`Migration20200101012500_oauth_2.ts`). Every other owning relation in + * the schema already agreed with `PRAGMA foreign_key_list`'s `to` column, + * `school_holiday_regions.country -> school_holiday_countries.code` + * included (`code` IS that table's primary key, so the generator's default + * assumption was already correct there — unchanged by Rule 11, confirmed + * by this same test). The deleteRule half stays skipped — see + * `PARITY-009b` immediately below for why and by how many rows. + */ + it('PARITY-009: FK referenced columns agree with PRAGMA foreign_key_list', () => { + const failures: string[] = []; + for (const meta of tableBackedMetas()) { + const table = meta.tableName; + const fkRows = testDb.prepare(`PRAGMA foreign_key_list("${table}")`).all() as { + from: string; + table: string; + to: string; + }[]; + const fkByColumn = new Map(fkRows.map((row) => [row.from, row])); + for (const prop of meta.props) { + const isOwningToOne = + prop.kind === ReferenceKind.MANY_TO_ONE || (prop.kind === ReferenceKind.ONE_TO_ONE && prop.owner); + if (!isOwningToOne) continue; + const fieldName = prop.fieldNames[0]; + const fk = fkByColumn.get(fieldName); + if (!fk) { + failures.push(`${table}.${fieldName}: entity relation has no matching db foreign key`); + continue; + } + const entityReferencedColumn = prop.referencedColumnNames?.[0]; + if (entityReferencedColumn !== fk.to) { + failures.push(`${table}.${fieldName}: referencedColumn entity=${String(entityReferencedColumn)} vs db=${fk.to}`); + } + } + } + expect(failures).toEqual([]); + }); + + /** + * The deleteRule half of the same check (Task 0, Step 3). Task 0 found + * deleteRule disagreeing on 11 of 180 owning relations; Task 3's rewrite + * fixed one of those eleven as a side effect (`school_holiday_regions. + * country` now has a real, explicit `joinColumn`, so it reports a genuine + * FK row) and re-ran this check against the other 124 rewritten/added + * entities, composite-PK relations included (`packing_item_contributors`, + * `vacay_user_settings`, `document_connections`, `trip_document_links` — + * their `.entity.ts` source never spells `.deleteRule(...)` on those + * relations, but MikroORM discovery still resolves the correct rule at + * runtime, confirmed by running this check, so they are not drift): the + * count was 10. Plan 3b interlude A's `RULE11_referencedColumns` only + * touches the referenced COLUMN, never `deleteRule` (`OauthTokens.client`/ + * `OauthConsents.client` already agreed on `deleteRule('cascade')` — + * unaffected either way), so re-running this exact query still finds + * **10** disagreements, unchanged: `trip_members.invited_by` entity=set + * null vs db=no action, `roadtrip_day_tracks.day_id` entity=set null vs + * db=cascade, `reservation_travelers.reservation_id`/`.user_id` and * `assignment_participants.assignment_id`/`.user_id` entity=no action vs * db=cascade, `oauth_tokens.parent_token_id` entity=set null vs db=no * action, `journey_contributors.user_id` entity=cascade vs db=no action, * `budget_settlements.created_by_user_id`/`budget_items.paid_by_user_id` - * entity=set null vs db=no action). None of these are in Task 0's or - * Task 3's scope; an allow-list would need 10 one-line entries for drift - * neither task was asked to fix, and index-name parity looked clean on - * inspection but was not exhaustively checked. Left skipped, with the - * findings above, for Plan 4 to turn into either fixes or a real - * allow-list. + * entity=set null vs db=no action. None of these are in this task's scope + * (or Task 0's/Task 3's); an allow-list would need 10 one-line entries for + * drift no task so far was asked to fix. Index-name parity looked clean on + * inspection but was not exhaustively checked either, and stays out of + * scope here too. Left skipped, with the count pinned above (Plan 4 owns + * turning this into either fixes or a real allow-list — the number to + * reconcile against is exactly 10, not "some"). */ - it.skip('PARITY-009 (report-only, deferred to Plan 4): FK deleteRule and index names agree', () => { + it.skip('PARITY-009b (report-only, deferred to Plan 4): FK deleteRule agrees — 10 known disagreements, see doc comment', () => { const failures: string[] = []; for (const meta of tableBackedMetas()) { const table = meta.tableName; diff --git a/server/tests/unit/db/generate-entities.test.ts b/server/tests/unit/db/generate-entities.test.ts index 6aee080c8..1b013e1fd 100644 --- a/server/tests/unit/db/generate-entities.test.ts +++ b/server/tests/unit/db/generate-entities.test.ts @@ -13,6 +13,7 @@ import { JoinColumnFixup, JsonColumnFixup, KNOWN_DIFFS, + ReferencedColumnsFixup, RepositoryTypeMarkerFixup, RULE1_fixUnknownScalarTypes, RULE1b_markJsonColumns, @@ -27,6 +28,7 @@ import { RULE8_bindRepositories, RULE9_addImplicitUniqueConstraints, RULE10_repositoryTypeMarker, + RULE11_referencedColumns, RULE_normalizeLiteralDefaults, applyTextPasses, checkEntities, @@ -38,6 +40,7 @@ import { injectJsonInterfaces, injectJsonTypeParams, injectMissingDefaults, + injectReferencedColumns, injectRepositoryTypeMarker, regenerateEntitiesIndex, stripHiddenTypeAnnotation, @@ -437,6 +440,58 @@ describe('RULE10_repositoryTypeMarker', () => { }); }); +describe('RULE11_referencedColumns', () => { + it('RULE11-001: an FK targeting its target\'s own single-column PK gets no fixup (the common case)', () => { + const rel = fixtureProp({ name: 'user', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['user_id'], type: 'Users', referencedColumnNames: ['id'] }); + const owner = fixtureMeta('OauthTokens', 'oauth_tokens', [rel]); + const target = fixtureMeta('Users', 'users', [fixtureProp({ name: 'id', primary: true })]); + const fixups = RULE11_referencedColumns([owner, target]); + expect(fixups).toEqual([]); + }); + + it('RULE11-002: an FK targeting a non-PK column (the real oauth_tokens.client_id -> oauth_clients.client_id case) gets a fixup', () => { + const rel = fixtureProp({ name: 'client', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['client_id'], type: 'OauthClients', referencedColumnNames: ['client_id'] }); + const owner = fixtureMeta('OauthTokens', 'oauth_tokens', [rel]); + const target = fixtureMeta('OauthClients', 'oauth_clients', [fixtureProp({ name: 'id', primary: true })]); + const fixups = RULE11_referencedColumns([owner, target]); + expect(fixups).toEqual([{ className: 'OauthTokens', propName: 'client', referencedColumnNames: ['client_id'] }]); + }); + + it('RULE11-003: a text PK target whose FK genuinely references that PK (school_holiday_regions.country -> school_holiday_countries.code) gets no fixup', () => { + const rel = fixtureProp({ name: 'countryRef', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['country'], type: 'SchoolHolidayCountries', referencedColumnNames: ['code'] }); + const owner = fixtureMeta('SchoolHolidayRegions', 'school_holiday_regions', [rel]); + const target = fixtureMeta('SchoolHolidayCountries', 'school_holiday_countries', [fixtureProp({ name: 'code', primary: true })]); + const fixups = RULE11_referencedColumns([owner, target]); + expect(fixups).toEqual([]); + }); + + it('RULE11-004: an unresolved relation target (no matching metadata) is skipped, not thrown or flagged', () => { + const rel = fixtureProp({ name: 'something', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['something_id'], type: 'Nowhere', referencedColumnNames: ['id'] }); + const owner = fixtureMeta('X', 'x', [rel]); + expect(() => RULE11_referencedColumns([owner])).not.toThrow(); + expect(RULE11_referencedColumns([owner])).toEqual([]); + }); + + it('RULE11-005: an inverse relation (oneToMany) is never a candidate — only an owning to-one is checked', () => { + const inverse = fixtureProp({ name: 'oauth_tokens_collection', kind: ReferenceKind.ONE_TO_MANY, type: 'OauthTokens', mappedBy: 'client', referencedColumnNames: ['id'] }); + const owner = fixtureMeta('OauthClients', 'oauth_clients', [inverse]); + const target = fixtureMeta('OauthTokens', 'oauth_tokens', [fixtureProp({ name: 'id', primary: true })]); + const fixups = RULE11_referencedColumns([owner, target]); + expect(fixups).toEqual([]); + }); + + it('RULE11-006: a composite-PK target compares the full column list, in order', () => { + const rel = fixtureProp({ name: 'setting', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['a', 'b'], type: 'Composite', referencedColumnNames: ['b', 'a'] }); + const owner = fixtureMeta('X', 'x', [rel]); + const target = fixtureMeta('Composite', 'composite', [ + fixtureProp({ name: 'a', primary: true }), + fixtureProp({ name: 'b', primary: true }), + ]); + const fixups = RULE11_referencedColumns([owner, target]); + expect(fixups).toEqual([{ className: 'X', propName: 'setting', referencedColumnNames: ['b', 'a'] }]); + }); +}); + describe('RULE9_addImplicitUniqueConstraints', () => { it('RULE9-001: a table with a matching implicit-unique entry gets a uniques: block referencing the columns as properties — a plain (non-FK) column keeps its own name', () => { const userId = fixtureProp({ name: 'user_id', primary: false }); @@ -806,6 +861,51 @@ describe('injectRepositoryTypeMarker', () => { }); }); +describe('injectReferencedColumns', () => { + it('TEXT-REFCOL-001: appends .referencedColumnNames() before the trailing comma (the real client_id shape)', () => { + const source = " client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden(),\n"; + const out = injectReferencedColumns(source, [{ className: 'OauthTokens', propName: 'client', referencedColumnNames: ['client_id'] }]); + expect(out).toBe(" client: () => p.manyToOne(OauthClients).ref().name('client_id').deleteRule('cascade').hidden().referencedColumnNames('client_id'),\n"); + }); + + it('TEXT-REFCOL-002: multiple referenced columns render as bare comma-separated quoted strings, no array brackets (rest-args API)', () => { + const source = ' setting: () => p.manyToOne(Composite).ref(),\n'; + const out = injectReferencedColumns(source, [{ className: 'X', propName: 'setting', referencedColumnNames: ['b', 'a'] }]); + expect(out).toContain(".referencedColumnNames('b', 'a')"); + expect(out).not.toContain('['); + }); + + it('TEXT-REFCOL-003: a line already carrying an explicit .referencedColumnNames( call is left untouched (legitimate no-op)', () => { + const source = " client: () => p.manyToOne(OauthClients).ref().referencedColumnNames('client_id'),\n"; + const out = injectReferencedColumns(source, [{ className: 'X', propName: 'client', referencedColumnNames: ['client_id'] }]); + expect(out).toBe(source); + }); + + it('TEXT-REFCOL-004: appends before a trailing comma even with a line comment after it', () => { + const source = ' client: () => p.manyToOne(Y).ref(), // pins client\n'; + const out = injectReferencedColumns(source, [{ className: 'X', propName: 'client', referencedColumnNames: ['client_id'] }]); + expect(out).toBe(" client: () => p.manyToOne(Y).ref().referencedColumnNames('client_id'), // pins client\n"); + }); + + it('TEXT-REFCOL-005: throws, naming the class and property, if the property line is not found', () => { + expect(() => injectReferencedColumns('', [{ className: 'X', propName: 'client', referencedColumnNames: ['client_id'] }])).toThrow( + /could not find the property line for "X\.client"/, + ); + }); + + it('TEXT-REFCOL-006: throws if the property line has no trailing comma to anchor on', () => { + const source = ' client: () => p.manyToOne(Y).ref()'; + expect(() => injectReferencedColumns(source, [{ className: 'X', propName: 'client', referencedColumnNames: ['client_id'] }])).toThrow( + /could not append/, + ); + }); + + it('TEXT-REFCOL-007: an empty fixups list is a no-op', () => { + const source = " client: () => p.manyToOne(OauthClients).ref().name('client_id'),\n"; + expect(injectReferencedColumns(source, [])).toBe(source); + }); +}); + describe('applyTextPasses', () => { it('TEXT-ALL-001: composes every pass without one undoing another', () => { const source = @@ -821,8 +921,10 @@ describe('applyTextPasses', () => { jsonColumns: [], retypedScalars: [], repositoryMarkers: [{ className: 'X', repositoryClassName: 'XRepository' }], + referencedColumns: [{ className: 'X', propName: 'countryRef', referencedColumnNames: ['code'] }], }); expect(out).toContain(".joinColumn('country')"); + expect(out).toContain(".referencedColumnNames('code')"); expect(out).not.toContain('& Hidden'); expect(out).toContain('export class X {\n [EntityRepositoryType]?: XRepository;\n countryRef!: Ref;\n'); }); diff --git a/server/tests/unit/db/repositories/OauthTokens.repository.test.ts b/server/tests/unit/db/repositories/OauthTokens.repository.test.ts index 48bea5281..2abcaf030 100644 --- a/server/tests/unit/db/repositories/OauthTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/OauthTokens.repository.test.ts @@ -344,7 +344,7 @@ describe('OauthTokensRepository', () => { }); }); - describe('listActiveByUser (OA26) — Kysely join on the real oauth_clients.client_id column', () => { + describe('listActiveByUser (OA26) — QueryBuilder join on the real oauth_clients.client_id column', () => { it('OAUTHTOKREPO-022: lists only this user\'s non-revoked, non-expired-refresh sessions, newest first, with the client name joined in', async () => { const { user } = createUser(testDb); const { user: other } = createUser(testDb); @@ -434,6 +434,57 @@ describe('OauthTokensRepository', () => { }); }); + describe('client relation join (Plan 3b interlude A — RULE11_referencedColumns)', () => { + it('OAUTHTOKREPO-030: a QueryBuilder join on the client relation resolves the right client, even though oauth_clients.id !== client_id', async () => { + const { user } = createUser(testDb); + // seedClient's own `id` ("row-") is already never equal to + // `client_id` — this test additionally asserts the two literally + // differ, so a future fixture change can't silently make the + // distinction disappear and hide a regression. + seedClient(user.id, 'proto-30', 'Client Thirty'); + const clientRow = testDb.prepare('SELECT id, client_id FROM oauth_clients WHERE client_id = ?').get('proto-30') as { id: string; client_id: string }; + expect(clientRow.id).not.toBe(clientRow.client_id); + const id = seedToken({ clientId: 'proto-30', userId: user.id }); + + const em = t.orm.em.fork(); + const qb = em.getRepository(OauthTokens).qb('ot').innerJoin('ot.client', 'oc').select(['ot.id', 'oc.name as client_name']).where({ 'ot.id': id }); + expect(qb.getFormattedQuery()).toContain('inner join `oauth_clients` as `oc` on `ot`.`client_id` = `oc`.`client_id`'); + const row = await qb.execute<{ id: number; client_name: string }>('get', false); + expect(row).toEqual({ id, client_name: 'Client Thirty' }); + }); + + it('OAUTHTOKREPO-031: a DECOY client whose row id equals the real client_id — regression-proofs the join against silently falling back to oauth_clients.id (task-4-review.md, "For Task 5 / interlude A")', async () => { + const { user } = createUser(testDb); + seedClient(user.id, 'proto-31-real', 'Real Client'); + const realClientRow = testDb + .prepare('SELECT id, client_id FROM oauth_clients WHERE client_id = ?') + .get('proto-31-real') as { id: string; client_id: string }; + // The decoy's own PRIMARY KEY (`id`) is deliberately set to the REAL + // client's `client_id` — a wrong join (`oc.id = ot.client_id`, the pre- + // Rule-11 defect) would match THIS row instead of the real one, and + // return the decoy's name, not silently return nothing. A regression + // to the wrong column is caught by a WRONG answer, not just an absent + // one — the strongest form of this proof. + testDb.prepare( + `INSERT INTO oauth_clients (id, user_id, name, client_id, client_secret_hash, redirect_uris, allowed_scopes) VALUES (?, ?, ?, ?, ?, '[]', '[]')`, + ).run(realClientRow.client_id, user.id, 'Decoy Client', 'proto-31-decoy', 'hash'); + + const id = seedToken({ clientId: 'proto-31-real', userId: user.id }); + + const em = t.orm.em.fork(); + const qb = em.getRepository(OauthTokens).qb('ot').innerJoin('ot.client', 'oc').select(['ot.id', 'oc.name as client_name']).where({ 'ot.id': id }); + const row = await qb.execute<{ id: number; client_name: string }>('get', false); + expect(row).toEqual({ id, client_name: 'Real Client' }); + + // Same proof through the actual repository methods (OA26/OA31), not + // just a hand-rolled QueryBuilder query. + const active = await tokens.listActiveByUser(user.id); + expect(active.map((r) => r.client_name)).toEqual(['Real Client']); + const allActive = await tokens.listAllActiveWithClientAndUser(); + expect(allActive.map((r) => r.client_name)).toEqual(['Real Client']); + }); + }); + describe('identity-map write-back regression (program RULING, D-shape)', () => { it('OAUTHTOKREPO-029: findByRefreshTokenHash (projection A) then findParent (projection B) then revokeById inside uow.transactional — the write survives', async () => { const { user } = createUser(testDb); diff --git a/server/tests/unit/nest/common/row-id.test.ts b/server/tests/unit/nest/common/row-id.test.ts index 29412e9fd..1c9ef9b03 100644 --- a/server/tests/unit/nest/common/row-id.test.ts +++ b/server/tests/unit/nest/common/row-id.test.ts @@ -7,11 +7,11 @@ describe('toRowId', () => { expect(toRowId('abc')).toBeNull(); }); - it('ROWID-002: rejects a hex-shaped string — Number() would accept it, SQLite affinity never would', () => { + it('ROWID-002: rejects a hex-shaped string — Number() would accept it, SQLite affinity never would (verified against better-sqlite3)', () => { expect(toRowId('0x10')).toBeNull(); }); - it('ROWID-003: rejects an exponent-shaped string — Number() would accept it, SQLite affinity never would', () => { + it('ROWID-003: rejects an exponent-shaped string ("1e3") — a DELIBERATE, ACCEPTED deviation: SQLite\'s own numeric-affinity comparison DOES match this against integer 1000 (verified against better-sqlite3, not assumed — the row-id.ts docstring previously claimed the opposite), but no id this guard ever sees originates that way (every id is a canonical decimal string from our own client), so this fails closed to the route\'s legacy not-found result instead of widening the guard to match SQLite\'s affinity exactly', () => { expect(toRowId('1e3')).toBeNull(); }); @@ -19,14 +19,22 @@ describe('toRowId', () => { expect(toRowId('-1')).toBeNull(); }); - it('ROWID-005: rejects a leading space', () => { + it('ROWID-005: rejects a leading space (" 1") — same deliberate deviation as ROWID-003: SQLite\'s affinity conversion ignores leading whitespace and matches integer 1 (verified against better-sqlite3)', () => { expect(toRowId(' 1')).toBeNull(); }); - it('ROWID-006: rejects a decimal point', () => { + it('ROWID-005B: rejects a trailing space ("1 ") — same deliberate deviation: SQLite\'s affinity conversion ignores trailing whitespace too and matches integer 1 (verified against better-sqlite3)', () => { + expect(toRowId('1 ')).toBeNull(); + }); + + it('ROWID-006: rejects a decimal point ("1.0") — same deliberate deviation: SQLite\'s affinity conversion parses this as REAL 1.0, which compares equal to INTEGER 1 (verified against better-sqlite3)', () => { expect(toRowId('1.0')).toBeNull(); }); + it('ROWID-006B: rejects a leading plus sign ("+1") — same deliberate deviation: SQLite\'s affinity conversion accepts a leading sign and matches integer 1 (verified against better-sqlite3)', () => { + expect(toRowId('+1')).toBeNull(); + }); + it('ROWID-007: rejects an empty string', () => { expect(toRowId('')).toBeNull(); }); @@ -39,6 +47,10 @@ describe('toRowId', () => { expect(toRowId('42')).toBe(42); }); + it('ROWID-009B: accepts a leading-zero digit string ("007") — SQLite\'s own affinity conversion resolves this to integer 7 too (verified against better-sqlite3), and the guard\'s regex (/^\\d+$/) must keep matching it: never tighten it to /^(0|[1-9]\\d*)$/', () => { + expect(toRowId('007')).toBe(7); + }); + it('ROWID-010: accepts an already-numeric safe integer', () => { expect(toRowId(42)).toBe(42); });