diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index ccde3f7e1..af8509e21 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/server/src/nest/reservations/reservations.controller.ts @@ -172,11 +172,22 @@ export class ReservationsController { * and a foreign accommodation_id used to be stored verbatim and deleted with * the reservation. The MCP tools have refused foreign ids since they were * written; this is the REST half of the same rule. + * + * Two questions, two answers. Reaching into another trip is the older one and + * keeps its wording. An id that resolves to nothing at all is the other half + * of the same rule — it used to reach the statement and come back as the bare + * 500 SQLite's foreign keys produce (#2355) — and saying "not part of this + * trip" about an id that is part of nothing would send the caller looking in + * the wrong place. */ private rejectForeignReferences(tripId: string, body: ReservationBody): void { const offenders = this.reservations.referencesOutsideTrip(tripId, body as never); if (offenders.length > 0) { throw new HttpException({ error: `Not part of this trip: ${offenders.join(', ')}` }, 400); } + const unknown = this.reservations.unresolvedReferences(tripId, body as never); + if (unknown.length > 0) { + throw new HttpException({ error: `Unknown reference: ${unknown.join(', ')}` }, 400); + } } } diff --git a/server/src/nest/reservations/reservations.rpc.ts b/server/src/nest/reservations/reservations.rpc.ts index 2f73bb8c8..6786c8728 100644 --- a/server/src/nest/reservations/reservations.rpc.ts +++ b/server/src/nest/reservations/reservations.rpc.ts @@ -38,6 +38,7 @@ export class ReservationsRpc { const input = parsed.data as Record; this.requireValidEndpoints(input.endpoints); this.guards.requireTripEdit(tripId, actor, RESERVATION_EDIT_ACTION); + this.requireResolvableReferences(tripId, input); const { reservation, accommodationCreated } = this.reservations.create(String(tripId), input as never); if (accommodationCreated) this.realtime.broadcast(tripId, 'accommodation:created', {}, undefined); const i = input as { title?: string; type?: string; create_budget_entry?: unknown }; @@ -59,6 +60,7 @@ export class ReservationsRpc { this.guards.requireTripEdit(tripId, actor, RESERVATION_EDIT_ACTION); const current = this.reservations.getReservation(String(reservationId), String(tripId)); if (!current) throw new ForbiddenResource(`no reservation ${reservationId} on trip ${tripId}`); + this.requireResolvableReferences(tripId, input); const { reservation, accommodationChanged } = this.reservations.update(String(reservationId), String(tripId), input as never, current as never); if (accommodationChanged) this.realtime.broadcast(tripId, 'accommodation:updated', {}, undefined); const cur = current as { title: string; type?: string }; @@ -97,6 +99,22 @@ export class ReservationsRpc { if (!parsed.success) throw new BadParams(`invalid endpoints: ${schemaMessage(parsed.error)}`); } + /** + * An id that resolves to nothing is a foreign-key error the plugin reads as a + * crash (#2355), so it is named here instead. Only that case: a plugin has + * always been able to point at another trip's day or place through this + * surface, and taking that away is a decision for the plugin contract, not a + * side effect of a bug fix — so the ids the REST route refuses for being + * somebody else's are filtered back out. + */ + private requireResolvableReferences(tripId: number, input: Record): void { + const elsewhere = new Set(this.reservations.referencesOutsideTrip(String(tripId), input as never)); + const unknown = this.reservations + .unresolvedReferences(String(tripId), input as never) + .filter((field) => !elsewhere.has(field)); + if (unknown.length > 0) throw new BadParams(`unknown reference: ${unknown.join(', ')}`); + } + /** Fire-and-forget, exactly as the REST controller sends it, so it never blocks the write. */ private notifyBooking(actingUserId: number, tripId: number, booking: string, type: string): void { this.reservations.notifyBookingChange(tripId, actingUserId, booking, type); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 4d24a1fa3..5d190e8ba 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -1,4 +1,4 @@ -import { Injectable } from '@nestjs/common'; +import { BadRequestException, Injectable } from '@nestjs/common'; import { DatabaseService, type TripAccess } from '../database/database.service'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; @@ -565,6 +565,104 @@ export class ReservationsService { return offenders; } + /** + * Name every id in the body that resolves to nothing on this trip. + * + * A second guard rather than a stricter referencesOutsideTrip: that one + * answers "does this id belong to someone else", and a row that exists + * nowhere cannot. accommodation_id depends on that answer staying no, or a + * booking whose stay was cascaded away could never be saved again (#522). + * The fields here are the ones carrying a real foreign key, where an id that + * resolves to nothing is not a gap but a constraint failure, and SQLite + * raising it reaches the caller as a bare 500 (#2355). accommodation_id is + * absent by design; do not complete the list. + * + * Only a truthy id is looked up. The write paths below coerce 0 and '' to + * NULL before they reach SQL, so naming one here would turn a body that + * stores a null today into a 400. + * + * Returns the offending field names, empty when the body is clean. An id + * that belongs to another trip is named here too — the REST controller asks + * the older guard first, so that case keeps its own answer. + */ + unresolvedReferences(tripId: string | number, data: CreateReservationData | UpdateReservationData): string[] { + const offenders: string[] = []; + const onTrip = (table: 'days' | 'places', id: unknown) => + !!this.db.get(`SELECT id FROM ${table} WHERE id = ? AND trip_id = ?`, id, tripId); + + if (data.day_id && !onTrip('days', data.day_id)) offenders.push('day_id'); + if (data.end_day_id && !onTrip('days', data.end_day_id)) offenders.push('end_day_id'); + if (data.place_id && !onTrip('places', data.place_id)) offenders.push('place_id'); + if (data.assignment_id) { + // An assignment belongs to a trip through its day, the same join the + // other guard walks. + const row = this.db.get( + 'SELECT da.id FROM day_assignments da JOIN days d ON da.day_id = d.id WHERE da.id = ? AND d.trip_id = ?', + data.assignment_id, tripId, + ); + if (!row) offenders.push('assignment_id'); + } + + // Only a hotel booking writes the stay row, so only there do these ids + // reach SQL. Any other body carries them as dead weight today, and a 400 + // on a write that currently succeeds is not what this guard is for. + if (data.create_accommodation && data.type === 'hotel') { + const acc = data.create_accommodation; + const errors = this.accommodations.validateAccommodationRefs( + tripId, acc.place_id || undefined, acc.start_day_id || undefined, acc.end_day_id || undefined, + ); + for (const { field } of errors) offenders.push(`create_accommodation.${field}`); + } + + return offenders; + } + + /** Is there still a row behind this id? Existence only — which trip it sits + * on is the guards' question, and they answer it before the write. */ + private referenceExists(table: 'days' | 'places' | 'day_assignments', id: unknown): boolean { + return !!this.db.get(`SELECT id FROM ${table} WHERE id = ?`, id); + } + + /** + * An id whose row is gone reads as no id at all. + * + * day_id, end_day_id, place_id and assignment_id are declared ON DELETE SET + * NULL, so a reference that resolves to nothing is precisely the state the + * cascade leaves behind, and clearing it is what the column already promises. + * Binding it instead is the foreign-key error that arrives as a bare 500 + * (#2355), and an update rebinds whatever the row already held, so a guard + * on the body alone never reaches it. + */ + private resolvedOrNull(table: 'days' | 'places' | 'day_assignments', id: number | null): number | null { + return id != null && this.referenceExists(table, id) ? id : null; + } + + /** + * day_accommodations.start_day_id and end_day_id are NOT NULL, so there is + * nothing to heal an unresolvable one to and it can only be refused. The + * write surfaces name the field long before this; this is the floor under + * the importers and the plugin host, and it refuses rather than skipping so + * an edit is never dropped in silence. + * + * Both days are already known to be set where this is called. A place is + * not: the booking form writes stays that never had one. + * + * BadRequestException rather than a domain error class, because the filter + * already knows what to do with it: a caller that reaches this over HTTP gets + * the same 400 { error } the controller's own guard sends, and not the 500 an + * unplaceable class would collapse to (#2355). Its message survives, so the + * importer that logs and moves on still names the field. + */ + private requireResolvableStay(acc: CreateAccommodation): void { + const missing: string[] = []; + if (acc.place_id && !this.referenceExists('places', acc.place_id)) missing.push('place_id'); + if (!this.referenceExists('days', acc.start_day_id)) missing.push('start_day_id'); + if (!this.referenceExists('days', acc.end_day_id)) missing.push('end_day_id'); + if (missing.length > 0) { + throw new BadRequestException(`Unknown reference: ${missing.map((field) => `create_accommodation.${field}`).join(', ')}`); + } + } + /** The accommodation insert, the reservation insert, the endpoint save and * the metadata sync are one logical write — all-or-nothing. */ create(tripId: string | number, data: CreateReservationData): { reservation: ReservationRow; accommodationCreated: boolean } { @@ -594,6 +692,7 @@ export class ReservationsService { if (type === 'hotel' && !resolvedAccommodationId && create_accommodation) { const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation; if (start_day_id && end_day_id) { + this.requireResolvableStay(create_accommodation); const accResult = this.db.run( 'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_out, confirmation) VALUES (?, ?, ?, ?, ?, ?, ?)', tripId, accPlaceId || null, start_day_id, end_day_id, check_in || null, check_out || null, accConf || confirmation_number || null @@ -621,6 +720,11 @@ export class ReservationsService { resolvedEndDayId = this.resolveDayIdFromTime(tripId, reservation_end_time); } + resolvedDayId = this.resolvedOrNull('days', resolvedDayId); + resolvedEndDayId = this.resolvedOrNull('days', resolvedEndDayId); + const resolvedPlaceId = this.resolvedOrNull('places', place_id || null); + const resolvedAssignmentId = this.resolvedOrNull('day_assignments', assignment_id || null); + const result = this.db.run(` INSERT INTO reservations (trip_id, day_id, end_day_id, place_id, assignment_id, title, reservation_time, reservation_end_time, location, confirmation_number, notes, url, status, type, accommodation_id, metadata, needs_review) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) @@ -628,8 +732,8 @@ export class ReservationsService { tripId, resolvedDayId, resolvedEndDayId, - place_id || null, - assignment_id || null, + resolvedPlaceId, + resolvedAssignmentId, title, reservation_time || null, reservation_end_time || null, @@ -740,6 +844,7 @@ export class ReservationsService { if (type === 'hotel' && create_accommodation) { const { place_id: accPlaceId, start_day_id, end_day_id, check_in, check_out, confirmation: accConf } = create_accommodation; if (start_day_id && end_day_id) { + this.requireResolvableStay(create_accommodation); if (resolvedAccId) { this.db.run( 'UPDATE day_accommodations SET place_id = ?, start_day_id = ?, end_day_id = ?, check_in = ?, check_out = ?, confirmation = ? WHERE id = ?', @@ -804,6 +909,11 @@ export class ReservationsService { nextEndDayId = current.end_day_id ?? null; } + nextDayId = this.resolvedOrNull('days', nextDayId); + nextEndDayId = this.resolvedOrNull('days', nextEndDayId); + const nextPlaceId = this.resolvedOrNull('places', place_id !== undefined ? (place_id || null) : (current.place_id ?? null)); + const nextAssignmentId = this.resolvedOrNull('day_assignments', assignment_id !== undefined ? (assignment_id || null) : (current.assignment_id ?? null)); + this.db.run(` UPDATE reservations SET title = COALESCE(?, title), @@ -833,8 +943,8 @@ export class ReservationsService { url !== undefined ? (url || null) : (current as Reservation & { url?: string | null }).url, nextDayId, nextEndDayId, - place_id !== undefined ? (place_id || null) : current.place_id, - assignment_id !== undefined ? (assignment_id || null) : current.assignment_id, + nextPlaceId, + nextAssignmentId, status || null, type || null, resolvedAccId, diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index 30e91e563..3a8320e60 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -27,6 +27,10 @@ const { db } = vi.hoisted(() => { const Database = require('better-sqlite3'); const tmp = new Database(':memory:'); tmp.exec('PRAGMA journal_mode = WAL'); + // What production runs (db/database.ts) and what createTestDb gives every + // unit suite. Without it the reservation foreign keys are inert here, and an + // id that resolves to nothing passes the mount unnoticed. + tmp.exec('PRAGMA foreign_keys = ON'); return { db: tmp }; }); @@ -146,6 +150,54 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real expect(bad.body.error).toContain('title'); }); + // The reported repro (#2355): an id that resolves to nothing used to reach + // the statement and come back as an unhandled SqliteError, i.e. a bare 500. + it('400 on an update whose place_id exists nowhere, and the row is left alone', async () => { + const rid = Number(db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Dinner', 'other')").run(tripId).lastInsertRowid); + + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/${rid}`) + .set('Cookie', sessionCookie(1)) + .send({ title: 'Dinner, later', place_id: 999999 }); + + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Unknown reference: place_id' }); + expect(db.prepare('SELECT title, place_id FROM reservations WHERE id = ?').get(rid)).toEqual({ title: 'Dinner', place_id: null }); + }); + + it('400 on a create whose create_accommodation day exists nowhere, and no stay is written', async () => { + const placeId = Number(db.prepare('INSERT INTO places (trip_id, name) VALUES (?, ?)').run(tripId, 'Hotel Unknown').lastInsertRowid); + const dayId = Number(db.prepare('INSERT INTO days (trip_id, day_number, date) VALUES (?, 7, ?)').run(tripId, '2026-03-07').lastInsertRowid); + const before = db.prepare('SELECT COUNT(*) as c FROM day_accommodations WHERE trip_id = ?').get(tripId); + + const res = await request(server) + .post(`/api/trips/${tripId}/reservations`) + .set('Cookie', sessionCookie(1)) + .send({ title: 'Stay', type: 'hotel', create_accommodation: { place_id: placeId, start_day_id: dayId, end_day_id: 999999 } }); + + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Unknown reference: create_accommodation.end_day_id' }); + expect(db.prepare('SELECT COUNT(*) as c FROM day_accommodations WHERE trip_id = ?').get(tripId)).toEqual(before); + }); + + // #522, which must survive all of the above: shortening a trip cascades the + // stay away and leaves the booking pointing at a gap, and the booking still + // has to be savable. + it('200 on an update whose stored accommodation_id no longer resolves', async () => { + const rid = Number( + db.prepare("INSERT INTO reservations (trip_id, title, type, accommodation_id) VALUES (?, 'Stay', 'hotel', 999999)").run(tripId).lastInsertRowid, + ); + + const res = await request(server) + .put(`/api/trips/${tripId}/reservations/${rid}`) + .set('Cookie', sessionCookie(1)) + .send({ title: 'Stay, renamed', accommodation_id: 999999 }); + + expect(res.status).toBe(200); + expect(db.prepare('SELECT title, accommodation_id FROM reservations WHERE id = ?').get(rid)) + .toEqual({ title: 'Stay, renamed', accommodation_id: null }); + }); + it('200 list accommodations + 201 create (real insert + auto hotel reservation), 404 on bad refs', async () => { const placeId = Number(db.prepare('INSERT INTO places (trip_id, name) VALUES (?, ?)').run(tripId, 'Grand Hotel').lastInsertRowid); const dayId = Number(db.prepare('INSERT INTO days (trip_id, day_number, date) VALUES (?, 1, ?)').run(tripId, '2026-03-01').lastInsertRowid); diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index 4c994d001..bd21888d2 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -16,6 +16,7 @@ function makeService(overrides: Partial = {}): Reservations verifyTripAccess: vi.fn().mockReturnValue(trip), canEdit: vi.fn().mockReturnValue(true), referencesOutsideTrip: vi.fn().mockReturnValue([]), + unresolvedReferences: vi.fn().mockReturnValue([]), broadcast: vi.fn(), syncBudgetOnCreate: vi.fn(), syncBudgetOnUpdate: vi.fn(), @@ -68,6 +69,30 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese .toEqual({ status: 400, body: { error: 'Not part of this trip: accommodation_id' } }); expect(create).not.toHaveBeenCalled(); }); + + it('400s on a body id that exists nowhere, in its own words, without writing', () => { + const create = vi.fn(); + const svc = makeService({ + create, + unresolvedReferences: vi.fn().mockReturnValue(['place_id']), + } as Partial); + const body = { title: 'Hotel', place_id: 4711 }; + // Not 'Not part of this trip': an id that is part of nothing would send + // the caller looking for it on another trip. + expect(thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', body))) + .toEqual({ status: 400, body: { error: 'Unknown reference: place_id' } }); + expect(create).not.toHaveBeenCalled(); + }); + + it('answers a foreign id with the older message when it is both', () => { + const svc = makeService({ + create: vi.fn(), + referencesOutsideTrip: vi.fn().mockReturnValue(['place_id']), + unresolvedReferences: vi.fn().mockReturnValue(['place_id']), + } as Partial); + expect(thrown(() => new ReservationsController(svc, airtrailLink).create(user, '5', { title: 'Hotel', place_id: 4711 }))) + .toEqual({ status: 400, body: { error: 'Not part of this trip: place_id' } }); + }); }); describe('PUT /positions', () => { @@ -112,6 +137,20 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese .toEqual({ status: 400, body: { error: 'Not part of this trip: day_id, place_id' } }); expect(update).not.toHaveBeenCalled(); }); + + it('400s on a body id that exists nowhere, without writing', () => { + const update = vi.fn(); + const svc = makeService({ + getReservation: vi.fn().mockReturnValue({ title: 'Old', type: 'lodging' }), + update, + unresolvedReferences: vi.fn().mockReturnValue(['day_id', 'place_id']), + } as Partial); + // The reporter's request: a foreign-key error used to reach the caller as + // a bare 500 here. + expect(thrown(() => new ReservationsController(svc, airtrailLink).update(user, '5', '9', { day_id: 1, place_id: 2 }))) + .toEqual({ status: 400, body: { error: 'Unknown reference: day_id, place_id' } }); + expect(update).not.toHaveBeenCalled(); + }); }); describe('PUT /:id/travelers', () => { diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index 58f39e6f8..87ff3945c 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -43,10 +43,11 @@ const budget = { createBudgetItem: vi.fn(), updateBudgetItem: vi.fn(), deleteBud const { notif } = vi.hoisted(() => ({ notif: { send: vi.fn().mockResolvedValue(undefined) } })); +import { HttpException } from '@nestjs/common'; import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createReservation, createBudgetItem, createPlace, createDay, createDayAccommodation, addTripMember } from '../../helpers/factories'; +import { createUser, createTrip, createReservation, createBudgetItem, createPlace, createDay, createDayAccommodation, createDayAssignment, addTripMember } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; @@ -1054,6 +1055,151 @@ describe('ReservationsService — referenced ids stay inside the trip', () => { expect(testDb.prepare('SELECT check_in FROM day_accommodations WHERE id = ?').get(foreignAcc.id)).toEqual({ check_in: '14:00' }); expect(testDb.prepare('SELECT accommodation_id FROM reservations WHERE id = ?').get(res.id)).toEqual({ accommodation_id: null }); }); + + /** A row from before the foreign keys, or from a window where they were off: + * the only way to plant one today is the way it got there. */ + function withForeignKeysOff(plant: () => void) { + testDb.exec('PRAGMA foreign_keys = OFF'); + try { plant(); } finally { testDb.exec('PRAGMA foreign_keys = ON'); } + } + + it('RESV-SCOPE-006: an id that resolves to nothing is named too, accommodation_id excepted', () => { + const { mine } = twoTrips(); + const gone = 999999; + + expect(svc.unresolvedReferences(String(mine.id), { + title: 'x', type: 'hotel', + day_id: gone, end_day_id: gone, place_id: gone, assignment_id: gone, accommodation_id: gone, + create_accommodation: { place_id: gone, start_day_id: gone, end_day_id: gone }, + })).toEqual([ + 'day_id', 'end_day_id', 'place_id', 'assignment_id', + 'create_accommodation.place_id', 'create_accommodation.start_day_id', 'create_accommodation.end_day_id', + ]); + }); + + it('RESV-SCOPE-007: ids on the trip pass, and a falsy one is not a reference at all', () => { + const { mine } = twoTrips(); + const place = createPlace(testDb, mine.id); + const day = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').get(mine.id) as { id: number }; + const assignment = createDayAssignment(testDb, day.id, place.id); + + expect(svc.unresolvedReferences(String(mine.id), { + title: 'x', type: 'hotel', + day_id: day.id, end_day_id: day.id, place_id: place.id, assignment_id: assignment.id, + create_accommodation: { place_id: place.id, start_day_id: day.id, end_day_id: day.id }, + })).toEqual([]); + // The write paths coerce these to NULL before they reach SQL, so naming + // them would 400 a body that stores a null today. + expect(svc.unresolvedReferences(String(mine.id), { title: 'x', day_id: 0, end_day_id: 0, place_id: 0, assignment_id: 0 })).toEqual([]); + // Only a hotel booking writes the stay row. + expect(svc.unresolvedReferences(String(mine.id), { + title: 'x', type: 'flight', create_accommodation: { place_id: 999999, start_day_id: 999999, end_day_id: 999999 }, + })).toEqual([]); + // A stay without a place is a state the booking form writes. + expect(svc.unresolvedReferences(String(mine.id), { + title: 'x', type: 'hotel', create_accommodation: { start_day_id: day.id, end_day_id: day.id }, + })).toEqual([]); + }); + + it('RESV-SCOPE-008: an id from another trip is named by both guards, so the older one answers first', () => { + const { mine, theirs } = twoTrips(); + const foreignPlace = createPlace(testDb, theirs.id); + const body = { title: 'x', place_id: foreignPlace.id }; + + expect(svc.referencesOutsideTrip(String(mine.id), body)).toEqual(['place_id']); + expect(svc.unresolvedReferences(String(mine.id), body)).toEqual(['place_id']); + }); + + it('RESV-SCOPE-009: an update heals the stored ids whose rows are gone instead of failing on them', () => { + const { mine } = twoTrips(); + const res = createReservation(testDb, mine.id, { title: 'Dinner' }); + withForeignKeysOff(() => { + testDb.prepare('UPDATE reservations SET day_id = ?, end_day_id = ?, place_id = ?, assignment_id = ? WHERE id = ?') + .run(999999, 999998, 999997, 999996, res.id); + }); + const current = svc.getReservation(String(res.id), String(mine.id))!; + + // The body names none of them, so the statement rebinds what the row holds. + svc.update(String(res.id), String(mine.id), { title: 'Dinner, later' }, current); + + expect(testDb.prepare('SELECT title, day_id, end_day_id, place_id, assignment_id FROM reservations WHERE id = ?').get(res.id)) + .toEqual({ title: 'Dinner, later', day_id: null, end_day_id: null, place_id: null, assignment_id: null }); + }); + + it('RESV-SCOPE-010: a supplied id that resolves to nothing is stored as NULL, on create and on update', () => { + const { mine } = twoTrips(); + const { reservation } = svc.create(String(mine.id), { + title: 'Dinner', day_id: 999999, end_day_id: 999998, place_id: 999997, assignment_id: 999996, + }); + expect(testDb.prepare('SELECT day_id, end_day_id, place_id, assignment_id FROM reservations WHERE id = ?').get(reservation.id)) + .toEqual({ day_id: null, end_day_id: null, place_id: null, assignment_id: null }); + + const current = svc.getReservation(String(reservation.id), String(mine.id))!; + svc.update(String(reservation.id), String(mine.id), { place_id: 999997 }, current); + expect(testDb.prepare('SELECT place_id FROM reservations WHERE id = ?').get(reservation.id)).toEqual({ place_id: null }); + }); + + it('RESV-SCOPE-011: a stay whose refs resolve to nothing is refused, never written and never dropped in silence', () => { + const { mine } = twoTrips(); + const place = createPlace(testDb, mine.id); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(mine.id) as { id: number }[]; + + expect(() => svc.create(String(mine.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { place_id: 999999, start_day_id: 999998, end_day_id: 999997 }, + })).toThrow('Unknown reference: create_accommodation.place_id, create_accommodation.start_day_id, create_accommodation.end_day_id'); + expect(testDb.prepare('SELECT COUNT(*) as c FROM day_accommodations WHERE trip_id = ?').get(mine.id)).toEqual({ c: 0 }); + expect(testDb.prepare('SELECT COUNT(*) as c FROM reservations WHERE trip_id = ?').get(mine.id)).toEqual({ c: 0 }); + + // On an existing stay the edit is refused rather than skipped: dropping it + // would leave the caller with a 200 and the old dates. + const acc = createDayAccommodation(testDb, mine.id, place.id, days[0].id, days[1].id); + const res = createReservation(testDb, mine.id, { title: 'Hotel', type: 'hotel' }); + testDb.prepare('UPDATE reservations SET accommodation_id = ? WHERE id = ?').run(acc.id, res.id); + const current = svc.getReservation(String(res.id), String(mine.id))!; + + expect(() => svc.update(String(res.id), String(mine.id), { + type: 'hotel', create_accommodation: { place_id: place.id, start_day_id: days[0].id, end_day_id: 999999 }, + }, current)).toThrow('Unknown reference: create_accommodation.end_day_id'); + expect(testDb.prepare('SELECT end_day_id FROM day_accommodations WHERE id = ?').get(acc.id)).toEqual({ end_day_id: days[1].id }); + }); + + it('RESV-SCOPE-012: a stay booked without a place is still written', () => { + const { mine } = twoTrips(); + const days = testDb.prepare('SELECT id FROM days WHERE trip_id = ? ORDER BY day_number').all(mine.id) as { id: number }[]; + + const { reservation, accommodationCreated } = svc.create(String(mine.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { start_day_id: days[0].id, end_day_id: days[1].id, check_in: '15:00' }, + }); + + expect(accommodationCreated).toBe(true); + expect(testDb.prepare('SELECT place_id, start_day_id FROM day_accommodations WHERE id = ?').get(Number(reservation.accommodation_id))) + .toEqual({ place_id: null, start_day_id: days[0].id }); + }); + + it('RESV-SCOPE-013: the refusal carries the 400 the write surfaces send, not a status the filter has to guess', () => { + const { mine } = twoTrips(); + + // The write surfaces name the field before the service ever sees it, so the + // class only matters to whoever reaches this floor without one. It answers + // as they do — the filter passes a 400 through and keeps the message, where + // a class it cannot place would become 'Internal server error' (#2355). + let thrown: unknown; + try { + svc.create(String(mine.id), { + title: 'Hotel', type: 'hotel', + create_accommodation: { start_day_id: 999998, end_day_id: 999997 }, + }); + } catch (e) { + thrown = e; + } + + expect(thrown).toBeInstanceOf(HttpException); + expect((thrown as HttpException).getStatus()).toBe(400); + expect((thrown as HttpException).message) + .toBe('Unknown reference: create_accommodation.start_day_id, create_accommodation.end_day_id'); + }); }); describe('the day stop a hotel booking implies', () => { diff --git a/server/tests/unit/plugins/rpc-fallbacks.test.ts b/server/tests/unit/plugins/rpc-fallbacks.test.ts index 6caa8503d..ae7a0a1e2 100644 --- a/server/tests/unit/plugins/rpc-fallbacks.test.ts +++ b/server/tests/unit/plugins/rpc-fallbacks.test.ts @@ -195,6 +195,8 @@ describe('service errors are translated into the RPC taxonomy', () => { getReservation: vi.fn(() => ({ id: 5, title: 'Hotel', type: 'lodging' })), update: vi.fn(() => ({ reservation: { id: 5 }, accommodationChanged: false })), syncBudgetOnUpdate: vi.fn(), + referencesOutsideTrip: vi.fn(() => []), + unresolvedReferences: vi.fn(() => []), notifyBookingChange, } as never; const host = new PluginRpcHost('p', ALL, makeDeps(), createTestPluginRegistry([new ReservationsRpc(reservations, realtime(), guards)])); diff --git a/server/tests/unit/reservations/bookings.rpc.test.ts b/server/tests/unit/reservations/bookings.rpc.test.ts index 15603e29d..b17501159 100644 --- a/server/tests/unit/reservations/bookings.rpc.test.ts +++ b/server/tests/unit/reservations/bookings.rpc.test.ts @@ -30,7 +30,7 @@ const req = (method: string, params: Record = {}): RpcRequest = const events = (r: { broadcast: ReturnType }) => r.broadcast.mock.calls.map((c) => c[1]); /** Trip 1 belongs to user 42; reservation 5 and accommodation 11 sit on it. */ -function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; seenActions?: string[] } = {}) { +function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; seenActions?: string[]; unresolved?: string[]; foreign?: string[] } = {}) { const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const reservations = { create: vi.fn(() => ({ reservation: { id: 40 }, accommodationCreated: !!opts.cascade })), @@ -44,6 +44,9 @@ function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; see syncBudgetOnCreate: vi.fn(), syncBudgetOnUpdate: vi.fn(), notifyBookingChange: vi.fn(), + // Both guards run before a write now, so the fixture answers for them. + referencesOutsideTrip: vi.fn(() => opts.foreign ?? []), + unresolvedReferences: vi.fn(() => opts.unresolved ?? []), } as unknown as ReservationsService & Record>; /** What a stay write did to the day plan, on top of writing the stay itself. */ type Mirror = { created: { id: number; day_id: number } | null; removed: { id: number; dayId: number }[]; stamped: null }; @@ -193,6 +196,34 @@ describe('ReservationsRpc', () => { expect(f.reservations.update).not.toHaveBeenCalled(); }); + it('BOOK-RPC-018 an id that resolves to nothing is BAD_PARAMS, not a constraint failure', async () => { + const f = build({ unresolved: ['place_id'] }); + const res = (await f.host().dispatch( + req('reservations.create', { tripId: 1, input: { title: 'Hotel', type: 'lodging', place_id: 999999 } }), 42, + )) as RpcError; + expect(res.error.code).toBe('BAD_PARAMS'); + expect(res.error.message).toBe('unknown reference: place_id'); + expect(f.reservations.create).not.toHaveBeenCalled(); + }); + + it('BOOK-RPC-019 the same on update, while an id on another trip still writes', async () => { + const f = build({ unresolved: ['day_id'] }); + const res = (await f.host().dispatch( + req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'lodging', day_id: 999999 } }), 42, + )) as RpcError; + expect(res.error.code).toBe('BAD_PARAMS'); + expect(f.reservations.update).not.toHaveBeenCalled(); + + // A plugin has always been able to name another trip's row through this + // surface. Taking that away is a change to the plugin contract, not part of + // turning a crash into an error. + const foreign = build({ unresolved: ['day_id'], foreign: ['day_id'] }); + expect((await foreign.host().dispatch( + req('reservations.update', { tripId: 1, reservationId: 5, input: { title: 'x', type: 'lodging', day_id: 7 } }), 42, + )).ok).toBe(true); + expect(foreign.reservations.update).toHaveBeenCalled(); + }); + it('BOOK-RPC-009 the class is listed in its module providers', () => { expectRegisteredProvider(ReservationsModule, ReservationsRpc); });