mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-22 23:16:46 +00:00
fix(db): row-out repository reads bypass the identity map — stale write-back over nativeUpdate at flush; insertUser via em.insert; settings round-trip pinned
This commit is contained in:
@@ -20,14 +20,17 @@ export class AddonsRepository extends EntityRepository<Addons> {
|
||||
/**
|
||||
* `SELECT enabled FROM addons WHERE id = ?`
|
||||
*
|
||||
* `refresh: true` (Task 0 review, I1 — carried forward for every PK read in
|
||||
* this plan): a primary-key `findOne` answers a repeat call from the
|
||||
* identity map, invisible to a write on the same id inside the same
|
||||
* request (`admin.service.ts`'s still-raw `UPDATE addons SET enabled = ?`
|
||||
* on the same connection).
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan
|
||||
* 3a's I1 "`refresh: true` on every PK-only `findOne`" — see
|
||||
* `Users.repository.ts`'s class-level docstring and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): a
|
||||
* throwaway forked context always sees a write on the same id inside the
|
||||
* same request (`admin.service.ts`'s still-raw `UPDATE addons SET enabled
|
||||
* = ?` on the same connection), and the entity never lands in the
|
||||
* request's identity map.
|
||||
*/
|
||||
async isEnabled(id: string): Promise<boolean> {
|
||||
const row = await this.findOne({ id }, { fields: ['enabled'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['enabled'], disableIdentityMap: true });
|
||||
return !!row?.enabled;
|
||||
}
|
||||
|
||||
@@ -40,7 +43,7 @@ export class AddonsRepository extends EntityRepository<Addons> {
|
||||
* for no parity benefit.
|
||||
*/
|
||||
async listEnabled(): Promise<AddonRow[]> {
|
||||
const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' } });
|
||||
const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' }, disableIdentityMap: true });
|
||||
return rows.map((row) => toRow(row) as AddonRow);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,19 +14,17 @@ export class AppSettingsRepository extends EntityRepository<AppSettings> {
|
||||
/**
|
||||
* `SELECT value FROM app_settings WHERE key = ?`
|
||||
*
|
||||
* `refresh: true` (Task 0 review, I1): this is a primary-key lookup, so
|
||||
* without it MikroORM answers a repeat call from the identity map rather
|
||||
* than re-querying — invisible to a raw/native write or `deleteValue` on
|
||||
* the same key inside the same request, which is exactly the coexistence
|
||||
* case this plan is built on (raw SQL and the ORM sharing one connection
|
||||
* while callers convert one at a time). `refresh` keeps this a single
|
||||
* query either way; only a stale identity-map hit is avoided.
|
||||
* Side effect of `refresh`: an UNFLUSHED in-memory change to the selected
|
||||
* field on that entity is discarded (the entity reverts to the row). No
|
||||
* caller mutates these entities before reading, by design (D4: rows out).
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan
|
||||
* 3a's I1 "`refresh: true` on every PK-only `findOne`" — see
|
||||
* `Users.repository.ts`'s class-level docstring for the full mechanism and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): the read
|
||||
* is answered from a throwaway forked context that is cleared afterwards,
|
||||
* so a raw/native write or `deleteValue` on the same key inside the same
|
||||
* request is always seen, and the entity never lands in the request's
|
||||
* identity map to become a stale pending change at a later `flush()`.
|
||||
*/
|
||||
async getValue(key: string): Promise<string | null> {
|
||||
const row = await this.findOne({ key }, { fields: ['value'], refresh: true });
|
||||
const row = await this.findOne({ key }, { fields: ['value'], disableIdentityMap: true });
|
||||
return row?.value ?? null;
|
||||
}
|
||||
|
||||
@@ -37,7 +35,7 @@ export class AppSettingsRepository extends EntityRepository<AppSettings> {
|
||||
* value" from "no row" (both read as absent).
|
||||
*/
|
||||
async getValues(keys: string[]): Promise<Map<string, string>> {
|
||||
const rows = await this.find({ key: { $in: keys } });
|
||||
const rows = await this.find({ key: { $in: keys } }, { disableIdentityMap: true });
|
||||
const values = new Map<string, string>();
|
||||
for (const row of rows) {
|
||||
if (row.key != null && row.value != null) values.set(row.key, row.value);
|
||||
@@ -82,7 +80,7 @@ export class AppSettingsRepository extends EntityRepository<AppSettings> {
|
||||
* a prefix that itself contains `%`/`_`.
|
||||
*/
|
||||
async findByKeyPrefix(prefix: string): Promise<AppSettingsRow[]> {
|
||||
const rows = await this.find({ key: { $like: `${prefix}%` } });
|
||||
const rows = await this.find({ key: { $like: `${prefix}%` } }, { disableIdentityMap: true });
|
||||
return rows.map((row) => toRow(row) as AppSettingsRow);
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ const _categoryRowKeys: AssertRowKeys<CategoryRow, Categories> = true;
|
||||
export class CategoriesRepository extends EntityRepository<Categories> {
|
||||
/** `SELECT * FROM categories ORDER BY name ASC` */
|
||||
async list(): Promise<CategoryRow[]> {
|
||||
const categories = await this.find({}, { orderBy: { name: 'asc' } });
|
||||
const categories = await this.find({}, { orderBy: { name: 'asc' }, disableIdentityMap: true });
|
||||
return categories.map((category) => toRow(category) as CategoryRow);
|
||||
}
|
||||
|
||||
@@ -30,19 +30,18 @@ export class CategoriesRepository extends EntityRepository<Categories> {
|
||||
* global") and was deleted rather than kept as dead code (Task 0
|
||||
* re-review, R6; see `_shared/owned-lookup.ts`'s docstring).
|
||||
*
|
||||
* `refresh: true` (Task 0 review, I1 — every PK-only `findOne` in a
|
||||
* repository takes it) and it is load-bearing here, not decorative: `{ id
|
||||
* }` is exactly the primary key, so MikroORM answers this call from the
|
||||
* identity map with zero queries whenever the row is already loaded
|
||||
* (Task 3 review, Important 2's correction — the short-circuit is driven
|
||||
* by the filter being PK-only, not by a `fields` restriction, which was
|
||||
* the wrong mechanism this docstring stated before the fix). Without
|
||||
* `refresh`, a raw write to this row earlier in the same request would
|
||||
* make this method return the stale snapshot instead of the current DB
|
||||
* row.
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan
|
||||
* 3a's I1 "`refresh: true` on every PK-only `findOne`" — see
|
||||
* `Users.repository.ts`'s class-level docstring and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): this is
|
||||
* a "rows out" read, converted via `toRow` and discarded — the entity is
|
||||
* loaded in a throwaway forked context and never lands in the request's
|
||||
* identity map, so a raw write to this row earlier in the same request is
|
||||
* always seen, and there is nothing here for a later `flush()` to find
|
||||
* dirty.
|
||||
*/
|
||||
async findById(id: number): Promise<CategoryRow | null> {
|
||||
const category = await this.findOne({ id }, { refresh: true });
|
||||
const category = await this.findOne({ id }, { disableIdentityMap: true });
|
||||
return category ? (toRow(category) as CategoryRow) : null;
|
||||
}
|
||||
|
||||
@@ -103,6 +102,16 @@ export class CategoriesRepository extends EntityRepository<Categories> {
|
||||
* managed (and now guaranteed-fresh) entity in place, and no column here
|
||||
* is DB-computed on UPDATE (unlike `created_at` on insert), so the
|
||||
* in-memory entity already holds exactly what was written.
|
||||
*
|
||||
* **Excluded from the Plan 3b Task 1 fix round's `disableIdentityMap: true`
|
||||
* ruling, deliberately** — see `TagsRepository.patch`'s docstring for the
|
||||
* full reasoning: this is not a "rows out" read, the entity must stay
|
||||
* MANAGED for `assign`+`flush`, and `disableIdentityMap: true` would
|
||||
* silently stop the write from persisting (a detached entity is outside
|
||||
* `flush()`'s unit of work). `refresh: true` stays for the identity-map
|
||||
* freshness reason above; there is no B1-shaped staleness risk because
|
||||
* this is the only read of this row left un-`disableIdentityMap`-ed in the
|
||||
* repository.
|
||||
*/
|
||||
async patch(id: number, changes: { name?: string; color?: string; icon?: string }): Promise<CategoryRow | null> {
|
||||
const category = await this.findOne({ id }, { refresh: true });
|
||||
|
||||
@@ -28,9 +28,12 @@ export class PhotoProviderFieldsRepository extends EntityRepository<PhotoProvide
|
||||
* `persist(false)` twin of the `provider` relation (Days.trip_id
|
||||
* precedent): reading it back after `find()` needs no extra populate, only
|
||||
* a `create()`/twin write would.
|
||||
*
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round) — a "rows out"
|
||||
* read, converted via `toRow` and discarded.
|
||||
*/
|
||||
async listAllOrdered(): Promise<PhotoProviderFieldRow[]> {
|
||||
const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' } });
|
||||
const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' }, disableIdentityMap: true });
|
||||
return rows.map((row) => toRow(row) as PhotoProviderFieldRow);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -21,9 +21,12 @@ export class PhotoProvidersRepository extends EntityRepository<PhotoProviders> {
|
||||
* entity (unlike `Addons.enabled`, which is `p.boolean()`), so it comes
|
||||
* back as the raw stored `0`/`1`; `AddonsService.list()` does the `!!`
|
||||
* coercion itself, matching the legacy `!!p.enabled`.
|
||||
*
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round) — a "rows out"
|
||||
* read, converted via `toRow` and discarded.
|
||||
*/
|
||||
async listEnabled(): Promise<PhotoProviderRow[]> {
|
||||
const rows = await this.find({ enabled: 1 }, { orderBy: { sort_order: 'asc', id: 'asc' } });
|
||||
const rows = await this.find({ enabled: 1 }, { orderBy: { sort_order: 'asc', id: 'asc' }, disableIdentityMap: true });
|
||||
return rows.map((row) => toRow(row) as PhotoProviderRow);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -13,28 +13,33 @@ export interface SettingRow {
|
||||
const _settingRowKeys: AssertRowKeys<SettingRow, Settings> = true;
|
||||
|
||||
export class SettingsRepository extends EntityRepository<Settings> {
|
||||
/** `SELECT key, value FROM settings WHERE user_id = ?` */
|
||||
/**
|
||||
* `SELECT key, value FROM settings WHERE user_id = ?`
|
||||
*
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round) — a "rows out"
|
||||
* read, converted via `toRow` and discarded.
|
||||
*/
|
||||
async getForUser(userId: number): Promise<SettingRow[]> {
|
||||
const rows = await this.find({ user: userId });
|
||||
const rows = await this.find({ user: userId }, { disableIdentityMap: true });
|
||||
return rows.map((row) => toRow(row) as SettingRow);
|
||||
}
|
||||
|
||||
/**
|
||||
* `SELECT value FROM settings WHERE user_id = ? AND key = ?`
|
||||
*
|
||||
* No `refresh: true`: the identity-map short-circuit (Task 0 review, I1;
|
||||
* corrected by the Task 3 review — it is NOT about a `fields` restriction)
|
||||
* only fires for a PRIMARY-KEY-ONLY filter (`findOne({ id })` / `findOne(pk)`):
|
||||
* MikroORM answers that shape straight from the identity map with zero
|
||||
* queries, invisible to a write on the same row earlier in the request.
|
||||
* This filter is `(user, key)` — `Settings`'s PK is the surrogate `id`, not
|
||||
* this composite — so it is never PK-only and always re-queries, the same
|
||||
* guarantee a `find()` call gets. `SETTINGSREPO-012`/`013` pin this
|
||||
* directly (a raw UPDATE/DELETE on the same row, then `getOne` in the same
|
||||
* request, sees it) rather than assume it.
|
||||
* `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan
|
||||
* 3a's I1 — see `Users.repository.ts`'s class-level docstring and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): this
|
||||
* filter is `(user, key)` — `Settings`'s PK is the surrogate `id`, not
|
||||
* this composite — so it was never PK-only and always re-queried even
|
||||
* before this ruling; `disableIdentityMap: true` is applied uniformly
|
||||
* regardless, matching every other read in this repository.
|
||||
* `SETTINGSREPO-012`/`013` pin the always-fresh-read property directly (a
|
||||
* raw UPDATE/DELETE on the same row, then `getOne` in the same request,
|
||||
* sees it) rather than assume it.
|
||||
*/
|
||||
async getOne(userId: number, key: string): Promise<SettingRow | null> {
|
||||
const row = await this.findOne({ user: userId, key });
|
||||
const row = await this.findOne({ user: userId, key }, { disableIdentityMap: true });
|
||||
return row ? (toRow(row) as SettingRow) : null;
|
||||
}
|
||||
|
||||
|
||||
@@ -32,21 +32,12 @@ export class TagsRepository extends EntityRepository<Tags> {
|
||||
* own write (`createTag`/`patch` refresh/return their own entity
|
||||
* directly).
|
||||
*
|
||||
* `refresh: true` (Task 0 review, I1 — the ruling every Plan 3 repository
|
||||
* follows for a PK-scoped read). This call's filter is `{ id, user }`,
|
||||
* not PK-only, so MikroORM's identity-map short-circuit — which fires
|
||||
* only when the filter is exactly the primary key, not (as an earlier
|
||||
* version of this docstring said) when a `fields` restriction is present
|
||||
* (Task 3 review, Important 2's correction) — never engages here anyway:
|
||||
* this call already re-queries and merges fresh data on every call
|
||||
* regardless of `refresh`. `refresh` stays threaded through to match the
|
||||
* blanket ruling and to stay correct if a future caller narrows the
|
||||
* filter to PK-only. `findOwnedByUser`'s optional `options` parameter
|
||||
* (added here, not duplicated — Task 0's helper had no reads that needed
|
||||
* it yet) passes straight through to the underlying `findOne`.
|
||||
* `disableIdentityMap: true` per `findOwnedByUser`'s own docstring (Plan
|
||||
* 3b Task 1 fix round, supersedes Plan 3a's I1) — this is a "rows out"
|
||||
* read, converted via `toRow` and discarded.
|
||||
*/
|
||||
async findByIdAndUser(id: number, userId: number): Promise<TagRow | null> {
|
||||
const tag = await findOwnedByUser<Tags, 'user'>(this, id, 'user', userId, { refresh: true });
|
||||
const tag = await findOwnedByUser<Tags, 'user'>(this, id, 'user', userId);
|
||||
return tag ? (toRow(tag) as TagRow) : null;
|
||||
}
|
||||
|
||||
@@ -98,6 +89,20 @@ export class TagsRepository extends EntityRepository<Tags> {
|
||||
* `CategoriesRepository.patch`: `assign` already mutated the managed (and
|
||||
* now guaranteed-fresh) entity in place, and no column here is
|
||||
* DB-computed on UPDATE.
|
||||
*
|
||||
* **Excluded from the Plan 3b Task 1 fix round's `disableIdentityMap: true`
|
||||
* ruling, deliberately**: unlike every other read in this repository, this
|
||||
* one is not a "rows out" read — the entity is kept MANAGED on purpose so
|
||||
* `assign`+`flush` can diff and write it. `disableIdentityMap: true` would
|
||||
* return a *detached* entity (per the ruling's own mechanism); `assign`ing
|
||||
* it would still mutate the JS object, but the following `flush()` would
|
||||
* have no effect on it (a detached entity is not in `flush()`'s unit of
|
||||
* work) — the write would silently stop persisting. `refresh: true` stays
|
||||
* here for exactly the reason it always was: this is the only call in the
|
||||
* repository that manages this row, so there is no B1-shaped staleness
|
||||
* risk (that requires a SECOND managed, differently-projected read of the
|
||||
* same row still lingering in the identity map — nothing else here leaves
|
||||
* one now that every other read is `disableIdentityMap: true`).
|
||||
*/
|
||||
async patch(id: number, changes: { name?: string; color?: string }): Promise<TagRow | null> {
|
||||
const tag = await this.findOne({ id }, { refresh: true });
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import type { Users } from '../entities/Users.entity';
|
||||
import { Users } from '../entities/Users.entity';
|
||||
import { toRow, type AssertRowKeys } from './_shared/rows';
|
||||
import { columnIncrementedBy, currentTimestamp, lower } from '../dialect/sql-functions';
|
||||
import { EntityRepository } from '@mikro-orm/sql';
|
||||
@@ -177,18 +177,23 @@ export interface ResetTargetRow {
|
||||
|
||||
export class UsersRepository extends EntityRepository<Users> {
|
||||
/**
|
||||
* `SELECT email FROM users WHERE id = ?`
|
||||
*
|
||||
* `refresh: true` (Task 0 review, I1): a primary-key `findOne` is answered
|
||||
* from the identity map on a repeat call, which would hide a raw/native
|
||||
* `UPDATE users SET email = ...` on the same id inside the same request.
|
||||
* `refresh` keeps this a single query and always sees the current row.
|
||||
* Side effect of `refresh`: an UNFLUSHED in-memory change to the selected
|
||||
* field on that entity is discarded (the entity reverts to the row). No
|
||||
* caller mutates these entities before reading, by design (D4: rows out).
|
||||
* **Ruling (Plan 3b Task 1 fix round, supersedes Plan 3a's I1 "`refresh:
|
||||
* true` on every PK-only `findOne`"):** every row-out read in this
|
||||
* repository (`findOne`/`find`) passes `disableIdentityMap: true` instead.
|
||||
* MikroORM forks the EM with `keepTransactionContext: true`, loads inside
|
||||
* that fork (so the read still sees the transaction's own uncommitted
|
||||
* writes), and clears the fork afterwards
|
||||
* (`node_modules/@mikro-orm/core/EntityManager.js:807-814`) — the entity
|
||||
* is never added to the *request's* identity map, so it can never become a
|
||||
* stale pending change-set entry that a later `flush()` (including the
|
||||
* implicit one at `UnitOfWork.transactional` commit) writes back over an
|
||||
* intervening `nativeUpdate`. `refresh: true` is dropped as redundant: it
|
||||
* only mattered for an entity that stayed in the map, and none does now.
|
||||
* See https://mikro-orm.io/docs/entity-manager#disableidentitymap and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` (B1).
|
||||
*/
|
||||
async getEmail(userId: number): Promise<string | null> {
|
||||
const row = await this.findOne({ id: userId }, { fields: ['email'], refresh: true });
|
||||
const row = await this.findOne({ id: userId }, { fields: ['email'], disableIdentityMap: true });
|
||||
return row?.email ?? null;
|
||||
}
|
||||
|
||||
@@ -198,10 +203,10 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
* not an interpolated column — MikroORM's `fields` option only accepts a
|
||||
* real property of `Users`, so this can never select an arbitrary column.
|
||||
*
|
||||
* `refresh: true` for the same identity-map reason as `getEmail` above.
|
||||
* `disableIdentityMap: true` per the class-level ruling above.
|
||||
*/
|
||||
async getApiKeyColumn(userId: number, name: InstanceApiKeyName): Promise<string | null> {
|
||||
const row = await this.findOne({ id: userId }, { fields: [name], refresh: true });
|
||||
const row = await this.findOne({ id: userId }, { fields: [name], disableIdentityMap: true });
|
||||
return row ? (row[name] ?? null) : null;
|
||||
}
|
||||
|
||||
@@ -214,12 +219,12 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
*
|
||||
* The canonical session-invalidation read: `verifyJwtAndLoadUser` compares
|
||||
* the returned `password_version` against the JWT's `pv` claim on every
|
||||
* request. PK-only `findOne` → `refresh: true` (I1).
|
||||
* request. `disableIdentityMap: true` per the class-level ruling above.
|
||||
*/
|
||||
async findByIdWithPasswordVersion(id: number): Promise<UserWithPasswordVersion | null> {
|
||||
const row = await this.findOne(
|
||||
{ id },
|
||||
{ fields: ['id', 'username', 'email', 'role', 'password_version'], refresh: true },
|
||||
{ fields: ['id', 'username', 'email', 'role', 'password_version'], disableIdentityMap: true },
|
||||
);
|
||||
return row ? { id: row.id, username: row.username, email: row.email, role: row.role, password_version: row.password_version } : null;
|
||||
}
|
||||
@@ -230,7 +235,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT password_version FROM users WHERE id = ?`. `?? 0` stays in the caller (AU2/TK7/O2 each fall back differently in spirit, identically in value). */
|
||||
async getPasswordVersion(id: number): Promise<number | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['password_version'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['password_version'], disableIdentityMap: true });
|
||||
return row?.password_version ?? null;
|
||||
}
|
||||
|
||||
@@ -249,7 +254,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT id FROM users WHERE role = 'admin' AND must_change_password = 1 LIMIT 1`. */
|
||||
async findAdminNeedingPasswordChange(): Promise<number | null> {
|
||||
const row = await this.findOne({ role: 'admin', must_change_password: 1 }, { fields: ['id'] });
|
||||
const row = await this.findOne({ role: 'admin', must_change_password: 1 }, { fields: ['id'], disableIdentityMap: true });
|
||||
return row?.id ?? null;
|
||||
}
|
||||
|
||||
@@ -268,7 +273,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
$or: [{ [lower(platform, 'email')]: email.toLowerCase() }, { [lower(platform, 'username')]: username.toLowerCase() }],
|
||||
is_guest: 0,
|
||||
},
|
||||
{ fields: ['id'] },
|
||||
{ fields: ['id'], disableIdentityMap: true },
|
||||
);
|
||||
return row?.id ?? null;
|
||||
}
|
||||
@@ -287,12 +292,19 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
* always the literal `0` on insert, never a parameter, matching both
|
||||
* statements exactly.
|
||||
*
|
||||
* Followed by `refresh` (D4's create rule), not relied-on `RETURNING`:
|
||||
* the returned row carries the generated `id` and the `defaultRaw`
|
||||
* `em.insert` (F2, Plan 3b Task 1 fix round), never `create()` +
|
||||
* `persist().flush()`: `flush()` commits the *whole* unit of work of the
|
||||
* request's `EntityManager`, not just this row — the review caught it
|
||||
* emitting a full-row `UPDATE users … WHERE id = ?` against an unrelated,
|
||||
* previously-read user in the same EM. `em.insert` fires a single native
|
||||
* INSERT with no side effects on the context/identity map (the same rule
|
||||
* as `AuditLogRepository.insertEntry`). Followed by a read-back
|
||||
* (`findById`, `disableIdentityMap: true` per the class-level ruling): the
|
||||
* returned row carries the generated `id` and the `defaultRaw`
|
||||
* `created_at` the same way a legacy INSERT-then-reselect pair did.
|
||||
*/
|
||||
async insertUser(row: NewUserRow): Promise<UserRow> {
|
||||
const user = this.create({
|
||||
const id = await this.getEntityManager().insert(Users, {
|
||||
username: row.username,
|
||||
email: row.email,
|
||||
password_hash: row.password_hash,
|
||||
@@ -303,9 +315,11 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
oidc_issuer: row.oidc_issuer ?? null,
|
||||
avatar: row.avatar ?? null,
|
||||
});
|
||||
await this.getEntityManager().persist(user).flush();
|
||||
await this.getEntityManager().refresh(user);
|
||||
return toRow(user) as UserRow;
|
||||
const inserted = await this.findById(id as number);
|
||||
if (!inserted) {
|
||||
throw new Error('insertUser: read-back after insert found no row');
|
||||
}
|
||||
return inserted;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------
|
||||
@@ -315,7 +329,27 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
/** `SELECT * FROM users WHERE LOWER(email) = LOWER(?) AND COALESCE(is_guest, 0) = 0` */
|
||||
async findByEmailCI(email: string): Promise<UserRow | null> {
|
||||
const platform = this.getEntityManager().getPlatform();
|
||||
const user = await this.findOne({ [lower(platform, 'email')]: email.toLowerCase(), is_guest: 0 });
|
||||
const user = await this.findOne(
|
||||
{ [lower(platform, 'email')]: email.toLowerCase(), is_guest: 0 },
|
||||
{ disableIdentityMap: true },
|
||||
);
|
||||
return user ? (toRow(user) as UserRow) : null;
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------
|
||||
// AuthService.demoLogin (auth.service.ts:~376) — inventory-correction
|
||||
// addition, Plan 3b Task 1 fix round item 7 (pre-authorised for Task 5).
|
||||
// ---------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* `SELECT * FROM users WHERE email = ?` — case-**sensitive**, **no** guest
|
||||
* filter, full row. Distinct from both `findByEmailCI` (case-insensitive
|
||||
* + guest-filtered) and `findForPasswordReset` (case-sensitive but
|
||||
* projected + guest-filtered): `AuthService.demoLogin` needs the exact,
|
||||
* unfiltered legacy statement, including matching a guest row.
|
||||
*/
|
||||
async findByEmailExact(email: string): Promise<UserRow | null> {
|
||||
const user = await this.findOne({ email }, { disableIdentityMap: true });
|
||||
return user ? (toRow(user) as UserRow) : null;
|
||||
}
|
||||
|
||||
@@ -350,7 +384,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
{ id },
|
||||
{
|
||||
fields: ['id', 'username', 'email', 'role', 'avatar', 'oidc_issuer', 'created_at', 'mfa_enabled', 'must_change_password'],
|
||||
refresh: true,
|
||||
disableIdentityMap: true,
|
||||
},
|
||||
);
|
||||
return row
|
||||
@@ -374,7 +408,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT password_hash, password_version FROM users WHERE id = ?` */
|
||||
async getPasswordHashAndVersion(id: number): Promise<{ password_hash: string; password_version: number } | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['password_hash', 'password_version'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['password_hash', 'password_version'], disableIdentityMap: true });
|
||||
return row ? { password_hash: row.password_hash, password_version: row.password_version } : null;
|
||||
}
|
||||
|
||||
@@ -410,7 +444,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT role FROM users WHERE id = ?` */
|
||||
async getRole(id: number): Promise<string | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['role'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['role'], disableIdentityMap: true });
|
||||
return row?.role ?? null;
|
||||
}
|
||||
|
||||
@@ -430,7 +464,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
* collapse both into the same value.
|
||||
*/
|
||||
async getMfaEnabled(id: number): Promise<{ mfa_enabled: number | null } | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['mfa_enabled'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['mfa_enabled'], disableIdentityMap: true });
|
||||
return row ? { mfa_enabled: row.mfa_enabled ?? null } : null;
|
||||
}
|
||||
|
||||
@@ -460,10 +494,10 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
* caller that needs more than a handful of columns (AU30 `disableMfa`,
|
||||
* AU32 `verifyMfaLogin`, PK4 `passkeyRegisterOptions`, PK10
|
||||
* `passkeyLoginVerify`, O15 `findOrCreateUser`'s post-transaction re-select).
|
||||
* PK-only `findOne` → `refresh: true` (I1).
|
||||
* `disableIdentityMap: true` per the class-level ruling above.
|
||||
*/
|
||||
async findById(id: number): Promise<UserRow | null> {
|
||||
const user = await this.findOne({ id }, { refresh: true });
|
||||
const user = await this.findOne({ id }, { disableIdentityMap: true });
|
||||
return user ? (toRow(user) as UserRow) : null;
|
||||
}
|
||||
|
||||
@@ -518,7 +552,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
async findForPasswordReset(email: string): Promise<PasswordResetLookup | null> {
|
||||
const row = await this.findOne(
|
||||
{ email, is_guest: 0 },
|
||||
{ fields: ['id', 'email', 'password_hash', 'oidc_sub'] },
|
||||
{ fields: ['id', 'email', 'password_hash', 'oidc_sub'], disableIdentityMap: true },
|
||||
);
|
||||
return row ? { id: row.id, email: row.email, password_hash: row.password_hash, oidc_sub: row.oidc_sub ?? null } : null;
|
||||
}
|
||||
@@ -531,7 +565,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
async findResetTarget(id: number): Promise<ResetTargetRow | null> {
|
||||
const row = await this.findOne(
|
||||
{ id },
|
||||
{ fields: ['id', 'email', 'mfa_enabled', 'mfa_secret', 'mfa_backup_codes', 'password_version'], refresh: true },
|
||||
{ fields: ['id', 'email', 'mfa_enabled', 'mfa_secret', 'mfa_backup_codes', 'password_version'], disableIdentityMap: true },
|
||||
);
|
||||
return row
|
||||
? {
|
||||
@@ -551,7 +585,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT password_hash FROM users WHERE id = ?` */
|
||||
async getPasswordHash(id: number): Promise<string | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['password_hash'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['password_hash'], disableIdentityMap: true });
|
||||
return row?.password_hash ?? null;
|
||||
}
|
||||
|
||||
@@ -561,7 +595,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT id, email FROM users WHERE id = ?` */
|
||||
async findIdAndEmail(id: number): Promise<{ id: number; email: string } | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['id', 'email'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['id', 'email'], disableIdentityMap: true });
|
||||
return row ? { id: row.id, email: row.email } : null;
|
||||
}
|
||||
|
||||
@@ -571,7 +605,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT * FROM users WHERE oidc_sub = ? AND oidc_issuer = ?` */
|
||||
async findByOidcIdentity(sub: string, issuer: string): Promise<UserRow | null> {
|
||||
const user = await this.findOne({ oidc_sub: sub, oidc_issuer: issuer });
|
||||
const user = await this.findOne({ oidc_sub: sub, oidc_issuer: issuer }, { disableIdentityMap: true });
|
||||
return user ? (toRow(user) as UserRow) : null;
|
||||
}
|
||||
|
||||
@@ -624,7 +658,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
const platform = this.getEntityManager().getPlatform();
|
||||
const row = await this.findOne(
|
||||
{ [lower(platform, 'username')]: username.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 },
|
||||
{ fields: ['id'] },
|
||||
{ fields: ['id'], disableIdentityMap: true },
|
||||
);
|
||||
return row?.id ?? null;
|
||||
}
|
||||
@@ -638,7 +672,10 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
*/
|
||||
async findIdByUsernameCIAny(username: string): Promise<number | null> {
|
||||
const platform = this.getEntityManager().getPlatform();
|
||||
const row = await this.findOne({ [lower(platform, 'username')]: username.toLowerCase() }, { fields: ['id'] });
|
||||
const row = await this.findOne(
|
||||
{ [lower(platform, 'username')]: username.toLowerCase() },
|
||||
{ fields: ['id'], disableIdentityMap: true },
|
||||
);
|
||||
return row?.id ?? null;
|
||||
}
|
||||
|
||||
@@ -650,7 +687,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
async getApiKeyColumns(id: number): Promise<UserApiKeyColumns | null> {
|
||||
const row = await this.findOne(
|
||||
{ id },
|
||||
{ fields: ['role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key'], refresh: true },
|
||||
{ fields: ['role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key'], disableIdentityMap: true },
|
||||
);
|
||||
return row
|
||||
? {
|
||||
@@ -700,7 +737,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
{ id },
|
||||
{
|
||||
fields: ['id', 'username', 'email', 'role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key', 'avatar', 'mfa_enabled'],
|
||||
refresh: true,
|
||||
disableIdentityMap: true,
|
||||
},
|
||||
);
|
||||
return row
|
||||
@@ -728,7 +765,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
const platform = this.getEntityManager().getPlatform();
|
||||
const row = await this.findOne(
|
||||
{ [lower(platform, 'email')]: email.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 },
|
||||
{ fields: ['id'] },
|
||||
{ fields: ['id'], disableIdentityMap: true },
|
||||
);
|
||||
return row?.id ?? null;
|
||||
}
|
||||
@@ -761,7 +798,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT avatar FROM users WHERE id = ?` */
|
||||
async getAvatar(id: number): Promise<string | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['avatar'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['avatar'], disableIdentityMap: true });
|
||||
return row?.avatar ?? null;
|
||||
}
|
||||
|
||||
@@ -785,7 +822,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT id, username, email, role, avatar FROM users WHERE id = ?` */
|
||||
async findProfileBasic(id: number): Promise<UserProfileBasic | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['id', 'username', 'email', 'role', 'avatar'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['id', 'username', 'email', 'role', 'avatar'], disableIdentityMap: true });
|
||||
return row ? { id: row.id, username: row.username, email: row.email, role: row.role, avatar: row.avatar ?? null } : null;
|
||||
}
|
||||
|
||||
@@ -797,7 +834,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
async listOthersNonGuest(excludeId: number): Promise<UserDirectoryRow[]> {
|
||||
const rows = await this.find(
|
||||
{ id: { $ne: excludeId }, is_guest: 0 },
|
||||
{ fields: ['id', 'username', 'avatar'], orderBy: { username: 'asc' } },
|
||||
{ fields: ['id', 'username', 'avatar'], orderBy: { username: 'asc' }, disableIdentityMap: true },
|
||||
);
|
||||
return rows.map((row) => ({ id: row.id, username: row.username, avatar: row.avatar ?? null }));
|
||||
}
|
||||
@@ -808,7 +845,7 @@ export class UsersRepository extends EntityRepository<Users> {
|
||||
|
||||
/** `SELECT role, openweather_api_key FROM users WHERE id = ?` */
|
||||
async getRoleAndWeatherKey(id: number): Promise<{ role: string; openweather_api_key: string | null } | null> {
|
||||
const row = await this.findOne({ id }, { fields: ['role', 'openweather_api_key'], refresh: true });
|
||||
const row = await this.findOne({ id }, { fields: ['role', 'openweather_api_key'], disableIdentityMap: true });
|
||||
return row ? { role: row.role, openweather_api_key: row.openweather_api_key ?? null } : null;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,7 +36,13 @@ import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from
|
||||
*
|
||||
* listForOwner<Tags, 'user', 'name'>(tags, 'user', userId, 'name');
|
||||
* findOwnedByUser<Tags, 'user'>(tags, id, 'user', userId);
|
||||
* findOwnedByUser<Tags, 'user'>(tags, id, 'user', userId, { refresh: true });
|
||||
*
|
||||
* Both helpers pass `disableIdentityMap: true` (Plan 3b Task 1 fix round,
|
||||
* supersedes Plan 3a's I1 "`refresh: true` on every PK-only `findOne`" — see
|
||||
* `Users.repository.ts`'s class-level docstring and
|
||||
* `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): both are
|
||||
* "rows out" reads whose entity is converted via `toRow` and discarded, never
|
||||
* assigned to nor flushed, so a throwaway forked context is exactly right.
|
||||
*
|
||||
* Typing (Task 0 review, I2): `T extends { id: unknown }` and `K extends
|
||||
* keyof T` tie `ownerField` to a real property of `T` and `ownerId` to
|
||||
@@ -64,6 +70,7 @@ export async function listForOwner<T extends { id: unknown }, K extends keyof T,
|
||||
): Promise<T[]> {
|
||||
return repo.find({ [ownerField]: ownerId } as FilterQuery<T>, {
|
||||
orderBy: { [orderField]: 'asc' } as OrderDefinition<T>,
|
||||
disableIdentityMap: true,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -71,24 +78,15 @@ export async function listForOwner<T extends { id: unknown }, K extends keyof T,
|
||||
* Find one row by id, strictly scoped to an owner:
|
||||
* `SELECT * FROM <table> WHERE id = ? AND <ownerField> = ?`.
|
||||
*
|
||||
* `options.refresh` passes straight through to the underlying `findOne`
|
||||
* (Task 0 review, I1's PK-read ruling: a caller whose read can follow a
|
||||
* raw/native write on the same row within the same request sets it). This
|
||||
* filter is `{ id, [ownerField]: ownerId }` — not PK-only — so MikroORM's
|
||||
* identity-map short-circuit never engages here: that short-circuit fires
|
||||
* only when the filter is exactly the primary key (Task 3 review, Important
|
||||
* 2's correction — `fields` has nothing to do with it, unlike what an
|
||||
* earlier version of this docstring claimed). `refresh` is a harmless
|
||||
* no-op on every call this helper serves today; it stays threaded through
|
||||
* to match the blanket ruling and to stay correct if a future caller
|
||||
* narrows the filter to PK-only.
|
||||
* `disableIdentityMap: true` per the module-level docstring above — this is
|
||||
* always a "rows out" read here, never a caller's staging read for
|
||||
* `assign`+`flush`.
|
||||
*/
|
||||
export async function findOwnedByUser<T extends { id: unknown }, K extends keyof T>(
|
||||
repo: EntityRepository<T>,
|
||||
id: T['id'],
|
||||
ownerField: K,
|
||||
ownerId: FilterValue<T[K]>,
|
||||
options?: { refresh?: boolean },
|
||||
): Promise<T | null> {
|
||||
return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery<T>, options);
|
||||
return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery<T>, { disableIdentityMap: true });
|
||||
}
|
||||
|
||||
@@ -27,9 +27,21 @@ import { extractToken, verifyJwtAndLoadUser } from './jwt-verify';
|
||||
* ORM owns `users` everywhere else too) — so no module needs new wiring.
|
||||
* `this.em.getRepository(Users)` inside `canActivate` is the same
|
||||
* `orm.em.getRepository(Users)`-inside-the-request pattern
|
||||
* `platform.routes.ts::servePhoto` uses (Task 0); `this.em` here is already
|
||||
* the request-scoped fork, since `canActivate` only ever runs inside a real
|
||||
* HTTP request Nest's `@mikro-orm/nestjs` middleware has already wrapped.
|
||||
* `platform.routes.ts::servePhoto` uses (Task 0).
|
||||
*
|
||||
* **Correction (Plan 3b Task 1 fix round, task-1-review.md's Ruling):**
|
||||
* `this.em` here is NOT a request-scoped fork — with no `scope` on
|
||||
* `MikroOrmModule.forRoot` and no `Scope.REQUEST` anywhere in `src/`,
|
||||
* `@mikro-orm/nestjs`'s provider factory injects `orm.em`, the ORM's single
|
||||
* GLOBAL, context-resolving `EntityManager`, the same object every request
|
||||
* shares. It resolves to the request's transactional fork at QUERY TIME:
|
||||
* `EntityRepository.getRepository` builds the repository holding a
|
||||
* reference to `this.em` with no `getContext()` call, and
|
||||
* `EntityRepository.findOne` calls `this.getEntityManager().findOne(...)`,
|
||||
* which resolves the live `AsyncLocalStorage`/`TransactionContext` inside
|
||||
* that call — so the repository built here still finds the request's own
|
||||
* fork correctly, even though `this.em` itself is the shared global
|
||||
* instance, not a per-request object.
|
||||
*/
|
||||
@Injectable()
|
||||
export class JwtAuthGuard implements CanActivate {
|
||||
|
||||
@@ -79,6 +79,10 @@ export class MfaPolicyGuard implements CanActivate {
|
||||
// A user-verified passkey is phishing-resistant and inherently two-factor, so
|
||||
// owning at least one satisfies require_mfa exactly like TOTP does.
|
||||
// (All stored passkeys were registered with userVerification required.)
|
||||
// `=== 1`, not the legacy `row.mfa_enabled === 1 || row.mfa_enabled === true`:
|
||||
// `mfa_enabled` is a SQLite INTEGER column, and `UsersRepository.getMfaEnabled`
|
||||
// types it `number | null` — a JS `boolean` from this column never occurs
|
||||
// through the ORM, so the `=== true` half was dead code (task-1-review.md F4).
|
||||
const mfaOk = row.mfa_enabled === 1;
|
||||
const passkeyOk = await this.webauthnCredentials.hasAny(user.id);
|
||||
if (mfaOk || passkeyOk) return true;
|
||||
|
||||
Reference in New Issue
Block a user