diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 8875a6ece..49ddfd068 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -17,7 +17,7 @@ "src/nest/places/places.service.ts": 2020, "src/nest/plugins/plugin-runtime.service.ts": 1463, "src/nest/plugins/runtime/plugin-sdk.ts": 1094, - "src/nest/reservations/reservations.service.ts": 1240, + "src/nest/reservations/reservations.service.ts": 1239, "src/nest/trips/trips.service.ts": 1009, "src/nest/vacay/vacay.service.ts": 1440 } diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json index 19b3d759f..387d50046 100644 --- a/server/scripts/tx-writes-baseline.json +++ b/server/scripts/tx-writes-baseline.json @@ -8,7 +8,6 @@ "src/nest/auth/passkey.service.ts#PasskeyService.passkeyLoginVerify": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterOptions": 2, "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterVerify": 2, - "src/nest/booking-import/booking-import.service.ts#BookingImportService.confirm": 4, "src/nest/doc-sync/doc-sync-config.service.ts#DocSyncConfigService.upsertConnection": 3, "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.createLink": 4, "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.deleteConnection": 2, diff --git a/server/src/nest/booking-import/booking-import.service.ts b/server/src/nest/booking-import/booking-import.service.ts index 089c671bf..9856e4f5d 100644 --- a/server/src/nest/booking-import/booking-import.service.ts +++ b/server/src/nest/booking-import/booking-import.service.ts @@ -4,6 +4,7 @@ import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; import { ReservationsService } from '../reservations/reservations.service'; import { PlacesService } from '../places/places.service'; +import { UnitOfWork } from '../database/unit-of-work'; import { BudgetService } from '../budget/budget.service'; import { imageMimeType } from '../llm-parse/image-input'; import { AddonsService } from '../addons/addons.service'; @@ -35,6 +36,7 @@ export class BookingImportService { private readonly realtime: RealtimeService, private readonly maps: MapsService, private readonly places: PlacesService, + private readonly uow: UnitOfWork, ) {} /** @@ -266,9 +268,13 @@ export class BookingImportService { /** * Persist a confirmed list of parsed items. - * Creates place rows for hotel/restaurant/event venues, then writes each booking with - * its linked cost (createWithCost). Broadcasts reservation:created, accommodation:created - * if applicable and the cost per item, after each write commits. + * Per item, the network work runs first (venue and endpoint geocoding, the cost's + * frozen rate). Then the venue's place row, the booking and its linked cost are one + * transaction, so a booking that fails leaves no stray place behind. place:created, + * reservation:created, accommodation:created and the cost go out after the commit. + * + * @txIndependent one transaction per imported item: each booking lands whole with its + * venue and cost, and one that fails is skipped without undoing the ones before it. */ async confirm( tripId: string, @@ -282,8 +288,8 @@ export class BookingImportService { try { const { _venue, _accommodation, source: _src, ...reservationData } = item; - // Auto-create a place row for venue-based reservations - let placeId: number | undefined; + // A place row for venue-based reservations, written with the booking below. + let venue: Parameters[1] | undefined; if (_venue?.name) { // Geocode before creating so the broadcast carries the coordinates let lat = _venue.lat; @@ -309,7 +315,7 @@ export class BookingImportService { } } - const place = await this.places.create(tripId, { + venue = { name: _venue.name, lat, lng, @@ -318,9 +324,7 @@ export class BookingImportService { // it lands as https or not at all (#2483). website: normalizePlaceWebsite(_venue.website) ?? undefined, phone: _venue.phone, - }); - placeId = (place as any).id; - this.realtime.broadcast(tripId, 'place:created', { place }, socketId); + }; } // The same lookup preview() runs, through the same helper. On anything @@ -346,7 +350,6 @@ export class BookingImportService { const startDayId = await this.resolveDayId(tripId, _accommodation.check_in); const endDayId = await this.resolveDayId(tripId, _accommodation.check_out); createAccommodation = { - place_id: placeId, start_day_id: startDayId ?? undefined, end_day_id: endDayId ?? undefined, check_in: _accommodation.check_in, @@ -355,20 +358,27 @@ export class BookingImportService { }; } - // The booking and its linked cost are one write, through the same service - // method REST, MCP and the plugin RPC take. The cost's rate is frozen first, - // outside that write, since freezing it can fetch rates over the network. + // The venue, the booking and its linked cost are one write; the booking goes + // through the same service core REST, MCP and the plugin RPC take. The cost's + // rate is frozen first, outside that write, since it can fetch rates over the network. const cost = await this.linkedCost(tripId, item); - const { reservation, accommodationCreated, costEvents } = await this.reservations.createWithCost(tripId, { - ...reservationData, - place_id: placeId, - create_accommodation: createAccommodation, - } as any, cost); + const { place, reservation, accommodationCreated, costEvents, stayMirror } = await this.uow.transactional(async () => { + const placeRow = venue ? await this.places.create(tripId, venue) : null; + const placeId = placeRow?.id; + const written = await this.reservations.createWithCostInTx(tripId, { + ...reservationData, + place_id: placeId, + create_accommodation: createAccommodation && { ...createAccommodation, place_id: placeId }, + } as never, cost); + return { place: placeRow, ...written }; + }); + if (place) this.realtime.broadcast(tripId, 'place:created', { place }, socketId); this.realtime.broadcast(tripId, 'reservation:created', { reservation }, socketId); if (accommodationCreated) { this.realtime.broadcast(tripId, 'accommodation:created', {}, socketId); } + await this.reservations.announceStayMirror(tripId, stayMirror); this.reservations.announceCost(tripId, costEvents, socketId); created.push(reservation); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 82042a4a5..260bd52c9 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -219,7 +219,7 @@ export class ReservationsService { * booking event on this surface is echo-suppressed because the client already * drew what it sent, and it never sent this. */ - private async announceStayMirror(tripId: string | number, mirror: AccommodationMirror): Promise { + async announceStayMirror(tripId: string | number, mirror: AccommodationMirror): Promise { await this.accommodations.announceMirror(tripId, mirror, (event, payload) => this.realtime.broadcast(tripId, event, payload)); } @@ -654,20 +654,19 @@ export class ReservationsService { return written; } - /** - * create and the linked cost the booking carries, as one write. The price's rate - * is frozen before (withFrozenRate, network I/O). REST, the plugin RPC and MCP - * all come through here and send the cost events after the commit. - */ + /** create and the booking's linked cost as one write; the rate is frozen before (withFrozenRate, network I/O). */ async createWithCost(tripId: string, data: CreateReservationData, entry: BudgetEntry) { - const costEvents: CostEvent[] = []; - const { stayMirror, ...written } = await this.uow.transactional(async () => { - const done = await this.createInTx(tripId, data); - await this.syncBudgetOnCreate(tripId, done.reservation.id, data.title, data.type, entry, undefined, costEvents); - return done; - }); + const { stayMirror, ...written } = await this.uow.transactional(() => this.createWithCostInTx(tripId, data, entry)); await this.announceStayMirror(tripId, stayMirror); - return { ...written, costEvents }; + return written; + } + + /** createWithCost's writes, for a caller whose transaction holds more (an imported venue). It announces after. */ + async createWithCostInTx(tripId: string, data: CreateReservationData, entry: BudgetEntry) { + const costEvents: CostEvent[] = []; + const done = await this.createInTx(tripId, data); + await this.syncBudgetOnCreate(tripId, done.reservation.id, data.title, data.type, entry, undefined, costEvents); + return { ...done, costEvents }; } /** update and the linked cost it moves, as one write, the way createWithCost is. */ diff --git a/server/tests/unit/nest/booking-import/booking-import.service.test.ts b/server/tests/unit/nest/booking-import/booking-import.service.test.ts index 2b3128ea0..32ae88ead 100644 --- a/server/tests/unit/nest/booking-import/booking-import.service.test.ts +++ b/server/tests/unit/nest/booking-import/booking-import.service.test.ts @@ -11,6 +11,8 @@ vi.mock('../../../../src/db/database', () => ({ })); vi.mock('../../../../src/websocket', () => ({ broadcast: vi.fn() })); const permissionsStub = { checkPermission: vi.fn(() => true) }; +// Runs the callback in place; the tests that care about the boundary wrap it to record it. +const uowStub = { transactional: (fn: () => Promise) => fn() }; import { BookingImportService } from '../../../../src/nest/booking-import/booking-import.service'; @@ -34,6 +36,7 @@ function make(opts: { kit?: boolean; ai?: boolean; extract?: any; parse?: any }) svc: new BookingImportService( extractor as any, llmParse as any, undefined as never, undefined as never, reservations as never, permissionsStub as never, undefined as never, undefined as never, undefined as never, maps as never, places as never, + uowStub as never, ), extractor, llmParse, reservations, maps, places, }; @@ -208,13 +211,16 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { // used to reach the place row exactly as the mail or the review form had it. describe('BookingImportService.confirm venue website (#2483)', () => { it('BOOKING-IMPORT-2483-01: a bare host is saved with https, a script link or nothing not at all', async () => { - const reservations = { createWithCost: vi.fn(() => ({ reservation: { id: 1 }, accommodationCreated: false, costEvents: [] })), announceCost: vi.fn() }; + const reservations = { + createWithCostInTx: vi.fn(() => ({ reservation: { id: 1 }, accommodationCreated: false, costEvents: [], stayMirror: {} })), + announceStayMirror: vi.fn(), announceCost: vi.fn(), + }; const places = { create: vi.fn((_tripId: string, _input: { website?: string }) => ({ id: 7 })) }; // No dates on a restaurant, so the day repositories are never reached. const svc = new BookingImportService( {} as never, {} as never, undefined as never, undefined as never, reservations as never, permissionsStub as never, undefined as never, { isAddonEnabled: () => false } as never, { broadcast: vi.fn() } as never, - { geocodeQuery: vi.fn() } as never, places as never, + { geocodeQuery: vi.fn() } as never, places as never, uowStub as never, ); const item = (website?: string) => ({ type: 'restaurant', @@ -235,15 +241,22 @@ describe('BookingImportService.confirm venue website (#2483)', () => { }); // The booking and the cost its extracted price becomes go in as one write, through -// ReservationsService.createWithCost, with the FX rate frozen before it. +// ReservationsService.createWithCostInTx with the venue, and the FX rate frozen before it. describe('BookingImportService.confirm writes the booking with its cost', () => { - function makeConfirm(opts: { budget?: boolean; freeze?: (tripId: string, entry: { currency?: string | null; exchange_rate?: number }) => Promise } = {}) { + function makeConfirm(opts: { budget?: boolean; failWrite?: boolean; freeze?: (tripId: string, entry: { currency?: string | null; exchange_rate?: number }) => Promise } = {}) { const order: string[] = []; const costEvents = [{ event: 'budget:created', payload: { item: { id: 9 } } }]; const reservations = { - createWithCost: vi.fn(async () => { order.push('write'); return { reservation: { id: 1 }, accommodationCreated: false, costEvents }; }), + createWithCostInTx: vi.fn(async () => { + if (opts.failWrite) throw new Error('write failed'); + order.push('write'); + return { reservation: { id: 1 }, accommodationCreated: false, costEvents, stayMirror: {} }; + }), + announceStayMirror: vi.fn(async () => { order.push('announceStayMirror'); }), announceCost: vi.fn(() => { order.push('announceCost'); }), }; + const places = { create: vi.fn(async () => { order.push('place'); return { id: 7 }; }) }; + const uow = { transactional: vi.fn(async (fn: () => Promise) => { order.push('begin'); const out = await fn(); order.push('commit'); return out; }) }; const budget = { freezeForeignRate: vi.fn(async (tripId: string, entry: { currency?: string | null; exchange_rate?: number }) => { order.push('freeze'); @@ -254,9 +267,9 @@ describe('BookingImportService.confirm writes the booking with its cost', () => const svc = new BookingImportService( {} as never, {} as never, undefined as never, undefined as never, reservations as never, permissionsStub as never, budget as never, { isAddonEnabled: async () => opts.budget ?? true } as never, realtime as never, - { geocodeQuery: vi.fn() } as never, { create: vi.fn() } as never, + { geocodeQuery: vi.fn() } as never, places as never, uow as never, ); - return { svc, reservations, budget, order, costEvents }; + return { svc, reservations, budget, places, realtime, order, costEvents }; } const priced = { type: 'train', title: 'ICE 123', metadata: { price: '49.9', priceCurrency: 'CHF' }, source: { fileName: 'ticket.pdf', index: 0 } }; @@ -266,23 +279,51 @@ describe('BookingImportService.confirm writes the booking with its cost', () => const res = await svc.confirm('5', [priced as never], 'sock'); expect(res.created).toEqual([{ id: 1 }]); - expect(reservations.createWithCost).toHaveBeenCalledWith('5', expect.objectContaining({ title: 'ICE 123', type: 'train' }), { + expect(reservations.createWithCostInTx).toHaveBeenCalledWith('5', expect.objectContaining({ title: 'ICE 123', type: 'train' }), { total_price: 49.9, category: 'transport', currency: 'CHF', exchange_rate: 0.5, }); expect(reservations.announceCost).toHaveBeenCalledWith('5', costEvents, 'sock'); - expect(order).toEqual(['freeze', 'write', 'reservation:created', 'announceCost']); + expect(order).toEqual(['freeze', 'begin', 'write', 'commit', 'reservation:created', 'announceStayMirror', 'announceCost']); }); it('BOOKING-IMPORT-TX-002: no cost without the Costs addon or without a price', async () => { const off = makeConfirm({ budget: false }); await off.svc.confirm('5', [priced as never], undefined); expect(off.budget.freezeForeignRate).not.toHaveBeenCalled(); - expect(off.reservations.createWithCost).toHaveBeenCalledWith('5', expect.anything(), undefined); + expect(off.reservations.createWithCostInTx).toHaveBeenCalledWith('5', expect.anything(), undefined); const free = makeConfirm(); await free.svc.confirm('5', [{ ...priced, metadata: { price: '0' } } as never], undefined); expect(free.budget.freezeForeignRate).not.toHaveBeenCalled(); - expect(free.reservations.createWithCost).toHaveBeenCalledWith('5', expect.anything(), undefined); + expect(free.reservations.createWithCostInTx).toHaveBeenCalledWith('5', expect.anything(), undefined); + }); + + const atVenue = { type: 'restaurant', title: 'Dîner', _venue: { name: 'Crêperie', lat: 48.03, lng: -3.49 }, source: { fileName: 'booking.eml', index: 0 } }; + + it('BOOKING-IMPORT-TX-004: writes the venue in the booking transaction and announces it after the commit', async () => { + const { svc, places, reservations, order } = makeConfirm({ budget: false }); + + await svc.confirm('5', [atVenue as never], 'sock'); + + expect(order).toEqual(['begin', 'place', 'write', 'commit', 'place:created', 'reservation:created', 'announceStayMirror', 'announceCost']); + expect(places.create).toHaveBeenCalledWith('5', expect.objectContaining({ name: 'Crêperie', lat: 48.03, lng: -3.49 })); + expect(reservations.createWithCostInTx).toHaveBeenCalledWith('5', expect.objectContaining({ place_id: 7 }), undefined); + }); + + it('BOOKING-IMPORT-TX-005: a booking that fails takes its venue with it and announces nothing', async () => { + const quiet = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const { svc, realtime, order } = makeConfirm({ budget: false, failWrite: true }); + + const res = await svc.confirm('5', [atVenue as never], 'sock'); + + expect(res.created).toEqual([]); + // The place insert ran inside the transaction the failure rolls back, never on its own. + expect(order).toEqual(['begin', 'place']); + expect(realtime.broadcast).not.toHaveBeenCalled(); + } finally { + quiet.mockRestore(); + } }); it('BOOKING-IMPORT-TX-003: a rate that cannot be frozen keeps the booking, without its cost', async () => { @@ -291,7 +332,7 @@ describe('BookingImportService.confirm writes the booking with its cost', () => const { svc, reservations } = makeConfirm({ freeze: async () => { throw new Error('rates down'); } }); const res = await svc.confirm('5', [priced as never], undefined); expect(res.created).toEqual([{ id: 1 }]); - expect(reservations.createWithCost).toHaveBeenCalledWith('5', expect.anything(), undefined); + expect(reservations.createWithCostInTx).toHaveBeenCalledWith('5', expect.anything(), undefined); } finally { quiet.mockRestore(); }