diff --git a/server/src/nest/common/body-contract-allow-list.ts b/server/src/nest/common/body-contract-allow-list.ts index 188c15fac..ffac7898a 100644 --- a/server/src/nest/common/body-contract-allow-list.ts +++ b/server/src/nest/common/body-contract-allow-list.ts @@ -119,10 +119,6 @@ export const BODY_CONTRACT_ALLOW_LIST: string[] = [ 'PluginsController.setEgressHosts', 'PluginsController.uninstall', 'PluginsController.updateConfig', - 'ReservationsController.create', - 'ReservationsController.update', - 'ReservationsController.updatePositions', - 'ReservationsController.updateTravelers', 'SynologyMemoriesController.putSettings', 'SynologyMemoriesController.search', 'SynologyMemoriesController.test', diff --git a/server/src/nest/reservations/reservations.controller.ts b/server/src/nest/reservations/reservations.controller.ts index bfba35e9c..de546c0dc 100644 --- a/server/src/nest/reservations/reservations.controller.ts +++ b/server/src/nest/reservations/reservations.controller.ts @@ -15,6 +15,12 @@ import { ReservationsService } from './reservations.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; import { CurrentUser } from '../auth/current-user.decorator'; import { pushReservationToAirtrail } from '../../services/airtrail/airtrailSync'; +import { + ReservationCreateDto, + ReservationUpdateDto, + ReservationPositionsDto, + ReservationTravelersDto, +} from './reservations.dto'; type ReservationBody = Record & { title?: string; @@ -27,9 +33,11 @@ type ReservationBody = Record & { * * Byte-identical to the legacy Express route (server/src/routes/reservations.ts): * trip access (404), 'reservation_edit' permission (403), create 201 / rest 200, - * the bespoke 400/404 bodies, the accommodation + budget side effects, the - * booking notifications, and all WebSocket broadcasts with the forwarded - * X-Socket-Id. /positions is declared before /:id so it wins over the param. + * the bespoke 404 bodies, the accommodation + budget side effects, the booking + * notifications, and all WebSocket broadcasts with the forwarded X-Socket-Id. + * Invalid bodies (missing title, non-array positions/user_ids) now 400 through + * the global ZodValidationPipe envelope instead of the bespoke strings. + * /positions is declared before /:id so it wins over the param. */ @Controller('api/trips/:tripId/reservations') @UseGuards(JwtAuthGuard) @@ -60,14 +68,12 @@ export class ReservationsController { create( @CurrentUser() user: User, @Param('tripId') tripId: string, - @Body() body: ReservationBody, + @Body() rawBody: ReservationCreateDto, @Headers('x-socket-id') socketId?: string, ) { + const body = rawBody as ReservationBody & { title: string }; const trip = this.requireTrip(tripId, user); this.requireEdit(trip, user); - if (!body.title) { - throw new HttpException({ error: 'Title is required' }, 400); - } const { reservation, accommodationCreated } = this.reservations.create(tripId, body as never); if (accommodationCreated) { this.reservations.broadcast(tripId, 'accommodation:created', {}, socketId); @@ -82,15 +88,14 @@ export class ReservationsController { updatePositions( @CurrentUser() user: User, @Param('tripId') tripId: string, - @Body() body: { positions?: unknown; day_id?: unknown }, + @Body() body: ReservationPositionsDto, @Headers('x-socket-id') socketId?: string, ) { const trip = this.requireTrip(tripId, user); this.requireEdit(trip, user); - if (!Array.isArray(body.positions)) { - throw new HttpException({ error: 'positions must be an array' }, 400); - } - this.reservations.updatePositions(tripId, body.positions, body.day_id); + // The legacy signature declares day_plan_position required, but the wire + // contract tolerates absent values (bind NULL) — see the shared schema. + this.reservations.updatePositions(tripId, body.positions as { id: number; day_plan_position: number }[], body.day_id); this.reservations.broadcast(tripId, 'reservation:positions', { positions: body.positions, day_id: body.day_id }, socketId); return { success: true }; } @@ -100,9 +105,10 @@ export class ReservationsController { @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, - @Body() body: ReservationBody, + @Body() rawBody: ReservationUpdateDto, @Headers('x-socket-id') socketId?: string, ) { + const body = rawBody as ReservationBody; const trip = this.requireTrip(tripId, user); this.requireEdit(trip, user); const current = this.reservations.getReservation(id, tripId); @@ -130,15 +136,12 @@ export class ReservationsController { @CurrentUser() user: User, @Param('tripId') tripId: string, @Param('id') id: string, - @Body('user_ids') userIds: unknown, + @Body() body: ReservationTravelersDto, @Headers('x-socket-id') socketId?: string, ) { const trip = this.requireTrip(tripId, user); this.requireEdit(trip, user); - if (!Array.isArray(userIds)) { - throw new HttpException({ error: 'user_ids must be an array' }, 400); - } - const result = this.reservations.setTravelers(id, tripId, userIds as number[]); + const result = this.reservations.setTravelers(id, tripId, body.user_ids); if (!result) { throw new HttpException({ error: 'Reservation not found' }, 404); } diff --git a/server/src/nest/reservations/reservations.dto.ts b/server/src/nest/reservations/reservations.dto.ts new file mode 100644 index 000000000..3f2cdea8b --- /dev/null +++ b/server/src/nest/reservations/reservations.dto.ts @@ -0,0 +1,21 @@ +import { createZodDto } from 'nestjs-zod'; +import { + reservationCreateRequestSchema, + reservationUpdateRequestSchema, + reservationPositionsRequestSchema, + reservationTravelersRequestSchema, +} from '@trek/shared'; + +/** + * Server-side createZodDto wrappers over the @trek/shared reservation + * contracts. The global ZodValidationPipe (APP_PIPE in app.module.ts) + * validates any @Body() parameter typed with one of these classes by + * metatype — the Zod schemas in shared/ remain the single source of truth + * for the wire contract. Create/update stay open records on purpose: the + * legacy route accepted the whole Reservation shape as an arbitrary + * partial, and parity forbids tightening it here. + */ +export class ReservationCreateDto extends createZodDto(reservationCreateRequestSchema) {} +export class ReservationUpdateDto extends createZodDto(reservationUpdateRequestSchema) {} +export class ReservationPositionsDto extends createZodDto(reservationPositionsRequestSchema) {} +export class ReservationTravelersDto extends createZodDto(reservationTravelersRequestSchema) {} diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index 4e372cc6d..05570dd55 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -1,12 +1,14 @@ /** * Reservations + accommodations module e2e — exercises both migrated mounts - * through the real JwtAuthGuard against a temp SQLite db. The reservation/day/ - * budget services, the permission check, canAccessTrip and the WebSocket - * broadcast are mocked. + * through the real JwtAuthGuard against a temp SQLite db. Reservation SQL runs + * for real (ReservationsService is DI-native, no mock — the temp db carries the + * full schema via createTables + runMigrations); the day/budget services, the + * permission check and the WebSocket broadcast stay mocked. */ import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; +import { ZodValidationPipe } from '../../src/nest/common/zod-validation.pipe'; import { ReservationsModule } from '../../src/nest/reservations/reservations.module'; -import { seedUser, sessionCookie } from './harness'; +import { sessionCookie } from './harness'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { Test } from '@nestjs/testing'; @@ -20,9 +22,6 @@ const { db } = vi.hoisted(() => { const Database = require('better-sqlite3'); const tmp = new Database(':memory:'); tmp.exec('PRAGMA journal_mode = WAL'); - tmp.exec(`CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, - email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT);'); return { db: tmp }; }); @@ -36,23 +35,13 @@ vi.mock('../../src/db/database', () => ({ reinitialize: () => {}, })); vi.mock('../../src/websocket', () => ({ broadcast: vi.fn() })); -vi.mock('../../src/services/notificationService', () => ({ send: vi.fn().mockResolvedValue(undefined) })); +const { notificationSend } = vi.hoisted(() => ({ notificationSend: vi.fn().mockResolvedValue(undefined) })); +vi.mock('../../src/services/notificationService', () => ({ send: notificationSend })); const { checkPermission } = vi.hoisted(() => ({ checkPermission: vi.fn() })); vi.mock('../../src/services/permissions', () => ({ checkPermission })); -const { resv, budget, day } = vi.hoisted(() => ({ - resv: { - verifyTripAccess: vi.fn(), - listReservations: vi.fn(), - createReservation: vi.fn(), - updatePositions: vi.fn(), - getReservation: vi.fn(), - updateReservation: vi.fn(), - deleteReservation: vi.fn(), - getUpcomingReservations: vi.fn(), - notifyBookingChange: vi.fn(), - }, +const { budget, day } = vi.hoisted(() => ({ budget: { createBudgetItem: vi.fn(), updateBudgetItem: vi.fn(), @@ -68,39 +57,47 @@ const { resv, budget, day } = vi.hoisted(() => ({ deleteAccommodation: vi.fn(), }, })); -vi.mock('../../src/services/reservationService', () => resv); vi.mock('../../src/services/budgetService', () => budget); vi.mock('../../src/services/dayService', () => day); -describe('Reservations + accommodations e2e (real auth guard + temp SQLite)', () => { +import { createTables } from '../../src/db/schema'; +import { runMigrations } from '../../src/db/migrations'; + +describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real reservation SQL)', () => { let server: Server; let app: Awaited>; + let tripId: number; async function build() { const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, ReservationsModule] }).compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); + nest.useGlobalPipes(new ZodValidationPipe()); await nest.init(); return nest; } beforeAll(async () => { - seedUser(db as never, { id: 1 }); + createTables(db); + runMigrations(db); + // The temp db carries the real schema (password_hash NOT NULL), so seed the + // auth user directly instead of via the trimmed-DDL seedUser helper. + db.prepare( + "INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (1, 'e2e-user', 'e2e@example.test', 'x', 'user', 0)", + ).run(); + tripId = Number(db.prepare("INSERT INTO trips (user_id, title) VALUES (1, 'E2E Trip')").run().lastInsertRowid); app = await build(); server = app.getHttpServer(); - resv.listReservations.mockReturnValue([{ id: 1, title: 'Hotel' }]); - resv.createReservation.mockReturnValue({ reservation: { id: 9, title: 'Hotel' }, accommodationCreated: false }); day.listAccommodations.mockReturnValue([{ id: 1 }]); day.validateAccommodationRefs.mockReturnValue([]); day.createAccommodation.mockReturnValue({ id: 9 }); - resv.getUpcomingReservations.mockReturnValue([{ id: 1, trip_id: 5, title: 'Flight' }]); }); beforeEach(() => { - resv.verifyTripAccess.mockReturnValue({ id: 5, user_id: 1 }); - canAccessTrip.mockReturnValue({ id: 5, user_id: 1 }); + canAccessTrip.mockImplementation((id: unknown) => db.prepare('SELECT * FROM trips WHERE id = ?').get(id)); checkPermission.mockReturnValue(true); + notificationSend.mockClear(); }); afterAll(async () => { @@ -108,13 +105,15 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite)', () }); it('401 without a cookie (reservations)', async () => { - expect((await request(server).get('/api/trips/5/reservations')).status).toBe(401); + expect((await request(server).get(`/api/trips/${tripId}/reservations`)).status).toBe(401); }); - it('200 list reservations', async () => { - const res = await request(server).get('/api/trips/5/reservations').set('Cookie', sessionCookie(1)); + it('200 list reservations (real SQL, joins attached)', async () => { + const rid = db.prepare("INSERT INTO reservations (trip_id, title, type) VALUES (?, 'Hotel', 'hotel')").run(tripId).lastInsertRowid; + const res = await request(server).get(`/api/trips/${tripId}/reservations`).set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); - expect(res.body).toEqual({ reservations: [{ id: 1, title: 'Hotel' }] }); + const row = res.body.reservations.find((r: { id: number }) => r.id === Number(rid)); + expect(row).toMatchObject({ title: 'Hotel', type: 'hotel', endpoints: [], travelers: [] }); }); it('401 without a cookie (upcoming feed)', async () => { @@ -122,37 +121,42 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite)', () }); it('200 cross-trip upcoming reservations feed', async () => { + db.prepare("INSERT INTO reservations (trip_id, title, type, reservation_time) VALUES (?, 'Flight', 'flight', '2999-01-01T10:00:00')").run(tripId); const res = await request(server).get('/api/reservations/upcoming').set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); - expect(res.body).toEqual({ reservations: [{ id: 1, trip_id: 5, title: 'Flight' }] }); + expect(res.body.reservations.map((r: { title: string }) => r.title)).toContain('Flight'); }); it('404 when trip not accessible (reservations)', async () => { - resv.verifyTripAccess.mockReturnValue(undefined); - const res = await request(server).get('/api/trips/5/reservations').set('Cookie', sessionCookie(1)); + canAccessTrip.mockReturnValue(undefined); + const res = await request(server).get(`/api/trips/${tripId}/reservations`).set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Trip not found' }); }); - it('201 create reservation, 400 without title', async () => { + it('201 create reservation (real insert + booking notification), 400 without title', async () => { const ok = await request(server) - .post('/api/trips/5/reservations') + .post(`/api/trips/${tripId}/reservations`) .set('Cookie', sessionCookie(1)) .send({ title: 'Hotel' }); expect(ok.status).toBe(201); - expect(ok.body).toEqual({ reservation: { id: 9, title: 'Hotel' } }); - expect(resv.notifyBookingChange).toHaveBeenCalledWith('5', 1, 'Hotel', ''); - const bad = await request(server).post('/api/trips/5/reservations').set('Cookie', sessionCookie(1)).send({}); + expect(ok.body.reservation).toMatchObject({ title: 'Hotel', type: 'other', status: 'pending' }); + expect(db.prepare('SELECT title FROM reservations WHERE id = ?').get(ok.body.reservation.id)).toEqual({ title: 'Hotel' }); + // The fire-and-forget booking notification reaches the (mocked) notification service. + await vi.waitFor(() => expect(notificationSend).toHaveBeenCalled()); + expect(notificationSend).toHaveBeenCalledWith(expect.objectContaining({ event: 'booking_change', actorId: 1 })); + + const bad = await request(server).post(`/api/trips/${tripId}/reservations`).set('Cookie', sessionCookie(1)).send({}); expect(bad.status).toBe(400); - expect(bad.body).toEqual({ error: 'Title is required' }); + expect(bad.body.error).toContain('title'); }); it('200 list accommodations + 201 create', async () => { - const list = await request(server).get('/api/trips/5/accommodations').set('Cookie', sessionCookie(1)); + const list = await request(server).get(`/api/trips/${tripId}/accommodations`).set('Cookie', sessionCookie(1)); expect(list.status).toBe(200); expect(list.body).toEqual({ accommodations: [{ id: 1 }] }); const create = await request(server) - .post('/api/trips/5/accommodations') + .post(`/api/trips/${tripId}/accommodations`) .set('Cookie', sessionCookie(1)) .send({ place_id: 2, start_day_id: 10, end_day_id: 11 }); expect(create.status).toBe(201); @@ -161,14 +165,14 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite)', () it('404 when trip not accessible (accommodations)', async () => { canAccessTrip.mockReturnValue(undefined); - const res = await request(server).get('/api/trips/5/accommodations').set('Cookie', sessionCookie(1)); + const res = await request(server).get(`/api/trips/${tripId}/accommodations`).set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Trip not found' }); }); it('400 accommodation create without refs', async () => { const res = await request(server) - .post('/api/trips/5/accommodations') + .post(`/api/trips/${tripId}/accommodations`) .set('Cookie', sessionCookie(1)) .send({ place_id: 2 }); expect(res.status).toBe(400); diff --git a/server/tests/unit/nest/reservations.controller.test.ts b/server/tests/unit/nest/reservations.controller.test.ts index e24ea881d..a4190bca1 100644 --- a/server/tests/unit/nest/reservations.controller.test.ts +++ b/server/tests/unit/nest/reservations.controller.test.ts @@ -45,9 +45,8 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese expect(thrown(() => new ReservationsController(svc).create(user, '5', { title: 'Hotel' }))).toEqual({ status: 403, body: { error: 'No permission' } }); }); - it('400 without a title', () => { - expect(thrown(() => new ReservationsController(makeService()).create(user, '5', {}))).toEqual({ status: 400, body: { error: 'Title is required' } }); - }); + // The bespoke 'Title is required' 400 moved to the global ZodValidationPipe + // (ReservationCreateDto) — covered by the e2e suite. it('creates, runs budget sync, broadcasts accommodation + reservation, notifies', () => { const create = vi.fn().mockReturnValue({ reservation: { id: 9 }, accommodationCreated: true }); @@ -63,9 +62,8 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese }); describe('PUT /positions', () => { - it('400 when positions is not an array', () => { - expect(thrown(() => new ReservationsController(makeService()).updatePositions(user, '5', { positions: 'no' }))).toEqual({ status: 400, body: { error: 'positions must be an array' } }); - }); + // The 'positions must be an array' 400 moved to the global + // ZodValidationPipe (ReservationPositionsDto). it('updates positions and broadcasts', () => { const updatePositions = vi.fn(); const broadcast = vi.fn(); @@ -96,13 +94,12 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese }); describe('PUT /:id/travelers', () => { - it('400 when user_ids is not an array', () => { - expect(thrown(() => new ReservationsController(makeService()).updateTravelers(user, '5', '9', 'nope'))).toEqual({ status: 400, body: { error: 'user_ids must be an array' } }); - }); + // The 'user_ids must be an array' 400 moved to the global + // ZodValidationPipe (ReservationTravelersDto). it('404 when the reservation is off-trip / missing', () => { const svc = makeService({ setTravelers: vi.fn().mockReturnValue(null) } as Partial); - expect(thrown(() => new ReservationsController(svc).updateTravelers(user, '5', '9', [1]))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); + expect(thrown(() => new ReservationsController(svc).updateTravelers(user, '5', '9', { user_ids: [1] }))).toEqual({ status: 404, body: { error: 'Reservation not found' } }); }); it('assigns travelers, broadcasts, and returns { travelers, reservation }', () => { @@ -111,7 +108,7 @@ describe('ReservationsController (parity with the legacy /api/trips/:tripId/rese const setTravelers = vi.fn().mockReturnValue({ travelers, reservation }); const broadcast = vi.fn(); const svc = makeService({ setTravelers, broadcast } as Partial); - expect(new ReservationsController(svc).updateTravelers(user, '5', '9', [2], 'sock')).toEqual({ travelers, reservation }); + expect(new ReservationsController(svc).updateTravelers(user, '5', '9', { user_ids: [2] }, 'sock')).toEqual({ travelers, reservation }); expect(setTravelers).toHaveBeenCalledWith('9', '5', [2]); expect(broadcast).toHaveBeenCalledWith('5', 'reservation:travelers-updated', { reservationId: 9, travelers }, 'sock'); }); diff --git a/shared/src/reservation/reservation.schema.spec.ts b/shared/src/reservation/reservation.schema.spec.ts index 8659ad508..3f2f3bcbb 100644 --- a/shared/src/reservation/reservation.schema.spec.ts +++ b/shared/src/reservation/reservation.schema.spec.ts @@ -20,14 +20,17 @@ describe('reservationCreateRequestSchema', () => { }); describe('reservationPositionsRequestSchema', () => { - it('requires positions with id + day_plan_position', () => { + it('requires positions with an id; day_plan_position stays optional (legacy wire tolerance)', () => { expect( reservationPositionsRequestSchema.safeParse({ positions: [{ id: 1, day_plan_position: 0 }], day_id: 3, }).success, ).toBe(true); - expect(reservationPositionsRequestSchema.safeParse({ positions: [{ id: 1 }] }).success).toBe(false); + // The legacy route accepted items without day_plan_position (binds NULL) — + // RESV-006 pins this on the server side. + expect(reservationPositionsRequestSchema.safeParse({ positions: [{ id: 1 }] }).success).toBe(true); + expect(reservationPositionsRequestSchema.safeParse({ positions: [{}] }).success).toBe(false); }); }); diff --git a/shared/src/reservation/reservation.schema.ts b/shared/src/reservation/reservation.schema.ts index 44242b290..349a73038 100644 --- a/shared/src/reservation/reservation.schema.ts +++ b/shared/src/reservation/reservation.schema.ts @@ -162,7 +162,10 @@ export const reservationTravelersRequestSchema = z.object({ export type ReservationTravelersRequest = z.infer; export const reservationPositionsRequestSchema = z.object({ - positions: z.array(z.object({ id: z.number(), day_plan_position: z.number() })), + // day_plan_position is optional on the wire: the legacy route never + // validated position items, and an absent value binds NULL (clears the + // stored position) — pinned by the server integration suite (RESV-006). + positions: z.array(z.object({ id: z.number(), day_plan_position: z.number().optional() })), day_id: z.union([z.number(), z.string()]).nullable().optional(), }); export type ReservationPositionsRequest = z.infer;