diff --git a/server/src/db/repositories/Categories.repository.ts b/server/src/db/repositories/Categories.repository.ts index fc0c94ce0..9d9243038 100644 --- a/server/src/db/repositories/Categories.repository.ts +++ b/server/src/db/repositories/Categories.repository.ts @@ -30,14 +30,16 @@ export class CategoriesRepository extends EntityRepository { * 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 — the ruling every Plan 3 repository - * follows for a PK read). For this unrestricted (no `fields` option) - * findOne, MikroORM already re-queries and merges fresh data on every call - * regardless of `refresh` — its identity-map-skip optimisation only - * engages together with a `fields` restriction (the case - * `AppSettingsRepository.getValue` actually needed it for). `refresh` - * stays on this read to match the blanket ruling and to stay correct if a - * future caller adds `fields` here. + * `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. */ async findById(id: number): Promise { const category = await this.findOne({ id }, { refresh: true }); @@ -84,13 +86,26 @@ export class CategoriesRepository extends EntityRepository { * coalesce-away-empty-string behaviour. Returns `null` when no row matches * `id`, mirroring the legacy re-select's `undefined`. * + * The lookup `findOne({ id }, { refresh: true })` is PK-only, so without + * `refresh: true` MikroORM would answer it from the identity map instead + * of the DB (Task 3 review, Important 1): a category loaded earlier in + * this request (list/findById/an earlier patch) would stay pinned to that + * stale snapshot, so (a) an untouched column could read stale even though + * the DB has since changed underneath it, (b) `assign` could diff a patch + * against a stale value that happens to match the new one and emit no + * UPDATE at all — reporting success while silently dropping the write — + * and (c) a `remove()` earlier in the same request (a `nativeDelete`, + * which does not clear the identity map) would let `patch` fabricate a + * row for an id that no longer exists instead of returning `null`. + * `refresh: true` forces a real re-query every time, closing all three. + * * No re-`findOne`/`refresh` after `flush`: `assign` already mutated the - * managed 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. + * 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. */ async patch(id: number, changes: { name?: string; color?: string; icon?: string }): Promise { - const category = await this.findOne({ id }); + const category = await this.findOne({ id }, { refresh: true }); if (!category) return null; this.assign(category, changes); await this.getEntityManager().flush(); diff --git a/server/src/db/repositories/Tags.repository.ts b/server/src/db/repositories/Tags.repository.ts index b2f9a4285..1b5446fef 100644 --- a/server/src/db/repositories/Tags.repository.ts +++ b/server/src/db/repositories/Tags.repository.ts @@ -33,16 +33,17 @@ export class TagsRepository extends EntityRepository { * directly). * * `refresh: true` (Task 0 review, I1 — the ruling every Plan 3 repository - * follows for a PK-scoped read). For this unrestricted (no `fields` - * option) findOne, MikroORM already re-queries and merges fresh data on - * every call regardless of `refresh` — its identity-map-skip optimisation - * only engages together with a `fields` restriction (the case - * `AppSettingsRepository.getValue` actually needed it for). `refresh` - * stays on this read to match the blanket ruling and to stay correct if a - * future caller adds `fields` here. `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`. + * 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`. */ async findByIdAndUser(id: number, userId: number): Promise { const tag = await findOwnedByUser(this, id, 'user', userId, { refresh: true }); @@ -84,12 +85,22 @@ export class TagsRepository extends EntityRepository { * coalesce-away-empty-string behaviour. Returns `null` when no row matches * `id`, mirroring the legacy re-select's `undefined`. * + * The lookup `findOne({ id }, { refresh: true })` is PK-only, so without + * `refresh: true` MikroORM would answer it from the identity map instead + * of the DB (Task 3 review, Important 1) — same three consequences as + * `CategoriesRepository.patch`: a stale untouched column, a patch that + * diffs against a stale value and silently drops the UPDATE, or (since + * `remove()` is a `nativeDelete` that does not clear the identity map) a + * fabricated row for an id already deleted earlier in the request instead + * of `null`. `refresh: true` forces a real re-query every time. + * * No re-`findOne`/`refresh` after `flush`, same reasoning as - * `CategoriesRepository.patch`: `assign` already mutated the managed - * entity in place, and no column here is DB-computed on UPDATE. + * `CategoriesRepository.patch`: `assign` already mutated the managed (and + * now guaranteed-fresh) entity in place, and no column here is + * DB-computed on UPDATE. */ async patch(id: number, changes: { name?: string; color?: string }): Promise { - const tag = await this.findOne({ id }); + const tag = await this.findOne({ id }, { refresh: true }); if (!tag) return null; this.assign(tag, changes); await this.getEntityManager().flush(); diff --git a/server/src/db/repositories/_shared/owned-lookup.ts b/server/src/db/repositories/_shared/owned-lookup.ts index 336af5c5d..d7e31956b 100644 --- a/server/src/db/repositories/_shared/owned-lookup.ts +++ b/server/src/db/repositories/_shared/owned-lookup.ts @@ -73,14 +73,15 @@ export async function listForOwner( repo: EntityRepository, diff --git a/server/src/nest/categories/categories.service.ts b/server/src/nest/categories/categories.service.ts index 20a753856..896623303 100644 --- a/server/src/nest/categories/categories.service.ts +++ b/server/src/nest/categories/categories.service.ts @@ -51,7 +51,11 @@ export class CategoriesService { if (name) changes.name = name; if (color) changes.color = color; if (icon) changes.icon = icon; - return (await this.categories.patch(Number(id), changes)) as unknown as Category; + // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2); + // the legacy re-select returned `undefined` for the same case, and every + // caller pre-checks with a 404 first, so `?? undefined` restores that + // exact parity for the race-condition path the pre-check doesn't cover. + return ((await this.categories.patch(Number(id), changes)) ?? undefined) as unknown as Category; } async remove(id: string | number): Promise { diff --git a/server/src/nest/tags/tags.service.ts b/server/src/nest/tags/tags.service.ts index fb65f83e1..c20f2d9f3 100644 --- a/server/src/nest/tags/tags.service.ts +++ b/server/src/nest/tags/tags.service.ts @@ -49,7 +49,11 @@ export class TagsService { const changes: { name?: string; color?: string } = {}; if (name) changes.name = name; if (color) changes.color = color; - return (await this.tags.patch(Number(id), changes)) as unknown as Tag; + // `patch` returns `null` for a non-existent id (Task 3 review, Minor 2); + // the legacy re-select returned `undefined` for the same case, and every + // caller pre-checks with a 404 first, so `?? undefined` restores that + // exact parity for the race-condition path the pre-check doesn't cover. + return ((await this.tags.patch(Number(id), changes)) ?? undefined) as unknown as Tag; } async remove(id: string | number): Promise { diff --git a/server/tests/unit/db/repositories/Categories.repository.test.ts b/server/tests/unit/db/repositories/Categories.repository.test.ts index e8636177c..f84e14606 100644 --- a/server/tests/unit/db/repositories/Categories.repository.test.ts +++ b/server/tests/unit/db/repositories/Categories.repository.test.ts @@ -89,10 +89,11 @@ describe('CategoriesRepository', () => { }); // I1 (Task 0 review): findById carries `refresh: true`, the ruling every - // Plan 3 PK read follows. This unrestricted (no `fields` option) findOne - // always re-queries and merges fresh data regardless of `refresh` — see - // CategoriesRepository's docstring — so this proves the (still correct) - // outcome in one query rather than a refresh-vs-no-refresh contrast. + // Plan 3 PK read follows — and it is load-bearing here (Task 3 review, + // Important 2's correction): `{ id }` is exactly the primary key, so + // without `refresh` this would be served from the identity map with + // zero queries and the stale pre-UPDATE color. `queries === 1` proves + // the real re-query happens. it('CATREPO-008: a raw UPDATE on the same row then findById reads the new value, in one query', async () => { const created = createCategory(testDb, { name: 'RepoStale', color: '#111111' }); expect((await categories.findById(created.id))?.color).toBe('#111111'); // populate the identity map @@ -151,6 +152,46 @@ describe('CategoriesRepository', () => { it('CATREPO-014: returns null for a non-existent id', async () => { expect(await categories.patch(99999999, { name: 'Nope' })).toBeNull(); }); + + // Task 3 review, Important 1, consequence (a) — and Minor 1: the + // identity map is populated through `list()` here, not `findById`, so + // this doubles as the "list → raw UPDATE → patch" case the review + // asked for. Without `refresh: true` in `patch`'s lookup, `icon` would + // still read the pre-UPDATE 'OLD' (verified: fails without the fix). + it('CATREPO-018: an untouched column reflects a raw UPDATE made after the identity map was populated by list()', async () => { + const created = createCategory(testDb, { name: 'RepoRaceIcon', color: '#aaaaaa', icon: 'OLD' }); + await categories.list(); // populate the identity map with icon: 'OLD' + testDb.prepare('UPDATE categories SET icon = ? WHERE id = ?').run('NEW', created.id); + const updated = await categories.patch(created.id, { color: '#bbbbbb' }); + expect(updated?.icon).toBe('NEW'); + }); + + // Task 3 review, Important 1, consequence (b): without `refresh: true`, + // MikroORM diffs the patch against the stale in-memory snapshot; when + // the concurrent raw write and the patch happen to agree on the value, + // no changeset is produced and the UPDATE is silently dropped even + // though the method reports success. The raw-row assertion is what + // catches it — `updated?.color` alone would still read '#111111' + // either way (verified: fails without the fix). + it('CATREPO-019: a patch matching a concurrent raw write is not silently dropped', async () => { + const created = createCategory(testDb, { name: 'RepoRaceColor', color: '#111111' }); + await categories.findById(created.id); // populate the identity map with '#111111' + testDb.prepare('UPDATE categories SET color = ? WHERE id = ?').run('#333333', created.id); + const updated = await categories.patch(created.id, { color: '#111111' }); + expect(updated?.color).toBe('#111111'); + expect(rawCategory(created.id)).toMatchObject({ color: '#111111' }); + }); + + // Task 3 review, Important 1, consequence (c): `remove` uses + // `nativeDelete`, which does not clear the identity map, so a stale + // patch afterwards could fabricate a row for an id that no longer + // exists instead of returning `null` (verified: fails without the fix). + it('CATREPO-020: patch after remove in the same request returns null, not a fabricated row', async () => { + const created = createCategory(testDb, { name: 'RepoRaceDelete', color: '#444444' }); + await categories.findById(created.id); // populate the identity map + await categories.remove(created.id); + expect(await categories.patch(created.id, { color: '#555555' })).toBeNull(); + }); }); describe('remove', () => { diff --git a/server/tests/unit/db/repositories/Tags.repository.test.ts b/server/tests/unit/db/repositories/Tags.repository.test.ts index 726e32740..8817e5a1a 100644 --- a/server/tests/unit/db/repositories/Tags.repository.test.ts +++ b/server/tests/unit/db/repositories/Tags.repository.test.ts @@ -94,10 +94,12 @@ describe('TagsRepository', () => { // I1 (Task 0 review): findByIdAndUser carries `{ refresh: true }` (via // the shared findOwnedByUser helper), the ruling every Plan 3 PK read - // follows. This unrestricted (no `fields` option) findOne always - // re-queries and merges fresh data regardless of `refresh` — see - // TagsRepository's docstring — so this proves the (still correct) - // outcome in one query rather than a refresh-vs-no-refresh contrast. + // follows. This call's filter is `{ id, user }`, not PK-only, so it + // always re-queries and merges fresh data regardless of `refresh` (Task + // 3 review, Important 2's correction: the identity-map short-circuit + // fires only for an exactly-PK filter, not for a `fields` restriction) + // — this proves the (still correct) outcome in one query rather than a + // refresh-vs-no-refresh contrast. it('TAGREPO-009: a raw UPDATE on the same row then findByIdAndUser reads the new value, in one query', async () => { const { user } = createUser(testDb); const created = createTag(testDb, user.id, { name: 'Stale', color: '#111111' }); @@ -155,6 +157,50 @@ describe('TagsRepository', () => { it('TAGREPO-015: returns null for a non-existent id', async () => { expect(await tags.patch(99999999, { name: 'Nope' })).toBeNull(); }); + + // Task 3 review, Important 1, consequence (a) — and Minor 1: the + // identity map is populated through `listByUser()` here, not + // `findByIdAndUser`, so this doubles as the "list → raw UPDATE → + // patch" case the review asked for. Without `refresh: true` in + // `patch`'s lookup, `name` would still read the pre-UPDATE 'OLD' + // (verified: fails without the fix). + it('TAGREPO-019: an untouched column reflects a raw UPDATE made after the identity map was populated by listByUser()', async () => { + const { user } = createUser(testDb); + const created = createTag(testDb, user.id, { name: 'OLD', color: '#aaaaaa' }); + await tags.listByUser(user.id); // populate the identity map with name: 'OLD' + testDb.prepare('UPDATE tags SET name = ? WHERE id = ?').run('NEW', created.id); + const updated = await tags.patch(created.id, { color: '#bbbbbb' }); + expect(updated?.name).toBe('NEW'); + }); + + // Task 3 review, Important 1, consequence (b): without `refresh: true`, + // MikroORM diffs the patch against the stale in-memory snapshot; when + // the concurrent raw write and the patch happen to agree on the value, + // no changeset is produced and the UPDATE is silently dropped even + // though the method reports success. The raw-row assertion is what + // catches it — `updated?.color` alone would still read '#111111' + // either way (verified: fails without the fix). + it('TAGREPO-020: a patch matching a concurrent raw write is not silently dropped', async () => { + const { user } = createUser(testDb); + const created = createTag(testDb, user.id, { name: 'RaceColor', color: '#111111' }); + await tags.findByIdAndUser(created.id, user.id); // populate the identity map with '#111111' + testDb.prepare('UPDATE tags SET color = ? WHERE id = ?').run('#333333', created.id); + const updated = await tags.patch(created.id, { color: '#111111' }); + expect(updated?.color).toBe('#111111'); + expect(rawTag(created.id)).toMatchObject({ color: '#111111' }); + }); + + // Task 3 review, Important 1, consequence (c): `remove` uses + // `nativeDelete`, which does not clear the identity map, so a stale + // patch afterwards could fabricate a row for an id that no longer + // exists instead of returning `null` (verified: fails without the fix). + it('TAGREPO-021: patch after remove in the same request returns null, not a fabricated row', async () => { + const { user } = createUser(testDb); + const created = createTag(testDb, user.id, { name: 'RaceDelete', color: '#444444' }); + await tags.findByIdAndUser(created.id, user.id); // populate the identity map + await tags.remove(created.id); + expect(await tags.patch(created.id, { color: '#555555' })).toBeNull(); + }); }); describe('remove', () => { diff --git a/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts b/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts index 0d640a5d2..146b92d49 100644 --- a/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts +++ b/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts @@ -61,16 +61,18 @@ describe('findOwnedByUser', () => { }); // Task 0 review I1's PK-read ruling, extended to this helper (Task 3): - // `options.refresh` passes through to `findOne`. For an unrestricted - // (full-row) findOne like this one, MikroORM already re-queries and merges - // fresh data on every call regardless of `refresh` (proven below: a raw - // write on the same row is visible either way) — `refresh`'s - // identity-map-skip optimisation (and the staleness it can cause without - // it) only engages when `options.fields` is also set, which is why - // AppSettingsRepository.getValue (a `fields: ['value']`-scoped findOne) - // needed it to fix a real bug and this one does not. It stays on this - // helper regardless, matching the Task 0 review's blanket PK-read ruling - // and staying correct if a future caller adds a `fields` restriction here. + // `options.refresh` passes through to `findOne`. This helper's filter is + // `{ id, [ownerField]: ownerId }`, not PK-only, so it always re-queries + // and merges fresh data regardless of `refresh` (proven below: a raw + // write on the same row is visible either way) — the identity-map + // short-circuit (and the staleness it can cause without `refresh`) fires + // only when the filter is exactly the primary key, which is what + // AppSettingsRepository.getValue's PK-only `findOne` hit and this + // multi-condition filter never does (Task 3 review, Important 2's + // correction — `fields` has nothing to do with it). `refresh` stays on + // this helper regardless, matching the Task 0 review's blanket PK-read + // ruling and staying correct if a future caller narrows the filter to + // PK-only. it('OWNEDLOOKUP-006: { refresh: true } sees a raw UPDATE on the same row in the same request, in one query', async () => { const { user: owner } = createUser(testDb); const tag = createTag(testDb, owner.id, { name: 'Old' });