From 55cb2148d44fd90e8c52df7df2059e2f7e229822 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 27 Jul 2026 20:58:03 +0200 Subject: [PATCH] fix(server): fix the quirks preserved by the day DI migration --- server/src/nest/days/days.controller.ts | 5 +- server/src/nest/days/days.mcp.ts | 2 +- server/src/nest/days/days.service.ts | 92 ++++++++++++--------- server/tests/e2e/days.e2e.test.ts | 11 +++ server/tests/unit/mcp/tools-days.test.ts | 16 ++++ server/tests/unit/nest/days.service.test.ts | 70 +++++++++++++++- 6 files changed, 156 insertions(+), 40 deletions(-) diff --git a/server/src/nest/days/days.controller.ts b/server/src/nest/days/days.controller.ts index 13d43d67c..388e9ea7e 100644 --- a/server/src/nest/days/days.controller.ts +++ b/server/src/nest/days/days.controller.ts @@ -108,7 +108,10 @@ export class DaysController { if (!current) { throw new HttpException({ error: 'Day not found' }, 404); } - const day = this.days.update(id, current as never, { notes: body.notes, title: body.title }); + // The zod-parsed body carries only the keys the client actually sent, so + // the service's presence sentinels preserve the omitted column (the client + // updates notes and title in separate requests). + const day = this.days.update(id, current as never, body); this.days.broadcast(tripId, 'day:updated', { day }, socketId); return { day }; } diff --git a/server/src/nest/days/days.mcp.ts b/server/src/nest/days/days.mcp.ts index e94a94210..c0dce2eae 100644 --- a/server/src/nest/days/days.mcp.ts +++ b/server/src/nest/days/days.mcp.ts @@ -55,7 +55,7 @@ export class DaysMcp { if (!hasTripPermission('day_edit', tripId, ctx.userId)) return permissionDenied(); const current = this.days.getDay(dayId, tripId); if (!current) return { content: [{ type: 'text' as const, text: 'Day not found.' }], isError: true }; - const updated = this.days.update(dayId, current, title !== undefined ? { title } : {}); + const updated = this.days.update(dayId, current, { title }); safeBroadcast(tripId, 'day:updated', { day: updated }); return ok({ day: updated }); } diff --git a/server/src/nest/days/days.service.ts b/server/src/nest/days/days.service.ts index 4e7b3b128..96eff9db2 100644 --- a/server/src/nest/days/days.service.ts +++ b/server/src/nest/days/days.service.ts @@ -70,7 +70,12 @@ export interface CreateAccommodationData { * defaults, the post-write re-selects, the two-phase negative-day_number * renumber and the reservation re-stamping). The legacy hand-rolled * BEGIN/COMMIT blocks in reorder/insert became db.transaction() (same - * rollback-on-throw semantics, savepoint-safe when nested). Trip access + * rollback-on-throw semantics, savepoint-safe when nested). Verified defects + * were fixed after the port (2026-07): update() now presence-sentinels BOTH + * columns (the legacy always-write wiped notes when only a title was sent), + * createAccommodation/deleteAccommodation run their multi-statement writes in + * a transaction, and getAssignmentsForDay batch-loads tags instead of one + * query per assignment. Trip access * mirrors the requireTripAccess middleware (canAccessTrip); mutations use the * 'day_edit' permission; the WebSocket broadcast keeps its legacy call path. * Non-Nest consumers (legacy tripService and the transit/transports MCP @@ -113,12 +118,13 @@ export class DaysService { ORDER BY da.order_index ASC, da.created_at ASC `, dayId); + // One batched tag load instead of the legacy per-assignment query; the + // non-compact loader returns the same full tag rows (t.* minus the join + // key), so the output shape is unchanged. + const tagsByPlaceId = loadTagsByPlaceIds([...new Set(assignments.map(a => a.place_id))]); + return assignments.map(a => { - const tags = this.db.all(` - SELECT t.* FROM tags t - JOIN place_tags pt ON t.id = pt.tag_id - WHERE pt.place_id = ? - `, a.place_id); + const tags = tagsByPlaceId[a.place_id] || []; return { id: a.id, @@ -237,9 +243,13 @@ export class DaysService { } update(id: string | number, current: Day, fields: { notes?: string; title?: string | null }) { + // Both columns use the presence sentinel: an absent key preserves the + // current value (the legacy version always wrote notes, so setting a title + // silently wiped the day's notes — the client sends the two fields in + // separate requests). this.db.run('UPDATE days SET notes = ?, title = ? WHERE id = ?', - fields.notes || null, - 'title' in fields ? (fields.title ?? null) : current.title, + 'notes' in fields ? (fields.notes || null) : (current.notes ?? null), + 'title' in fields ? (fields.title ?? null) : (current.title ?? null), id ); const updatedDay = this.db.get('SELECT * FROM days WHERE id = ?', id)!; @@ -533,28 +543,34 @@ export class DaysService { createAccommodation(tripId: string | number, data: CreateAccommodationData) { const { place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes } = data; - const result = this.db.run( - 'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', - tripId, place_id, start_day_id, end_day_id, check_in || null, check_in_end || null, check_out || null, confirmation || null, notes || null - ); + // The stay and its partner hotel reservation are one logical write — + // atomic, so a failed reservation insert can't leave an orphan stay. + const accommodationId = this.db.transaction(() => { + const result = this.db.run( + 'INSERT INTO day_accommodations (trip_id, place_id, start_day_id, end_day_id, check_in, check_in_end, check_out, confirmation, notes) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', + tripId, place_id, start_day_id, end_day_id, check_in || null, check_in_end || null, check_out || null, confirmation || null, notes || null + ); - const accommodationId = result.lastInsertRowid; + const newId = result.lastInsertRowid; - // Auto-create linked reservation for this accommodation - const placeName = this.db.get<{ name: string }>('SELECT name FROM places WHERE id = ?', place_id)?.name || 'Hotel'; - const startDayDate = this.db.get<{ date: string }>('SELECT date FROM days WHERE id = ?', start_day_id)?.date || null; - const meta: Record = {}; - if (check_in) meta.check_in_time = check_in; - if (check_in_end) meta.check_in_end_time = check_in_end; - if (check_out) meta.check_out_time = check_out; - this.db.run(` + // Auto-create linked reservation for this accommodation + const placeName = this.db.get<{ name: string }>('SELECT name FROM places WHERE id = ?', place_id)?.name || 'Hotel'; + const startDayDate = this.db.get<{ date: string }>('SELECT date FROM days WHERE id = ?', start_day_id)?.date || null; + const meta: Record = {}; + if (check_in) meta.check_in_time = check_in; + if (check_in_end) meta.check_in_end_time = check_in_end; + if (check_out) meta.check_out_time = check_out; + this.db.run(` INSERT INTO reservations (trip_id, day_id, title, reservation_time, location, confirmation_number, notes, status, type, accommodation_id, metadata) VALUES (?, ?, ?, ?, ?, ?, ?, 'confirmed', 'hotel', ?, ?) `, - tripId, start_day_id, placeName, startDayDate || null, null, - confirmation || null, notes || null, accommodationId, - Object.keys(meta).length > 0 ? JSON.stringify(meta) : null - ); + tripId, start_day_id, placeName, startDayDate || null, null, + confirmation || null, notes || null, newId, + Object.keys(meta).length > 0 ? JSON.stringify(meta) : null + ); + + return newId; + }); return this.getAccommodationWithPlace(accommodationId); } @@ -595,20 +611,22 @@ export class DaysService { return this.getAccommodationWithPlace(Number(id)); } - /** Delete accommodation and its linked reservation (and any linked budget item). */ + /** Delete accommodation and its linked reservation (and any linked budget item), atomically. */ deleteAccommodation(id: string | number): { linkedReservationId: number | null; deletedBudgetItemId: number | null } { - const linkedRes = this.db.get<{ id: number }>('SELECT id FROM reservations WHERE accommodation_id = ?', Number(id)); - let deletedBudgetItemId: number | null = null; - if (linkedRes) { - const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE reservation_id = ?', linkedRes.id); - if (linkedBudget) { - this.db.run('DELETE FROM budget_items WHERE id = ?', linkedBudget.id); - deletedBudgetItemId = linkedBudget.id; + return this.db.transaction(() => { + const linkedRes = this.db.get<{ id: number }>('SELECT id FROM reservations WHERE accommodation_id = ?', Number(id)); + let deletedBudgetItemId: number | null = null; + if (linkedRes) { + const linkedBudget = this.db.get<{ id: number }>('SELECT id FROM budget_items WHERE reservation_id = ?', linkedRes.id); + if (linkedBudget) { + this.db.run('DELETE FROM budget_items WHERE id = ?', linkedBudget.id); + deletedBudgetItemId = linkedBudget.id; + } + this.db.run('DELETE FROM reservations WHERE id = ?', linkedRes.id); } - this.db.run('DELETE FROM reservations WHERE id = ?', linkedRes.id); - } - this.db.run('DELETE FROM day_accommodations WHERE id = ?', id); - return { linkedReservationId: linkedRes ? linkedRes.id : null, deletedBudgetItemId }; + this.db.run('DELETE FROM day_accommodations WHERE id = ?', id); + return { linkedReservationId: linkedRes ? linkedRes.id : null, deletedBudgetItemId }; + }); } } diff --git a/server/tests/e2e/days.e2e.test.ts b/server/tests/e2e/days.e2e.test.ts index de31ef3cb..e8ebe60a9 100644 --- a/server/tests/e2e/days.e2e.test.ts +++ b/server/tests/e2e/days.e2e.test.ts @@ -139,6 +139,17 @@ describe('Days + day-notes e2e (real auth guard + temp SQLite, real day SQL)', ( .send({ notes: 'Walking day', title: 'Arrival' }); expect(res.status).toBe(200); expect(res.body.day).toMatchObject({ id: 3, notes: 'Walking day', title: 'Arrival', assignments: [] }); + // The client updates title and notes in separate requests — an omitted + // field must survive (post-port defect fix: the legacy update always wrote + // both columns, so a title-only PUT wiped the notes). + const titleOnly = await request(server).put('/api/trips/5/days/3').set('Cookie', sessionCookie(1)) + .send({ title: 'Renamed' }); + expect(titleOnly.status).toBe(200); + expect(titleOnly.body.day).toMatchObject({ id: 3, notes: 'Walking day', title: 'Renamed' }); + const notesOnly = await request(server).put('/api/trips/5/days/3').set('Cookie', sessionCookie(1)) + .send({ notes: 'Museum day' }); + expect(notesOnly.status).toBe(200); + expect(notesOnly.body.day).toMatchObject({ id: 3, notes: 'Museum day', title: 'Renamed' }); const miss = await request(server).put('/api/trips/5/days/99').set('Cookie', sessionCookie(1)).send({ notes: 'x' }); expect(miss.status).toBe(404); expect(miss.body).toEqual({ error: 'Day not found' }); diff --git a/server/tests/unit/mcp/tools-days.test.ts b/server/tests/unit/mcp/tools-days.test.ts index b4a83d48e..ad3ac0444 100644 --- a/server/tests/unit/mcp/tools-days.test.ts +++ b/server/tests/unit/mcp/tools-days.test.ts @@ -95,6 +95,22 @@ describe('Tool: update_day', () => { }); }); + it('setting a title preserves the day notes (post-port defect fix)', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + testDb.prepare('UPDATE days SET notes = ? WHERE id = ?').run('Walking day', day.id); + + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'update_day', + arguments: { tripId: trip.id, dayId: day.id, title: 'Arrival' }, + }); + const data = parseToolResult(result) as { day: { title: string; notes: string } }; + expect(data.day).toMatchObject({ title: 'Arrival', notes: 'Walking day' }); + }); + }); + it('broadcasts day:updated event', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index 298d9c908..847c7b669 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -1,7 +1,9 @@ /** * Unit tests for the DI-native DaysService — DAY-SVC-001 through DAY-SVC-026 * moved 1:1 from the legacy tests/unit/services/dayService.test.ts; - * DAY-SVC-027 through DAY-SVC-032 pin the days.bridge delegation. + * DAY-SVC-027 through DAY-SVC-032 pin the days.bridge delegation; + * DAY-SVC-033 through DAY-SVC-036 pin the post-port defect fixes (update + * presence sentinels, accommodation write atomicity, batched tag load). * Uses a real in-memory SQLite DB so SQL logic is exercised faithfully. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; @@ -479,3 +481,69 @@ describe('days.bridge', () => { }); }); +// ── post-port defect fixes ──────────────────────────────────────────────────── + +describe('quirk fixes', () => { + it('DAY-SVC-033 — update preserves the omitted column (title-only keeps notes, notes-only keeps title)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + let day = createDay(testDb, trip.id); + day = svc.update(day.id, day, { notes: 'Walking day' }) as never; + day = svc.update(day.id, day, { title: 'Arrival' }) as never; + expect(day).toMatchObject({ title: 'Arrival', notes: 'Walking day' }); + day = svc.update(day.id, day, { notes: 'Museum day' }) as never; + expect(day).toMatchObject({ title: 'Arrival', notes: 'Museum day' }); + // A present key still clears via the legacy falsy coercion. + day = svc.update(day.id, day, { notes: '' }) as never; + expect(day.notes).toBeNull(); + expect(day.title).toBe('Arrival'); + }); + + it('DAY-SVC-034 — createAccommodation is atomic: a failed reservation insert leaves no orphan stay', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + testDb.exec("CREATE TRIGGER boom BEFORE INSERT ON reservations BEGIN SELECT RAISE(ABORT, 'boom'); END"); + try { + expect(() => svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, + })).toThrow(); + expect(testDb.prepare('SELECT COUNT(*) as n FROM day_accommodations WHERE trip_id = ?').get(trip.id)).toMatchObject({ n: 0 }); + } finally { + testDb.exec('DROP TRIGGER boom'); + } + }); + + it('DAY-SVC-035 — deleteAccommodation is atomic: a failed stay delete keeps the linked reservation', () => { + 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 accom = svc.createAccommodation(trip.id, { + place_id: place.id, start_day_id: day.id, end_day_id: day.id, + }) as { id: number }; + testDb.exec("CREATE TRIGGER boom BEFORE DELETE ON day_accommodations BEGIN SELECT RAISE(ABORT, 'boom'); END"); + try { + expect(() => svc.deleteAccommodation(accom.id)).toThrow(); + // The earlier reservation delete inside the transaction rolled back. + expect(testDb.prepare('SELECT COUNT(*) as n FROM reservations WHERE accommodation_id = ?').get(accom.id)).toMatchObject({ n: 1 }); + } finally { + testDb.exec('DROP TRIGGER boom'); + } + }); + + it('DAY-SVC-036 — getAssignmentsForDay returns full tag rows from the batched load', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Tagged' }); + createDayAssignment(testDb, day.id, place.id); + const tagId = Number(testDb.prepare('INSERT INTO tags (user_id, name, color) VALUES (?, ?, ?)').run(user.id, 'Food', '#ff0000').lastInsertRowid); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tagId); + + const assignments = svc.getAssignmentsForDay(day.id); + expect(assignments[0].place.tags).toHaveLength(1); + expect(assignments[0].place.tags[0]).toMatchObject({ id: tagId, name: 'Food', color: '#ff0000' }); + }); +});