From 4cb8432ade91a9becdf49be51cebe622b9b3e0ed Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 10:09:38 +0200 Subject: [PATCH] fix(server): trip ids parsed once at the gate for places and assignments; NUL-safe value quoting on the SQLite platform; places parity tests completed --- server/src/db/nul-safe-sqlite-platform.ts | 69 ++++ server/src/db/orm-driver.ts | 10 + .../nest/assignments/assignments.service.ts | 36 +- server/src/nest/database/database.service.ts | 15 + server/src/nest/places/places.service.ts | 176 +++++++-- server/tests/integration/assignments.test.ts | 24 ++ server/tests/integration/places.test.ts | 175 +++++++++ .../unit/db/nul-safe-sqlite-platform.test.ts | 88 +++++ .../db/repositories/Places.repository.test.ts | 59 +++ server/tests/unit/nest/places.service.test.ts | 359 +++++++++++++++++- 10 files changed, 958 insertions(+), 53 deletions(-) create mode 100644 server/src/db/nul-safe-sqlite-platform.ts create mode 100644 server/tests/unit/db/nul-safe-sqlite-platform.test.ts diff --git a/server/src/db/nul-safe-sqlite-platform.ts b/server/src/db/nul-safe-sqlite-platform.ts new file mode 100644 index 000000000..432830769 --- /dev/null +++ b/server/src/db/nul-safe-sqlite-platform.ts @@ -0,0 +1,69 @@ +import { SqlitePlatform } from '@mikro-orm/sql'; + +/** + * Program rule 22 (`docs/superpowers/plans/2026-09-21-orm-migration-program.md`), + * surfaced by Plan 3c Task 4 review M1: MikroORM 7 never sends the driver a + * parameterised statement. `AbstractSqlConnection.execute` calls + * `platform.formatQuery(sql, params)` (`@mikro-orm/core`'s `Platform.formatQuery`) + * and hands better-sqlite3 the fully INLINED SQL text — `formatQuery` walks the + * `?` placeholders and splices in `this.quoteValue(param)` for each one, so + * every bound value this ORM ever writes is a literal in the statement string, + * not a bound parameter the driver escapes separately. `SqlitePlatform`'s own + * `quoteValue` (`@mikro-orm/sql`'s `AbstractSqlPlatform.quoteValue`, for + * anything that isn't a `Date`) delegates to `escape`, which wraps a string in + * single quotes and doubles embedded `'` — the standard SQL literal shape. + * + * A `\u0000` inside that string survives the JS-level quoting untouched (it + * is not `'`, so nothing escapes it), but the C string this literal eventually + * becomes IS NUL-terminated: SQLite's own tokenizer reads the statement text + * as a NUL-terminated buffer, so it stops scanning at the embedded `\0` — + * inside the still-open `'…'` literal, which is exactly why the failure mode + * is always "unterminated string", never anything an attacker could turn into + * an injection (verified in the Task 4 review: every NUL statement in this + * class fails closed with a syntax error, 500, never a different query). + * + * The legacy raw-SQL layer never had this problem: `better-sqlite3` binds `?` + * parameters as real, separately-transmitted values (no inlining, no NUL + * truncation), so a NUL byte in a user string round-tripped byte-for-byte — + * `GET /api/trips/:id/places?search=%00` was 200 and a place named + * `"nul\u0000name"` created (201) with its name intact. Once a route's + * statement moves off `DatabaseService`'s raw bind onto anything this ORM + * inlines (a QB read, `em.insert`, `nativeUpdate` — every one of them, + * regardless of domain), that same input 500s instead, byte-for-byte across + * every domain: this is corollary to rule 22, not specific to any one plan. + * + * The fix restores the round-trip AT THE PLATFORM, once, program-wide: a + * string containing `\u0000` renders not as one `'…'` literal but as a + * parenthesised `||` (SQL string concatenation) chain, splicing in SQLite's + * builtin `char(0)` — which returns a one-character string holding the NUL + * codepoint — everywhere the original string held one. `char()` is a core + * SQLite function (no extension, no `PRAGMA`), and `||` binds tighter than + * every arithmetic/comparison/logical operator that could otherwise appear + * around a bound value, so `(...)`'s parens are redundant for a scalar + * comparison — kept anyway so this expression composes safely as an + * IN-list element or the operand of anything else `escape()`'s caller + * splices it into (`AbstractSqlPlatform.escape` recurses through arrays + * with `value.map(v => this.escape(v)).join(', ')`, so a NUL inside one + * array element must stay a self-contained expression). Splitting on + * `\u0000` and re-quoting each piece with the platform's own `escape` (via + * `super.escape`) reuses the exact same `'`-doubling every non-NUL string + * already goes through, so a NUL next to a quote or a backslash (neither + * of which SQLite string literals treat specially) round-trips exactly as + * it did before this override existed. + * + * Deliberately scoped to `typeof value === 'string'`: this is the M1 fix, + * not a general re-implementation of `escape`/`quoteValue`. Non-finite + * numbers rendering as bare, unquoted tokens is the SAME inlining mechanism + * (rule 22's corollary) but is already fenced off upstream by rule 21's + * `toRowId` id guards — it is not this override's job to guess at every + * caller's numeric validation. + */ +export class NulSafeSqlitePlatform extends SqlitePlatform { + override escape(value: unknown): string { + if (typeof value === 'string' && value.includes('\u0000')) { + const literals = value.split('\u0000').map(part => super.escape(part)); + return `(${literals.join(' || char(0) || ')})`; + } + return super.escape(value); + } +} diff --git a/server/src/db/orm-driver.ts b/server/src/db/orm-driver.ts index 63288cab5..3ae7b1680 100644 --- a/server/src/db/orm-driver.ts +++ b/server/src/db/orm-driver.ts @@ -1,6 +1,8 @@ import { getRawConnection } from './database'; import type { Configuration } from '@mikro-orm/core'; +import type { SqlitePlatform } from '@mikro-orm/sql'; import { SqliteConnection, SqliteDriver } from '@mikro-orm/sqlite'; +import { NulSafeSqlitePlatform } from './nul-safe-sqlite-platform'; import type Database from 'better-sqlite3'; import { SqliteDialect, SqliteDriver as KyselySqliteDriver, type Dialect, type SqliteDialectConfig } from 'kysely'; @@ -61,6 +63,14 @@ export function createBoundSqliteDriver(getHandle: () => Database.Database): typ // and SqliteDriver's constructor hardcodes the class, so there is no // supported hook for supplying it. (this as unknown as { connection: SqliteConnection }).connection = new BoundSqliteConnection(config); + // Program rule 22 (NulSafeSqlitePlatform's own docstring): `SqliteDriver`'s + // constructor hardcodes `new SqlitePlatform()` too, with no config hook to + // supply a different one. `DatabaseDriver.platform` is a plain, reassignable + // field — `DatabaseDriver.setMetadata()` (called once, during + // `MikroORM.init()`, always after this constructor returns) is what actually + // propagates it to the connection via `connection.setPlatform(this.platform)`, + // so replacing it here, synchronously, is in time for every later read. + (this as unknown as { platform: SqlitePlatform }).platform = new NulSafeSqlitePlatform(); } }; } diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 808034b14..c98f5aaaa 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -185,17 +185,32 @@ export class AssignmentsService { * `toRowId(dayId)!` — a non-canonical id (`"3 "`, `"3.0"`) must answer * "not found" HERE, not pass `DaysRepository.existsInTrip`'s own raw-bind * affinity match and then hit `toRowId(dayId)!` downstream returning - * `null` (the live regression: a `day_id: null` INSERT, 500). `tripId` - * keeps the trip-scoping seam every gate in this cluster uses (D4's T5, - * `TripsRepository.findAccessible`'s precedent) — trip access is already - * verified upstream of every route this class serves, so `Number(tripId)` - * is the correct, unchanged shape (`DaysService.getDay`'s own precedent - * keeps its `tripId` unconverted too). + * `null` (the live regression: a `day_id: null` INSERT, 500). + * + * **`tripId` is `toRowId`'d here too (Task 4 review H1, the trip-id half, + * fixed in the same fix round).** This docstring previously claimed + * `Number(tripId)` was "the correct, unchanged shape", citing + * `DaysService.getDay` as a precedent that "keeps its `tripId` + * unconverted too" — both halves of that claim were wrong: the legacy + * raw-SQL base (`94c6efbbc`) bound `tripId` RAW here (it was never + * `Number(tripId)` to begin with, so nothing was "unchanged"), and + * `DaysService.getDay` keeps the raw string, it does not convert it + * either — there was no precedent for `Number()` at all. `Number('0xb')` + * is `11`, a real trip's row id, so a hex trip id passed this gate + * against a REAL trip while `createAssignment`'s place lookup and the + * actual INSERT ran scoped to the trip in the route string — live: + * `POST /api/trips/0xb/days/:d/assignments` was the legacy's 404 "Day not + * found" and is now fixed back to that 404, not the 201 the `Number()` + * gate produced. `toRowId` rejects the same non-canonical trip-id + * spellings the place/day/assignment ids already reject, so a hex trip id + * answers "not found" here instead of reaching a real trip's row. */ async dayExists(dayId: string | number, tripId: string | number) { const id = toRowId(dayId); if (id === null) return false; - return await this.daysRepo.existsInTrip(id, Number(tripId)); + const tid = toRowId(tripId); + if (tid === null) return false; + return await this.daysRepo.existsInTrip(id, tid); } /** @@ -205,12 +220,15 @@ export class AssignmentsService { * narrowed its `id` parameter to `number` for this exact reason * (`Places.repository.ts`'s own docstring) — Plan 3c Task 1 originally * landed the method, Task 4 repointed this call site and applied the H1 - * fix in the same change. + * fix in the same change. `tripId` is `toRowId`'d here too, for the same + * trip-id-half-of-H1 reason `dayExists` above now documents. */ async placeExists(placeId: unknown, tripId: string | number) { const id = toRowId(placeId); if (id === null) return false; - return await this.placesRepo.existsInTrip(id, Number(tripId)); + const tid = toRowId(tripId); + if (tid === null) return false; + return await this.placesRepo.existsInTrip(id, tid); } /** diff --git a/server/src/nest/database/database.service.ts b/server/src/nest/database/database.service.ts index 91d248c78..460bcbab4 100644 --- a/server/src/nest/database/database.service.ts +++ b/server/src/nest/database/database.service.ts @@ -93,6 +93,21 @@ export class DatabaseService { return await this.entityManager().getRepository(Trips).isOwner(tripId, userId); } + /** + * PL28 (`PlacesService.exportGpx`'s trip-title read, L3/task-4-review.md): + * `SELECT title FROM trips WHERE id = ?`, via `TripsRepository.getTitle`. + * The same narrow-helper shape as `canAccessTrip`/`isOwner`/`rosterUserIds` + * above — a caller outside the trips domain that needs one trip column + * without taking on a full `TripsRepository` DI dependency (`PlacesModule` + * would need its own `@InjectRepository(Trips)` constructor param, which + * ripples through every `new PlacesService(...)` test-helper call site; + * this stays a one-line method here instead, matching the precedent the + * three siblings above already set). + */ + async getTripTitle(tripId: number | string): Promise { + return await this.entityManager().getRepository(Trips).getTitle(tripId); + } + /** * The user ids a trip may refer to: its members plus the owner. Guests count — * a guest is a credential-less users row joined into trip_members, and #1362 diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 7e9e6d6ec..3d73c1351 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -275,6 +275,20 @@ export class PlacesService { transport_mode, route_geometry, route_color, stop_type, fill_percent, tags = [], } = body; + // Rule 21 (H1): one trip-id notion, parsed once, used by every write + // below — including PL5's `tagsOnTrip` roster read. Every other gate in + // this class answers the legacy not-found when the trip id doesn't parse + // canonically; create() has no existence gate to answer instead — the + // legacy raw bind (base 94c6efbbc) stored the non-canonical spelling + // straight into the `trip_id` column and let the FK constraint fail + // closed (verified live: a hex trip id 500s on create, where every other + // route 404s). `toRowId` rejects the same non-canonical spellings that + // never reach a real row; `?? -1` substitutes a trip id that can never + // exist (ids are autoincrement from 1), so the FK constraint fails the + // same way — without widening `insertPlace`'s typed `trip_id: number` + // column to accept the raw string. + const tid = toRowId(tripId) ?? -1; + // PL4 — the 25-column INSERT. lat/lng/price/duration_minutes/fill_percent // use an explicit undefined check, not `||`: 0 is a legitimate value for // all five (Null Island, a free entry, a drive-by stop, an empty tank) @@ -286,7 +300,7 @@ export class PlacesService { // figure — ten minutes for fuel, twenty for a rest area — so the kinds that would // be misread as an hour never take the default in the first place. const placeId = await this.placesRepo.insertPlace({ - trip_id: Number(tripId), + trip_id: tid, name, description: description || null, lat: lat ?? null, @@ -315,7 +329,7 @@ export class PlacesService { // PL5 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. if (tags && tags.length > 0) { - await this.tagsRepo.insertIgnore(placeId, await this.tagsOnTrip(tripId, tags)); + await this.tagsRepo.insertIgnore(placeId, await this.tagsOnTrip(tid, tags)); } // PL6 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. @@ -334,7 +348,15 @@ export class PlacesService { // exist. const id = toRowId(placeId); if (id === null) return null; - if (!(await this.placesRepo.existsInTrip(id, Number(tripId)))) return null; + // Rule 21 (H1): the trip id gets the SAME `toRowId` treatment, parsed + // once here and used for the existence read — `Number(tripId)` used to + // disagree with this method's own place-id gate above (`0xb` = 11 is a + // real trip's row id), which let a hex trip id find a place that + // belongs to a DIFFERENT trip than the one in the route. A non-canonical + // trip id answers the same legacy not-found the place-id gate does. + const tid = toRowId(tripId); + if (tid === null) return null; + if (!(await this.placesRepo.existsInTrip(id, tid))) return null; // PL8 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. return await this.placesRepo.findWithTagsAndRatings(id); } @@ -374,7 +396,17 @@ export class PlacesService { // that returns `null`. const id = toRowId(placeId); if (id === null) return { result: null }; - const existingPlace = await this.placesRepo.findInTrip(id, Number(tripId)); + // Rule 21 (H1): the trip id gets the same `toRowId` treatment, parsed + // once here and reused for every later call in this method, including + // PL12/PL13's `tagsOnTrip` roster read — `Number(tripId)` used to + // disagree with those raw survivors (and with this method's own + // place-id gate above), so a hex trip id could pass this existence read + // against the wrong trip's place and then wipe its tags via a roster + // read against a roster that never matched (H1, live: tags silently + // dropped to `[]`). + const tid = toRowId(tripId); + if (tid === null) return { result: null }; + const existingPlace = await this.placesRepo.findInTrip(id, tid); if (!existingPlace) return { result: null }; // Optimistic concurrency (#1135): when the caller sent the version it based its @@ -443,7 +475,7 @@ export class PlacesService { await this.tagsRepo.deleteForPlace(id); if (tags.length > 0) { // PL13 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. - await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tripId, tags)); + await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); } } @@ -469,10 +501,17 @@ export class PlacesService { */ async linkedExpenseIds(tripId: string | number, placeIds: Array): Promise { if (placeIds.length === 0) return []; + // Rule 21 (H1): `toRowId` first — a non-canonical trip id never + // affinity-matches a real `budget_items.trip_id`, so this is the same + // "no rows" answer the raw legacy bind gave it; called independently of + // `remove()`/`removeMany()` (the controller reads it before either), so + // it cannot rely on a sibling method's gate having already run. + const tid = toRowId(tripId); + if (tid === null) return []; // PL15 — `budget_items` is Plan 3e's table; stays raw. const rows = this.dbs.all<{ id: number }>( `SELECT id FROM budget_items WHERE trip_id = ? AND place_id IN (${placeIds.map(() => '?').join(',')})`, - tripId, ...placeIds, + tid, ...placeIds, ); return rows.map(r => r.id); } @@ -513,16 +552,26 @@ export class PlacesService { // (`deleteById`) must use the SAME id this gate's existence read used. const id = toRowId(placeId); if (id === null) return { deleted: false, cancelled }; + // Rule 21 (H1): the trip id gets the same `toRowId` treatment, parsed + // once here and reused for the gate AND every raw survivor below + // (`cancelStaysAt`'s PL16, the PL18 `budget_items` DELETE) — + // `Number(tripId)` used to disagree with those raw binds, so a hex trip + // id could pass this gate against a real trip's place while the raw + // deletes below matched nothing, leaving the place deleted but its + // linked expense and stay orphaned (H1, live: verified with a linked + // expense and a stay, both survived with `place_id: null`). + const tid = toRowId(tripId); + if (tid === null) return { deleted: false, cancelled }; // PL17 — the reclaim-candidate projection, read before the delete. - const place = await this.placesRepo.reclaimInputs(id, Number(tripId)); + const place = await this.placesRepo.reclaimInputs(id, tid); if (!place) return { deleted: false, cancelled }; // The linked expense goes with the place, the same way a booking takes its // expense with it (#1298). One transaction, so a place can never survive // half-detached from its money. await this.uow.transactional(async () => { - await this.cancelStaysAt(tripId, id, cancelled); + await this.cancelStaysAt(tid, id, cancelled); // PL18 — `budget_items` is Plan 3e's table; stays raw. - this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, id); + this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tid, id); // PL19 — `DELETE FROM places WHERE id = ?`. await this.placesRepo.deleteById(id); }); @@ -534,6 +583,14 @@ export class PlacesService { async removeMany(tripId: string, ids: number[]): Promise<{ deleted: number[]; cancelled: CancelledStays }> { const cancelled = noCancelledStays(); if (ids.length === 0) return { deleted: [], cancelled }; + // Rule 21 (H1): the trip id gets the same `toRowId` treatment as + // `remove()`'s, parsed once and reused for every id in the loop below — + // a non-canonical trip id never matches a real place's row (the PL20 + // gate would answer "not found" for every id anyway), so this returns + // early with nothing deleted, the same shape the legacy raw bind + // produced when the trip id didn't affinity-match any row. + const tid = toRowId(tripId); + if (tid === null) return { deleted: [], cancelled }; const deleted: number[] = []; const reclaimable: { google_place_id: string | null; image_url: string | null }[] = []; await this.uow.transactional(async () => { @@ -542,11 +599,11 @@ export class PlacesService { // (same statement as PL17). `id` is already a genuine `number` // here (a body-validated array, not a route string) — no `toRowId` // gate needed, the H1 class of bug is a route-string problem. - const row = await this.placesRepo.reclaimInputs(id, Number(tripId)); + const row = await this.placesRepo.reclaimInputs(id, tid); if (!row) continue; - await this.cancelStaysAt(tripId, id, cancelled); + await this.cancelStaysAt(tid, id, cancelled); // PL22 — `budget_items` is Plan 3e's table; stays raw. - this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tripId, id); + this.dbs.run('DELETE FROM budget_items WHERE trip_id = ? AND place_id = ?', tid, id); // PL21 — `DELETE FROM places WHERE id = ?`. await this.placesRepo.deleteById(id); deleted.push(id); @@ -607,12 +664,11 @@ export class PlacesService { /** Build a lookup of names/coords for places already in a trip. */ private async buildDedupSet(tripId: string): Promise { - const rows = this.dbs.all<{ - name: string | null; lat: number | null; lng: number | null; - google_place_id: string | null; google_ftid: string | null; osm_id: string | null; amap_poi_id: string | null; - }>( - 'SELECT name, lat, lng, google_place_id, google_ftid, osm_id, amap_poi_id FROM places WHERE trip_id = ?', tripId, - ); + // PL24 — `PlacesRepository.listDedupInputs` replaces the raw statement + // (Task 5 review L6 ruling). The JS dedup semantics below (name + // lowercase+trim, coordinates only for unnamed rows, provider ids for + // every row) are unchanged — only the read moved. + const rows = await this.placesRepo.listDedupInputs(tripId); const names = new Set(); const coords: Array<{ lat: number; lng: number }> = []; // Provider ids are collected for every place, named or not: they are what lets a @@ -712,14 +768,19 @@ export class PlacesService { * than handing over a file that imports as nothing on the other end. */ async exportGpx(tripId: string, opts: GpxExportOptions = {}): Promise<{ gpx: string; filename: string } | null> { - // PL28 — `SELECT title FROM trips WHERE id = ?`. `trips` is outside this - // task's owned repositories (Plan 3c Task 7's domain) and - // `Trips.repository.ts` has a live concurrent editor in this tree this - // session (Task 6, adding its own byte-identical `getTitle` for a - // different caller) — stays raw here rather than risk a duplicate-method - // collision; once Task 6 lands, this call can reuse its `getTitle`. - const trip = this.dbs.get<{ title: string }>('SELECT title FROM trips WHERE id = ?', tripId); - if (!trip) return null; + // PL28 — `SELECT title FROM trips WHERE id = ?`, via + // `DatabaseService.getTripTitle` (Task 4 review L3: this comment + // previously said the repoint was blocked on `Trips.repository.ts` + // landing `getTitle`, which it did at `94c6efbbc` — Task 4's own parent + // commit — so the block was stale from the moment this task started). + // `getTripTitle` is a narrow `DatabaseService` helper, the same shape as + // `canAccessTrip`/`isOwner`/`rosterUserIds`: `trips` stays outside this + // domain's owned repositories, and a full `TripsRepository` + // `@InjectRepository` constructor param on `PlacesService` would ripple + // through every `new PlacesService(...)` test-helper call site for one + // read. The legacy `if (!trip) return null` maps onto `title === null`. + const title = await this.dbs.getTripTitle(tripId); + if (title === null) return null; // PL29 — the waypoint projection. const places = await this.placesRepo.listForGpx(tripId) as GpxExportPlace[]; @@ -738,8 +799,8 @@ export class PlacesService { day.points.push({ name: stop.name, lat: stop.lat, lng: stop.lng }); } - const gpx = buildGpx({ tripTitle: trip.title, places, days: [...days.values()] }, opts); - return gpx ? { gpx, filename: gpxFilename(trip.title) } : null; + const gpx = buildGpx({ tripTitle: title, places, days: [...days.values()] }, opts); + return gpx ? { gpx, filename: gpxFilename(title) } : null; } private async importGpxRows(tripId: string, fileBuffer: Buffer, opts: GpxImportOptions = {}): Promise { @@ -814,6 +875,14 @@ export class PlacesService { if (waypoints.length === 0) return null; + // Task 5 review L5 (rule 21): parsed once here, reused for the one write + // below. `buildDedupSet` keeps the raw-bind seam (L6 ruling, PL24) — + // this does not touch it. A non-canonical trip id (this route has no + // existence gate to answer "not found" instead, matching `create`'s own + // H1 fix) substitutes a trip id that can never exist, so `insertPlace` + // fails its FK constraint the same way the legacy raw bind did (base: + // 500 on a hex trip id, live-verified in the Task 5 review). + const tid = toRowId(tripId) ?? -1; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -833,7 +902,7 @@ export class PlacesService { continue; } const placeId = await this.placesRepo.insertPlace({ - trip_id: Number(tripId), + trip_id: tid, name: wp.name, description: wp.description, lat: wp.lat, @@ -916,6 +985,8 @@ export class PlacesService { // PL33 — `CategoriesRepository.listIdName` (Plan 3a's repository). const categories = await this.categoriesRepo.listIdName(); const categoryLookup = buildCategoryNameLookup(categories); + // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. + const tid = toRowId(tripId) ?? -1; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let dupCount = 0; @@ -964,7 +1035,7 @@ export class PlacesService { // value. Re-selected via `findWithTagsAndRatings` INSIDE the same // transaction. const placeId = await this.placesRepo.insertPlace({ - trip_id: Number(tripId), + trip_id: tid, name, description: parsedPlacemark.description, lat: parsedPlacemark.lat, @@ -1194,6 +1265,8 @@ export class PlacesService { tripId: string, places: { name: string; lat: number; lng: number; notes: string | null; googleFtid: string | null }[], ): Promise<{ created: PlaceWithTags[]; skipped: number }> { + // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. + const tid = toRowId(tripId) ?? -1; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -1221,7 +1294,7 @@ export class PlacesService { // Re-selected via `findWithTagsAndRatings` INSIDE the same // transaction. const placeId = await this.placesRepo.insertPlace({ - trip_id: Number(tripId), + trip_id: tid, name: p.name, description: null, lat: p.lat, @@ -1473,6 +1546,8 @@ export class PlacesService { return { error: 'No places with coordinates found in list', status: 400 }; } + // Task 5 review L5 (rule 21) — same fix and reasoning as `importGpxRows`. + const tid = toRowId(tripId) ?? -1; const dedup = await this.buildDedupSet(tripId); const created: PlaceWithTags[] = []; let skipped = 0; @@ -1487,7 +1562,7 @@ export class PlacesService { continue; } const placeId = await this.placesRepo.insertPlace({ - trip_id: Number(tripId), + trip_id: tid, name: p.name, description: null, lat: p.lat, @@ -1551,6 +1626,19 @@ export class PlacesService { if (place.google_place_id) return; if (typeof place.lat !== 'number' || typeof place.lng !== 'number') return; + // Task 5 review L5 (rule 21): parsed once, reused by both `fillIfEmpty` + // calls below. A non-canonical trip id (never a real client spelling — + // this whole path only ever runs right after this service's own import, + // which now also parses `tripId` once) substitutes a trip id that can + // never exist, so `fillIfEmpty`'s `WHERE id = ? AND trip_id = ?` matches + // no row — a silent no-op, matching this method's own "never throws" + // contract, not a 500 the caller (a detached background task) could not + // observe anyway. Deliberately NOT applied to `this.realtime.broadcast` + // below: every broadcast call in this domain, controller included, keys + // the room by the raw route string, and changing this one call's room + // shape alone would desync it from what the client actually joined. + const tid = toRowId(tripId) ?? -1; + // Asked for a Google identity rather than for the best answer: the whole // point here is the `google_place_id` that `pickEnrichmentMatch` selects on, // and the TREK index and OpenStreetMap have none to give. Without this the @@ -1573,7 +1661,7 @@ export class PlacesService { // PL43 — `fillIfEmpty`, COALESCE-per-column so enrichment only fills // empty columns — never overwrites data the import already captured // (e.g. Naver's address) or anything the user edited. - await this.placesRepo.fillIfEmpty(place.id, Number(tripId), { + await this.placesRepo.fillIfEmpty(place.id, tid, { google_place_id: gpid, google_ftid: gftid, address: trimOrNull(match.address), @@ -1588,7 +1676,7 @@ export class PlacesService { const photo = await this.maps.getPlacePhoto(userId, gpid, place.lat, place.lng, place.name); if (photo?.photoUrl) { // PL44 — `fillIfEmpty`, the `image_url`-only shape. - await this.placesRepo.fillIfEmpty(place.id, Number(tripId), { image_url: photo.photoUrl }); + await this.placesRepo.fillIfEmpty(place.id, tid, { image_url: photo.photoUrl }); } } catch { /* no photo — leave image_url as-is */ @@ -1655,6 +1743,8 @@ export class PlacesService { console.warn(`[Places] address backfill skipped for trip ${tripId}: ${pending.length} places exceeds the ${ADDRESS_BACKFILL_MAX_PLACES} cap`); return; } + // Task 5 review L5 (rule 21) — same fix and reasoning as `enrichOne`. + const tid = toRowId(tripId) ?? -1; // Serial on purpose: the background lane throttles to roughly one request a // second anyway, so concurrency would only build a queue. for (const place of pending) { @@ -1665,7 +1755,7 @@ export class PlacesService { }); if (!address) continue; // PL46 — `fillIfEmpty`, the `address`-only shape. - await this.placesRepo.fillIfEmpty(place.id, Number(tripId), { address }); + await this.placesRepo.fillIfEmpty(place.id, tid, { address }); // PL47 — same broadcast shape as PL45: `socketId: undefined`, no // exclusion. const updated = await this.placesRepo.findWithTagsAndRatings(place.id); @@ -1688,8 +1778,14 @@ export class PlacesService { // as every other place-id route in this service. const id = toRowId(placeId); if (id === null) return { error: 'Place not found', status: 404 }; + // Rule 21 (H1): the trip id gets the same `toRowId` treatment as the + // place id above, parsed once and used for the one downstream read — + // `Number(tripId)` used to disagree with it and let a hex trip id read + // a different trip's place. + const tid = toRowId(tripId); + if (tid === null) return { error: 'Place not found', status: 404 }; // PL48 — same statement as PL9 (`applyUpdate`'s pre-image read). - const place = await this.placesRepo.findInTrip(id, Number(tripId)); + const place = await this.placesRepo.findInTrip(id, tid); if (!place) return { error: 'Place not found', status: 404 }; return this.unsplash.searchUnsplashPhotos(place.name + (place.address ? ' ' + place.address : ''), 5, await this.unsplash.getUnsplashKey(userId)); @@ -1712,8 +1808,14 @@ export class PlacesService { // rating against an id `place_ratings`'s own FK never actually named. const id = toRowId(placeId); if (id === null) return null; + // Rule 21 (H1): the trip id gets the same `toRowId` treatment as the + // place id above, parsed once and used for the existence gate below — + // `Number(tripId)` used to disagree with it and let a hex trip id rate + // a different trip's place. + const tid = toRowId(tripId); + if (tid === null) return null; // PL49 — the existence gate. - if (!(await this.placesRepo.existsInTrip(id, Number(tripId)))) return null; + if (!(await this.placesRepo.existsInTrip(id, tid))) return null; if (rating === null) { // PL50 — `DELETE FROM place_ratings WHERE place_id = ? AND user_id = ?`. await this.placeRatingsRepo.deleteRating(id, userId); diff --git a/server/tests/integration/assignments.test.ts b/server/tests/integration/assignments.test.ts index 66d61c90e..4c6d993f7 100644 --- a/server/tests/integration/assignments.test.ts +++ b/server/tests/integration/assignments.test.ts @@ -415,3 +415,27 @@ describe('Assignment participants', () => { expect(update.body.assignment.place.end_time).toBe('16:00'); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// H1 (task-4-review.md, the trip-id half fixed here) — `dayExists`/ +// `placeExists` gate on `toRowId(tripId)` now, not `Number(tripId)`: a +// hex-spelled trip id whose `Number()` value is a real, accessible trip must +// answer the legacy "Day not found", not reach that trip's real day/place +// (rule 21). +// ───────────────────────────────────────────────────────────────────────────── + +describe('H1 — trip id parsed once at the gate (rule 21)', () => { + it('POST create-assignment by the hex-spelled trip id 404s "Day not found" (legacy: 404, not 201)', async () => { + const { user } = createUser(testDb); + const { trip, day, place } = setupAssignmentFixtures(user.id); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .post(`/api/trips/${hexTripId}/days/${day.id}/assignments`) + .set('Cookie', authCookie(user.id)) + .send({ place_id: place.id }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Day not found' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM day_assignments WHERE day_id = ?').get(day.id)).toEqual({ n: 0 }); + }); +}); diff --git a/server/tests/integration/places.test.ts b/server/tests/integration/places.test.ts index 9d5d349fb..84eb855c8 100644 --- a/server/tests/integration/places.test.ts +++ b/server/tests/integration/places.test.ts @@ -35,6 +35,7 @@ import { createUser, createAdmin, createTrip, createPlace, addTripMember } from import { authCookie } from '../helpers/auth'; import { PlacesService } from '../../src/nest/places/places.service'; import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions-cache'; +import { broadcast } from '../../src/websocket'; let nestApp: INestApplication; let app: Application; @@ -1049,3 +1050,177 @@ describe('Custom place image upload', () => { expect(res.status).toBe(400); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// H1 (task-4-review.md) — the trip id is parsed ONCE, at the gate, and that +// value is what every later call uses (rule 21). A hex-spelled trip id whose +// `Number()` value is a REAL, accessible trip must answer the same not-found +// every place/day/assignment id already does — not reach the real trip +// through a `Number(tripId)` gate while a raw-bound write behind it (or vice +// versa) misses. +// ───────────────────────────────────────────────────────────────────────────── + +describe('H1 — trip id parsed once at the gate (rule 21)', () => { + it('GET by the trip\'s hex-spelled id 404s — it does not read the real trip\'s place', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Spot' }); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .get(`/api/trips/${hexTripId}/places/${place.id}`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Place not found' }); + }); + + it('PUT with tags by the hex-spelled trip id 404s and leaves the tags untouched (H1 live: they used to be wiped)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const tagResult = testDb.prepare('INSERT INTO tags (name, user_id) VALUES (?, ?)').run('Original', user.id); + const tagId = tagResult.lastInsertRowid as number; + const createRes = await request(app) + .post(`/api/trips/${trip.id}/places`) + .set('Cookie', authCookie(user.id)) + .send({ name: 'Taggable', tags: [tagId] }); + expect(createRes.status).toBe(201); + const placeId = createRes.body.place.id; + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .put(`/api/trips/${hexTripId}/places/${placeId}`) + .set('Cookie', authCookie(user.id)) + .send({ name: 'Renamed', tags: [] }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Place not found' }); + + const after = await request(app) + .get(`/api/trips/${trip.id}/places/${placeId}`) + .set('Cookie', authCookie(user.id)); + expect(after.body.place.name).toBe('Taggable'); + expect((after.body.place.tags as { id: number }[]).some((t) => t.id === tagId)).toBe(true); + }); + + it('PUT :id/rating by the hex-spelled trip id 404s', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Rated' }); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .put(`/api/trips/${hexTripId}/places/${place.id}/rating`) + .set('Cookie', authCookie(user.id)) + .send({ rating: 4 }); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Place not found' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM place_ratings WHERE place_id = ?').get(place.id)).toEqual({ n: 0 }); + }); + + it('POST create by the hex-spelled trip id mirrors the legacy 500 (base 94c6efbbc: FK violation on the raw-bound trip id) — every OTHER route 404s, this is the one documented exception', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const hexTripId = '0x' + trip.id.toString(16); + + const res = await request(app) + .post(`/api/trips/${hexTripId}/places`) + .set('Cookie', authCookie(user.id)) + .send({ name: 'Should not land' }); + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: 'Internal server error' }); + expect(testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id)).toEqual({ n: 0 }); + }); + + it('DELETE :id by the hex-spelled trip id 404s, deletes nothing and broadcasts nothing, even with a linked expense (#1298)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Louvre' }); + testDb.prepare("INSERT INTO budget_items (trip_id, name, total_price, place_id) VALUES (?, 'Tickets', 34, ?)").run(trip.id, place.id); + const hexTripId = '0x' + trip.id.toString(16); + vi.mocked(broadcast).mockClear(); + + const res = await request(app) + .delete(`/api/trips/${hexTripId}/places/${place.id}`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Place not found' }); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(place.id)).toBeTruthy(); + expect(testDb.prepare('SELECT id FROM budget_items WHERE place_id = ?').get(place.id)).toBeTruthy(); + expect(broadcast).not.toHaveBeenCalled(); + }); + + it('POST bulk-delete by the hex-spelled trip id deletes nothing (scopedIds already binds the trip id raw, so this was never divergent — kept as a regression alongside the single-delete case)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Louvre' }); + testDb.prepare("INSERT INTO budget_items (trip_id, name, total_price, place_id) VALUES (?, 'Tickets', 34, ?)").run(trip.id, place.id); + const hexTripId = '0x' + trip.id.toString(16); + vi.mocked(broadcast).mockClear(); + + const res = await request(app) + .post(`/api/trips/${hexTripId}/places/bulk-delete`) + .set('Cookie', authCookie(user.id)) + .send({ ids: [place.id] }); + expect(res.status).toBe(200); + expect(res.body).toEqual({ deleted: [], count: 0 }); + expect(testDb.prepare('SELECT id FROM places WHERE id = ?').get(place.id)).toBeTruthy(); + expect(broadcast).not.toHaveBeenCalled(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// M1 / program rule 22 (task-4-review.md) — a NUL byte in a user string no +// longer 500s: the platform override restores the legacy raw-bind's +// byte-for-byte round-trip (base 94c6efbbc: 200/201; before this fix, every +// ORM-converted statement 500'd on NUL). +// ───────────────────────────────────────────────────────────────────────────── + +describe('M1 — NUL-safe value quoting on the SQLite platform (rule 22)', () => { + it('GET ?search=%00 is 200, not 500 (status parity — SQLite\'s LIKE pattern matcher itself iterates its RHS as NUL-terminated, an independent SQLite limitation this platform fix does not touch, so a NUL search matches everything on BOTH trees, not nothing)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createPlace(testDb, trip.id, { name: 'Spot' }); + + // A raw, pre-encoded `%00` in the URL — the review's exact literal + // request shape (`GET /api/trips/:id/places?search=%00`). `.query({...})` + // goes through superagent's own `qs` encoder, which is not guaranteed to + // round-trip a NUL character the same way; the literal query string is + // what Express/the route actually receives in production. + const res = await request(app) + .get(`/api/trips/${trip.id}/places?search=%00`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(200); + }); + + it('POST create with a NUL in the name is 201 and the name round-trips byte-for-byte', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const name = 'nul\u0000name'; + + const res = await request(app) + .post(`/api/trips/${trip.id}/places`) + .set('Cookie', authCookie(user.id)) + .send({ name }); + expect(res.status).toBe(201); + expect(res.body.place.name).toBe(name); + + const stored = testDb.prepare('SELECT name FROM places WHERE id = ?').get(res.body.place.id) as { name: string }; + expect(stored.name).toBe(name); + }); + + it('fuzzes every 0x01-0x1F control character in a created place name: none 500 and every one round-trips', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + // 0x00 is covered by its own dedicated test above; this sweeps the rest + // of the control-character range the review's fuzz asked for. + for (let code = 0x01; code <= 0x1f; code++) { + const name = `ctrl${String.fromCharCode(code)}char`; + const res = await request(app) + .post(`/api/trips/${trip.id}/places`) + .set('Cookie', authCookie(user.id)) + .send({ name }); + expect(res.status).toBe(201); + expect(res.body.place.name).toBe(name); + } + }); +}); diff --git a/server/tests/unit/db/nul-safe-sqlite-platform.test.ts b/server/tests/unit/db/nul-safe-sqlite-platform.test.ts new file mode 100644 index 000000000..13823c37c --- /dev/null +++ b/server/tests/unit/db/nul-safe-sqlite-platform.test.ts @@ -0,0 +1,88 @@ +import Database from 'better-sqlite3'; +import { describe, expect, it } from 'vitest'; + +import { NulSafeSqlitePlatform } from '../../../src/db/nul-safe-sqlite-platform'; + +/** + * Plan 3c Task 4 review M1 / program rule 22: MikroORM 7 inlines every bound + * value into the SQL text via `platform.quoteValue`/`escape` (see + * `NulSafeSqlitePlatform`'s own docstring for the full mechanism). These + * tests prove the override round-trips a NUL byte at every position SQLite + * itself will accept, by handing the platform's own generated SQL literal to + * a REAL `better-sqlite3` connection and reading the value back — not a + * string-shape assertion, an actual SQLite round-trip. + */ +describe('NulSafeSqlitePlatform', () => { + const platform = new NulSafeSqlitePlatform(); + const db = new Database(':memory:'); + + function roundTrip(value: string): string { + const literal = platform.escape(value); + const row = db.prepare(`SELECT ${literal} as v`).get() as { v: string }; + return row.v; + } + + it('round-trips a string with no NUL unchanged (baseline: unaffected by the override)', () => { + expect(roundTrip('plain string')).toBe('plain string'); + }); + + it('round-trips a NUL in the middle of a string', () => { + const value = 'nul\u0000name'; + expect(roundTrip(value)).toBe(value); + }); + + it('round-trips a NUL at the very start of a string', () => { + const value = '\u0000leading'; + expect(roundTrip(value)).toBe(value); + }); + + it('round-trips a NUL at the very end of a string', () => { + const value = 'trailing\u0000'; + expect(roundTrip(value)).toBe(value); + }); + + it('round-trips a string made ENTIRELY of NUL bytes', () => { + expect(roundTrip('\u0000')).toBe('\u0000'); + expect(roundTrip('\u0000\u0000\u0000')).toBe('\u0000\u0000\u0000'); + }); + + it('round-trips a NUL next to a single quote (the string-literal escape character)', () => { + const value = "o'brien\u0000's café"; + expect(roundTrip(value)).toBe(value); + }); + + it('round-trips a NUL next to a backslash (not special in a SQLite string literal)', () => { + const value = 'back\\slash\u0000end\\'; + expect(roundTrip(value)).toBe(value); + }); + + it('round-trips multiple NULs mixed with quotes and backslashes', () => { + const value = "a'\u0000b\\\u0000c''\u0000\\\\d"; + expect(roundTrip(value)).toBe(value); + }); + + it('fuzzes every 0x00-0x1F control character: none corrupt the round-trip and none throw', () => { + for (let code = 0x00; code <= 0x1f; code++) { + const ch = String.fromCharCode(code); + const value = `before${ch}after`; + expect(() => roundTrip(value)).not.toThrow(); + expect(roundTrip(value)).toBe(value); + } + }); + + it('leaves non-string values to the base platform (numbers, null, dates, buffers untouched)', () => { + expect(platform.escape(42)).toBe('42'); + expect(platform.escape(null)).toBe('null'); + expect(platform.escape(true)).toBe('true'); + }); + + it('formatQuery (the real call path AbstractSqlConnection.execute uses: formatQuery -> quoteValue -> escape) inlines a NUL-safe literal for a bound `?`, matched via a real table', () => { + db.exec('CREATE TABLE probe (name TEXT)'); + const value = 'search\u0000term'; + const insertSql = platform.formatQuery('insert into probe (name) values (?)', [value]); + db.exec(insertSql); + const selectSql = platform.formatQuery('select name from probe where name = ?', [value]); + const row = db.prepare(selectSql).get() as { name: string } | undefined; + expect(row?.name).toBe(value); + }); +}); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 7ec14c57b..5de359600 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -393,6 +393,17 @@ describe('PlacesRepository.findDuplicateByExternalId / findDuplicateByName / fin expect(hit?.id).toBe(first.id); }); + // Task 4 review L2: only osm_id/google_ftid were exercised — a mutation + // dropping `amap_poi_id` from the `$or` survived every suite until this. + it('PLACEREPO-022b (PL25, L2): matches on amap_poi_id alone (no osm_id/google_ftid/google_place_id set)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Amap-only' }); + testDb.prepare('UPDATE places SET amap_poi_id = ? WHERE id = ?').run('B0FFH00X01', place.id); + const hit = await places.findDuplicateByExternalId(String(trip.id), 'B0FFH00X01'); + expect(hit?.id).toBe(place.id); + }); + it('PLACEREPO-023 (PL26, rule 18 exception): lower(trim(name)) matches ASCII-folded names; a non-ASCII cased letter does NOT match — the documented legacy divergence, pinned not fixed', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -422,6 +433,19 @@ describe('PlacesRepository.findDuplicateByExternalId / findDuplicateByName / fin const miss = await places.findDuplicateByCoords(String(trip.id), 48.9, 2.5, 0.0001); expect(miss).toBeUndefined(); }); + + // Task 4 review L2: PLACEREPO-024's miss case differed on both lat AND + // lng, so `lng` alone was never proven — a mutation widening the lng + // tolerance (or dropping the lng check entirely) survived every suite + // until this. + it('PLACEREPO-024b (PL27, L2): a candidate within lat tolerance but OUTSIDE lng tolerance is a miss', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createPlace(testDb, trip.id, { lat: 48.8566, lng: 2.3522 }); + // lat matches exactly; lng is 0.01 off — 100x the tolerance below. + const miss = await places.findDuplicateByCoords(String(trip.id), 48.8566, 2.3622, 0.0001); + expect(miss).toBeUndefined(); + }); }); describe('PlacesRepository.listForGpx (PL29) and existsByImageUrl (PI1)', () => { @@ -455,6 +479,41 @@ describe('PlacesRepository.listForGpx (PL29) and existsByImageUrl (PI1)', () => }); }); +/** + * PL24 (Task 5 review L6 ruling): `listDedupInputs` carries only the + * legacy projection — `PlacesService.buildDedupSet`'s JS folding (name + * lowercase+trim, coordinates only for unnamed rows, provider ids for every + * row) is untested here on purpose; it belongs to, and is already covered + * by, the service-level dedup tests. + */ +describe('PlacesRepository.listDedupInputs (PL24)', () => { + it('PLACEREPO-048: projects exactly the seven dedup columns, scoped to the trip', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const other = createTrip(testDb, user.id); + testDb.prepare( + `INSERT INTO places (trip_id, name, lat, lng, google_place_id, google_ftid, osm_id, amap_poi_id) + VALUES (?, 'Louvre', 48.86, 2.34, 'gp1', 'ft1', 'osm1', 'amap1')`, + ).run(trip.id); + testDb.prepare( + `INSERT INTO places (trip_id, name, lat, lng) VALUES (?, '', 1.1, 2.2)`, + ).run(trip.id); + createPlace(testDb, other.id, { name: 'Not this trip' }); + + const rows = await places.listDedupInputs(String(trip.id)); + expect(rows).toEqual([ + { name: 'Louvre', lat: 48.86, lng: 2.34, google_place_id: 'gp1', google_ftid: 'ft1', osm_id: 'osm1', amap_poi_id: 'amap1' }, + { name: '', lat: 1.1, lng: 2.2, google_place_id: null, google_ftid: null, osm_id: null, amap_poi_id: null }, + ]); + }); + + it('PLACEREPO-049: an empty trip returns an empty array', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(await places.listDedupInputs(String(trip.id))).toEqual([]); + }); +}); + describe('PlacesRepository.listForTrip (PL3) — filter fragments', () => { it('PLACEREPO-027: no filters — every trip place, DISTINCT, newest first', async () => { const { user } = createUser(testDb); diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index a36951ed2..07f1924e5 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -81,6 +81,22 @@ import { makeStorageFixture } from '../../helpers/storage-fixture'; import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestTagsRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestPlacesRepo, createTestTripMembersRepo, createTestDayAssignmentsRepo, createTestCategoriesRepo, sharedTestOrm } from '../../helpers/test-uow'; import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; +import { isUpdateConflict, type UpdateConflict } from '../../../src/nest/common/conflictResult'; + +/** + * Narrows `PlacesService.update`'s `PlaceWithTags | UpdateConflict | null` + * union to the successful-write case, for tests that only ever exercise the + * happy path and previously reached for `as any` to read a field off the + * result (L4/L1 — no new `any`). + */ +function asUpdatedPlace( + result: Awaited>, +): Exclude>, null | UpdateConflict> { + if (result === null || isUpdateConflict(result)) { + throw new Error('expected a successful update, got null or a conflict'); + } + return result; +} // The default `maps` MapsService below never has its key-resolving methods // exercised by this file's own cases (a real caller passes its own `maps` @@ -412,6 +428,96 @@ describe('update', () => { expect(created.route_geometry).toBe('[[48.0,2.0],[49.0,3.0]]'); expect(created.route_color).toBe('#7c3aed'); }); + + // ── PL11's three COALESCE columns (Task 4 review M2) ───────────────────────── + // + // `name`/`currency`/`transport_mode` bind `x || null` and fold with + // `?? existingPlace.x` — a falsy input (undefined, null OR '') keeps the old + // value, exactly like SQL's `COALESCE(?, col)` with a null bind. Only a + // truthy string writes. This is the opposite polarity of route_color/ + // stop_type/fill_percent just above (an explicit null clears those). The + // review's mutation table: M3 (`name: name ?? existing`, so `''` would + // write instead of keeping) and M4/M4b (`transport_mode`/`currency` treated + // as `!== undefined ? x : existing`, so `null` would clear instead of + // keeping) all SURVIVED every test in this file before these three. + + it('PLACE-SVC-013c (PL11, M3) — an empty-string name keeps the old name, it does not clear it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Original' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { name: '' })); + expect(updated.name).toBe('Original'); + }); + + it('PLACE-SVC-013d (PL11, M3) — an explicit null name keeps the old name', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Original' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { name: null as unknown as string })); + expect(updated.name).toBe('Original'); + }); + + it('PLACE-SVC-013e (PL11) — a truthy name still overwrites (the COALESCE bind is not a one-way lock)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Original' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { name: 'Renamed' })); + expect(updated.name).toBe('Renamed'); + }); + + it('PLACE-SVC-013f (PL11, M4b) — an explicit null currency keeps the old currency, it does not clear it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Priced' }); + await svc.update(String(trip.id), String(place.id), { currency: 'EUR' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { currency: null })); + expect(updated.currency).toBe('EUR'); + }); + + it('PLACE-SVC-013g (PL11, M4b) — an empty-string currency keeps the old currency', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Priced' }); + await svc.update(String(trip.id), String(place.id), { currency: 'EUR' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { currency: '' })); + expect(updated.currency).toBe('EUR'); + }); + + it('PLACE-SVC-013h (PL11) — a truthy currency still overwrites', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Priced' }); + await svc.update(String(trip.id), String(place.id), { currency: 'EUR' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { currency: 'USD' })); + expect(updated.currency).toBe('USD'); + }); + + it('PLACE-SVC-013i (PL11, M4) — an explicit null transport_mode keeps the old mode, it does not clear it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Moving' }); + await svc.update(String(trip.id), String(place.id), { transport_mode: 'driving' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { transport_mode: null })); + expect(updated.transport_mode).toBe('driving'); + }); + + it('PLACE-SVC-013j (PL11, M4) — an empty-string transport_mode keeps the old mode', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Moving' }); + await svc.update(String(trip.id), String(place.id), { transport_mode: 'driving' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { transport_mode: '' })); + expect(updated.transport_mode).toBe('driving'); + }); + + it('PLACE-SVC-013k (PL11) — a truthy transport_mode still overwrites', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Moving' }); + await svc.update(String(trip.id), String(place.id), { transport_mode: 'driving' }); + const updated = asUpdatedPlace(await svc.update(String(trip.id), String(place.id), { transport_mode: 'cycling' })); + expect(updated.transport_mode).toBe('cycling'); + }); }); // ── updateMany ──────────────────────────────────────────────────────────────── @@ -660,6 +766,50 @@ describe('importGpx', () => { expect(result.places[1].name).toBe('London'); }); + // Task 5 review M2: the four importers' hand-written 25-field `insertPlace` + // partials had no test at the service level — only repository-level tests + // that already seed `duration_minutes: 60`/`transport_mode: 'walking'` in + // their own fixture, so they test `insertPlace` in general, not what THIS + // service actually passes it. Mutants that survived every suite until this + // test: GPX `transport_mode → 'driving'`, `duration_minutes: 30`. + it('PLACE-SVC-021b (M2) — the full stored row matches the legacy 7-column GPX shape, every other column null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const gpx = Buffer.from(` + Paris + `); + const result = await svc.importGpx(String(trip.id), gpx) as { places: { id: number }[] }; + const row = testDb.prepare('SELECT * FROM places WHERE id = ?').get(result.places[0].id) as Record; + expect(row).toMatchObject({ + trip_id: trip.id, name: 'Paris', description: null, lat: 48.8566, lng: 2.3522, + address: null, category_id: null, price: null, currency: null, place_time: null, end_time: null, + duration_minutes: 60, notes: null, image_url: null, google_place_id: null, google_ftid: null, + osm_id: null, amap_poi_id: null, website: null, phone: null, transport_mode: 'walking', + route_geometry: null, route_color: null, stop_type: null, fill_percent: null, + }); + }); + + it('PLACE-SVC-021c (L2) — an import is atomic: a failure partway through the loop leaves nothing stored', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const placesRepo = await createTestPlacesRepo(dbs.connection); + const realInsertPlace = placesRepo.insertPlace.bind(placesRepo); + let calls = 0; + const insertSpy = vi.spyOn(placesRepo, 'insertPlace').mockImplementation(async (input) => { + calls++; + if (calls === 2) throw new Error('boom'); + return realInsertPlace(input); + }); + const gpx = Buffer.from(` + Paris + London + `); + await expect(svc.importGpx(String(trip.id), gpx)).rejects.toThrow('boom'); + const count = testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id) as { n: number }; + expect(count.n).toBe(0); + insertSpy.mockRestore(); + }); + it('PLACE-SVC-022 — imports as a single polyline-place with routeGeometry', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -821,6 +971,53 @@ describe('importGoogleList', () => { expect(result.places[1].name).toBe('London'); }); + // Task 5 review M2/L2. Mutants that survived every suite until these + // tests: Google `notes → null`, `duration_minutes: 30`, an insert loop + // outside `uow.transactional`. + it('PLACE-SVC-028f (M2) — the full stored row matches the legacy 7-column Google shape, notes included, every other column null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const listPayload = [ + [null, null, null, null, 'My Test List', null, null, null, [ + [null, [null, null, null, null, null, [null, null, 51.5074, -0.1278]], 'London', 'Great city'], + ]], + ]; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, text: async () => 'prefix\n' + JSON.stringify(listPayload) })); + const result = await svc.importGoogleList(String(trip.id), 'https://www.google.com/maps/placelists/list/ABC123DEF456') as { places: { id: number }[] }; + const row = testDb.prepare('SELECT * FROM places WHERE id = ?').get(result.places[0].id) as Record; + expect(row).toMatchObject({ + trip_id: trip.id, name: 'London', description: null, lat: 51.5074, lng: -0.1278, + address: null, category_id: null, price: null, currency: null, place_time: null, end_time: null, + duration_minutes: 60, notes: 'Great city', image_url: null, google_place_id: null, + osm_id: null, amap_poi_id: null, website: null, phone: null, transport_mode: 'walking', + route_geometry: null, route_color: null, stop_type: null, fill_percent: null, + }); + }); + + it('PLACE-SVC-028g (L2) — a failure partway through the Google-list loop leaves nothing stored', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const listPayload = [ + [null, null, null, null, 'My Test List', null, null, null, [ + [null, [null, null, null, null, null, [null, null, 48.8566, 2.3522]], 'Paris', null], + [null, [null, null, null, null, null, [null, null, 51.5074, -0.1278]], 'London', 'Great city'], + ]], + ]; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, text: async () => 'prefix\n' + JSON.stringify(listPayload) })); + const placesRepo = await createTestPlacesRepo(dbs.connection); + const realInsertPlace = placesRepo.insertPlace.bind(placesRepo); + let calls = 0; + const insertSpy = vi.spyOn(placesRepo, 'insertPlace').mockImplementation(async (input) => { + calls++; + if (calls === 2) throw new Error('boom'); + return realInsertPlace(input); + }); + await expect(svc.importGoogleList(String(trip.id), 'https://www.google.com/maps/placelists/list/ABC123DEF456')).rejects.toThrow('boom'); + const count = testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id) as { n: number }; + expect(count.n).toBe(0); + insertSpy.mockRestore(); + }); + it('PLACE-SVC-028b — stores a Google Maps ftid separately from google_place_id', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -997,9 +1194,12 @@ describe('searchImage', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id); - const result = await svc.searchImage(String(trip.id), `${place.id} `, user.id) as any; - expect(result.error).toBeDefined(); - expect(result.status).toBe(404); + const result: Awaited> = await svc.searchImage(String(trip.id), `${place.id} `, user.id); + expect('error' in result).toBe(true); + if ('error' in result) { + expect(result.error).toBeDefined(); + expect(result.status).toBe(404); + } }); it('PLACE-SVC-031 — searches Unsplash without a stored API key', async () => { @@ -1116,6 +1316,51 @@ describe('importKmlPlaces deduplication', () => { }); }); +// Task 5 review M2/L2 — the same full-stored-row/atomicity gap for KML. +// Mutants that survived every suite until these tests: KML `route_geometry +// → null`, `duration_minutes: 30`, the GPX loop's `uow.transactional` +// removal class repeated here for KML's own loop. +describe('importKmlPlaces — full stored row and atomicity (M2/L2)', () => { + it('PLACE-SVC-036b (M2) — a LineString placemark stores route_geometry, duration_minutes: 60 and transport_mode: walking', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const kml = Buffer.from(` + + Ridge Path2.0,48.0,0 2.1,48.1,0 +`); + const result = await svc.importKmlPlaces(String(trip.id), kml); + expect(result.count).toBe(1); + const row = testDb.prepare('SELECT * FROM places WHERE trip_id = ? AND name = ?').get(trip.id, 'Ridge Path') as Record; + expect(row.route_geometry).toBeTruthy(); + expect(JSON.parse(row.route_geometry as string)).toHaveLength(2); + expect(row.duration_minutes).toBe(60); + expect(row.transport_mode).toBe('walking'); + expect(row.category_id).toBeNull(); + }); + + it('PLACE-SVC-036c (L2) — a failure partway through the KML loop leaves nothing stored', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const placesRepo = await createTestPlacesRepo(dbs.connection); + const realInsertPlace = placesRepo.insertPlace.bind(placesRepo); + let calls = 0; + const insertSpy = vi.spyOn(placesRepo, 'insertPlace').mockImplementation(async (input) => { + calls++; + if (calls === 2) throw new Error('boom'); + return realInsertPlace(input); + }); + const kml = Buffer.from(` + + A2.0,48.0,0 + B2.1,48.1,0 +`); + await expect(svc.importKmlPlaces(String(trip.id), kml)).rejects.toThrow('boom'); + const count = testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id) as { n: number }; + expect(count.n).toBe(0); + insertSpy.mockRestore(); + }); +}); + // ── Custom place image reclaim (#1136) ────────────────────────────────────────── describe('custom place image reclaim', () => { @@ -1228,6 +1473,12 @@ describe('custom place image reclaim', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Silent Vote' }) as { id: number }; + // Task 4 review M2: a freshly-created row's `updated_at` can share the + // same one-second `datetime('now')` bucket as a bump this test means to + // catch, so `before === after` proved nothing either way (M1 — `upsertRating` + // also bumping `places.updated_at` — survived this test as written). Seed a + // visibly stale timestamp first so any bump is unmistakably a different value. + testDb.prepare("UPDATE places SET updated_at = datetime('now', '-1 hour') WHERE id = ?").run(place.id); const before = (testDb.prepare('SELECT updated_at FROM places WHERE id = ?').get(place.id) as { updated_at: string | null }).updated_at; await svc.rate(String(trip.id), String(place.id), user.id, 5); @@ -1371,6 +1622,43 @@ describe('PlacesService — automatic track colours (#776)', () => { expect(tracksOf(trip.id)).toEqual([]); }); + + // Task 5 review M1: `PLACEREPO-039` proves the driver's single-connection + // serialisation for `t.em.transactional`, not that `colorizeImportedTracks` + // actually wraps its read+write in one transaction — removing + // `this.uow.transactional` from the service survived every suite until + // this test. Numbered 018 (not the review's suggested 008) to avoid + // colliding with the pre-existing PLACES-SVC-008 in the + // `findMatchingPlaceId` describe block below. + it('PLACES-SVC-018 (M1) — two concurrent colourings of the same trip never share a colour', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const ids = [1, 2].map((n) => (testDb.prepare( + "INSERT INTO places (trip_id, name, lat, lng, route_geometry) VALUES (?, ?, 1, 1, '[[1,1],[2,2]]') RETURNING id", + ).get(trip.id, 'cc' + n) as { id: number }).id); + await Promise.all(ids.map((id) => (svc as unknown as { colorizeImportedTracks: (tripId: string, result: { places: { id: number; route_geometry: string; route_color: string | null }[] }) => Promise }).colorizeImportedTracks(String(trip.id), { + places: [{ id, route_geometry: '[[1,1],[2,2]]', route_color: null }], + }))); + expect(new Set(tracksOf(trip.id).map(t => t.route_color)).size).toBe(2); + }); + + // Task 5 review L1: the palette-wrap arm (`TRACK_COLORS[(i - free.length) % + // len]`) had no test — a mutation collapsing every wrapped index to + // `TRACK_COLORS[0]` survived every suite until this test. + it('PLACES-SVC-019 (L1) — more tracks than free colours wrap from the palette start', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const places = Array.from({ length: TRACK_COLORS.length + 2 }, (_, i) => ({ + id: (testDb.prepare( + "INSERT INTO places (trip_id, name, lat, lng, route_geometry) VALUES (?, ?, 1, 1, '[[1,1],[2,2]]') RETURNING id", + ).get(trip.id, 'w' + i) as { id: number }).id, + route_geometry: '[[1,1],[2,2]]', + route_color: null as string | null, + })); + await (svc as unknown as { colorizeImportedTracks: (tripId: string, result: { places: typeof places }) => Promise }).colorizeImportedTracks(String(trip.id), { places }); + const stored = tracksOf(trip.id).map(t => t.route_color); + expect(stored).toEqual([...TRACK_COLORS, TRACK_COLORS[0], TRACK_COLORS[1]]); + }); }); // ── Import enrichment (#886) ────────────────────────────────────────────────── @@ -1424,7 +1712,7 @@ describe('enrichImportedPlaces', () => { it('PLACE-SVC-060b (PL45) — broadcasts place:updated with socketId: undefined, no exclusion (the importer\'s own client gets the late update)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }) as any; + const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }); const broadcastSpy = vi.spyOn(RealtimeService.prototype, 'broadcast'); const svcWithMaps = await enrichSvc({ @@ -1439,7 +1727,10 @@ describe('enrichImportedPlaces', () => { const [tripArg, event, payload, socketId] = broadcastSpy.mock.calls[0]; expect(tripArg).toBe(String(trip.id)); expect(event).toBe('place:updated'); - expect((payload as { place: { id: number } }).place.id).toBe(place.id); + // Task 5 review L7: the payload carries the just-written value, not a + // stale pre-enrichment snapshot (the qb read bypasses the identity map). + expect((payload as { place: { id: number; google_place_id: string | null } }).place.id).toBe(place.id); + expect((payload as { place: { google_place_id: string | null } }).place.google_place_id).toBe('ChIJ1'); expect(socketId).toBeUndefined(); broadcastSpy.mockRestore(); }); @@ -1714,6 +2005,57 @@ describe('importNaverList provider payload', () => { expect(result.places).toHaveLength(1); expect(result.places[0].name).toBe('Gyeongbokgung'); }); + + // Task 5 review M2/L2. Mutants that survived every suite until these + // tests: `duration_minutes: 30` (all four importers), an insert loop + // outside `uow.transactional`. + it('PLACE-SVC-077b (M2) — the full stored row matches the legacy 7-column Naver shape, address+notes included, every other column null', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + text: async () => JSON.stringify({ + folder: { name: 'Seoul', bookmarkCount: 1 }, + bookmarkList: [{ name: 'Gyeongbokgung', px: 126.977, py: 37.5796, memo: 'A palace', address: 'Sejongno' }], + }), + })); + const result = await svc.importNaverList(String(trip.id), FOLDER_URL) as { places: { id: number }[] }; + const row = testDb.prepare('SELECT * FROM places WHERE id = ?').get(result.places[0].id) as Record; + expect(row).toMatchObject({ + trip_id: trip.id, name: 'Gyeongbokgung', description: null, lat: 37.5796, lng: 126.977, + address: 'Sejongno', category_id: null, price: null, currency: null, place_time: null, end_time: null, + duration_minutes: 60, notes: 'A palace', image_url: null, google_place_id: null, google_ftid: null, + osm_id: null, amap_poi_id: null, website: null, phone: null, transport_mode: 'walking', + route_geometry: null, route_color: null, stop_type: null, fill_percent: null, + }); + }); + + it('PLACE-SVC-077c (L2) — a failure partway through the Naver-list loop leaves nothing stored', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + text: async () => JSON.stringify({ + folder: { name: 'Seoul', bookmarkCount: 2 }, + bookmarkList: [ + { name: 'Gyeongbokgung', px: 126.977, py: 37.5796, memo: null, address: 'Sejongno' }, + { name: 'Namsan Tower', px: 126.988, py: 37.5512, memo: null, address: 'Yongsan' }, + ], + }), + })); + const placesRepo = await createTestPlacesRepo(dbs.connection); + const realInsertPlace = placesRepo.insertPlace.bind(placesRepo); + let calls = 0; + const insertSpy = vi.spyOn(placesRepo, 'insertPlace').mockImplementation(async (input) => { + calls++; + if (calls === 2) throw new Error('boom'); + return realInsertPlace(input); + }); + await expect(svc.importNaverList(String(trip.id), FOLDER_URL)).rejects.toThrow('boom'); + const count = testDb.prepare('SELECT COUNT(*) AS n FROM places WHERE trip_id = ?').get(trip.id) as { n: number }; + expect(count.n).toBe(0); + insertSpy.mockRestore(); + }); }); // ── Free address backfill for list imports (#1954) ─────────────────────────── @@ -1745,7 +2087,7 @@ describe('backfillMissingAddresses', () => { it('PLACE-SVC-078b (PL47) — broadcasts place:updated with socketId: undefined, no exclusion', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); - const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }) as any; + const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }); const broadcastSpy = vi.spyOn(RealtimeService.prototype, 'broadcast'); const reverseGeocode = vi.fn(async () => ({ name: null, address: '1 Rue de Rivoli, Paris' })); @@ -1757,7 +2099,10 @@ describe('backfillMissingAddresses', () => { const [tripArg, event, payload, socketId] = broadcastSpy.mock.calls[0]; expect(tripArg).toBe(String(trip.id)); expect(event).toBe('place:updated'); - expect((payload as { place: { id: number } }).place.id).toBe(place.id); + // Task 5 review L7: the payload carries the just-filled address, not a + // stale pre-backfill snapshot (the qb read bypasses the identity map). + expect((payload as { place: { id: number; address: string | null } }).place.id).toBe(place.id); + expect((payload as { place: { address: string | null } }).place.address).toBe('1 Rue de Rivoli, Paris'); expect(socketId).toBeUndefined(); broadcastSpy.mockRestore(); });