From 6b4c124a38a658efe8bd92306d582e0b3de7eb7a Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 17:44:48 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=203d=20close-out=20?= =?UTF-8?q?=E2=80=94=20legacy=20text=20shape=20for=20stored=20accommodatio?= =?UTF-8?q?n=20ids=20restores=20the=20date-change=20restamp;=20reservation?= =?UTF-8?q?=20ids=20parsed=20once=20at=20the=20gate;=20parity=20tests=20fo?= =?UTF-8?q?r=20the=20reservation=20repositories?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ReservationEndpoints.repository.ts | 135 ------------- .../repositories/Reservations.repository.ts | 165 ++++++++++++---- .../accommodations/accommodations.service.ts | 27 +-- server/src/nest/common/row-id.ts | 26 +++ .../nest/reservations/reservations.service.ts | 73 +++++-- server/src/nest/roadtrip/roadtrip.service.ts | 23 ++- server/src/nest/trips/trips.service.ts | 46 ++--- server/tests/e2e/roadtrip.e2e.test.ts | 38 ++++ .../airtrail-sync-request-context.test.ts | 11 ++ .../integration/feeds-request-context.test.ts | 52 +++++ server/tests/integration/reservations.test.ts | 185 +++++++++++++++++- .../DayAccommodations.repository.test.ts | 123 ++++++++++++ .../db/repositories/Places.repository.test.ts | 52 +++++ ...ReservationDayPositions.repository.test.ts | 62 ++++++ .../ReservationEndpoints.repository.test.ts | 85 ++++++++ .../ReservationTravelers.repository.test.ts | 69 +++++++ .../Reservations.repository.test.ts | 121 ++++++++++++ .../_shared/reservation-visibility.test.ts | 62 ++++++ .../tests/unit/nest/roadtrip-planning.test.ts | 76 +++++++ .../tests/unit/nest/roadtrip.service.test.ts | 43 ++++ 20 files changed, 1224 insertions(+), 250 deletions(-) create mode 100644 server/tests/unit/db/repositories/DayAccommodations.repository.test.ts create mode 100644 server/tests/unit/db/repositories/ReservationDayPositions.repository.test.ts create mode 100644 server/tests/unit/db/repositories/ReservationEndpoints.repository.test.ts create mode 100644 server/tests/unit/db/repositories/ReservationTravelers.repository.test.ts diff --git a/server/src/db/repositories/ReservationEndpoints.repository.ts b/server/src/db/repositories/ReservationEndpoints.repository.ts index 31100f8e0..ed1a2eca9 100644 --- a/server/src/db/repositories/ReservationEndpoints.repository.ts +++ b/server/src/db/repositories/ReservationEndpoints.repository.ts @@ -1,5 +1,4 @@ import type { ReservationEndpoints } from '../entities/ReservationEndpoints.entity'; -import { Reservations } from '../entities/Reservations.entity'; import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; @@ -21,51 +20,6 @@ export interface ReservationEndpointRow { const _reservationEndpointRowKeys: AssertRowKeys = true; -/** - * TP58's `SELECT *` shape — every scalar column of `Reservations`. Declared - * here, not on `Reservations.repository.ts` (which has its own, differently - * named row type for the same shape, RS35's `ReservationAllColumnsRow`): - * Task 4 owns that file for Plan 3d Task 6's tree window, so the - * reservations-copy read/write (TP58/TP59) live on this repository instead - * — see {@link ReservationEndpointsRepository.listAllForTrip}. - */ -export interface ReservationCopySourceRow { - id: number; - trip_id: number; - day_id: number | null; - end_day_id: number | null; - place_id: number | null; - assignment_id: number | null; - title: string; - accommodation_id: string | null; - reservation_time: string | null; - reservation_end_time: string | null; - location: string | null; - confirmation_number: string | null; - notes: string | null; - status: string | null; - type: string | null; - created_at: string | null; - metadata: string | null; - day_plan_position: number | null; - needs_review: number; - external_source: string | null; - external_id: string | null; - external_owner_user_id: number | null; - external_synced_at: string | null; - sync_enabled: number | null; - external_hash: string | null; - url: string | null; - ingest_state: string; -} - -const _reservationCopySourceRowKeys: AssertRowKeys = true; - -/** Kysely typing for TP58 — this repository is templated on `ReservationEndpoints`, which has no relation to walk to `reservations`' own scalar columns, so the read goes through Kysely directly (program rule 23). */ -interface ReservationsCopyKyselyDB { - reservations: ReservationCopySourceRow; -} - export class ReservationEndpointsRepository extends TrekRepository { /** * RS3 (`ReservationsService.loadEndpointsByTrip`) — `SELECT e.* FROM @@ -144,93 +98,4 @@ export class ReservationEndpointsRepository extends TrekRepository { await this.nativeUpdate({ id }, { local_date }); } - - // --------------------------------------------------------------------------- - // Plan 3d Task 6 (`trips.service.ts::copy`) — additive. Endpoints/travelers/ - // day-positions are never copied with a reservation (R7, `copy` never - // copied them — kept); these two methods reach the PARENT `reservations` - // table only, for TP58/TP59, because Task 4 owns `Reservations - // .repository.ts` for this same tree window ("a needed method in the - // other's repository is requested via the report, not written"). - // --------------------------------------------------------------------------- - - /** - * TP58 (`trips.service.ts::copy`'s reservations read) — `SELECT * FROM - * reservations WHERE trip_id = ?`, no `ORDER BY` (matching the legacy - * statement; the copy loop's own id-remap doesn't depend on read order). - */ - async listAllForTrip(trip_id: number): Promise { - return await this.kysely() - .selectFrom('reservations') - .selectAll() - .where('trip_id', '=', trip_id) - .execute(); - } - - /** - * TP59 (`trips.service.ts::copy`'s reservation INSERT) — `INSERT INTO - * reservations (trip_id, day_id, end_day_id, place_id, assignment_id, - * accommodation_id, title, reservation_time, reservation_end_time, - * location, confirmation_number, notes, url, status, type, metadata, - * day_plan_position, needs_review, ingest_state) VALUES (?×19)`. The - * `external_*`/`sync_enabled`/`created_at` columns are deliberately - * omitted (the legacy statement's own column list never named them — a - * copy must not inherit the source's external sync identity). - * - * `this.getEntityManager().insert(Reservations, …)`, not `this.insert()`: - * this repository is templated on `ReservationEndpoints`, so `this.insert` - * would write THAT table. `getEntityManager()` is `TrekRepository`'s own - * validated override (its class docstring), so this still fails closed - * outside a request context/open transaction exactly like every other - * write in this file — the escape hatch is safe, not a bypass. - * - * `accommodation_id` arrives ALREADY formatted as the legacy `'.0'` - * TEXT shape (`trips.service.ts`'s own `legacyAccommodationIdText`, the - * program's Plan 3d Task 3 `"14.0"` finding): a plain JS number written - * here would be inlined as a SQL literal (rule 22) and stored as `'14'`, - * not the shape the legacy raw-bound statement produced. - */ - async insertReservationCopy(input: { - trip_id: number; - day_id: number | null; - end_day_id: number | null; - place_id: number | null; - assignment_id: number | null; - accommodation_id: string | null; - title: string; - reservation_time: string | null; - reservation_end_time: string | null; - location: string | null; - confirmation_number: string | null; - notes: string | null; - url: string | null; - status: string | null; - type: string | null; - metadata: string | null; - day_plan_position: number | null; - needs_review: number; - ingest_state: string; - }): Promise { - return await this.getEntityManager().insert(Reservations, { - trip: input.trip_id, - day: input.day_id, - endDay: input.end_day_id, - place: input.place_id, - assignment: input.assignment_id, - accommodation_id: input.accommodation_id, - title: input.title, - reservation_time: input.reservation_time, - reservation_end_time: input.reservation_end_time, - location: input.location, - confirmation_number: input.confirmation_number, - notes: input.notes, - url: input.url, - status: input.status, - type: input.type, - metadata: input.metadata, - day_plan_position: input.day_plan_position, - needs_review: input.needs_review, - ingest_state: input.ingest_state, - }); - } } diff --git a/server/src/db/repositories/Reservations.repository.ts b/server/src/db/repositories/Reservations.repository.ts index 460200fea..9ba0acd2a 100644 --- a/server/src/db/repositories/Reservations.repository.ts +++ b/server/src/db/repositories/Reservations.repository.ts @@ -124,35 +124,42 @@ interface ReservationRestampKyselyDB { /** * Kysely typing for AC37/AC40 (`listIdMetadataByStay`/`listIdsByStay`). * - * A bound-parameter `WHERE accommodation_id = ?` on this TEXT column is NOT - * the same comparison the legacy statement's own `Number(id)` bind produced - * (Task 3 finding, verified empirically against the live driver, not - * assumed from the inventory's §18.1 wording): `better-sqlite3` binds every - * plain JS number as SQLite REAL, integer-valued or not (`typeof(?)` on a - * bound `14` reads back `'real'`) — including through MikroORM's Kysely - * dialect — and SQLite's TEXT-affinity conversion of a REAL for a - * comparison renders its decimal form (`14` → `'14.0'`), not the plain - * integer text `'14'`. A row this repository itself writes through - * `em.insert()`/`nativeUpdate()` stores the OTHER shape (`'14'`, no `.0` — - * MikroORM inlines the value as a literal in the generated SQL, rule 22, - * which SQLite's parser treats as a genuine INTEGER token, not a - * REAL-bound parameter) — so a plain `WHERE accommodation_id = ?` bound - * with a number would consistently MISS every row this cluster's own - * `em.insert()` calls write, not just the documented `"14.0"` edge case. - * {@link castIntegerKysely} (SQLF-057, `sql-functions.test.ts`) sidesteps - * the whole shape question: `CAST(accommodation_id AS INTEGER) = ?` matches - * `'14'` AND `'14.0'` alike (SQLite compares the CAST result against the - * bound REAL/INTEGER numerically, not as text), which is what "every - * linked booking" (AC37/AC40's own contract — more than one reservation, - * written by different call sites with different shapes, can point at one - * stay) actually needs, and is R2's own "tested with both `'14'` and - * `'14.0'` rows" requirement satisfied structurally rather than by picking - * a side. + * **Revert to the REAL-bound compare (Plan 3d Task 7 whole-plan review, M3; + * corrects this docstring's earlier claim).** A bound-parameter `WHERE + * accommodation_id = ?` with a plain JS `number` is EXACTLY the comparison + * the legacy statement's own `Number(id)` bind produced: `better-sqlite3` + * binds every plain JS number as SQLite REAL, integer-valued or not + * (`typeof(?)` on a bound `14` reads back `'real'`) — including through + * MikroORM's Kysely dialect — and SQLite's TEXT-affinity conversion of a + * REAL for a comparison renders its decimal form (`14` → `'14.0'`), so a + * bound `WHERE accommodation_id = ?` matches `'14.0'` ONLY, same as the + * legacy statement — never `'14'`. The earlier version of this docstring + * claimed the opposite (that `em.insert()`/`nativeUpdate()` calls in this + * cluster store the bare `'14'` shape and a REAL-bound compare would + * therefore miss them): that was true only because RS28/RS41 + * (`reservations.service.ts`, outside this repository) used to write + * `String(n)` instead of the legacy's REAL-bound shape (H1, the same + * review). H1 fixes every write in this cluster to store `'.0'` + * (`legacyBoundIntegerText`), so the widening below has no remaining + * reason to exist, and parity is law once it doesn't: `CAST(accommodation_id + * AS INTEGER) = ?` (via {@link castIntegerKysely}) matches `'14'`, `'14.0'`, + * `'14 '` and `'14abc'` alike — a WIDER row set than the legacy's own + * `'14.0'`-only compare (measured, task-7-review.md M3) — which is an + * unpinned parity break, not a fix, on a read the legacy also bound + * `Number(id)` for. `castIntegerKysely` stays in use elsewhere in this file + * (RS20/RV2's correlated subqueries) where the legacy statement's OWN + * comparison used a `CAST`, not a bound number — this interface's two + * consumers are not that case. */ interface ReservationsByAccommodationKyselyDB { reservations: { id: number; - accommodation_id: string | null; + // `string | number | null`, not just `string` (R2's ruling, the same + // widening {@link ReservationRestampKyselyDB} documents for DY23): the + // column is TEXT, but the bind below is a plain `number`, matching the + // legacy's own `Number(id)` bind — the interface only needs to + // type-check that bind, not describe the column's storage type. + accommodation_id: string | number | null; metadata: string | null; }; } @@ -924,16 +931,16 @@ export class ReservationsRepository extends TrekRepository { * AC37 (`AccommodationsService.updateAccommodation`) — `SELECT id, * metadata FROM reservations WHERE accommodation_id = ?`, every linked * booking (no unique constraint on the column — more than one booking can - * point at the same stay, and different write paths can leave it in - * either TEXT shape). `castIntegerKysely` (see - * {@link ReservationsByAccommodationKyselyDB}'s docstring for why). + * point at the same stay). Bound as a plain `number`, matching the + * legacy's own `Number(id)` bind byte-for-byte — see + * {@link ReservationsByAccommodationKyselyDB}'s docstring (M3, reverted + * from the `castIntegerKysely` widening). */ async listIdMetadataByStay(accommodation_id: number): Promise<{ id: number; metadata: string | null }[]> { - const platform = this.getEntityManager().getPlatform(); const rows = await this.kysely() .selectFrom('reservations') .select(['id', 'metadata']) - .where((eb) => eb(castIntegerKysely(platform, eb, 'accommodation_id'), '=', accommodation_id)) + .where('accommodation_id', '=', accommodation_id) .execute(); return rows as { id: number; metadata: string | null }[]; } @@ -960,18 +967,110 @@ export class ReservationsRepository extends TrekRepository { /** * AC40 (`AccommodationsService.deleteAccommodation`) — `SELECT id FROM * reservations WHERE accommodation_id = ?`, ALL linked bookings. Same - * `castIntegerKysely` reasoning as {@link listIdMetadataByStay}. + * REAL-bound compare as {@link listIdMetadataByStay} (M3). */ async listIdsByStay(accommodation_id: number): Promise<{ id: number }[]> { - const platform = this.getEntityManager().getPlatform(); const rows = await this.kysely() .selectFrom('reservations') .select(['id']) - .where((eb) => eb(castIntegerKysely(platform, eb, 'accommodation_id'), '=', accommodation_id)) + .where('accommodation_id', '=', accommodation_id) .execute(); return rows as { id: number }[]; } + // --------------------------------------------------------------------------- + // Plan 3d Task 6 (`trips.service.ts::copy`) — moved here from + // `ReservationEndpoints.repository.ts` in the Task 7 whole-plan review's + // fix wave (item 8/D4, "one repository per table"): Task 4 owned this file + // for the Task 6 tree window, so TP58/TP59 (the reservations copy + // read/write) landed on `ReservationEndpointsRepository` instead at the + // time — a pure move now that the window is closed, same statements, + // same parity proof (`Reservations.repository.test.ts`). Endpoints/ + // travelers/day-positions are never copied with a reservation (R7, + // `copy` never copied them — kept as-is on their own repositories). + // --------------------------------------------------------------------------- + + /** + * TP58 (`trips.service.ts::copy`'s reservations read) — `SELECT * FROM + * reservations WHERE trip_id = ?`, no `ORDER BY` (matching the legacy + * statement; the copy loop's own id-remap doesn't depend on read order). + * `ReservationAllColumnsRow` (RS35's own `SELECT *` shape) rather than a + * second, differently-named copy of the same fields. + */ + async listAllForTrip(trip_id: number): Promise { + return await this.kysely<{ reservations: ReservationAllColumnsRow }>() + .selectFrom('reservations') + .selectAll() + .where('trip_id', '=', trip_id) + .execute(); + } + + /** + * TP59 (`trips.service.ts::copy`'s reservation INSERT) — `INSERT INTO + * reservations (trip_id, day_id, end_day_id, place_id, assignment_id, + * accommodation_id, title, reservation_time, reservation_end_time, + * location, confirmation_number, notes, url, status, type, metadata, + * day_plan_position, needs_review, ingest_state) VALUES (?×19)`. The + * `external_*`/`sync_enabled`/`created_at` columns are deliberately + * omitted (the legacy statement's own column list never named them — a + * copy must not inherit the source's external sync identity). + * + * `this.insert(...)`, not `this.getEntityManager().insert(Reservations, + * …)`: this repository IS templated on `Reservations` (unlike + * `ReservationEndpointsRepository`, where this method used to live and + * needed the escape hatch to reach a table it wasn't templated on) — the + * ordinary `TrekRepository.insert` is the right call here. + * + * `accommodation_id` arrives ALREADY formatted as the legacy `'.0'` + * TEXT shape (`row-id.ts`'s `legacyBoundIntegerText`, the program's + * Plan 3d Task 3 `"14.0"` finding): a plain JS number written here would + * be inlined as a SQL literal (rule 22) and stored as `'14'`, not the + * shape the legacy raw-bound statement produced. + */ + async insertReservationCopy(input: { + trip_id: number; + day_id: number | null; + end_day_id: number | null; + place_id: number | null; + assignment_id: number | null; + accommodation_id: string | null; + title: string; + reservation_time: string | null; + reservation_end_time: string | null; + location: string | null; + confirmation_number: string | null; + notes: string | null; + url: string | null; + status: string | null; + type: string | null; + metadata: string | null; + day_plan_position: number | null; + needs_review: number; + ingest_state: string; + }): Promise { + return await this.insert({ + trip: input.trip_id, + day: input.day_id, + endDay: input.end_day_id, + place: input.place_id, + assignment: input.assignment_id, + accommodation_id: input.accommodation_id, + title: input.title, + reservation_time: input.reservation_time, + reservation_end_time: input.reservation_end_time, + location: input.location, + confirmation_number: input.confirmation_number, + notes: input.notes, + url: input.url, + status: input.status, + type: input.type, + metadata: input.metadata, + day_plan_position: input.day_plan_position, + needs_review: input.needs_review, + ingest_state: input.ingest_state, + }); + } + // --------------------------------------------------------------------------- // Plan 3d Task 4 — additive (see the file-level comment above the interfaces // this section's methods return). diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index 171554347..0c8c7c10f 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -6,7 +6,7 @@ import { DatabaseService, type PlaceWithTags, type TripAccess } from '../databas import { UnitOfWork } from '../database/unit-of-work'; import { PermissionsService } from '../permissions/permissions.service'; import { AssignmentsService } from '../assignments/assignments.service'; -import { toRowId } from '../common/row-id'; +import { toRowId, legacyBoundIntegerText } from '../common/row-id'; import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; import type { DayAccommodationsRepository, DayAccommodationRow } from '../../db/repositories/DayAccommodations.repository'; import { DayAssignments } from '../../db/entities/DayAssignments.entity'; @@ -55,31 +55,6 @@ export interface AccommodationMirror { * stay row themselves and have to answer with a mirror either way. */ export const noStayMirror = (): AccommodationMirror => ({ created: null, moved: null, updated: [], removed: [], stamped: null }); -/** - * AC33's `accommodation_id` value, in the exact TEXT shape the TRUE legacy - * (pre-ORM) statement stored (Task 3 finding, verified empirically against - * the live `better-sqlite3` driver, not assumed): the legacy `INSERT INTO - * reservations (…, accommodation_id, …) VALUES (…, ?, …)` bound the new - * stay's id — a plain JS `number` from `result.lastInsertRowid` — as a - * `?` parameter, and `better-sqlite3` binds every plain JS number as SQLite - * REAL regardless of integer-ness (confirmed via `typeof(?)`); SQLite's own - * "numeric data inserted into a TEXT column is converted to text first" - * rule then renders a REAL's decimal form, `14` → `'14.0'`, not the plain - * integer text `'14'`. `accommodations.service.test.ts`'s DAY-SVC-020/023/ - * 025 etc. read this column back with their OWN plain-number-bound raw SQL - * (`testDb.prepare('… WHERE accommodation_id = ?').get(accom.id)`, matching - * the same REAL-binding path), so this write must reproduce that exact - * shape for those UNCHANGED "proof" tests to keep passing — `String(id)` - * (`'14'`, what `MikroORM.em.insert()` would store for either a number or a - * string, since it inlines the value as a SQL-text literal, rule 22, a - * DIFFERENT shape) does not. `ReservationsRepository`'s own reads of this - * column (`listIdMetadataByStay`/`listIdsByStay`) go through - * `castIntegerKysely` instead, which matches this shape AND `'14'` alike — - * see that file's docstring. - */ -function legacyBoundIntegerText(id: number): string { - return `${id}.0`; -} const noMirror = noStayMirror; export interface DayAccommodation { diff --git a/server/src/nest/common/row-id.ts b/server/src/nest/common/row-id.ts index 95cf21d1d..3cbf5125a 100644 --- a/server/src/nest/common/row-id.ts +++ b/server/src/nest/common/row-id.ts @@ -92,3 +92,29 @@ export function toRowId(value: unknown): number | null { } return null; } + +/** + * The TEXT shape the legacy raw-SQL layer actually stored for an + * `accommodation_id` value bound as a plain JS number (Plan 3d Task 3/7 + * finding, verified empirically against the live `better-sqlite3` driver, + * not assumed): `better-sqlite3` binds every plain JS `number` as SQLite + * REAL regardless of integer-ness (confirmed via `typeof(?)`), and SQLite's + * own "numeric value inserted into a TEXT column becomes text" rule then + * renders a REAL's decimal form — `14` → `'14.0'`, never the plain integer + * text `'14'`. `String(id)` (what a SQL-literal-inlined `em.insert()`, + * rule 22, would produce for a bound `number`) renders `'14'` instead — a + * DIFFERENT stored shape from the legacy's — and a reader that still + * compares against the REAL-bound form (`restampLinkedReservation`/DY23 + * binds the id as a number through Kysely, which is REAL again) silently + * misses every row written that way. + * + * Use this wherever a `number` id is written into the `accommodation_id` + * TEXT column so the stored bytes match what the legacy statement would + * have stored for the same input — never `String(id)` for that column. + * `ReservationsRepository`'s reads of this column that must match BOTH the + * legacy shape and a `String(id)` shape go through `castIntegerKysely` + * instead (see that file's docstring); this helper is for WRITES only. + */ +export function legacyBoundIntegerText(id: number): string { + return `${id}.0`; +} diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 6980cfa38..634754f97 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -12,7 +12,7 @@ import { BudgetService } from '../budget/budget.service'; import { typeToCostCategory } from '@trek/shared'; import { NotificationsService } from '../notifications/notifications.service'; import { AccommodationsService, noStayMirror, type AccommodationMirror } from '../accommodations/accommodations.service'; -import { toRowId } from '../common/row-id'; +import { toRowId, legacyBoundIntegerText } from '../common/row-id'; import { Reservations } from '../../db/entities/Reservations.entity'; import type { ReservationsRepository } from '../../db/repositories/Reservations.repository'; import { ReservationEndpoints as ReservationEndpointsEntity } from '../../db/entities/ReservationEndpoints.entity'; @@ -733,10 +733,13 @@ export class ReservationsService { const resolvedAssignmentId = await this.resolvedOrNull('day_assignments', assignment_id || null); // RS28. `accommodation_id` bound as the TEXT column's string form (R2 — - // the legacy statement bound a plain JS number and let better-sqlite3's - // driver / SQLite's own TEXT-affinity conversion at write time store it - // as text; `String(n)` on a normal integer id renders the identical - // bytes, so this is byte-parity, not a behaviour change). + // the legacy statement bound a plain JS number, which better-sqlite3 + // binds as SQLite REAL, so the TEXT column stores the `'.0'` shape, + // not the plain integer text `String(n)` renders — H1, Plan 3d Task 7 + // review: this used to be `String(n)`, which stored a DIFFERENT shape + // than the legacy and silently broke `restampLinkedReservation`/DY23's + // REAL-bound compare. `legacyBoundIntegerText` reproduces the legacy + // bytes exactly. const insertedId = await this.reservationsRepo.insertReservation({ trip_id: this.rowIdNum(tripId), day_id: resolvedDayId, @@ -752,7 +755,7 @@ export class ReservationsService { url: url || null, status: status || 'pending', type: resolvedType, - accommodation_id: resolvedAccommodationId == null ? null : String(resolvedAccommodationId), + accommodation_id: resolvedAccommodationId == null ? null : legacyBoundIntegerText(resolvedAccommodationId), metadata: metadata ? JSON.stringify(metadata) : null, needs_review: needs_review ? 1 : 0, }); @@ -793,8 +796,16 @@ export class ReservationsService { // agree on it. Doing that in the statement rather than as a pre-check also // makes a stale id a quiet no-op instead of a foreign-key error surfacing // as a 500. - const dayIdNum = this.rowIdNum(dayId); + // + // M4, Plan 3d Task 7 review: `dayId` parsed with `toRowId` (rule 21), + // not `rowIdNum` — `rowIdNum`'s `Number(...)` fallback let a + // hex/exponent `dayId` (`0x1`, `1e1`) coerce to a real day and reach + // the join, where the legacy raw-bind statement's affinity never + // converts a hex string and so matched no row. A miss here is the same + // quiet no-op the join already gives a stale id — no write happens. + const dayIdNum = toRowId(dayId); const tripIdNum = this.rowIdNum(tripId); + if (dayIdNum === null) return; await this.uow.transactional(async () => { for (const item of positions) { // RS31/RS32. position is NOT NULL while the wire contract leaves the value optional. @@ -858,16 +869,32 @@ export class ReservationsService { // `resolvedAccId` deliberately keeps BOTH shapes alive, exactly as the // legacy code's own type lie did at runtime (`Reservation // .accommodation_id` claimed `number` while the raw row underneath was - // always the TEXT column's string): when the payload names - // `accommodation_id` it is a genuine `number` (`UpdateReservationData`'s - // declared type); when it falls back to `current.accommodation_id` it is - // the UNCONVERTED string this reservation already stored (R2 — a + // always the TEXT column's string; `UpdateReservationData.accommodation_id` + // carries the same lie for the INCOMING payload — the wire schema is + // `z.union([z.number(), z.string()])`, so a string reaches here despite + // the declared `number` type): when the payload names `accommodation_id` + // it is USER INPUT and goes through `toRowId` (L2, Plan 3d Task 7 + // review — `Number('0x1')` used to coerce a hex-spelled link and pass + // the existence check below where the legacy raw-bind statement's own + // affinity never would; `toRowId` returns `null` for any non-canonical + // shape, including a `'1.0'`/`' 1'` string — an ACCEPTED rule-15 + // narrowing here, since a payload value is untrusted input, unlike the + // fallback below); when it falls back to `current.accommodation_id` it + // is the UNCONVERTED string this reservation already stored (R2 — a // `"14.0"`-shaped value must round-trip byte-identical through a no-op - // update, which a `Number()`/`String()` round-trip would truncate to - // `"14"`). `accIdForRead(...)` below is the READ-only numeric form every - // repository call needs; the final write still binds `resolvedAccId` - // itself (via `String(...)`, a no-op on an already-string value). - let resolvedAccId: number | string | null = accommodation_id !== undefined ? (accommodation_id || null) : (current.accommodation_id ?? null); + // update, which running it through `toRowId` — or a `Number()`/ + // `String()` round-trip — would break: `toRowId('14.0')` is `null`, + // which would silently drop the link on every edit that doesn't touch + // `accommodation_id`; OUR OWN previously-written data is not the input + // L2 is about). `accIdForRead(...)` below is the READ-only numeric form + // every repository call needs; the final write formats a genuine + // `number` through `legacyBoundIntegerText` (H1, Plan 3d Task 7 review — + // a bare `String(...)` stored `''`, not the legacy's REAL-bound + // `'.0'`) and passes an already-string value through unchanged, a + // no-op. + let resolvedAccId: number | string | null = accommodation_id !== undefined + ? (accommodation_id == null ? null : toRowId(accommodation_id)) + : (current.accommodation_id ?? null); const accIdForRead = (v: number | string | null): number | null => (v == null ? null : Number(v)); if (resolvedAccId) { // Scoped to the trip on purpose: an id belonging to someone else's trip @@ -987,7 +1014,7 @@ export class ReservationsService { assignment_id: nextAssignmentId, status: status || current.status, type: type || current.type, - accommodation_id: resolvedAccId == null ? null : String(resolvedAccId), + accommodation_id: resolvedAccId == null ? null : (typeof resolvedAccId === 'number' ? legacyBoundIntegerText(resolvedAccId) : resolvedAccId), metadata: nextMetadata !== undefined ? (nextMetadata ? JSON.stringify(nextMetadata) : null) : (current.metadata ?? null), needs_review: needs_review === undefined ? (current.needs_review ?? 0) : (needs_review ? 1 : 0), }); @@ -1023,8 +1050,16 @@ export class ReservationsService { /** The accommodation + budget-item + reservation deletes are one logical * cascade — all-or-nothing. */ async remove(id: string | number, tripId: string | number): Promise<{ deleted: { id: number; title: string; type: string | null; accommodation_id: string | null } | undefined; accommodationDeleted: boolean; deletedBudgetItemId: number | null }> { - const idNum = this.rowIdNum(id); - const tripIdNum = this.rowIdNum(tripId); + // M4, Plan 3d Task 7 review: parsed ONCE here with `toRowId` (rule 21), + // not `rowIdNum` — `rowIdNum`'s `Number(...)` fallback let a hex/exponent + // id (`0x1`, `1e1`) reach `findHeaderInTrip` and delete a real row where + // the legacy raw-bind statement 404'd. A miss here answers exactly the + // `findHeaderInTrip`-miss shape below, without touching the DB. + const idNum = toRowId(id); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) { + return { deleted: undefined, accommodationDeleted: false, deletedBudgetItemId: null }; + } const removed = await this.uow.transactional(async () => { // RS45 const reservation = await this.reservationsRepo.findHeaderInTrip(idNum, tripIdNum); diff --git a/server/src/nest/roadtrip/roadtrip.service.ts b/server/src/nest/roadtrip/roadtrip.service.ts index 4c9a0eee5..7911da412 100644 --- a/server/src/nest/roadtrip/roadtrip.service.ts +++ b/server/src/nest/roadtrip/roadtrip.service.ts @@ -84,13 +84,24 @@ export class RoadtripService { return await this.daysRepo.existsInTrip(dayId, tripId); } + // L1 (Plan 3d Task 7 whole-plan review): `toRowId`, not `Number()` — a + // hex/exponent-spelled id used to coerce to a real row and return its + // vias/tracks, where the legacy raw-bind statement's affinity never + // converts a hex string and so matched nothing. `TripAccessGuard` already + // authorised the SAME hex-spelled id through its own `Number()` (not a + // leak, rule-21 seam widening) — a miss here answers the legacy empty + // shape, matching every other read path in this class. async listForDay(dayId: string | number): Promise { - return await this.viasRepo.listForDay(Number(dayId)); + const dayIdNum = toRowId(dayId); + if (dayIdNum === null) return []; + return await this.viasRepo.listForDay(dayIdNum); } /** Every via of a trip, so the client can route all days without one request per day. */ async listForTrip(tripId: string | number): Promise { - return await this.viasRepo.listForTrip(Number(tripId)); + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) return []; + return await this.viasRepo.listForTrip(tripIdNum); } async create(dayId: string | number, input: { after_order_index: number; lat: number; lng: number; sequence?: number }): Promise { @@ -162,12 +173,16 @@ export class RoadtripService { * second route for a handful of rows would be a second round trip for nothing. */ async tracksForTrip(tripId: string | number): Promise { - return await this.tracksRepo.listForTrip(Number(tripId)); + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) return []; + return await this.tracksRepo.listForTrip(tripIdNum); } /** Whether a place is on this trip, and is a track rather than an ordinary place. RT13 → `PlacesRepository.isTrackInTrip`. */ async trackExists(placeId: number, tripId: string | number): Promise { - return await this.placesRepo.isTrackInTrip(placeId, Number(tripId)); + const tripIdNum = toRowId(tripId); + if (tripIdNum === null) return false; + return await this.placesRepo.isTrackInTrip(placeId, tripIdNum); } /** Moving a via is the whole edit; where it sits in the chain does not change. */ diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index a7770d1e1..d3b31cce5 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -15,7 +15,7 @@ import { RoadtripDayTracks } from '../../db/entities/RoadtripDayTracks.entity'; import { RoadtripPreferences } from '../../db/entities/RoadtripPreferences.entity'; import { RoadtripDayBoundaries } from '../../db/entities/RoadtripDayBoundaries.entity'; import { DayAccommodations } from '../../db/entities/DayAccommodations.entity'; -import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.entity'; +import { Reservations } from '../../db/entities/Reservations.entity'; import { MAX_TRIP_DAYS, tripSpanDays, type ActiveTrip, type TrekWsPayload, type TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -28,6 +28,7 @@ import { UnsplashService } from '../unsplash/unsplash.service'; import { StorageService } from '../storage/storage.service'; import { NotFoundError, ValidationError } from '../common/domain-errors'; import { UnitOfWork } from '../database/unit-of-work'; +import { legacyBoundIntegerText } from '../common/row-id'; export const MS_PER_DAY = 86400000; @@ -58,26 +59,6 @@ export function withoutFeedToken(row: T): T { return row; } -/** - * `copy`'s (TP59) `reservations.accommodation_id` write. Mirrors - * `AccommodationsService`'s own `legacyBoundIntegerText` (Plan 3d Task 3, - * `accommodations.service.ts` — unexported there, so not importable across - * this task's file-ownership boundary; reproduced here instead, same - * one-line shape): a plain JS number bound as a `?` parameter through - * better-sqlite3 is stored as SQLite REAL regardless of integer-ness, and - * SQLite's "numeric value inserted into a TEXT column becomes text" rule - * then renders `14` as `'14.0'`, not the plain integer text `'14'` a - * SQL-literal-inlined `em.insert()` (rule 22) would produce. The legacy raw - * `copy` statement bound the new stay's id (a plain number from `accomMap`) - * the same way — reproduced explicitly here since the write now goes - * through a typed insert (`ReservationEndpointsRepository - * .insertReservationCopy`) instead of a raw bound parameter; passing an - * already-formatted STRING sidesteps the shape question entirely (SQLite - * applies no numeric conversion to a value that already has TEXT affinity). - */ -function legacyAccommodationIdText(id: number): string { - return `${id}.0`; -} interface CreateTripData { title: string; @@ -257,13 +238,14 @@ export class TripsService { return this.em.getRepository(DayAccommodations); } - // `ReservationEndpointsRepository`, not `ReservationsRepository`: Task 4 - // owns `Reservations.repository.ts` for this tree window, so TP58/TP59 - // (the reservations read/insert) live on a repository this task owns - // instead — see `ReservationEndpoints.repository.ts`'s own docstring on - // `listAllForTrip`/`insertReservationCopy`. - private get reservationEndpointsRepo() { - return this.em.getRepository(ReservationEndpoints); + // Plan 3d Task 7 whole-plan review (item 8/D4): TP58/TP59 (the + // reservations copy read/insert) moved onto `ReservationsRepository` — + // the table they read/write — out of `ReservationEndpointsRepository`, + // where they used to live only because Task 4 owned this file for the + // Task 6 tree window. Same `this.em.getRepository(...)` pattern as every + // other getter above. + private get reservationsRepo() { + return this.em.getRepository(Reservations); } async canAccessTrip(tripId: string | number, userId: number) { @@ -814,7 +796,7 @@ export class TripsService { if (newAssignmentId && newAccomId) await this.dayAssignmentsRepo.setAccommodation(newAssignmentId, newAccomId); // TP57 } - const oldReservations = await this.reservationEndpointsRepo.listAllForTrip(Number(sourceTripId)); // TP58 + const oldReservations = await this.reservationsRepo.listAllForTrip(Number(sourceTripId)); // TP58 // The external_* / sync_enabled columns are deliberately not copied: the // duplicate must not inherit the source's external sync identity. const reservationMap = new Map(); @@ -823,7 +805,7 @@ export class TripsService { // as a string — coerce before the number-keyed map lookup or the // link silently nulls. const newAccomId = r.accommodation_id != null ? (accomMap.get(Number(r.accommodation_id)) ?? null) : null; - const newReservationId = await this.reservationEndpointsRepo.insertReservationCopy({ + const newReservationId = await this.reservationsRepo.insertReservationCopy({ trip_id: newTripId, day_id: r.day_id ? (dayMap.get(r.day_id) ?? null) : null, // end_day_id is a day reference too (multi-day transport) — remap it like @@ -832,8 +814,8 @@ export class TripsService { place_id: r.place_id ? (placeMap.get(r.place_id) ?? null) : null, assignment_id: r.assignment_id ? (assignmentMap.get(r.assignment_id) ?? null) : null, // the NEW value is re-formatted to the legacy raw-bound `'.0'` - // TEXT shape on the way back out — see `legacyAccommodationIdText`. - accommodation_id: newAccomId != null ? legacyAccommodationIdText(newAccomId) : null, + // TEXT shape on the way back out — see `legacyBoundIntegerText`. + accommodation_id: newAccomId != null ? legacyBoundIntegerText(newAccomId) : null, title: r.title, reservation_time: r.reservation_time, reservation_end_time: r.reservation_end_time, location: r.location, confirmation_number: r.confirmation_number, notes: r.notes, url: r.url, status: r.status, type: r.type, // ingest_state travels with the copy: a staged booking must not turn diff --git a/server/tests/e2e/roadtrip.e2e.test.ts b/server/tests/e2e/roadtrip.e2e.test.ts index f7b2cf32b..49c21bc0d 100644 --- a/server/tests/e2e/roadtrip.e2e.test.ts +++ b/server/tests/e2e/roadtrip.e2e.test.ts @@ -121,6 +121,16 @@ describe('Roadtrip e2e (real guard chain + temp SQLite)', () => { expect(read).toHaveBeenCalledTimes(1); } finally { read.mockRestore(); } }); + // L3 (Plan 3d Task 7 review): unlike every other case in this file, + // `ChargingService.read` is NOT mocked here — this is the one place CH1 + // (`PlacesRepository.findChargingProbe`) runs for real, through the whole + // guard chain. Place 11 is a real row, just on trip 6, not trip 5 — the + // gate answers "Place not found", the same shape a place that never + // existed gets, never a leak of trip 6's row. + it('CH1 — a place id that belongs to a DIFFERENT trip is "Place not found", not trip 6\'s row (real findChargingProbe, no service mock)', async () => { + await request(server).get('/api/trips/5/roadtrip/charging/11').set('Cookie', cookie()) + .expect(404, { error: 'Place not found' }); + }); it('gates the coordinate lookup like the saved stop and validates before fetching', async () => { const lookup = vi.spyOn(app.get(ChargingService), 'lookup').mockResolvedValue({} as never); const body = { lat: 48.137, lng: 11.575, name: 'Ladepark Nord' }; @@ -351,6 +361,34 @@ describe('Roadtrip e2e (real guard chain + temp SQLite)', () => { expect(after.body.vias).toEqual([]); }); + // L1 (Plan 3d Task 7 whole-plan review): `listForDay`/`listForTrip`/ + // `tracksForTrip` used to bind `Number(tripId)`, which a hex-spelled id + // coerces to a real trip — `TripAccessGuard` had already authorised the + // SAME id through its own `Number()` (not a leak), but the legacy + // raw-bind statement's affinity never converts a hex string, so it + // always answered empty. Every one of these three now answers the + // legacy empty shape for the same hex id, not the real trip's rows. + it('L1 — vias/tracks by a hex-spelled trip id answer the legacy empty shape, not trip 5\'s real rows', async () => { + const created = await request(server) + .post('/api/trips/5/roadtrip/days/3/vias') + .set('Cookie', cookie()) + .send({ after_order_index: 0, lat: 53, lng: 10 }); + expect(created.status).toBe(201); + + const hexTripId = '0x' + (5).toString(16); + const res = await request(server).get(`/api/trips/${hexTripId}/roadtrip/vias`).set('Cookie', cookie()); + expect(res.status).toBe(200); + expect(res.body).toEqual({ vias: [], tracks: [] }); + + const hexDayId = '0x' + (3).toString(16); + const dayRes = await request(server).get(`/api/trips/5/roadtrip/days/${hexDayId}/vias`).set('Cookie', cookie()); + // `requireDay` (`toRowId`, unrelated to this fix) already 404s a + // hex-spelled day id — asserted here only to document that this route + // never reaches `listForDay` with one, so `listForDay`'s own fix is + // pinned at the service level instead (see roadtrip.service.test.ts). + expect(dayRes.status).toBe(404); + }); + it('ROADTRIP-E2E-010: the batch route answers 200 and records the track it was fitted to', async () => { const res = await request(server) .post('/api/trips/5/roadtrip/days/3/vias/batch') diff --git a/server/tests/integration/airtrail-sync-request-context.test.ts b/server/tests/integration/airtrail-sync-request-context.test.ts index 380746145..a03ffd17b 100644 --- a/server/tests/integration/airtrail-sync-request-context.test.ts +++ b/server/tests/integration/airtrail-sync-request-context.test.ts @@ -121,6 +121,15 @@ describe('airtrail-sync cron tick runs inside a request context', () => { } return originalGetReservation(id, tripId); }); + // Item 10 (Plan 3d Task 7 review): the tick's own request-context proof + // stopped at `getReservation` — `syncOwner` only calls `update` AFTER + // that read, when the incoming flight's hash actually differs from the + // stored `external_hash` (it does here, 'stale-hash' vs anything real), + // so a context regression that only showed up on the WRITE half of this + // same tick was invisible to this test. `update` is real here (not + // re-mocked), so this also proves it runs to completion without itself + // throwing `cannotUseGlobalContext`. + const updateSpy = vi.spyOn(reservations, 'update'); const isEnabledSpy = vi.spyOn(registrar, 'isEnabled').mockReturnValue(true); const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); @@ -141,6 +150,7 @@ describe('airtrail-sync cron tick runs inside a request context', () => { expect(caught).toBeUndefined(); expect(repoRead).toBeTruthy(); expect(getReservationSpy).toHaveBeenCalled(); + expect(updateSpy).toHaveBeenCalled(); const suspicious = errSpy.mock.calls .map((args) => args.map(String).join(' ')) @@ -151,6 +161,7 @@ describe('airtrail-sync cron tick runs inside a request context', () => { isEnabledSpy.mockRestore(); errSpy.mockRestore(); getReservationSpy.mockRestore(); + updateSpy.mockRestore(); flightsSpy.mockRestore(); credsSpy.mockRestore(); syncEnabledSpy.mockRestore(); diff --git a/server/tests/integration/feeds-request-context.test.ts b/server/tests/integration/feeds-request-context.test.ts index e5ff85eda..17be546f0 100644 --- a/server/tests/integration/feeds-request-context.test.ts +++ b/server/tests/integration/feeds-request-context.test.ts @@ -163,4 +163,56 @@ describe('Anonymous ICS feed routes run inside a request context', () => { expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Feed not found' }); }); + + /** + * M2 (Plan 3d Task 0 review, folded into Task 7's fix wave): SEAM-FEED-001.. + * 004 only ever drove `/api/feed/trip/…`; the user feed had no valid-token + * case at all. `FeedsService.buildUserIcs` catches EVERY per-trip failure + * into `// skip failed trips` (`feeds.service.ts:151`) and still answers + * 200 with an EMPTY calendar — so a `cannotUseGlobalContext` regression on + * this route is invisible to a status/body check the same way SEAM-FEED-003 + * showed for the trip feed, but WORSE: the trip feed at least degrades to a + * 404, while this route stays 200 either way. The assertion that actually + * catches it is SEAM-FEED-001's own shape — a genuine repository read + * happened, against the real request-scoped `MikroORM`, AND the sentinel + * event the stub returns is actually IN the body — never a status check + * alone, which cannot tell "the read happened" from "every trip's read was + * silently swallowed". + */ + it('SEAM-FEED-005: GET /api/feed/user/:token.ics for a VALID token performs a real repository read and the resulting event reaches the body, inside the request, against the SAME MikroORM app.get(MikroORM) does', async () => { + const orm = app.get(MikroORM); + const { user } = createUser(testDb); + createTrip(testDb, user.id, { title: 'User-Feed Context-Proof Trip' }); + const token = randomUUID(); + testDb.prepare('UPDATE users SET feed_token = ? WHERE id = ?').run(token, user.id); + + let repoRead: unknown; + let caught: unknown; + const calendar = app.get(CalendarService); + const spy = vi.spyOn(calendar, 'buildTripCalendar').mockImplementation(async (tripId: string | number) => { + try { + repoRead = await orm.em.getRepository(Trips).findOne({ id: Number(tripId) }); + } catch (e) { + caught = e; + } + return { calName: 'Stub Calendar', filename: 'stub.ics', timezones: new Map(), events: ['BEGIN:VEVENT\r\nSUMMARY:Sentinel Event\r\nEND:VEVENT\r\n'] }; + }); + try { + const httpApp = app.getHttpAdapter().getInstance() as Application; + const res = await request(httpApp).get(`/api/feed/user/${token}.ics`); + + expect(caught).toBeUndefined(); + expect(repoRead).toBeTruthy(); + expect(res.status).toBe(200); + expect(res.headers['content-type']).toMatch(/text\/calendar/); + // NOT a status-only check: this is what distinguishes a genuine read + // from the route's own per-trip swallow, which would return the SAME + // 200 with an EMPTY calendar (no VEVENT at all) if the read failed + // with a request-context error. + expect(res.text).toContain('BEGIN:VEVENT'); + expect(res.text).toContain('Sentinel Event'); + } finally { + spy.mockRestore(); + } + }); }); diff --git a/server/tests/integration/reservations.test.ts b/server/tests/integration/reservations.test.ts index 0b2787367..99566f7fa 100644 --- a/server/tests/integration/reservations.test.ts +++ b/server/tests/integration/reservations.test.ts @@ -25,7 +25,7 @@ vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi. import { db as testDb } from '../../src/db/database'; import { buildApp } from '../../src/bootstrap'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; -import { createUser, createTrip, createDay, createPlace, createReservation, addTripMember } from '../helpers/factories'; +import { createUser, createTrip, createDay, createPlace, createReservation, createDayAssignment, addTripMember } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; let nestApp: INestApplication; @@ -265,6 +265,145 @@ describe('Update reservation', () => { expect(accom.check_out).toBe('11:00'); expect(accom.confirmation).toBe('HTL-XYZ-999'); }); + + // L2 (Plan 3d Task 7 whole-plan review): a hex-spelled `accommodation_id` + // in the PUT body used to coerce via `Number('0x1')` and pass the + // existence check, storing the raw hex string as the link — where the + // legacy raw-bind existence check's own affinity never converts a hex + // string, so it always stored NULL instead. + it('L2 — PUT with a hex-spelled accommodation_id stores NULL, matching the legacy affinity miss', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + const { accommodation: accom } = ( + await request(app) + .post(`/api/trips/${trip.id}/accommodations`) + .set('Cookie', authCookie(user.id)) + .send({ place_id: place.id, start_day_id: day.id, end_day_id: day.id }) + ).body as { accommodation: { id: number } }; + const resv = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + + const hexAccId = '0x' + accom.id.toString(16); + const res = await request(app) + .put(`/api/trips/${trip.id}/reservations/${resv.id}`) + .set('Cookie', authCookie(user.id)) + .send({ accommodation_id: hexAccId }); + expect(res.status).toBe(200); + + const row = testDb.prepare('SELECT accommodation_id FROM reservations WHERE id = ?').get(resv.id) as { accommodation_id: string | null }; + expect(row.accommodation_id).toBeNull(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// H1 (Plan 3d Task 7 whole-plan review, live regression) — RS28's +// `accommodation_id` write must store the same TEXT shape the legacy +// raw-bind statement did (`'.0'`, a REAL-bound number, never `''`), +// because `DaysService.resyncAccommodationDays`'s DY23 restamp +// (`ReservationsRepository.restampLinkedReservation`) still compares +// against that REAL-bound shape (parity, not a fix — see its own +// docstring). A `String(n)` write silently orphans the linked reservation: +// the trip's dates change, the day plan moves, and the booking is left +// behind with no error anywhere. +// ───────────────────────────────────────────────────────────────────────────── + +describe('H1 — a booking on a stay is restamped when the trip\'s dates change (DY23)', () => { + it('the accommodation_id RS28 stores is the legacy REAL-bound TEXT shape, and a later date change restamps the linked booking', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-12-01', end_date: '2026-12-03' }); + const day1 = testDb.prepare('SELECT id FROM days WHERE trip_id = ? AND date = ?').get(trip.id, '2026-12-01') as { id: number }; + const day2 = testDb.prepare('SELECT id FROM days WHERE trip_id = ? AND date = ?').get(trip.id, '2026-12-02') as { id: number }; + const place = createPlace(testDb, trip.id, { name: 'Lighthouse Inn' }); + + const createRes = await request(app) + .post(`/api/trips/${trip.id}/reservations`) + .set('Cookie', authCookie(user.id)) + .send({ + title: 'Lighthouse Inn Stay', + type: 'hotel', + day_id: day1.id, + reservation_time: '2026-12-01T10:00', + create_accommodation: { place_id: place.id, start_day_id: day1.id, end_day_id: day2.id }, + }); + expect(createRes.status).toBe(201); + const resvId = createRes.body.reservation.id; + + // Stored-shape assert: the legacy REAL-bound TEXT shape (`'.0'`), not + // the SQL-literal-inlined shape (`''`) `String(n)` used to store. + const stored = testDb.prepare('SELECT accommodation_id FROM reservations WHERE id = ?').get(resvId) as { accommodation_id: string }; + expect(stored.accommodation_id).toMatch(/^\d+\.0$/); + + // Move the whole trip a day later (default date_shift_mode, i.e. NOT + // 'shift_all'): days re-date positionally in place, the accommodation + // stays glued to its (now re-dated) day rows (#1288), and its linked + // reservation must follow — DY23's restamp. + const updateRes = await request(app) + .put(`/api/trips/${trip.id}`) + .set('Cookie', authCookie(user.id)) + .send({ start_date: '2026-12-02', end_date: '2026-12-04' }); + expect(updateRes.status).toBe(200); + + const resvAfter = testDb.prepare('SELECT day_id, reservation_time FROM reservations WHERE id = ?').get(resvId) as + { day_id: number; reservation_time: string | null }; + expect(resvAfter.day_id).toBe(day1.id); + // Restamped onto day1's NEW date — red without the fix, where DY23's + // REAL-bound compare misses a `String(n)`-shaped accommodation_id and + // this stays '2026-12-01T10:00'. + expect(resvAfter.reservation_time).toBe('2026-12-02T10:00'); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// L3 (Plan 3d Task 7 whole-plan review): RS22 (`referencesOutsideTrip`'s +// `assignment_id` cross-trip check, `reservations.service.ts:540-541` + +// `Reservations.repository.ts::getAssignmentTripId`) had zero hits in the +// whole suite (lcov × diff). Pinned here through the real REST route. +// ───────────────────────────────────────────────────────────────────────────── + +describe('RS22 — assignment_id foreign-reference check on POST /reservations', () => { + it('RS22-own: an assignment_id belonging to the SAME trip is accepted (201)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id); + const assignment = createDayAssignment(testDb, day.id, place.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/reservations`) + .set('Cookie', authCookie(user.id)) + .send({ title: 'Dinner', type: 'restaurant', assignment_id: assignment.id }); + expect(res.status).toBe(201); + expect(res.body.reservation.assignment_id).toBe(assignment.id); + }); + + it('RS22-foreign: an assignment_id belonging to a DIFFERENT trip is refused (400 "Not part of this trip: assignment_id")', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const otherDay = createDay(testDb, otherTrip.id); + const otherPlace = createPlace(testDb, otherTrip.id); + const foreignAssignment = createDayAssignment(testDb, otherDay.id, otherPlace.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/reservations`) + .set('Cookie', authCookie(user.id)) + .send({ title: 'Dinner', type: 'restaurant', assignment_id: foreignAssignment.id }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Not part of this trip: assignment_id' }); + }); + + it('RS22-missing: an assignment_id that resolves to nothing is refused (400 "Unknown reference: assignment_id")', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + + const res = await request(app) + .post(`/api/trips/${trip.id}/reservations`) + .set('Cookie', authCookie(user.id)) + .send({ title: 'Dinner', type: 'restaurant', assignment_id: 999999 }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Unknown reference: assignment_id' }); + }); }); // ───────────────────────────────────────────────────────────────────────────── @@ -298,6 +437,25 @@ describe('Delete reservation', () => { .set('Cookie', authCookie(user.id)); expect(res.status).toBe(404); }); + + // M4 (Plan 3d Task 7 review): `remove`'s id parsed with `toRowId`, not the + // `Number()`-fallback `rowIdNum` — a hex-spelled id used to coerce to a + // real row and delete it, where the legacy raw-bind statement's affinity + // never converts a hex string and so 404'd. Rule 21. + it('M4 — DELETE …/reservations/0x answers the legacy 404 and deletes nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const resv = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + const hexId = '0x' + resv.id.toString(16); + + const res = await request(app) + .delete(`/api/trips/${trip.id}/reservations/${hexId}`) + .set('Cookie', authCookie(user.id)); + expect(res.status).toBe(404); + + const row = testDb.prepare('SELECT id FROM reservations WHERE id = ?').get(resv.id); + expect(row).toBeDefined(); + }); }); // ───────────────────────────────────────────────────────────────────────────── @@ -318,6 +476,31 @@ describe('Batch update positions', () => { expect(res.status).toBe(200); expect(res.body.success).toBe(true); }); + + // M4 (Plan 3d Task 7 review): `updatePositions`' `dayId` parsed with + // `toRowId`, not the `Number()`-fallback `rowIdNum` — a hex-spelled + // `day_id` used to coerce to a real day and reach the per-day upsert, + // where the legacy raw-bind statement's affinity never converts a hex + // string, so the join matched no row (a quiet no-op, not a write). Rule 21. + it('M4 — PUT /positions with a hex-spelled day_id is the legacy quiet no-op (no day-scoped row written)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id, { date: '2026-01-01' }); + const resv = createReservation(testDb, trip.id, { title: 'First', type: 'flight' }); + const hexDayId = '0x' + day.id.toString(16); + + const res = await request(app) + .put(`/api/trips/${trip.id}/reservations/positions`) + .set('Cookie', authCookie(user.id)) + .send({ positions: [{ id: resv.id, day_plan_position: 0 }], day_id: hexDayId }); + // Same success shape the legacy statement's own no-op miss returns — + // this is not a validation error, it silently writes nothing. + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + + const row = testDb.prepare('SELECT * FROM reservation_day_positions WHERE reservation_id = ?').get(resv.id); + expect(row).toBeUndefined(); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/unit/db/repositories/DayAccommodations.repository.test.ts b/server/tests/unit/db/repositories/DayAccommodations.repository.test.ts new file mode 100644 index 000000000..60f2bd093 --- /dev/null +++ b/server/tests/unit/db/repositories/DayAccommodations.repository.test.ts @@ -0,0 +1,123 @@ +/** + * DayAccommodationsRepository — parity tests (Plan 3d Task 7 whole-plan + * review, item 9/"missing parity tests": this repository had no standalone + * test file; its methods were only exercised indirectly through + * `accommodations.service.test.ts`/`reservations.service.test.ts`). ONE + * seeded world, one `toEqual()` test per read method. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createDayAccommodation, createPlace, createReservation, createTrip, createUser } from '../../../helpers/factories'; +import { DayAccommodations } from '../../../../src/db/entities/DayAccommodations.entity'; +import type { DayAccommodationsRepository } from '../../../../src/db/repositories/DayAccommodations.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let repo: DayAccommodationsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + repo = t.repo(DayAccommodations); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('DayAccommodationsRepository — fully seeded world', () => { + // One seeded world for every test below: a trip with two stays — + // `named` (a place, a check-in/out, a linked live booking) and `bare` + // (no place, no booking) — plus a second trip's stay, to prove scoping. + const seed = () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-05' }); + const otherTrip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-02' }); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(trip.id) as { id: number }[]; + const place = createPlace(testDb, trip.id, { name: 'The Plaza', description: 'Fancy' }); + const named = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id, { check_in: '15:00', check_out: '11:00', confirmation: 'CONF-1' }); + const booking = createReservation(testDb, trip.id, { title: 'Plaza Stay', type: 'hotel' }); + testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(`${named.id}.0`, booking.id); + const bare = createDayAccommodation(testDb, trip.id, place.id, days[2].id, days[3].id); + const otherDays = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(otherTrip.id) as { id: number }[]; + const otherPlace = createPlace(testDb, otherTrip.id, { name: 'Elsewhere' }); + const otherStay = createDayAccommodation(testDb, otherTrip.id, otherPlace.id, otherDays[0].id, otherDays[1].id); + return { trip, otherTrip, days, place, named, booking, bare, otherStay }; + }; + + it('AC2 findWithPlace — matches the legacy LEFT JOIN places statement, both with and without a place', async () => { + const { named, place } = seed(); + const legacySql = ` + SELECT a.*, p.name as place_name, p.address as place_address, p.image_url as place_image, p.lat as place_lat, p.lng as place_lng + FROM day_accommodations a LEFT JOIN places p ON a.place_id = p.id WHERE a.id = ?`; + const typed = await repo.findWithPlace(named.id); + const legacy = testDb.prepare(legacySql).get(named.id); + expect(typed).toEqual(legacy); + expect(typed?.place_name).toBe(place.name); + }); + + it('AC3 listForTripWithPlaceAndBooking — matches the legacy LEFT JOIN places/reservations statement, fanning out one row per linked booking', async () => { + const { trip, named, bare, booking } = seed(); + const legacySql = ` + SELECT a.*, p.name as place_name, p.address as place_address, p.image_url as place_image, p.lat as place_lat, p.lng as place_lng, + r.title as reservation_title + FROM day_accommodations a LEFT JOIN places p ON a.place_id = p.id LEFT JOIN reservations r ON r.accommodation_id = a.id + WHERE a.trip_id = ? ORDER BY a.created_at ASC`; + const typed = await repo.listForTripWithPlaceAndBooking(trip.id); + const legacy = testDb.prepare(legacySql).all(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id)).toEqual([named.id, bare.id]); + expect(typed.find((r) => r.id === named.id)?.reservation_title).toBe(booking.title); + expect(typed.find((r) => r.id === bare.id)?.reservation_title).toBeNull(); + }); + + it('AC34 findInTrip — matches the legacy statement, scoped by trip (a foreign-trip id misses)', async () => { + const { trip, named, otherStay } = seed(); + const legacy = testDb.prepare('SELECT * FROM day_accommodations WHERE id = ? AND trip_id = ?').get(named.id, trip.id); + expect(await repo.findInTrip(named.id, trip.id)).toEqual(legacy); + expect(await repo.findInTrip(otherStay.id, trip.id)).toBeUndefined(); + }); + + it('RS37/RS38/RS21 existsInTrip / getCheckIn / getTripId — match the legacy statements', async () => { + const { trip, named, otherStay } = seed(); + expect(await repo.existsInTrip(named.id, trip.id)).toBe(true); + expect(await repo.existsInTrip(otherStay.id, trip.id)).toBe(false); + expect(await repo.getCheckIn(named.id)).toBe('15:00'); + expect(await repo.getTripId(named.id)).toBe(trip.id); + expect(await repo.getTripId(999999)).toBeUndefined(); + }); + + it('DY19 listStartEndDayNumbers — matches the legacy joined day_number statement', async () => { + const { trip, named, bare } = seed(); + const legacy = testDb.prepare(` + SELECT a.id, s.day_number as start_no, e.day_number as end_no FROM day_accommodations a + JOIN days s ON s.id = a.start_day_id JOIN days e ON e.id = a.end_day_id WHERE a.trip_id = ?`).all(trip.id); + const typed = await repo.listStartEndDayNumbers(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id).sort()).toEqual([named.id, bare.id].sort()); + }); + + it('DY20 listForResync — matches the legacy statement, both persist(false) mirror columns present', async () => { + const { trip, named } = seed(); + const legacy = testDb.prepare('SELECT id, start_day_id, end_day_id FROM day_accommodations WHERE trip_id = ?').all(trip.id); + const typed = await repo.listForResync(trip.id); + expect(typed).toEqual(legacy); + expect(typed.find((r) => r.id === named.id)).toMatchObject({ start_day_id: named.start_day_id, end_day_id: named.end_day_id }); + }); + + it('PL16 listForPlace — matches the legacy statement, scoped by trip AND place', async () => { + const { trip, place, named, bare } = seed(); + const legacy = testDb.prepare('SELECT id FROM day_accommodations WHERE trip_id = ? AND place_id = ?').all(trip.id, place.id); + const typed = await repo.listForPlace(trip.id, place.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id).sort()).toEqual([named.id, bare.id].sort()); + }); + + it('TP55 listAllForTrip — matches the legacy SELECT *, no ORDER BY, scoped by trip', async () => { + const { trip, otherTrip, named, bare, otherStay } = seed(); + const legacy = testDb.prepare('SELECT * FROM day_accommodations WHERE trip_id = ?').all(trip.id); + const typed = await repo.listAllForTrip(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id).sort()).toEqual([named.id, bare.id].sort()); + expect(await repo.listAllForTrip(otherTrip.id)).toEqual([{ ...otherStay }]); + }); +}); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 9e03d2b7f..1450e38bd 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -1084,3 +1084,55 @@ describe('PlacesRepository.listAllForTrip (TP40)', () => { expect(await places.listAllForTrip(trip.id)).toEqual([]); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// L3 (Plan 3d Task 7 whole-plan review): CH1 (`findChargingProbe`, +// `ChargingService.read`'s trip-scoped gate) had zero coverage of the REAL +// repository call anywhere in the suite — every existing `charging.test.ts`/ +// `roadtrip.e2e.test.ts` case mocks `findChargingProbe` or `ChargingService +// .read` itself. Pinned here at the repository level (parity with the legacy +// `SELECT name, lat, lng, stop_type FROM places WHERE id = ? AND trip_id = +// ?`), and separately at the route level for the foreign-place 404 +// (`tests/e2e/roadtrip.e2e.test.ts`). +// ───────────────────────────────────────────────────────────────────────────── + +describe('PlacesRepository.findChargingProbe (CH1)', () => { + it('CH1-001: an own-trip place resolves name/lat/lng/stop_type, matching the legacy statement', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Ladepark Nord', lat: 48.137, lng: 11.575 }); + testDb.prepare("UPDATE places SET stop_type = 'charging' WHERE id = ?").run(place.id); + + const typed = await places.findChargingProbe(place.id, trip.id); + const legacy = testDb + .prepare('SELECT name, lat, lng, stop_type FROM places WHERE id = ? AND trip_id = ?') + .get(place.id, trip.id); + + expect(typed).toEqual(legacy); + expect(typed).toEqual({ name: 'Ladepark Nord', lat: 48.137, lng: 11.575, stop_type: 'charging' }); + }); + + it('CH1-002: a place belonging to a DIFFERENT trip resolves to undefined, matching the legacy miss', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const place = createPlace(testDb, otherTrip.id, { name: 'Theirs' }); + + const typed = await places.findChargingProbe(place.id, trip.id); + const legacy = testDb + .prepare('SELECT name, lat, lng, stop_type FROM places WHERE id = ? AND trip_id = ?') + .get(place.id, trip.id); + + expect(typed).toBeUndefined(); + expect(legacy).toBeUndefined(); + }); + + it('CH1-003: a non-charging place still resolves (the caller, not this query, decides what to do with stop_type)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Just a stop' }); + + const typed = await places.findChargingProbe(place.id, trip.id); + expect(typed).toEqual({ name: 'Just a stop', lat: 48.8566, lng: 2.3522, stop_type: null }); + }); +}); diff --git a/server/tests/unit/db/repositories/ReservationDayPositions.repository.test.ts b/server/tests/unit/db/repositories/ReservationDayPositions.repository.test.ts new file mode 100644 index 000000000..5d97c365f --- /dev/null +++ b/server/tests/unit/db/repositories/ReservationDayPositions.repository.test.ts @@ -0,0 +1,62 @@ +/** + * ReservationDayPositionsRepository — parity test (Plan 3d Task 7 whole-plan + * review, item 9/"missing parity tests": this repository had no standalone + * test file). ONE seeded world, one `toEqual()` test for the + * read method (RS19); RS31 (`upsertScoped`, a write) already has coverage + * through `reservations.service.test.ts`'s `updatePositions` cases. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createDay, createReservation, createTrip, createUser } from '../../../helpers/factories'; +import { ReservationDayPositions } from '../../../../src/db/entities/ReservationDayPositions.entity'; +import type { ReservationDayPositionsRepository } from '../../../../src/db/repositories/ReservationDayPositions.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let repo: ReservationDayPositionsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + repo = t.repo(ReservationDayPositions); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('ReservationDayPositionsRepository — fully seeded world', () => { + const seed = () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day1 = createDay(testDb, trip.id); + const day2 = createDay(testDb, trip.id); + const multiDay = createReservation(testDb, trip.id, { title: 'Rental car', type: 'car' }); + testDb.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)').run(multiDay.id, day1.id, 0); + testDb.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)').run(multiDay.id, day2.id, 1); + + const otherTrip = createTrip(testDb, user.id); + const otherDay = createDay(testDb, otherTrip.id); + const foreignRes = createReservation(testDb, otherTrip.id, { title: 'Foreign rental', type: 'car' }); + testDb.prepare('INSERT INTO reservation_day_positions (reservation_id, day_id, position) VALUES (?, ?, ?)').run(foreignRes.id, otherDay.id, 0); + + return { trip, otherTrip, multiDay, day1, day2 }; + }; + + it('RS19 listForTrip — matches the legacy JOIN statement, scoped by trip, both persist(false) mirror columns present', async () => { + const { trip, multiDay, day1, day2 } = seed(); + const legacy = testDb.prepare(` + SELECT rdp.reservation_id, rdp.day_id, rdp.position FROM reservation_day_positions rdp + JOIN reservations r ON rdp.reservation_id = r.id WHERE r.trip_id = ?`).all(trip.id); + const typed = await repo.listForTrip(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => [r.reservation_id, r.day_id, r.position]).sort()).toEqual( + [[multiDay.id, day1.id, 0], [multiDay.id, day2.id, 1]].sort(), + ); + }); + + it('RS19 listForTrip — an empty trip returns an empty array, not a throw', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(await repo.listForTrip(trip.id)).toEqual([]); + }); +}); diff --git a/server/tests/unit/db/repositories/ReservationEndpoints.repository.test.ts b/server/tests/unit/db/repositories/ReservationEndpoints.repository.test.ts new file mode 100644 index 000000000..0977e0618 --- /dev/null +++ b/server/tests/unit/db/repositories/ReservationEndpoints.repository.test.ts @@ -0,0 +1,85 @@ +/** + * ReservationEndpointsRepository — parity tests (Plan 3d Task 7 whole-plan + * review, item 9/"missing parity tests": this repository had no standalone + * test file). ONE seeded world, one `toEqual()` test per read + * method. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createReservation, createTrip, createUser } from '../../../helpers/factories'; +import { ReservationEndpoints } from '../../../../src/db/entities/ReservationEndpoints.entity'; +import type { ReservationEndpointsRepository } from '../../../../src/db/repositories/ReservationEndpoints.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let repo: ReservationEndpointsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + repo = t.repo(ReservationEndpoints); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +const insertEndpoint = ( + reservationId: number, + role: string, + sequence: number, + overrides: Partial<{ name: string; code: string | null; lat: number; lng: number; timezone: string | null; local_time: string | null; local_date: string | null }> = {}, +) => { + testDb.prepare(` + INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, code, lat, lng, timezone, local_time, local_date) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`).run( + reservationId, role, sequence, + overrides.name ?? `${role} endpoint`, overrides.code ?? null, overrides.lat ?? 48.0, overrides.lng ?? 11.0, + overrides.timezone ?? null, overrides.local_time ?? null, overrides.local_date ?? null, + ); +}; + +describe('ReservationEndpointsRepository — fully seeded world', () => { + const seed = () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const flight = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + const otherFlight = createReservation(testDb, trip.id, { title: 'Return flight', type: 'flight' }); + insertEndpoint(flight.id, 'from', 0, { name: 'MUC', code: 'MUC', local_date: '2026-09-01' }); + insertEndpoint(flight.id, 'to', 1, { name: 'JFK', code: 'JFK', local_date: '2026-09-01' }); + insertEndpoint(otherFlight.id, 'from', 0, { name: 'JFK', code: 'JFK' }); + + const otherTrip = createTrip(testDb, user.id); + const foreignFlight = createReservation(testDb, otherTrip.id, { title: 'Foreign', type: 'flight' }); + insertEndpoint(foreignFlight.id, 'from', 0, { name: 'LHR', code: 'LHR' }); + + return { trip, otherTrip, flight, otherFlight, foreignFlight }; + }; + + it('RS3 listForTrip — matches the legacy JOIN reservations statement, ordered by reservation then sequence, scoped by trip', async () => { + const { trip, flight, otherFlight } = seed(); + const legacy = testDb.prepare(` + SELECT e.* FROM reservation_endpoints e JOIN reservations r ON e.reservation_id = r.id + WHERE r.trip_id = ? ORDER BY e.reservation_id, e.sequence`).all(trip.id); + const typed = await repo.listForTrip(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((e) => [e.reservation_id, e.sequence])).toEqual([ + [flight.id, 0], [flight.id, 1], [otherFlight.id, 0], + ]); + }); + + it('RR2 listForReservation — matches the legacy statement, ordered by sequence, scoped to one reservation', async () => { + const { flight } = seed(); + const legacy = testDb.prepare('SELECT * FROM reservation_endpoints WHERE reservation_id = ? ORDER BY sequence').all(flight.id); + const typed = await repo.listForReservation(flight.id); + expect(typed).toEqual(legacy); + expect(typed.map((e) => e.role)).toEqual(['from', 'to']); + }); + + it('DY17 listIdAndDate — matches the legacy statement', async () => { + const { flight } = seed(); + const legacy = testDb.prepare('SELECT id, local_date FROM reservation_endpoints WHERE reservation_id = ?').all(flight.id); + const typed = await repo.listIdAndDate(flight.id); + expect(typed).toEqual(legacy); + expect(typed.every((r) => r.local_date === '2026-09-01')).toBe(true); + }); +}); diff --git a/server/tests/unit/db/repositories/ReservationTravelers.repository.test.ts b/server/tests/unit/db/repositories/ReservationTravelers.repository.test.ts new file mode 100644 index 000000000..8bc9e49c8 --- /dev/null +++ b/server/tests/unit/db/repositories/ReservationTravelers.repository.test.ts @@ -0,0 +1,69 @@ +/** + * ReservationTravelersRepository — parity tests (Plan 3d Task 7 whole-plan + * review, item 9/"missing parity tests": this repository had no standalone + * test file). ONE seeded world, one `toEqual()` test per read + * method. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createReservation, createTrip, createUser } from '../../../helpers/factories'; +import { ReservationTravelers } from '../../../../src/db/entities/ReservationTravelers.entity'; +import type { ReservationTravelersRepository } from '../../../../src/db/repositories/ReservationTravelers.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let repo: ReservationTravelersRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + repo = t.repo(ReservationTravelers); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +const legacyListForTrip = (trip_id: number) => testDb.prepare(` + SELECT rt.reservation_id, rt.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar, u.is_guest + FROM reservation_travelers rt JOIN reservations r ON rt.reservation_id = r.id JOIN users u ON rt.user_id = u.id + WHERE r.trip_id = ? ORDER BY rt.reservation_id`).all(trip_id); + +const legacyListForReservation = (reservation_id: number) => testDb.prepare(` + SELECT rt.user_id, COALESCE(u.display_name, u.username) AS username, u.avatar, u.is_guest + FROM reservation_travelers rt JOIN users u ON rt.user_id = u.id WHERE rt.reservation_id = ?`).all(reservation_id); + +describe('ReservationTravelersRepository — fully seeded world', () => { + const seed = () => { + const { user: owner } = createUser(testDb); + const { user: named } = createUser(testDb); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Named Traveler', named.id); + const { user: guest } = createUser(testDb); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const trip = createTrip(testDb, owner.id); + const flight = createReservation(testDb, trip.id, { title: 'Flight', type: 'flight' }); + const hotel = createReservation(testDb, trip.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare('INSERT INTO reservation_travelers (reservation_id, user_id) VALUES (?, ?)').run(flight.id, owner.id); + testDb.prepare('INSERT INTO reservation_travelers (reservation_id, user_id) VALUES (?, ?)').run(flight.id, named.id); + testDb.prepare('INSERT INTO reservation_travelers (reservation_id, user_id) VALUES (?, ?)').run(hotel.id, guest.id); + return { owner, named, guest, trip, flight, hotel }; + }; + + it('RS6 listForTrip — matches the legacy JOIN statement, COALESCE(display_name, username), ordered by reservation', async () => { + const { trip, flight, hotel } = seed(); + const legacy = legacyListForTrip(trip.id); + const typed = await repo.listForTrip(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.reservation_id)).toEqual([flight.id, flight.id, hotel.id]); + expect(typed.find((r) => r.username === 'Named Traveler')).toBeDefined(); + expect(typed.find((r) => r.is_guest === 1)).toBeDefined(); + }); + + it('RR3 listForReservation — matches the legacy statement (no ORDER BY, no reservation_id column)', async () => { + const { flight, owner, named } = seed(); + const legacy = legacyListForReservation(flight.id); + const typed = await repo.listForReservation(flight.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.user_id).sort((a, b) => a - b)).toEqual([owner.id, named.id].sort((a, b) => a - b)); + expect((typed[0] as Record).reservation_id).toBeUndefined(); + }); +}); diff --git a/server/tests/unit/db/repositories/Reservations.repository.test.ts b/server/tests/unit/db/repositories/Reservations.repository.test.ts index 22d4dd138..a196d0e75 100644 --- a/server/tests/unit/db/repositories/Reservations.repository.test.ts +++ b/server/tests/unit/db/repositories/Reservations.repository.test.ts @@ -384,3 +384,124 @@ describe('ReservationsRepository — RS20 (listUpcomingForUser)', () => { expect(baseline.some((r) => r.title === 'Hotel Ibis')).toBe(true); }); }); + +// ───────────────────────────────────────────────────────────────────────────── +// M3 (Plan 3d Task 7 whole-plan review): AC37/AC40 reverted to the legacy +// REAL-bound compare (parity is law — the widening had no reason to exist +// once H1 makes every write in this cluster store the legacy `'.0'` +// shape). Pinned against the SAME five-shape matrix the review measured. +// ───────────────────────────────────────────────────────────────────────────── + +describe('ReservationsRepository — AC37/AC40 (listIdMetadataByStay/listIdsByStay), M3', () => { + it('AC37/AC40 match the legacy REAL-bound compare — only the ".0" TEXT shape resolves, not the bare integer, a trailing/leading space or a prefix-numeric value', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-05' }); + const place = createPlace(testDb, trip.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(trip.id) as { id: number }[]; + const stay = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id); + + // `shapes[1]` ('.0') is the ONLY one the legacy `Number(id)` bind — + // and this repository's own reverted compare — ever matched. + const shapes = [ + `${stay.id}`, + `${stay.id}.0`, + `${stay.id} `, + `${stay.id}abc`, + ` ${stay.id}`, + ]; + const ids = shapes.map((shape, i) => { + const r = createReservation(testDb, trip.id, { title: `Link ${i}`, type: 'hotel' }); + testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(shape, r.id); + return r.id; + }); + + const typedMeta = await reservationsRepo.listIdMetadataByStay(stay.id); + const legacyMeta = testDb.prepare('SELECT id, metadata FROM reservations WHERE accommodation_id = ?').all(stay.id) as { id: number; metadata: string | null }[]; + expect(typedMeta).toEqual(legacyMeta); + expect(typedMeta.map((r) => r.id)).toEqual([ids[1]]); + + const typedIds = await reservationsRepo.listIdsByStay(stay.id); + const legacyIds = testDb.prepare('SELECT id FROM reservations WHERE accommodation_id = ?').all(stay.id) as { id: number }[]; + expect(typedIds).toEqual(legacyIds); + expect(typedIds.map((r) => r.id)).toEqual([ids[1]]); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// Plan 3d Task 7 whole-plan review, item 9 ("missing parity tests"): Task 2's +// own `ReservationsRepository` reads (`listForTrip`, `findWithJoins`, +// `findInTrip`, `listResyncCandidates`, `listForRestamp`) had no +// repository-level parity test — they rested on the reservations.service +// REAL-LEGACY suite only. ONE seeded world, one `toEqual()` +// test per method. +// ───────────────────────────────────────────────────────────────────────────── + +describe('ReservationsRepository — Task 2 reads (listForTrip / findWithJoins / findInTrip / listResyncCandidates / listForRestamp)', () => { + const legacyJoinedSql = ` + SELECT r.*, d.day_number as day_number, p.name as place_name, ap.place_id as accommodation_place_id, + acc_p.name as accommodation_name, ap.start_day_id as accommodation_start_day_id, ap.end_day_id as accommodation_end_day_id + FROM reservations r + LEFT JOIN days d ON d.id = r.day_id + LEFT JOIN places p ON p.id = r.place_id + LEFT JOIN day_accommodations ap ON ap.id = r.accommodation_id + LEFT JOIN places acc_p ON acc_p.id = ap.place_id`; + + const seed = () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-05' }); + const place = createPlace(testDb, trip.id, { name: 'Museum' }); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(trip.id) as { id: number }[]; + const hotelPlace = createPlace(testDb, trip.id, { name: 'Grand Hotel' }); + const stay = createDayAccommodation(testDb, trip.id, hotelPlace.id, days[0].id, days[1].id); + + // A dated, non-hotel booking — a restamp/resync candidate. + const dinner = createReservation(testDb, trip.id, { title: 'Dinner', type: 'restaurant', day_id: days[0].id }); + testDb.prepare("UPDATE reservations SET reservation_time = ? WHERE id = ?").run('2026-09-01T19:00', dinner.id); + + // A hotel booking linked to the stay above — the joined projection's place/accommodation columns. + const hotel = createReservation(testDb, trip.id, { title: 'Grand Hotel Stay', type: 'hotel', day_id: days[0].id }); + testDb.prepare('UPDATE reservations SET accommodation_id = ?, place_id = ? WHERE id = ?').run(`${stay.id}.0`, place.id, hotel.id); + + return { trip, days, place, hotelPlace, stay, dinner, hotel }; + }; + + it('RS18 listForTrip — matches the legacy joined statement, ordered by reservation_time then created_at', async () => { + const { trip } = seed(); + const legacy = testDb.prepare(`${legacyJoinedSql} WHERE r.trip_id = ? ORDER BY r.reservation_time ASC, r.created_at ASC`).all(trip.id); + const typed = await reservationsRepo.listForTrip(trip.id); + expect(typed).toEqual(legacy); + }); + + it('RR1 findWithJoins — matches the legacy joined statement for one row, place/accommodation columns populated', async () => { + const { hotel, stay, hotelPlace } = seed(); + const legacy = testDb.prepare(`${legacyJoinedSql} WHERE r.id = ?`).get(hotel.id); + const typed = await reservationsRepo.findWithJoins(hotel.id); + expect(typed).toEqual(legacy); + expect(typed).toMatchObject({ accommodation_place_id: hotelPlace.id, accommodation_name: hotelPlace.name, accommodation_start_day_id: stay.start_day_id }); + }); + + it('RS35 findInTrip — matches the legacy statement, scoped by trip', async () => { + const { trip, dinner } = seed(); + const legacy = testDb.prepare('SELECT * FROM reservations WHERE id = ? AND trip_id = ?').get(dinner.id, trip.id); + expect(await reservationsRepo.findInTrip(dinner.id, trip.id)).toEqual(legacy); + expect(await reservationsRepo.findInTrip(999999, trip.id)).toBeUndefined(); + }); + + it('RS12 listResyncCandidates — matches the legacy statement (dated, non-hotel-or-unlinked)', async () => { + const { trip, dinner } = seed(); + const legacy = testDb.prepare(` + SELECT id, reservation_time, reservation_end_time, day_id, end_day_id FROM reservations + WHERE trip_id = ? AND (type != 'hotel' OR accommodation_id IS NULL) AND reservation_time IS NOT NULL`).all(trip.id); + const typed = await reservationsRepo.listResyncCandidates(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id)).toEqual([dinner.id]); + }); + + it('DY14 listForRestamp — matches the legacy statement, every reservation of the trip regardless of type', async () => { + const { trip, dinner, hotel } = seed(); + const legacy = testDb.prepare('SELECT id, day_id, end_day_id, reservation_time, reservation_end_time FROM reservations WHERE trip_id = ?').all(trip.id); + const typed = await reservationsRepo.listForRestamp(trip.id); + expect(typed).toEqual(legacy); + expect(typed.map((r) => r.id).sort((a, b) => a - b)).toEqual([dinner.id, hotel.id].sort((a, b) => a - b)); + }); +}); diff --git a/server/tests/unit/db/repositories/_shared/reservation-visibility.test.ts b/server/tests/unit/db/repositories/_shared/reservation-visibility.test.ts index 6b3d4883f..bb7bd2c44 100644 --- a/server/tests/unit/db/repositories/_shared/reservation-visibility.test.ts +++ b/server/tests/unit/db/repositories/_shared/reservation-visibility.test.ts @@ -345,4 +345,66 @@ describe('reservation-visibility parity (RV2: publicStayExists vs publicStaySql) expect(typed.map((r) => r.id)).toEqual([unlinked.id, liveLinked.id]); expect(typed).toEqual(legacy); }); + + // M1 (Plan 3d Task 0 review, folded into Task 7's fix wave): the harness + // above never made the CAST load-bearing — dropping `eb2.cast(…, + // 'integer')` from `publicStayExists` stayed green on all of RESVIS-004.. + // 009, RESVIS-008 ("14.0") included, because a column-to-column compare + // already gets SQLite's own NUMERIC-affinity coercion for a + // WHOLLY-numeric TEXT value ('14 ', '14.0', ' 14' all equal 14 without + // any CAST — measured). Only a PREFIX-numeric value tells the two + // predicates apart: `CAST('14abc' AS INTEGER) = 14` is true, but the bare + // affinity compare `'14abc' = 14` is false, since '14abc' is not a + // well-formed number and keeps its TEXT affinity. + it('RESVIS-013 (mutation-sensitive): a staged-only booking with accommodation_id = "abc" still resolves the link — the stay is hidden (only the CAST recognises a prefix-numeric TEXT value)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-05' }); + const place = createPlace(testDb, trip.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(trip.id) as { id: number }[]; + const stay = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id); + const booking = createReservation(testDb, trip.id, { title: 'Staged prefix-numeric link', type: 'hotel' }); + testDb.prepare("UPDATE reservations SET accommodation_id = ?, ingest_state = 'staged' WHERE id = ?").run(`${stay.id}abc`, booking.id); + + const typed = await t.em.getKysely() + .selectFrom('day_accommodations as a') + .select('a.id') + .where('a.trip_id', '=', trip.id) + .where((eb) => publicStayExists(eb)) + .execute(); + const legacy = testDb + .prepare(`SELECT id FROM day_accommodations a WHERE a.trip_id = ? AND ${publicStaySql('a')}`) + .all(trip.id) as { id: number }[]; + + // The only link is staged, so the stay must be hidden — the SAME answer + // RESVIS-006 gives for a plain-integer-spelled staged link. Dropping the + // CAST makes the typed predicate miss the link entirely (a bare + // '14abc' = 14 compare is false) and fall into the NOT-EXISTS branch, + // wrongly reading the stay as public — this assertion goes red then, + // while `typed` still equals `legacy` under the untouched code. + expect(typed).toEqual([]); + expect(typed).toEqual(legacy); + }); + + it('RESVIS-014: a staged-only booking with accommodation_id = " " (trailing space) still resolves the link — the stay is hidden (both forms agree; SQLite\'s own affinity already coerces a wholly-numeric padded value)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2026-09-01', end_date: '2026-09-05' }); + const place = createPlace(testDb, trip.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number ASC').all(trip.id) as { id: number }[]; + const stay = createDayAccommodation(testDb, trip.id, place.id, days[0].id, days[1].id); + const booking = createReservation(testDb, trip.id, { title: 'Staged padded link', type: 'hotel' }); + testDb.prepare("UPDATE reservations SET accommodation_id = ?, ingest_state = 'staged' WHERE id = ?").run(`${stay.id} `, booking.id); + + const typed = await t.em.getKysely() + .selectFrom('day_accommodations as a') + .select('a.id') + .where('a.trip_id', '=', trip.id) + .where((eb) => publicStayExists(eb)) + .execute(); + const legacy = testDb + .prepare(`SELECT id FROM day_accommodations a WHERE a.trip_id = ? AND ${publicStaySql('a')}`) + .all(trip.id) as { id: number }[]; + + expect(typed).toEqual([]); + expect(typed).toEqual(legacy); + }); }); diff --git a/server/tests/unit/nest/roadtrip-planning.test.ts b/server/tests/unit/nest/roadtrip-planning.test.ts index 71e639bd2..0db7d8cd4 100644 --- a/server/tests/unit/nest/roadtrip-planning.test.ts +++ b/server/tests/unit/nest/roadtrip-planning.test.ts @@ -373,6 +373,70 @@ describe('Roadtrip MCP registration and search', () => { }); }); +// Item 11 (Plan 3d Task 7 whole-plan review, L5): `updateVia` above is the +// only tool in this class with its OWN non-member case — the mutation +// coverage reaches every tool (they all gate on the same `findAccessible` +// call), but only one had a dedicated test. One per tool, as +// `tools-days-accommodations` already does. +describe('RoadtripMcp — non-member refusal, one case per tool (Plan 3d Task 7 review, item 11)', () => { + function build(findAccessible: boolean) { + const service = { + dayExists: vi.fn(async () => true), + listForDay: vi.fn(async () => []), + listForTrip: vi.fn(async () => []), + tracksForTrip: vi.fn(async () => []), + trackExists: vi.fn(async () => true), + create: vi.fn(async () => ({ id: 1 })), + createMany: vi.fn(async () => []), + reanchor: vi.fn(async () => []), + remove: vi.fn(async () => true), + move: vi.fn(async () => ({ id: 1 })), + broadcast: vi.fn(), + }; + const tripsRepo = { findAccessible: vi.fn(async () => findAccessible) }; + const guards = { hasTripPermission: vi.fn(async () => true) }; + const auth = { isDemoUser: vi.fn(async () => false) }; + const mcp = new RoadtripMcp(service as never, tripsRepo as never, guards as never, auth as never, {} as never); + return { mcp, service, tripsRepo }; + } + + it('list_route_vias refuses a non-member trip (the legacy "not found or access denied" shape)', async () => { + const { mcp, service } = build(false); + const res = await mcp.listVias({ tripId: 10 }, ctx); + expect(res.isError).toBe(true); + expect(JSON.stringify(res)).toContain('access denied'); + expect(service.listForTrip).not.toHaveBeenCalled(); + }); + + it('add_route_via refuses a non-member trip', async () => { + const { mcp, service } = build(false); + const res = await mcp.addVia({ tripId: 10, dayId: 1, after_order_index: 0, lat: 48, lng: 10 }, ctx); + expect(res.isError).toBe(true); + expect(service.create).not.toHaveBeenCalled(); + }); + + it('add_route_vias refuses a non-member trip', async () => { + const { mcp, service } = build(false); + const res = await mcp.addVias({ tripId: 10, dayId: 1, vias: [] }, ctx); + expect(res.isError).toBe(true); + expect(service.createMany).not.toHaveBeenCalled(); + }); + + it('reanchor_route_vias refuses a non-member trip', async () => { + const { mcp, service } = build(false); + const res = await mcp.reanchorVias({ tripId: 10, dayId: 1, vias: [] }, ctx); + expect(res.isError).toBe(true); + expect(service.reanchor).not.toHaveBeenCalled(); + }); + + it('remove_route_via refuses a non-member trip', async () => { + const { mcp, service } = build(false); + const res = await mcp.removeVia({ tripId: 10, dayId: 1, viaId: 4 }, ctx); + expect(res.isError).toBe(true); + expect(service.remove).not.toHaveBeenCalled(); + }); +}); + describe('MCP trip preferences authorization', () => { it('reads shared preferences and refuses inaccessible trips', async () => { const s = setup(); @@ -382,6 +446,18 @@ describe('MCP trip preferences authorization', () => { expect((await tool.read({ tripId: 10 }, ctx)).isError).toBe(true); expect((await tool.update({ tripId: 10, settings: {} }, ctx)).isError).toBe(true); }); + + // Item 11 (Plan 3d Task 7 whole-plan review, L5): `update_roadtrip_settings` + // gets its OWN non-member case — the test above already asserts it as a + // second assertion piggybacked on the `read` test, but not in isolation. + it('update_roadtrip_settings refuses a non-member trip on its own, without a prior read call', async () => { + const s = setup(); + s.tripsRepo.findAccessible.mockResolvedValue(false); + const tool = new RoadtripPreferencesMcp(s.preferences, { isDemoUser: () => false } as never, {} as never, s.tripsRepo as never, { hasTripPermission: () => true } as never); + const res = await tool.update({ tripId: 10, settings: { roadtrip_range_km: 50 } }, ctx); + expect(res.isError).toBe(true); + expect(JSON.stringify(res)).toContain('access denied'); + }); it('rejects demos and readers, but permits a trip editor', async () => { const s = setup(); const auth = { isDemoUser: vi.fn(() => true) }; diff --git a/server/tests/unit/nest/roadtrip.service.test.ts b/server/tests/unit/nest/roadtrip.service.test.ts index e1d9ac140..530e170d0 100644 --- a/server/tests/unit/nest/roadtrip.service.test.ts +++ b/server/tests/unit/nest/roadtrip.service.test.ts @@ -281,6 +281,28 @@ describe('RoadtripService', () => { expect(await service.trackExists(999999, f.tripA.id)).toBe(false); }); + // L1 (Plan 3d Task 7 whole-plan review): `listForDay`/`listForTrip`/ + // `tracksForTrip`/`trackExists` used to bind `Number(id)` directly — a + // hex-spelled id coerces to a real row under `Number()`, where the legacy + // raw-bind statement's affinity never converts a hex string, so it always + // matched nothing. `toRowId` now answers that legacy empty/false shape. + it('L1: a hex-spelled day/trip id answers the legacy empty shape on every read path, not the real trip\'s rows', async () => { + await service.create(f.dayA1.id, { after_order_index: 0, lat: 53, lng: 10 }); + await service.createMany(f.dayA1.id, { vias: [], track: { place_id: f.track.id, stray_km: null } }); + + const hexDayId = '0x' + f.dayA1.id.toString(16); + const hexTripId = '0x' + f.tripA.id.toString(16); + + expect(await service.listForDay(hexDayId)).toEqual([]); + expect(await service.listForTrip(hexTripId)).toEqual([]); + expect(await service.tracksForTrip(hexTripId)).toEqual([]); + expect(await service.trackExists(f.track.id, hexTripId)).toBe(false); + + // Sanity: the canonical id still sees the real rows. + expect(await service.listForDay(f.dayA1.id)).toHaveLength(1); + expect(await service.tracksForTrip(f.tripA.id)).toHaveLength(1); + }); + it('ROADTRIP-SVC-007b: removing one reports whether there was anything to remove', async () => { const via = await service.create(f.dayA1.id, { after_order_index: 0, lat: 53, lng: 10 }); @@ -303,4 +325,25 @@ describe('RoadtripService', () => { } expect(await service.tracksForTrip(f.tripA.id)).toEqual([]); }); + + // R7 (Plan 3d Task 7 whole-plan review, item 12 — flag + pin, not fix, the + // MEMBERS-SVC-018 shape): `create`'s `nextSequence` (MAX(sequence)+1) READ + // and its `insertVia` WRITE are two separate statements with no lock + // between them. Measured under real concurrency (`Promise.allSettled`, no + // mocks): both calls succeed — there is no UNIQUE constraint to lose a + // race on, unlike `trip_members` — but both land on the SAME `sequence` + // (`0`), a genuine duplicate the un-serialized read-then-write leaves + // behind. Today's actual outcome, pinned; not a fix. + it('R7: two concurrent creates on the same leg both succeed but can land on the SAME sequence (unserialized nextSequence read)', async () => { + const results = await Promise.allSettled([ + service.create(f.dayA1.id, { after_order_index: 0, lat: 1, lng: 1 }), + service.create(f.dayA1.id, { after_order_index: 0, lat: 2, lng: 2 }), + ]); + expect(results.every((r) => r.status === 'fulfilled')).toBe(true); + const rows = await service.listForDay(f.dayA1.id); + expect(rows).toHaveLength(2); + // Both landed on the same leg, at the same sequence — the duplicate R7 flags. + expect(rows.map((r) => r.after_order_index)).toEqual([0, 0]); + expect(rows.map((r) => r.sequence)).toEqual([0, 0]); + }); });