From eaa027750ade68b0d0f7b49afdf7830dd871d73c Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 12:55:20 +0200 Subject: [PATCH] =?UTF-8?q?test(server):=20plan=203c=20close-out=20?= =?UTF-8?q?=E2=80=94=20copy-source=20full-row=20parity,=20Kysely=20rollbac?= =?UTF-8?q?k=20proofs,=20identity-map=20relabels,=20trips=20e2e=20on=20the?= =?UTF-8?q?=20snapshot=20DB?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- server/tests/e2e/trips.e2e.test.ts | 171 ++++++------------ .../AssignmentParticipants.repository.test.ts | 19 +- .../DayAssignments.repository.test.ts | 98 +++++++++- .../repositories/DayNotes.repository.test.ts | 42 +++-- .../GooglePlacePhotoMeta.repository.test.ts | 22 +-- .../PlaceDetailsCache.repository.test.ts | 14 +- .../PlaceRatings.repository.test.ts | 14 +- .../PlaceShadowPicks.repository.test.ts | 17 +- .../db/repositories/Places.repository.test.ts | 106 ++++++++++- .../db/repositories/Tags.repository.test.ts | 85 ++++++++- .../TripMembers.repository.test.ts | 12 +- .../db/repositories/Trips.repository.test.ts | 11 +- .../db/repositories/Users.repository.test.ts | 35 +++- server/tests/unit/nest/places.service.test.ts | 46 +++-- 14 files changed, 482 insertions(+), 210 deletions(-) diff --git a/server/tests/e2e/trips.e2e.test.ts b/server/tests/e2e/trips.e2e.test.ts index 8f57023b7..8fe8bc84e 100644 --- a/server/tests/e2e/trips.e2e.test.ts +++ b/server/tests/e2e/trips.e2e.test.ts @@ -1,8 +1,24 @@ /** * Trips module e2e — exercises the migrated /api/trips aggregate-root endpoints - * through the real JwtAuthGuard against a temp SQLite db. TripsService runs its - * real (DI-native) SQL — trips/trip_members/days DDL below; auditLog, demo, the - * permission check, canAccessTrip and the WebSocket broadcast are mocked. + * through the real JwtAuthGuard against a real migrated-and-seeded temp SQLite + * db (createSnapshotTestDb(), Task 9 fix wave M6 — this used to hand-roll + * about two dozen CREATE TABLEs, a second hand-maintained schema copy that + * diverged from the real migrated one in ways no `SELECT *`/`toMatchObject` + * read could see: the hand-rolled `journey_entries` table had no + * `author_id`/`entry_date` NOT NULL columns and no FK to a real `journeys` + * row, so the delete-trip test's inserts would have failed loudly against + * the real schema — see that test's setup below for the fix). TripsService + * and every domain `bundle()` touches (days, places, packing, files, + * reservations, todos) now run their real SQL over the same migrated + * connection; trip access resolves through `TripsRepository.findAccessible` + * via the real request-scoped `EntityManager` `createTestMikroOrmModule` + * wires in, not a hand-rolled `canAccessTrip`/`getPlaceWithTags` mock (the + * legacy overrides this file used to export from `db/database.ts` — deleted + * there since Plan 3c Task 0b; a stale mock here would silently do nothing, + * not fail loudly, which is worse than removing it — the same reasoning + * `days.e2e.test.ts`'s identical comment gives). Only the permission check, + * `BudgetService` (the budget fold's own container spy, unrelated to this + * conversion) and the WebSocket broadcast stay mocked. */ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi, type MockInstance } from 'vitest'; import request from 'supertest'; @@ -11,120 +27,20 @@ import type { Server } from 'http'; import { DatabaseModule } from '../../src/nest/database/database.module'; import { RealtimeModule } from '../../src/nest/realtime/realtime.module'; import { Test } from '@nestjs/testing'; -import { seedUser, sessionCookie } from './harness'; +import { sessionCookie } from './harness'; import { MAX_TRIP_DAYS } from '@trek/shared'; -const { db } = vi.hoisted(() => { - // eslint-disable-next-line @typescript-eslint/no-require-imports - 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, - avatar TEXT, display_name TEXT, is_guest INTEGER NOT NULL DEFAULT 0);`); - // TripsService runs its real SQL (DI-native since the trip fold) — full trip - // column set for TRIP_SELECT + create/update/delete, plus the membership and - // day-content tables generateDays/listMembers/deleteTrip touch. - tmp.exec(`CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, title TEXT NOT NULL, - description TEXT, start_date TEXT, end_date TEXT, currency TEXT DEFAULT 'EUR', is_archived INTEGER DEFAULT 0, - cover_image TEXT, reminder_days INTEGER DEFAULT 3, feed_token TEXT, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP, updated_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE trip_members (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - user_id INTEGER NOT NULL, invited_by INTEGER, added_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE day_assignments (id INTEGER PRIMARY KEY AUTOINCREMENT, day_id INTEGER NOT NULL, - place_id INTEGER NOT NULL, order_index INTEGER DEFAULT 0, notes TEXT, reservation_status TEXT, - reservation_notes TEXT, reservation_datetime TEXT, assignment_time TEXT, assignment_end_time TEXT, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE day_notes (id INTEGER PRIMARY KEY AUTOINCREMENT, day_id INTEGER NOT NULL, - trip_id INTEGER NOT NULL, text TEXT, time TEXT, icon TEXT, sort_order INTEGER DEFAULT 0, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - // deleteTrip cleans up synced journey entries before dropping the trip row. - tmp.exec(`CREATE TABLE journey_entries (id INTEGER PRIMARY KEY AUTOINCREMENT, journey_id INTEGER, - source_trip_id INTEGER, source_place_id INTEGER, source_assignment_id INTEGER, type TEXT NOT NULL);`); - // bundle()'s todoItems now runs TodoService's real SQL (DI-injected, no mock). - tmp.exec(`CREATE TABLE todo_items (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - name TEXT NOT NULL, checked INTEGER NOT NULL DEFAULT 0, category TEXT, sort_order INTEGER NOT NULL DEFAULT 0, - due_date TEXT, description TEXT, assigned_user_id INTEGER, priority INTEGER NOT NULL DEFAULT 0, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - // bundle()'s packingItems now runs PackingService's real SQL (DI-injected, no - // mock) — viewer-scoped (#858), so the recipients table must exist too. - tmp.exec(`CREATE TABLE packing_items (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - name TEXT NOT NULL, checked INTEGER DEFAULT 0, category TEXT, sort_order INTEGER DEFAULT 0, - weight_grams INTEGER, bag_id INTEGER, quantity INTEGER NOT NULL DEFAULT 1, - is_private INTEGER NOT NULL DEFAULT 0, owner_id INTEGER, updated_at DATETIME, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE packing_item_recipients (item_id INTEGER NOT NULL, user_id INTEGER NOT NULL, - PRIMARY KEY (item_id, user_id));`); - // bundle()'s files now runs FilesService's real SQL (DI-injected, no mock) — - // empty tables satisfy the FILE_SELECT joins and the file_links batch. - tmp.exec(`CREATE TABLE trip_files (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - place_id INTEGER, reservation_id INTEGER, message_id INTEGER, filename TEXT NOT NULL, - original_name TEXT NOT NULL, file_size INTEGER, mime_type TEXT, description TEXT, - uploaded_by INTEGER, starred INTEGER DEFAULT 0, - deleted_at DATETIME, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE file_links (id INTEGER PRIMARY KEY AUTOINCREMENT, file_id INTEGER NOT NULL, - reservation_id INTEGER, assignment_id INTEGER, place_id INTEGER, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - // bundle()'s reservations now runs ReservationsService's real SQL - // (DI-injected, no mock) — the joined list query needs the full reservation - // table set (trimmed from src/db/schema.ts; accommodation_id is TEXT there). - tmp.exec(`CREATE TABLE reservations (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER, title TEXT, - day_id INTEGER, end_day_id INTEGER, place_id INTEGER, assignment_id INTEGER, type TEXT, - status TEXT DEFAULT 'pending', reservation_time TEXT, reservation_end_time TEXT, location TEXT, - confirmation_number TEXT, notes TEXT, url TEXT, accommodation_id TEXT, metadata TEXT, - needs_review INTEGER DEFAULT 0, day_plan_position REAL, external_source TEXT, sync_enabled INTEGER, - ingest_state TEXT, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec('CREATE TABLE days (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, day_number INTEGER, date TEXT);'); - // Task 2 review M1: DaysRepository.listByTrip (a plain `em.find(Days)`) - // needs this table to exist — Days.entity.ts declares an inverse 1:1 to - // it (`roadtrip_day_tracks: () => p.oneToOne(RoadtripDayTracks).ref() - // .mappedBy('day')`), so MikroORM's find() resolves the relation's - // presence against the real table even though the property is hidden. - // Columns match the real migration - // (`Migration20200101032900_which_imported_track_a_day_s_drive.ts`). - tmp.exec(`CREATE TABLE roadtrip_day_tracks (day_id INTEGER PRIMARY KEY, place_id INTEGER NOT NULL, - stray_km REAL, created_at TEXT DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE places (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, name TEXT, - image_url TEXT, address TEXT, lat REAL, lng REAL, category_id INTEGER, description TEXT, - created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - // PlacesService.list joins categories and batch-loads tags/ratings. - tmp.exec('CREATE TABLE categories (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, color TEXT, icon TEXT);'); - // Plan 3c Task 1: TagsRepository.listForPlaces (QH1) selects `user_id` - // explicitly — see places.e2e.test.ts's identical comment. - tmp.exec('CREATE TABLE tags (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, name TEXT, color TEXT, created_at DATETIME);'); - tmp.exec('CREATE TABLE place_tags (place_id INTEGER NOT NULL, tag_id INTEGER NOT NULL, PRIMARY KEY (place_id, tag_id));'); - tmp.exec('CREATE TABLE place_ratings (place_id INTEGER NOT NULL, user_id INTEGER NOT NULL, rating INTEGER, created_at DATETIME, UNIQUE(place_id, user_id));'); - tmp.exec(`CREATE TABLE day_accommodations (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, - place_id INTEGER, start_day_id INTEGER, end_day_id INTEGER, check_in TEXT, check_in_end TEXT, - check_out TEXT, confirmation TEXT, notes TEXT, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); - tmp.exec(`CREATE TABLE reservation_day_positions (reservation_id INTEGER NOT NULL, day_id INTEGER NOT NULL, - position REAL, PRIMARY KEY (reservation_id, day_id));`); - tmp.exec(`CREATE TABLE reservation_endpoints (id INTEGER PRIMARY KEY AUTOINCREMENT, reservation_id INTEGER NOT NULL, - role TEXT, sequence INTEGER, name TEXT, code TEXT, lat REAL NOT NULL, lng REAL NOT NULL, - timezone TEXT, local_time TEXT, local_date TEXT);`); - tmp.exec(`CREATE TABLE reservation_travelers (reservation_id INTEGER NOT NULL, user_id INTEGER NOT NULL, - PRIMARY KEY (reservation_id, user_id));`); - // AuditService now runs its real INSERT (DI-injected, no mock) — slim - // audit_log mirror (no FKs), same shape as plugin-runtime.test.ts. - tmp.exec(`CREATE TABLE audit_log ( - id INTEGER PRIMARY KEY AUTOINCREMENT, created_at DATETIME DEFAULT CURRENT_TIMESTAMP, - user_id INTEGER, action TEXT NOT NULL, resource TEXT, details TEXT, ip TEXT);`); - // StorageRegistryService (behind StorageModule, now in this module chain) reads - // this at onModuleInit. - tmp.exec('CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);'); - return { db: tmp }; +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); }); - -const { canAccessTrip } = vi.hoisted(() => ({ canAccessTrip: vi.fn() })); -vi.mock('../../src/db/database', () => ({ - db, canAccessTrip, getPlaceWithTags: vi.fn(), closeDb: () => {}, reinitialize: () => {}, -})); vi.mock('../../src/websocket', () => ({ broadcast: vi.fn() })); // The audit domain is DI-native now: writeAudit runs for real against the temp // db's audit_log table; only the file logger is silenced. vi.mock('../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logDebug: vi.fn(), logError: vi.fn(), logWarn: vi.fn() })); vi.mock('../../src/nest/common/demo', () => ({ isDemoEmail: vi.fn(() => false) })); +import { db } from '../../src/db/database'; import { PermissionsService } from '../../src/nest/permissions/permissions.service'; // Since the permissions DI migration, the check is a spy on the container's @@ -132,13 +48,12 @@ import { PermissionsService } from '../../src/nest/permissions/permissions.servi let checkPermission: MockInstance; // TripsService itself is real since the trip fold — no tripService mock; the -// trips/trip_members/days DDL above serves its SQL. +// real migrated trips/trip_members/days tables serve its SQL. // bundle()'s days + accommodations now run DaysService's real SQL (DI-injected, -// no mock) — the days/places/day_accommodations/reservations DDL above serves them. -// bundle()'s places now run PlacesService's real SQL (DI-injected since the -// place fold, no mock) — the places/categories/tags DDL above serves them. -// bundle()'s budget items come from the DI-injected BudgetService since the -// budget fold — stubbed via a container spy in beforeAll (no budget DDL here). +// no mock). bundle()'s places now run PlacesService's real SQL (DI-injected +// since the place fold, no mock). bundle()'s budget items come from the +// DI-injected BudgetService since the budget fold — stubbed via a container +// spy in beforeAll (unrelated to this file's schema conversion). import { BudgetService } from '../../src/nest/budget/budget.service'; import { TripsModule } from '../../src/nest/trips/trips.module'; @@ -160,7 +75,12 @@ describe('Trips e2e (real auth guard + temp SQLite)', () => { } beforeAll(async () => { - seedUser(db as never, { id: 1 }); + // harness.ts's seedUser() omits password_hash, which the real migrated + // schema requires NOT NULL (days.e2e.test.ts/addons.e2e.test.ts hit the + // same thing) — a raw insert here instead. + db.prepare( + "INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (1, 'e2e-user', 'e2e@example.test', 'x', 'user', 0)", + ).run(); app = await build(); checkPermission = vi.spyOn(app.get(PermissionsService), 'checkPermission'); vi.spyOn(app.get(BudgetService), 'listBudgetItems').mockResolvedValue([]); @@ -297,8 +217,25 @@ describe('Trips e2e (real auth guard + temp SQLite)', () => { it('200 delete cleans up synced journey entries (real SQL)', async () => { const tripId = seedTrip('D'); - db.prepare("INSERT INTO journey_entries (journey_id, source_trip_id, type) VALUES (1, ?, 'skeleton')").run(tripId); - const filledId = Number(db.prepare("INSERT INTO journey_entries (journey_id, source_trip_id, type) VALUES (1, ?, 'story')").run(tripId).lastInsertRowid); + // Task 9 fix wave (M6) finding: the hand-rolled DDL this test used to run + // against had no NOT NULL/FK constraints on `journey_entries` at all + // (no `author_id`, no `entry_date`, no FK to a real `journeys` row) — a + // bare `INSERT INTO journey_entries (journey_id, source_trip_id, type) + // VALUES (1, ?, 'skeleton')` passed silently there. The real migrated + // schema requires `journey_id` to reference a real `journeys` row (NOT + // NULL FK) plus `author_id`/`entry_date`/`created_at`/`updated_at` NOT + // NULL — a real journey row (and those columns) are seeded here so the + // insert that used to pass for the wrong reason now passes for the real + // one. The assertions below are untouched, character-for-character. + const journeyId = Number(db.prepare( + "INSERT INTO journeys (user_id, title, created_at, updated_at) VALUES (1, 'J', 0, 0)", + ).run().lastInsertRowid); + db.prepare( + "INSERT INTO journey_entries (journey_id, source_trip_id, author_id, type, entry_date, created_at, updated_at) VALUES (?, ?, 1, 'skeleton', '2026-01-01', 0, 0)", + ).run(journeyId, tripId); + const filledId = Number(db.prepare( + "INSERT INTO journey_entries (journey_id, source_trip_id, author_id, type, entry_date, created_at, updated_at) VALUES (?, ?, 1, 'story', '2026-01-01', 0, 0)", + ).run(journeyId, tripId).lastInsertRowid); const res = await request(server).delete(`/api/trips/${tripId}`).set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); expect(res.body).toEqual({ success: true }); diff --git a/server/tests/unit/db/repositories/AssignmentParticipants.repository.test.ts b/server/tests/unit/db/repositories/AssignmentParticipants.repository.test.ts index b1fc93f21..e534d01eb 100644 --- a/server/tests/unit/db/repositories/AssignmentParticipants.repository.test.ts +++ b/server/tests/unit/db/repositories/AssignmentParticipants.repository.test.ts @@ -91,11 +91,12 @@ describe('AssignmentParticipantsRepository.listForAssignments', () => { expect(await participants.listForAssignments([assignment.id])).toEqual([]); }); - // D-shape (Task 1 brief item): `listForAssignments` is a `qb().execute('all', - // false)` projection, which never hydrates an entity into the identity map - // by construction — proven anyway, the honest "not required, proven - // regardless" shape. - it('ASSIGNPARTREPO-005 (D-shape): a participant added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled. `listForAssignments` is a + // `qb().execute('all', false)` projection, and `TrekRepository` applies + // `disableIdentityMap: true` to every read by construction anyway, so + // there is no live identity-map entry here to bypass — this proves a DB + // round-trip, not an identity-map bypass. + it('ASSIGNPARTREPO-005 (fresh after a raw UPDATE, not D-shape): a participant added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user: owner } = createUser(testDb); const { user: participant } = createUser(testDb, { username: 'fresh-participant' }); const trip = createTrip(testDb, owner.id); @@ -176,9 +177,11 @@ describe('AssignmentParticipantsRepository.listWithDisplayName (AS2/AS15/AS31)', expect(await participants.listWithDisplayName(assignment.id)).toEqual([]); }); - // D-shape: a `qb().execute('all', false)` projection, which never hydrates - // an entity into the identity map by construction — proven anyway. - it('ASSIGNPARTREPO-009 (D-shape): a participant added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled — same reasoning as ASSIGNPARTREPO-005 + // above: a `qb().execute('all', false)` projection never hydrates an + // entity into the identity map, and the base default leaves it disabled + // anyway, so this proves a DB round-trip, not an identity-map bypass. + it('ASSIGNPARTREPO-009 (fresh after a raw UPDATE, not D-shape): a participant added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user: owner } = createUser(testDb); const { user: participant } = createUser(testDb, { username: 'fresh-participant-2' }); const trip = createTrip(testDb, owner.id); diff --git a/server/tests/unit/db/repositories/DayAssignments.repository.test.ts b/server/tests/unit/db/repositories/DayAssignments.repository.test.ts index c465d78dc..16384ae24 100644 --- a/server/tests/unit/db/repositories/DayAssignments.repository.test.ts +++ b/server/tests/unit/db/repositories/DayAssignments.repository.test.ts @@ -171,7 +171,7 @@ describe('DayAssignmentsRepository — the DY1/DY3/AS1/AS3 projection', () => { // so, `listForDay` is `qb().execute('all', false)`, never `find`/ // `findOne`, so it cannot return that cached entity regardless; this test // proves the projection's OWN freshness, not the identity-map guard. - it('ASSIGNPLACEREPO-008 (D-shape): a place rename after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + it('ASSIGNPLACEREPO-008 (fresh after a raw UPDATE, not D-shape): a place rename after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); @@ -430,6 +430,34 @@ describe('DayAssignmentsRepository — AS10/AS11/AS13/AS14/AS19 (delete / order const row = testDb.prepare('SELECT day_id, order_index FROM day_assignments WHERE id = ?').get(a.id); expect(row).toEqual({ day_id: target.id, order_index: 4 }); }); + + // Task 9 fix wave (B-M5, the OAUTHTOKREPO-012/REANCHORDAY-003 rollback + // shape): `moveToDay` writes inside a `uow.transactional` body that then + // throws — the write must roll back with it. MUTATION-PROVED (see the + // report): removing the `throw` makes the post-rollback assertion below + // fail (the row WOULD have moved). + it('ASSIGNREPO-031: inside a uow.transactional that then ROLLS BACK, moveToDay is left un-moved', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const target = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id); + const a = createDayAssignment(testDb, day.id, place.id, { order_index: 0 }); + + let caught: unknown; + try { + await withRequestContext(t.orm, async () => { + await uow.transactional(async () => { + await assignments.moveToDay(a.id, target.id, 4); + throw new Error('force rollback'); + }); + }); + } catch (e) { caught = e; } + expect((caught as Error).message).toBe('force rollback'); + + const row = testDb.prepare('SELECT day_id, order_index FROM day_assignments WHERE id = ?').get(a.id); + expect(row).toEqual({ day_id: day.id, order_index: 0 }); + }); }); describe('DayAssignmentsRepository.effectiveStart (AS16, Kysely)', () => { @@ -541,6 +569,31 @@ describe('DayAssignmentsRepository — AS17/AS24/AS25/AS26/AS27 (single-column w .toEqual({ assignment_time: null, assignment_end_time: null }); }); + // Task 9 fix wave (B-M5, the OAUTHTOKREPO-012/REANCHORDAY-003 rollback + // shape): `setTimes` writes inside a `uow.transactional` body that then + // throws — the write must roll back with it. + it('ASSIGNREPO-032: inside a uow.transactional that then ROLLS BACK, setTimes is left unwritten', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id); + const a = createDayAssignment(testDb, day.id, place.id); + + let caught: unknown; + try { + await withRequestContext(t.orm, async () => { + await uow.transactional(async () => { + await assignments.setTimes(a.id, '09:00', '10:30'); + throw new Error('force rollback'); + }); + }); + } catch (e) { caught = e; } + expect((caught as Error).message).toBe('force rollback'); + + expect(testDb.prepare('SELECT assignment_time, assignment_end_time FROM day_assignments WHERE id = ?').get(a.id)) + .toEqual({ assignment_time: null, assignment_end_time: null }); + }); + it('ASSIGNREPO-020 (AS24, setEndDay): writes the 0/1 flag', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -585,7 +638,7 @@ describe('DayAssignmentsRepository — AS17/AS24/AS25/AS26/AS27 (single-column w // `{ disableIdentityMap: false }` so it genuinely caches the managed // assignment entity first (a bare `find({})` would be a no-op read under // the base's own default, the same vacuousness `DAYREPO-018` had). -it('ASSIGNREPO-023 (D-shape): a notes write after an unrelated identity-map read is visible in findInTrip', async () => { +it('ASSIGNREPO-023 (fresh after a raw UPDATE, not D-shape): a notes write after an unrelated identity-map read is visible in findInTrip', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); @@ -645,19 +698,52 @@ describe('DayAssignmentsRepository.listItineraryForGpx (PL30)', () => { // ── Plan 3c Task 8 (`TripsService.copy`, TP48/TP49/TP57) — additive ───────── describe('DayAssignmentsRepository.listAllForTrip (TP48)', () => { - it('ASSIGNREPO-027: every assignment of the trip, no ORDER BY guarantee, scoped by the day→trip join', async () => { + // Task 9 fix wave (B-M4, rule 19): widened from an ids-only assertion to + // full-row parity on a fully seeded fixture — every nullable column + // non-null in one row, null in the other, plus a unicode string and a + // '007'-style digit string — `toEqual()`, so a renamed/dropped column or a changed row + // order would fail this, not just the id list. + it('ASSIGNREPO-027: every assignment of the trip, no ORDER BY guarantee, scoped by the day→trip join — toEqual(legacy) on a fully seeded fixture', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const other = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); - const place = createPlace(testDb, trip.id); - const a1 = createDayAssignment(testDb, day.id, place.id); + const place = createPlace(testDb, trip.id, { name: 'Stop A' }); + const bare = createPlace(testDb, trip.id, { name: 'Stop B' }); + const accommodation = createDayAccommodation(testDb, trip.id, place.id, day.id, day.id); + + const insert = testDb.prepare(` + INSERT INTO day_assignments ( + day_id, place_id, order_index, notes, reservation_status, reservation_notes, + reservation_datetime, created_at, assignment_time, assignment_end_time, + leg_transport_mode, incoming_leg_transport_mode, end_day, accommodation_id + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `); + + // Row A: every nullable column non-null, a unicode string and a + // '007'-style digit string among the values. + const a1 = insert.run( + day.id, place.id, 5, 'Notes — 日本 ☕️ 007', 'confirmed', 'resv notes 007', + '2026-01-01T10:00:00Z', '2026-01-01 09:00:00', '10:00', '11:00', + 'walking', 'driving', 1, accommodation.id, + ).lastInsertRowid as number; + + // Row B: every nullable column NULL (end_day is NOT NULL, given 0). + const a2 = insert.run( + day.id, bare.id, null, null, null, null, + null, null, null, null, + null, null, 0, null, + ).lastInsertRowid as number; + const otherDay = createDay(testDb, other.id); const otherPlace = createPlace(testDb, other.id); createDayAssignment(testDb, otherDay.id, otherPlace.id); const rows = await assignments.listAllForTrip(trip.id); - expect(rows.map((r) => r.id)).toEqual([a1.id]); + const legacy = testDb.prepare('SELECT da.* FROM day_assignments da JOIN days d ON d.id = da.day_id WHERE d.trip_id = ?').all(trip.id); + expect(rows).toEqual(legacy); + expect(rows.map((r) => r.id)).toEqual([a1, a2]); }); it('ASSIGNREPO-028: an empty trip returns []', async () => { diff --git a/server/tests/unit/db/repositories/DayNotes.repository.test.ts b/server/tests/unit/db/repositories/DayNotes.repository.test.ts index e89aac9e9..854c6b36a 100644 --- a/server/tests/unit/db/repositories/DayNotes.repository.test.ts +++ b/server/tests/unit/db/repositories/DayNotes.repository.test.ts @@ -95,16 +95,21 @@ describe('DayNotesRepository.listByDayIds (DY4)', () => { }); // --------------------------------------------------------------------------- - // Plan 3c Task 2 review (task-2-review.md, L2) — appended per the Task 3 - // coordinator addendum: `listByDayIds` is `find`-based (unlike Task 3's own - // qb()/Kysely reads), so the identity-map guard is a real, provable - // guarantee here, not "proven regardless". The setup read passes `{ - // disableIdentityMap: false }` so it genuinely caches a managed entity - // first — a bare `find({})` would be a no-op under the base's own default - // (`DAYREPO-018`'s vacuousness). + // Task 9 fix wave (B-M3): relabelled. The Task 2 review comment this + // replaces claimed `listByDayIds` being `find`-based made the identity-map + // guard "a real, provable guarantee here, not 'proven regardless'" — that + // is not true: `TrekRepository.find` applies `disableIdentityMap: true` to + // EVERY read by default (rule 14), `listByDayIds` never opts in, and its + // filter (`day: { $in: day_ids }`) is not a primary-key lookup regardless. + // There is no live identity-map entry here to bypass. The setup read still + // passes `{ disableIdentityMap: false }` so it genuinely caches a managed + // entity first (a bare `find({})` would be a no-op under the base's own + // default, the same vacuousness `DAYREPO-018` had) — but what this proves + // is a DB round-trip (a text write after an unrelated wider read is + // visible), not an identity-map bypass. // --------------------------------------------------------------------------- - it('NOTEREPO-008 (D-shape): a text write after an unrelated identity-map read is visible in the FIRST wider listByDayIds', async () => { + it('NOTEREPO-008 (fresh after a raw UPDATE, not D-shape): a text write after an unrelated identity-map read is visible in the FIRST wider listByDayIds', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); @@ -120,17 +125,32 @@ describe('DayNotesRepository.listByDayIds (DY4)', () => { // ── Plan 3c Task 8 (`TripsService.copy`, TP70/TP71) — additive ────────────── describe('DayNotesRepository.listByTrip (TP70)', () => { - it('NOTEREPO-009: every note of the trip, scoped by trip_id (not day_id)', async () => { + // Task 9 fix wave (B-M4, rule 19): widened from an ids-only assertion to + // full-row parity on a fully seeded fixture — every nullable column + // non-null in one row, null in the other, plus a unicode string and a + // '007'-style digit string — `toEqual()`, so a renamed/dropped column or a changed + // row order would fail this, not just the id list. + it('NOTEREPO-009: every note of the trip, scoped by trip_id (not day_id) — toEqual(legacy) on a fully seeded fixture', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const other = createTrip(testDb, user.id); const day = createDay(testDb, trip.id); - const note = createDayNote(testDb, day.id, trip.id, { text: 'Keep' }); + + const insert = testDb.prepare('INSERT INTO day_notes (day_id, trip_id, text, time, icon, sort_order, color) VALUES (?, ?, ?, ?, ?, ?, ?)'); + // Row A: every nullable column non-null, a unicode string and a + // '007'-style digit string among the values. + const n1 = insert.run(day.id, trip.id, 'Keep — 日本 ☕️ 007', '09:00', '🗒️', 1, '#00ff00').lastInsertRowid as number; + // Row B: every nullable column NULL. + const n2 = insert.run(day.id, trip.id, 'Bare note', null, null, null, null).lastInsertRowid as number; + const otherDay = createDay(testDb, other.id); createDayNote(testDb, otherDay.id, other.id, { text: 'Not this trip' }); const rows = await notes.listByTrip(trip.id); - expect(rows.map((r) => r.id)).toEqual([note.id]); + const legacy = testDb.prepare('SELECT * FROM day_notes WHERE trip_id = ?').all(trip.id); + expect(rows).toEqual(legacy); + expect(rows.map((r) => r.id)).toEqual([n1, n2]); }); it('NOTEREPO-010: a trip with no notes returns []', async () => { diff --git a/server/tests/unit/db/repositories/GooglePlacePhotoMeta.repository.test.ts b/server/tests/unit/db/repositories/GooglePlacePhotoMeta.repository.test.ts index 7141f331b..d92057ea3 100644 --- a/server/tests/unit/db/repositories/GooglePlacePhotoMeta.repository.test.ts +++ b/server/tests/unit/db/repositories/GooglePlacePhotoMeta.repository.test.ts @@ -111,18 +111,18 @@ describe('GooglePlacePhotoMetaRepository.listPlaceIds', () => { }); }); -// D-shape (Task 1 brief item): findLive/findErrored are `findOne`-based -// (`TrekRepository`'s `disableIdentityMap: true` default), the class of read -// program rule 14 targets directly. -describe('GooglePlacePhotoMetaRepository — D-shape', () => { - it('GPPMREPO-014: a raw write after an unrelated identity-map read is visible in the next findLive call, in one query (disableIdentityMap regression)', async () => { +// Task 9 fix wave (B-M3): relabelled. `findLive`/`findErrored` are +// `findOne`-based, but their filter is `{ place_id, error_at: null }` / +// `{ place_id, error_at: { $ne: null } }` — the PK plus a second condition, +// not a PK-only lookup, so rule 20's D-shape guarantee does not apply (a +// non-PK-only `findOne` always re-runs its WHERE against the live table). +describe('GooglePlacePhotoMetaRepository — fresh after a raw UPDATE', () => { + it('GPPMREPO-014 (fresh after a raw UPDATE, not D-shape): a raw write after an unrelated identity-map read is visible in the next findLive call, in one query (disableIdentityMap regression)', async () => { testDb.prepare('INSERT INTO google_place_photo_meta (place_id, attribution, fetched_at, error_at) VALUES (?, ?, ?, NULL)').run('p1', 'Old', 1000); - // rule 20: the FIRST, wider setup read passes `disableIdentityMap: - // false` explicitly and carries the column the later write targets - // (`attribution`) — a PK-only `findOne`, not `findLive`'s own - // `error_at IS NULL`-narrowed filter, matching the shape that makes - // `PlaceDetailsCacheRepository`'s PDCREPO-008 fail under the - // disableIdentityMap-default mutation. + // The FIRST, wider setup read passes `disableIdentityMap: false` + // explicitly and carries the column the later write targets + // (`attribution`) — a PK-only `findOne` on `place_id` alone (not + // `findLive`'s own `error_at IS NULL`-narrowed filter under test). await t.repo(GooglePlacePhotoMeta).findOne({ place_id: 'p1' }, { disableIdentityMap: false }); testDb.prepare('UPDATE google_place_photo_meta SET attribution = ? WHERE place_id = ?').run('New', 'p1'); diff --git a/server/tests/unit/db/repositories/PlaceDetailsCache.repository.test.ts b/server/tests/unit/db/repositories/PlaceDetailsCache.repository.test.ts index 5776e0d6c..33110b107 100644 --- a/server/tests/unit/db/repositories/PlaceDetailsCache.repository.test.ts +++ b/server/tests/unit/db/repositories/PlaceDetailsCache.repository.test.ts @@ -83,12 +83,20 @@ describe('PlaceDetailsCacheRepository.upsertEntry — composite-key INSERT OR RE }); // D-shape (Task 1 brief item): `findEntry` is `findOne`-based -// (`TrekRepository`'s `disableIdentityMap: true` default). +// (`TrekRepository`'s `disableIdentityMap: true` default), filtered solely +// on the table's real composite PRIMARY KEY (`place_id`, `lang`, `expanded`) +// — a PK-only `findOne`, the class rule 20/21 targets directly. describe('PlaceDetailsCacheRepository — D-shape', () => { - it('PDCREPO-008: a raw write after findEntry is visible in the next findEntry call, in one query (disableIdentityMap regression)', async () => { + it('PDCREPO-008 (D-shape): a raw write after an unrelated identity-map read is visible in the next findEntry call, in one query (disableIdentityMap regression)', async () => { testDb.prepare('INSERT INTO place_details_cache (place_id, lang, expanded, payload_json, fetched_at) VALUES (?, ?, ?, ?, ?)') .run('p1', 'en', 2, '{"v":"old"}', 100); - expect(await cache.findEntry('p1', 'en', 2)).toEqual({ payload_json: '{"v":"old"}', fetched_at: 100 }); // populate the identity map + // Task 9 fix wave (B-M3): rule 20 requires the FIRST, wider setup read to + // opt out of the base default explicitly, SEPARATE from the method under + // test — `findEntry` itself always uses the base's own + // `disableIdentityMap: true` default, so calling it here could never + // populate an entry for the later assertion to prove stale (the D-shape + // claim would be vacuous, the same class of gap rule 20 exists to close). + await t.repo(PlaceDetailsCache).findOne({ place_id: 'p1', lang: 'en', expanded: 2 }, { disableIdentityMap: false }); testDb.prepare('UPDATE place_details_cache SET payload_json = ?, fetched_at = ? WHERE place_id = ? AND lang = ? AND expanded = ?') .run('{"v":"new"}', 200, 'p1', 'en', 2); diff --git a/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts b/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts index 1ba6999fd..2e03f0717 100644 --- a/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts +++ b/server/tests/unit/db/repositories/PlaceRatings.repository.test.ts @@ -81,13 +81,13 @@ describe('PlaceRatingsRepository.listForPlaces', () => { expect(await placeRatings.listForPlaces([place.id])).toEqual([]); }); - // D-shape (Task 1 brief item): `listForPlaces` is a `qb().execute('all', - // false)` projection, which never hydrates an entity into the identity - // map by construction (unlike `find`/`findOne`), so there is no staleness - // hazard here to reproduce — this proves it anyway, the same honest - // "not required, proven regardless" shape `Tags.repository.test.ts`'s - // TAGREPO comments already use for a method that IS at risk. - it('PLACERATINGSREPO-005 (D-shape): a rating written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled. `listForPlaces` is a + // `qb().execute('all', false)` projection, which never hydrates an entity + // into the identity map by construction (unlike `find`/`findOne`), and the + // base default leaves the identity map disabled for every read anyway — + // there is no live identity-map entry here to bypass. This proves a DB + // round-trip, not an identity-map bypass. + it('PLACERATINGSREPO-005 (fresh after a raw UPDATE, not D-shape): a rating written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user: owner } = createUser(testDb); const { user: seedVoter } = createUser(testDb, { username: 'seed-voter' }); const { user: voter } = createUser(testDb, { username: 'fresh-voter' }); diff --git a/server/tests/unit/db/repositories/PlaceShadowPicks.repository.test.ts b/server/tests/unit/db/repositories/PlaceShadowPicks.repository.test.ts index 416d912a1..3520cbc56 100644 --- a/server/tests/unit/db/repositories/PlaceShadowPicks.repository.test.ts +++ b/server/tests/unit/db/repositories/PlaceShadowPicks.repository.test.ts @@ -159,15 +159,14 @@ describe('PlaceShadowPicksRepository.deleteAll / purgeOlderThan', () => { }); }); -// D-shape (Task 1 brief item): `page`/`totals`/`countBySource`/`countByLiveRank` -// all go through `find`/`qb().execute()`, neither of which hydrates a -// long-lived entity here — proven anyway for `page` (the one `find()`-based -// read), the honest "not required, proven regardless" shape: `page()` itself -// always passes the base's own `disableIdentityMap: true` default (it never -// opts in), so a stale cached entity can't leak into it regardless of what -// the setup read below does. -describe('PlaceShadowPicksRepository — D-shape', () => { - it('PSPICKREPO-011: a row inserted after an unrelated identity-map read is visible in the FIRST wider projection (page)', async () => { +// Task 9 fix wave (B-M3): relabelled. `page`/`totals`/`countBySource`/ +// `countByLiveRank` all go through `find`/`qb().execute()`; `TrekRepository` +// applies `disableIdentityMap: true` to every read by default, so `page()` +// never has a live identity-map entry to bypass in the first place — this +// proves a DB round-trip (a row inserted after an unrelated wider read is +// visible), not an identity-map bypass. +describe('PlaceShadowPicksRepository — fresh after a raw UPDATE', () => { + it('PSPICKREPO-011 (fresh after a raw UPDATE, not D-shape): a row inserted after an unrelated identity-map read is visible in the FIRST wider projection (page)', async () => { await picks.insertPick(row({ query: 'seed' })); // rule 20: the FIRST, wider setup read passes `disableIdentityMap: false` // explicitly and carries the column the later write targets (`query`) — diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts index 5de359600..9e03d2b7f 100644 --- a/server/tests/unit/db/repositories/Places.repository.test.ts +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -246,7 +246,13 @@ describe('PlacesRepository.existsInTrip / findInTrip / reclaimInputs / deleteByI expect(await places.existsInTrip(999999, trip.id)).toBe(false); }); - it('PLACEREPO-013 (D-shape): a name write after an unrelated identity-map read is visible in findInTrip\'s FIRST wider read', async () => { + // Task 9 fix wave (B-M3): relabelled. `findInTrip` is a + // `qb().execute('get', false)` projection, which never hydrates an entity + // into the identity map by construction, and the base default leaves the + // identity map disabled for every read anyway — there is no live + // identity-map entry here to bypass. This proves a DB round-trip, not an + // identity-map bypass. + it('PLACEREPO-013 (fresh after a raw UPDATE, not D-shape): a name write after an unrelated identity-map read is visible in findInTrip\'s FIRST wider read', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id, { name: 'Before' }); @@ -258,16 +264,51 @@ describe('PlacesRepository.existsInTrip / findInTrip / reclaimInputs / deleteByI expect(row?.name).toBe('After'); }); - it('PLACEREPO-014: findInTrip returns every scalar column (PL9/PL48\'s SELECT *), undefined cross-trip', async () => { + // Task 9 fix wave (B-M4, rule 19): widened from `toMatchObject` on 3 keys + // (a title claiming "every scalar column" that only checked 3 of 28) to + // `toEqual(legacy SELECT p.* ... WHERE p.id = ? AND p.trip_id = ? run raw)` + // on the full key set, plus a fully NULL-nullable-column row, so a + // renamed or dropped column would fail this test. + it('PLACEREPO-014: findInTrip returns every scalar column (PL9/PL48\'s SELECT *), full toEqual(legacy) parity, undefined cross-trip', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const otherTrip = createTrip(testDb, user.id); + const category = createCategory(testDb); const place = createPlace(testDb, trip.id, { name: 'Full Row' }); + testDb.prepare(` + UPDATE places SET description = ?, lat = ?, lng = ?, address = ?, category_id = ?, price = ?, currency = ?, + reservation_status = ?, reservation_notes = ?, reservation_datetime = ?, place_time = ?, end_time = ?, + duration_minutes = ?, notes = ?, image_url = ?, google_place_id = ?, google_ftid = ?, website = ?, phone = ?, + transport_mode = ?, osm_id = ?, route_geometry = ?, route_color = ?, stop_type = ?, fill_percent = ?, + amap_poi_id = ?, source = ? + WHERE id = ? + `).run( + 'A description with 007-style digits', 48.1, 2.2, '007 Rue de Paris', category.id, 12.5, 'EUR', + 'confirmed', 'notes-a', '2026-01-01T10:00:00Z', '10:00', '11:00', + 45, 'place notes', '/img/a.png', 'gpid-007', 'gftid-a', 'https://a.example', '+33 1 23 45 67 89', + 'driving', 'osm-a', '{"type":"LineString"}', '#ff0000', 'hotel', 50, + 'amap-a', 'manual', + place.id, + ); + const row = await places.findInTrip(place.id, trip.id); - expect(row).toMatchObject({ id: place.id, trip_id: trip.id, name: 'Full Row' }); + const legacy = testDb.prepare('SELECT * FROM places WHERE id = ? AND trip_id = ?').get(place.id, trip.id); + expect(row).toEqual(legacy); + expect(row).toMatchObject({ id: place.id, trip_id: trip.id, name: 'Full Row', google_place_id: 'gpid-007' }); expect(await places.findInTrip(place.id, otherTrip.id)).toBeUndefined(); }); + it('PLACEREPO-052: findInTrip on a bare row — every nullable column comes back null (rule 16), matching legacy', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Bare Place' }); + const row = await places.findInTrip(place.id, trip.id); + const legacy = testDb.prepare('SELECT * FROM places WHERE id = ? AND trip_id = ?').get(place.id, trip.id); + expect(row).toEqual(legacy); + expect(row?.description).toBeNull(); + expect(row?.google_ftid).toBeNull(); + }); + it('PLACEREPO-015: reclaimInputs projects only google_place_id/image_url', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -984,3 +1025,62 @@ describe('PlacesRepository.listForTripOrdered (RP2)', () => { expect(await places.listForTripOrdered(trip.id)).toEqual([]); }); }); + +// Task 9 fix wave (B-M4, rule 19): `listAllForTrip` (TP40, `TripsService.copy`'s +// places read) had no repository test at all. Full-row parity on a fully +// seeded fixture — every nullable column non-null in one row, null in the +// other, plus a unicode string and a '007'-style digit string — so a +// renamed/dropped column or a changed row order would fail this, not a +// `toMatchObject` on a few keys. +describe('PlacesRepository.listAllForTrip (TP40)', () => { + it('PLACEREPO-050: every column, rowid scan order (no ORDER BY) — toEqual(legacy SELECT * run raw) on a fully seeded fixture', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const otherTrip = createTrip(testDb, user.id); + const category = createCategory(testDb); + + const insert = testDb.prepare(` + INSERT INTO places ( + trip_id, name, description, lat, lng, address, category_id, price, currency, + reservation_status, reservation_notes, reservation_datetime, place_time, end_time, + duration_minutes, notes, image_url, google_place_id, google_ftid, website, phone, + transport_mode, created_at, updated_at, osm_id, route_geometry, route_color, stop_type, + fill_percent, amap_poi_id, source + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `); + + // Row A: every nullable column non-null — a unicode string and a + // '007'-style digit string among the values (rule 19's fixture bar). + const rowA = insert.run( + trip.id, 'Café — 日本 ☕️', 'A description with 007-style digits', 48.1, 2.2, '007 Rue de Paris', category.id, 12.5, 'EUR', + 'confirmed', 'notes-a', '2026-01-01T10:00:00Z', '10:00', '11:00', + 45, 'place notes', '/img/a.png', 'gpid-007', 'gftid-a', 'https://a.example', '+33 1 23 45 67 89', + 'driving', '2026-01-01 09:00:00', '2026-01-01 09:30:00', 'osm-a', '{"type":"LineString"}', '#ff0000', 'hotel', + 50, 'amap-a', 'manual', + ).lastInsertRowid as number; + + // Row B: every nullable column NULL. + const rowB = insert.run( + trip.id, 'Bare Place', null, null, null, null, null, null, null, + null, null, null, null, null, + null, null, null, null, null, null, null, + null, null, null, null, null, null, null, + null, null, null, + ).lastInsertRowid as number; + + // A place on another trip — must not leak into this trip's listAllForTrip. + createPlace(testDb, otherTrip.id, { name: 'Elsewhere' }); + + const rows = await places.listAllForTrip(trip.id); + const legacy = testDb.prepare('SELECT * FROM places WHERE trip_id = ?').all(trip.id); + expect(rows).toEqual(legacy); + // rowid-ascending scan order (no ORDER BY), not insertion-reversed or re-sorted. + expect(rows.map((r) => r.id)).toEqual([rowA, rowB]); + }); + + it('PLACEREPO-051: an empty trip returns an empty array, not a throw', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect(await places.listAllForTrip(trip.id)).toEqual([]); + }); +}); diff --git a/server/tests/unit/db/repositories/Tags.repository.test.ts b/server/tests/unit/db/repositories/Tags.repository.test.ts index acd9c1ddf..079b14d54 100644 --- a/server/tests/unit/db/repositories/Tags.repository.test.ts +++ b/server/tests/unit/db/repositories/Tags.repository.test.ts @@ -5,14 +5,18 @@ import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createPlace, createTag, createTrip, createUser } from '../../../helpers/factories'; import { Tags } from '../../../../src/db/entities/Tags.entity'; import type { TagsRepository } from '../../../../src/db/repositories/Tags.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; +import { withRequestContext } from '../../../../src/nest/database/request-context'; const testDb = createSnapshotTestDb(); let t: TestOrm; let tags: TagsRepository; +let uow: UnitOfWork; beforeAll(async () => { t = await createTestOrm(testDb); tags = t.repo(Tags); + uow = new UnitOfWork(t.em); }); beforeEach(() => { resetTestDb(testDb); t.clear(); }); afterAll(async () => { await t.close(); testDb.close(); }); @@ -313,7 +317,12 @@ describe('TagsRepository', () => { expect(await tags.listForPlaces([place.id])).toEqual([]); }); - it('LISTFORPLACESREPO-006 (D-shape): a tag written via place_tags is visible in the FIRST wider (non-compact) projection, bypassing the identity map', async () => { + // Task 9 fix wave (B-M3): relabelled. `listForPlaces` is a + // `qb().execute('all', false)` projection, and the base default leaves + // the identity map disabled for every read anyway — there is no live + // identity-map entry here to bypass. This proves a DB round-trip, not an + // identity-map bypass. + it('LISTFORPLACESREPO-006 (fresh after a raw UPDATE, not D-shape): a tag written via place_tags is visible in the FIRST wider (non-compact) projection', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const place = createPlace(testDb, trip.id); @@ -384,6 +393,52 @@ describe('TagsRepository.insertIgnore / deleteForPlace (PL5/PL12/PL13 — place_ expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(place.id)).toEqual([]); expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(other.id)).toHaveLength(1); }); + + // Task 9 fix wave (B-M5, the OAUTHTOKREPO-012/REANCHORDAY-003 rollback + // shape): both Kysely pivot writes join the ambient `uow.transactional` + // rather than autocommitting on their own — a thrown error after the + // statement rolls the write back, and the raw read afterwards proves the + // pre-transaction state. + it('PLACETAGSREPO-006: insertIgnore inside a uow.transactional that then ROLLS BACK attaches nothing', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const tag = createTag(testDb, user.id); + + let caught: unknown; + try { + await withRequestContext(t.orm, async () => { + await uow.transactional(async () => { + await tags.insertIgnore(place.id, [tag.id]); + throw new Error('force rollback'); + }); + }); + } catch (e) { caught = e; } + expect((caught as Error).message).toBe('force rollback'); + + expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(place.id)).toEqual([]); + }); + + it('PLACETAGSREPO-007: deleteForPlace inside a uow.transactional that then ROLLS BACK leaves the pairs in place', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const tag = createTag(testDb, user.id); + await tags.insertIgnore(place.id, [tag.id]); + + let caught: unknown; + try { + await withRequestContext(t.orm, async () => { + await uow.transactional(async () => { + await tags.deleteForPlace(place.id); + throw new Error('force rollback'); + }); + }); + } catch (e) { caught = e; } + expect((caught as Error).message).toBe('force rollback'); + + expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(place.id)).toHaveLength(1); + }); }); // ── Plan 3c Task 8 (`TripsService.copy`, TP46) — additive ─────────────────── @@ -411,4 +466,32 @@ describe('TagsRepository.listPlaceTagsForTrip (TP46)', () => { createPlace(testDb, trip.id); expect(await tags.listPlaceTagsForTrip(trip.id)).toEqual([]); }); + + // Task 9 fix wave (B-M5, the OAUTHTOKREPO-012 read shape): `listPlaceTagsForTrip` + // is a Kysely read that joins the ambient `uow.transactional` rather than + // opening its own connection — it sees an uncommitted write made earlier + // in the SAME open transaction, and once that transaction rolls back the + // pair is gone again, proven by a raw read afterwards. + it('PLACETAGSREPO-008: sees an uncommitted insertIgnore inside the SAME open transaction; a ROLLBACK erases it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id); + const tag = createTag(testDb, user.id); + + let caught: unknown; + try { + await withRequestContext(t.orm, async () => { + await uow.transactional(async () => { + await tags.insertIgnore(place.id, [tag.id]); + const seenInTransaction = await tags.listPlaceTagsForTrip(trip.id); + expect(seenInTransaction).toEqual([{ place_id: place.id, tag_id: tag.id }]); + throw new Error('force rollback'); + }); + }); + } catch (e) { caught = e; } + expect((caught as Error).message).toBe('force rollback'); + + expect(await tags.listPlaceTagsForTrip(trip.id)).toEqual([]); + expect(testDb.prepare('SELECT * FROM place_tags WHERE place_id = ?').all(place.id)).toEqual([]); + }); }); diff --git a/server/tests/unit/db/repositories/TripMembers.repository.test.ts b/server/tests/unit/db/repositories/TripMembers.repository.test.ts index d32c415bd..e15dbff42 100644 --- a/server/tests/unit/db/repositories/TripMembers.repository.test.ts +++ b/server/tests/unit/db/repositories/TripMembers.repository.test.ts @@ -207,11 +207,13 @@ describe('TripMembersRepository.listWithUserAndInviter (TM2, Plan 3c Task 6)', ( expect(await tripMembers.listWithUserAndInviter(trip.id, owner.id)).toEqual([]); }); - // D-shape (rule 20): a `qb().execute('all', false)` projection, which - // never hydrates an entity into the identity map by construction — proven - // anyway, the `AssignmentParticipantsRepository`/`PlaceRatingsRepository` - // precedent shape. - it('TMEMREPO-014 (D-shape): a member added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled. `listWithUserAndInviter` is a + // `qb().execute('all', false)` projection, and `TrekRepository` applies + // `disableIdentityMap: true` to every read by default anyway, so there is + // no live identity-map entry here to bypass — this proves a DB round-trip + // (a member added after an unrelated wider read is visible), not an + // identity-map bypass. + it('TMEMREPO-014 (fresh after a raw UPDATE, not D-shape): a member added after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user: owner } = createUser(testDb); const { user: fresh } = createUser(testDb, { username: 'fresh-member' }); const trip = createTrip(testDb, owner.id); diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index 6d29c013f..d14e33cb4 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -227,10 +227,13 @@ describe('TripsRepository — TripMembersService / TripReadModelService (Plan 3c expect(await trips.findRaw('not-a-number')).toBeNull(); }); - // D-shape (rule 20): `findRaw` is a `qb().execute('get', false)` - // projection with an explicit `t.*` select, never hydrating an entity - // into the identity map by construction — proven anyway. - it('TRIPREPO-022 (D-shape): a title written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled. `findRaw` is a `qb().execute('get', + // false)` projection with an explicit `t.*` select, which never + // hydrates an entity into the identity map by construction, and the + // base default leaves the identity map disabled for every read anyway — + // there is no live identity-map entry here to bypass. This proves a DB + // round-trip, not an identity-map bypass. + it('TRIPREPO-022 (fresh after a raw UPDATE, not D-shape): a title written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { title: 'Before' }); await t.repo(Trips).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index ce73e9531..441df23ff 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -796,13 +796,14 @@ describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { expect((await users.findOwnerSummary(user.id))?.avatar).toBe('me.png'); }); - // D-shape (rule 20): `findOwnerSummary` is a `qb().execute('get', false)` - // projection, which never hydrates an entity into the identity map by - // construction — proven anyway, the same "not required, proven - // regardless" shape `AssignmentParticipantsRepository`'s D-shape tests - // document. The FIRST, wider setup read passes `{ disableIdentityMap: - // false }` explicitly per the program rule. - it('USERSREPO-062 (D-shape): a display_name written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { + // Task 9 fix wave (B-M3): relabelled. `findOwnerSummary` is a + // `qb().execute('get', false)` projection, which never hydrates an + // entity into the identity map by construction, and the base default + // leaves the identity map disabled for every read anyway — there is no + // live identity-map entry here to bypass. This proves a DB round-trip, + // not an identity-map bypass. The FIRST, wider setup read still passes + // `{ disableIdentityMap: false }` explicitly per the program rule. + it('USERSREPO-062 (fresh after a raw UPDATE, not D-shape): a display_name written after an unrelated identity-map read is visible in the FIRST wider projection', async () => { const { user } = createUser(testDb, { username: 'fresh-owner' }); await t.repo(Users).find({}, { disableIdentityMap: false }); // populate the identity map with an unrelated read testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Fresh Name', user.id); @@ -876,6 +877,26 @@ describe('UsersRepository — TripMembersService (Plan 3c Task 6)', () => { expect(await users.findIdEmailGuest(user.id)).toEqual({ id: user.id, email: 'target@example.test', is_guest: 1 }); expect(await users.findIdEmailGuest(999999)).toBeNull(); }); + + // Task 9 fix wave (B-M3): `findIdEmailGuest` is the third PK-only + // `findOne` this plan added (`{ id }` alone, nothing else) — a genuine + // rule 20/21 D-shape case, the TM10 guest gate. USERSREPO-066 above + // already catches the M7 identity-map-default mutation (it re-reads + // after a raw UPDATE without an intervening wider read), but the review + // asked for the explicit labelled case anyway. + it('USERSREPO-073 (D-shape): an is_guest flip written after an unrelated identity-map read is visible in the next findIdEmailGuest call, in one query (disableIdentityMap regression)', async () => { + const { user } = createUser(testDb, { email: 'guest-gate@example.test' }); + // rule 20: the FIRST, wider setup read passes `disableIdentityMap: + // false` explicitly and carries the column the later write targets + // (`is_guest`) — a PK-only `findOne` on `id` alone, the exact shape + // `findIdEmailGuest` itself uses. + await t.repo(Users).findOne({ id: user.id }, { disableIdentityMap: false }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + + const { value, queries } = await withQueryCount(() => users.findIdEmailGuest(user.id)); + expect(value).toEqual({ id: user.id, email: 'guest-gate@example.test', is_guest: 1 }); + expect(queries).toBe(1); + }); }); describe('insertGuest (TM16, security-sensitive)', () => { diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 0ad2e2837..deb853e50 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -28,19 +28,18 @@ const { testDb, dbMock } = vi.hoisted(() => { db, closeDb: () => {}, reinitialize: () => {}, - getPlaceWithTags: (placeId: any) => { - const place: any = db.prepare(` - SELECT p.*, c.name as category_name, c.color as category_color, c.icon as category_icon - FROM places p LEFT JOIN categories c ON p.category_id = c.id WHERE p.id = ? - `).get(placeId); - if (!place) return null; - const tags = db.prepare(`SELECT t.* FROM tags t JOIN place_tags pt ON t.id = pt.tag_id WHERE pt.place_id = ?`).all(placeId); - return { ...place, category: place.category_id ? { id: place.category_id, name: place.category_name, color: place.category_color, icon: place.category_icon } : null, tags }; - }, + // Task 9 fix wave (item 10, A-L4c follow-up): the `getPlaceWithTags`/ + // `isOwner` fakes removed — `DatabaseService.getPlaceWithTags`/`.isOwner` + // already go through the real ORM + // (`entityManager().getRepository(Places/Trips)...`), never through this + // mocked `db/database` module's exports, so these module-level fakes + // were dead (nothing calls `getPlaceWithTags`/`isOwner` AS MODULE + // FUNCTIONS — see the `dbs.getPlaceWithTags` INSTANCE-METHOD spy below, + // which is a different, still-live thing: `AccommodationsService` calls + // `this.db.getPlaceWithTags(...)` on the injected `DatabaseService` + // instance, not on this module). canAccessTrip: (tripId: any, userId: number) => db.prepare(`SELECT t.id, t.user_id FROM trips t LEFT JOIN trip_members m ON m.trip_id = t.id AND m.user_id = ? WHERE t.id = ? AND (t.user_id = ? OR m.user_id IS NOT NULL)`).get(userId, tripId, userId), - isOwner: (tripId: any, userId: number) => - !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId), }; return { testDb: db, dbMock: mock }; }); @@ -150,15 +149,26 @@ let accommodations: Awaited>; let svc: Awaited>; beforeAll(async () => { // Plan 3c Task 0b: `dbs` is constructed at module load, before any - // `beforeAll` can resolve a real `EntityManager` — the four - // repository-backed methods are spied directly on this instance instead, - // routed to a real `DatabaseService` built with one. + // `beforeAll` can resolve a real `EntityManager` — `canAccessTrip` and + // `getPlaceWithTags` are spied directly on this instance instead, routed + // to a real `DatabaseService` built with one. const real = new DatabaseService(testDb, (await sharedTestOrm(testDb)).em); vi.spyOn(dbs, 'canAccessTrip').mockImplementation((...a) => real.canAccessTrip(...a)); - vi.spyOn(dbs, 'isOwner').mockImplementation((...a) => real.isOwner(...a)); - // Task 9 fix wave (A-L4c): `dbs.rosterUserIds` is dead here — `PlacesService` - // now calls `TripMembersRepository.rosterUserIds` directly (PL1, injected, - // not through `DatabaseService`), so this spy was inert. + // Task 9 fix wave (A-L4c, then item 10): `dbs.rosterUserIds` was dead here + // — `PlacesService` now calls `TripMembersRepository.rosterUserIds` + // directly (PL1, injected, not through `DatabaseService`) — and its + // `rosterUserIds` spy was removed in that fix-wave commit. `dbs.isOwner` + // was ALSO dead the same way (verified: `grep -n "\.isOwner(" + // src/nest/places/places.service.ts src/nest/accommodations/ + // accommodations.service.ts` has zero hits reachable from this file's + // constructed service graph) — removed here, along with the now-unused + // `real.isOwner` target it pointed at. `dbs.getPlaceWithTags` is NOT dead, + // though (unlike the module-level fake above): `AccommodationsService + // .stampLodging` — constructed below via `accommodationsOver(dbs)` and + // exercised by the PLACE-SVC-019d/019e/057b place-delete tests — calls + // `this.db.getPlaceWithTags(...)` on this exact `dbs` instance, which + // without this spy throws "no EntityManager available" (`dbs` here has + // none; only `real` does). vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); accommodations = await accommodationsOver(dbs); svc = await makePlacesService();