diff --git a/server/src/db/repositories/JourneyEntries.repository.ts b/server/src/db/repositories/JourneyEntries.repository.ts index 4af644cc7..54d6e2d04 100644 --- a/server/src/db/repositories/JourneyEntries.repository.ts +++ b/server/src/db/repositories/JourneyEntries.repository.ts @@ -428,8 +428,11 @@ export class JourneyEntriesRepository extends TrekRepository { async listCachedCountriesForPlaceIds(placeIds: number[]): Promise<{ place_id: number; country_code: string | null }[]> { const out: { place_id: number; country_code: string | null }[] = []; for (let i = 0; i < placeIds.length; i += 400) { + // M4 (task-5-review.md) — `chunk` can never be empty here: the loop + // guard `i < placeIds.length` guarantees at least one element remains + // to slice, so a dead `if (!chunk.length) continue;` used to sit here + // (an uncovered branch the coverage gate flagged, worse than base). const chunk = placeIds.slice(i, i + 400); - if (!chunk.length) continue; const rows = await this.kysely() .selectFrom('place_regions') .select(['place_id', 'country_code']) diff --git a/server/src/db/repositories/JourneyShareTokens.repository.ts b/server/src/db/repositories/JourneyShareTokens.repository.ts index c5e6cf01b..c6770353d 100644 --- a/server/src/db/repositories/JourneyShareTokens.repository.ts +++ b/server/src/db/repositories/JourneyShareTokens.repository.ts @@ -1,7 +1,4 @@ -import type { Platform } from '@mikro-orm/core'; -import type { ExpressionBuilder } from 'kysely'; import type { JourneyEntry } from '../../types'; -import { concatKysely, unixEpochToIsoKysely } from '../dialect/sql-functions'; import type { JourneyShareTokens } from '../entities/JourneyShareTokens.entity'; import { TrekRepository } from './_shared/trek-repository'; @@ -168,42 +165,18 @@ interface JourneyPublicKyselyDB { }; } -/** - * R1's SECOND `GALLERY_CHRONOLOGICAL_ORDER` consumer (`getPublicJourney`'s - * gallery read, JS15) — Task 0's worked example (`task-0-report.md`), pasted - * verbatim against this file's own `JourneyPublicKyselyDB` shape rather than - * re-derived. Fixed to the `gp`/`tp` alias contract `.selectFrom('journey_photos - * as gp').innerJoin('trek_photos as tp', ...)` produces, same as the - * `publicStayExists`/SQLF-070 precedent: a table's alias is a property of the - * QUERY, not of the `DB` interface, so this is not generic over an arbitrary - * caller `DB`/`TB`. - */ -function galleryChronologicalOrderExpr( - platform: Platform, - eb: ExpressionBuilder, -) { - return eb.fn.coalesce( - eb.fn('nullif', [eb.ref('tp.taken_at'), eb.val('')]), - eb - .selectFrom('journey_entry_photos as jep') - .innerJoin('journey_entries as je', 'je.id', 'jep.entry_id') - .select((eb2) => - eb2.fn - .min( - concatKysely( - platform, - eb2, - { column: 'je.entry_date' }, - { value: 'T' }, - { expression: eb2.fn.coalesce(eb2.fn('nullif', [eb2.ref('je.entry_time'), eb2.val('')]), eb2.val('00:00')) }, - ), - ) - .as('min_dt'), - ) - .whereRef('jep.journey_photo_id', '=', 'gp.id'), - unixEpochToIsoKysely(platform, eb, 'gp.created_at'), - ); -} +// L2 — `galleryChronologicalOrderExpr`/`listGalleryForPublicJourney` (JS15) +// used to be a byte-identical duplicate of `JourneyPhotosRepository`'s own +// `galleryChronologicalOrderExpr`/`galleryRead` (same `GALLERY_COLUMNS` +// select list, same ORDER BY, differing only in which local `DB` interface +// the builder is typed against). `JourneyShareService.getPublicJourney` now +// calls `JourneyPhotosRepository.galleryRead` directly instead — one gallery +// ORDER BY builder, one gallery query, per the whole-plan review (M5/L2). +// `JourneyPublicGalleryRow` stays exported: it is still the accurate shape +// of what that query returns (`GalleryPhoto`, the type `galleryRead` is +// declared against, is a narrower public-API type missing `taken_at`/`lat`/ +// `lng`/`media_type`/`duration_ms`), and tests keep using it to type the +// public gallery response. /** * `journey_share_tokens` — public share links (Plan 3g Task 3, R4). Six of @@ -354,27 +327,6 @@ export class JourneyShareTokensRepository extends TrekRepository { - const platform = this.getEntityManager().getPlatform(); - return await this.kysely() - .selectFrom('journey_photos as gp') - .innerJoin('trek_photos as tp', 'tp.id', 'gp.photo_id') - .select([ - 'gp.id', 'gp.journey_id', 'gp.photo_id', 'gp.caption', 'gp.shared', 'gp.sort_order', 'gp.created_at', - 'tp.provider', 'tp.asset_id', 'tp.owner_id', 'tp.file_path', 'tp.thumbnail_path', 'tp.width', 'tp.height', - 'tp.media_type', 'tp.duration_ms', 'tp.taken_at', 'tp.lat', 'tp.lng', - ]) - .where('gp.journey_id', '=', journeyId) - .orderBy((eb) => galleryChronologicalOrderExpr(platform, eb), 'asc') - .orderBy('gp.sort_order', 'asc') - .orderBy('gp.id', 'asc') - .execute(); - } + // JS15 — `getPublicJourney`'s gallery read moved to + // `JourneyPhotosRepository.galleryRead` (L2, above). } diff --git a/server/src/db/repositories/TripPhotos.repository.ts b/server/src/db/repositories/TripPhotos.repository.ts index 92127fce1..1392275b7 100644 --- a/server/src/db/repositories/TripPhotos.repository.ts +++ b/server/src/db/repositories/TripPhotos.repository.ts @@ -67,7 +67,8 @@ export class TripPhotosRepository extends TrekRepository { /** * PH10's non-journey half — `SELECT 1 FROM trip_photos WHERE photo_id = ? * LIMIT 1`, used by `TrekPhotoRegistrationService.deleteIfOrphan` alongside - * the still-raw `journey_photos` existence check (`// PH10 — Plan 3g`). + * the `journey_photos` half of the same orphan check, converted onto + * {@link JourneyPhotosRepository.existsForPhoto} (Plan 3g Task 4). */ async existsForPhoto(photo_id: number): Promise { const row = await this.findOne({ photo: photo_id }, { fields: ['id'] }); diff --git a/server/src/nest/journey/journey-book.service.ts b/server/src/nest/journey/journey-book.service.ts index d5c0e52b7..709e21345 100644 --- a/server/src/nest/journey/journey-book.service.ts +++ b/server/src/nest/journey/journey-book.service.ts @@ -3,6 +3,7 @@ import { InjectRepository } from '@mikro-orm/nestjs'; import type { BookRecord, BookSummary } from '@trek/shared'; import { normalizeBookDocument } from '@trek/shared'; import { JourneyDomainService } from './journey-domain.service'; +import { UnitOfWork } from '../database/unit-of-work'; import { JourneyBooks } from '../../db/entities/JourneyBooks.entity'; import type { JourneyBookRow, JourneyBooksRepository } from '../../db/repositories/JourneyBooks.repository'; @@ -33,6 +34,9 @@ export class JourneyBookService { constructor( private readonly journey: JourneyDomainService, @InjectRepository(JourneyBooks) private readonly booksRepo: JourneyBooksRepository, + // M1 (task-5-review.md) — `saveBook`'s first-save read-then-insert + // needs the connection mutex to serialize two concurrent first saves. + private readonly uow: UnitOfWork, ) {} /** Null when the journey does not exist or the user cannot reach it. */ @@ -117,38 +121,50 @@ export class JourneyBookService { if (!(await this.canWrite(journeyId, userId))) return null; const document = JSON.stringify(normalizeBookDocument(input.document)); - // JB3. - const existing = await this.booksRepo.findFirstForJourney(journeyId); - if (!existing) { - // JB4. - const id = await this.booksRepo.insertBook(journeyId, input.title, document, userId); - return { record: (await this.byId(id))! }; - } + // M1 (rule 11/24) — JB3's existing-link read and JB4's insert are + // separate awaits, so two concurrent first saves of the same journey's + // book could both read "no book yet" and both insert, leaving one + // editor's save silently orphaned behind `getBook`'s `ORDER BY id LIMIT + // 1` (there is no unique index on `journey_id`). Wrapped whole: every + // statement below is DB-only, and the mutex serializes a second + // caller's read behind the first's commit, so it finds the row the + // first just created and takes the JB5 CAS-update path instead, the + // same outcome the synchronous legacy code always had. + return this.uow.transactional(async () => { + // JB3. + const existing = await this.booksRepo.findFirstForJourney(journeyId); - /* - * The version goes in the WHERE clause rather than being checked first. - * Read-then-write leaves a window between the two in which another save can - * land, and SQLite gives no guarantee across two statements — one UPDATE - * that matches on the version cannot lose that race with itself. - * - * A save with no base version is a first write from a client that has not - * loaded one; it is allowed to take the current version, since refusing it - * would break "open Studio and start editing" for the second person to - * arrive. - */ - const base = input.baseVersion ?? existing.version; - // JB5 (R2) — one conditional UPDATE, returning the affected-row count. - const changes = await this.booksRepo.casUpdate(existing.id, base, { - title: input.title, - document, - updatedBy: userId, + if (!existing) { + // JB4. + const id = await this.booksRepo.insertBook(journeyId, input.title, document, userId); + return { record: (await this.byId(id))! }; + } + + /* + * The version goes in the WHERE clause rather than being checked first. + * Read-then-write leaves a window between the two in which another save can + * land, and SQLite gives no guarantee across two statements — one UPDATE + * that matches on the version cannot lose that race with itself. + * + * A save with no base version is a first write from a client that has not + * loaded one; it is allowed to take the current version, since refusing it + * would break "open Studio and start editing" for the second person to + * arrive. + */ + const base = input.baseVersion ?? existing.version; + // JB5 (R2) — one conditional UPDATE, returning the affected-row count. + const changes = await this.booksRepo.casUpdate(existing.id, base, { + title: input.title, + document, + updatedBy: userId, + }); + + if (changes === 0) { + return { conflict: (await this.byId(existing.id))! }; + } + return { record: (await this.byId(existing.id))! }; }); - - if (changes === 0) { - return { conflict: (await this.byId(existing.id))! }; - } - return { record: (await this.byId(existing.id))! }; } async deleteBook(journeyId: number, userId: number): Promise { diff --git a/server/src/nest/journey/journey-domain.service.ts b/server/src/nest/journey/journey-domain.service.ts index ca76e93d4..064834017 100644 --- a/server/src/nest/journey/journey-domain.service.ts +++ b/server/src/nest/journey/journey-domain.service.ts @@ -460,43 +460,51 @@ export class JourneyDomainService { // ── Sync engine ────────────────────────────────────────────────────────── async syncTripPlaces(journeyId: number, tripId: number, authorId: number) { - const places = await this.journeyTripsRepo.listAssignedPlacesForTrip(tripId); + // M2 (rule 11/24) — the existence read and the skeleton insert are + // separate awaits now that the sync engine is async, so two concurrent + // syncs for the same trip could both miss the same place and each insert + // a skeleton for it. Wrapped whole: every statement inside is DB-only + // (repository calls), and the connection mutex serializes a second + // caller's statements behind this one until it commits. + await this.uow.transactional(async () => { + const places = await this.journeyTripsRepo.listAssignedPlacesForTrip(tripId); - const now = this.ts(); - const existing = await this.entriesRepo.listSourceKeysForTrip(journeyId, tripId); - const existingKeys = new Set(existing.map((e) => skeletonKey(e.source_place_id, e.source_assignment_id))); + const now = this.ts(); + const existing = await this.entriesRepo.listSourceKeysForTrip(journeyId, tripId); + const existingKeys = new Set(existing.map((e) => skeletonKey(e.source_place_id, e.source_assignment_id))); - // Track next sort_order per date so synced skeletons get unique, sequential positions. - const dateMaxOrder = new Map(); - const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId); - for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m); + // Track next sort_order per date so synced skeletons get unique, sequential positions. + const dateMaxOrder = new Map(); + const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId); + for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m); - for (const place of places) { - const key = skeletonKey(place.id, place.assignment_id); - if (existingKeys.has(key)) continue; - existingKeys.add(key); + for (const place of places) { + const key = skeletonKey(place.id, place.assignment_id); + if (existingKeys.has(key)) continue; + existingKeys.add(key); - const entryDate = place.day_date || new Date().toISOString().split('T')[0]; - const entryTime = place.assignment_time || place.place_time || null; - const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1; - dateMaxOrder.set(entryDate, nextOrder); + const entryDate = place.day_date || new Date().toISOString().split('T')[0]; + const entryTime = place.assignment_time || place.place_time || null; + const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1; + dateMaxOrder.set(entryDate, nextOrder); - await this.insertSkeletonEntry({ - journeyId, - tripId, - placeId: place.id, - assignmentId: place.assignment_id ?? null, - authorId, - title: place.name, - entryDate, - entryTime, - locationName: place.address || place.name, - lat: place.lat || null, - lng: place.lng || null, - sortOrder: nextOrder, - now, - }); - } + await this.insertSkeletonEntry({ + journeyId, + tripId, + placeId: place.id, + assignmentId: place.assignment_id ?? null, + authorId, + title: place.name, + entryDate, + entryTime, + locationName: place.address || place.name, + lat: place.lat || null, + lng: place.lng || null, + sortOrder: nextOrder, + now, + }); + } + }); } // called when a trip place is created @@ -511,38 +519,44 @@ export class JourneyDomainService { if (!assignments.length) return; // not assigned to a day yet — skip const now = this.ts(); - for (const journeyId of links) { - // JG40 — same text as JG2/JG54, resolved through `findOwnerId`. No - // undefined-guard here (matching the legacy statement's own unchecked - // `as { user_id: number }` cast): `journey_trips` cascades on the - // owning journey's delete, so a linked journey can never be missing. - const ownerId = (await this.journeysRepo.findOwnerId(journeyId)) as number; + // M2 (rule 11/24) — same fix as syncTripPlaces: the existence check + // (`existsForPlaceAssignment`) and the insert are separate awaits, so a + // concurrent reconcile racing this callback could both find nothing and + // both insert. Wrapped whole; DB-only, no broadcast inside. + await this.uow.transactional(async () => { + for (const journeyId of links) { + // JG40 — same text as JG2/JG54, resolved through `findOwnerId`. No + // undefined-guard here (matching the legacy statement's own unchecked + // `as { user_id: number }` cast): `journey_trips` cascades on the + // owning journey's delete, so a linked journey can never be missing. + const ownerId = (await this.journeysRepo.findOwnerId(journeyId)) as number; - for (const place of assignments) { - const already = await this.entriesRepo.existsForPlaceAssignment(journeyId, placeId, place.assignment_id ?? null); - if (already) continue; + for (const place of assignments) { + const already = await this.entriesRepo.existsForPlaceAssignment(journeyId, placeId, place.assignment_id ?? null); + if (already) continue; - const entryDate = place.day_date as string; - const maxOrder = await this.entriesRepo.maxSortOrderForDate(journeyId, entryDate); - const nextOrder = (maxOrder ?? -1) + 1; + const entryDate = place.day_date as string; + const maxOrder = await this.entriesRepo.maxSortOrderForDate(journeyId, entryDate); + const nextOrder = (maxOrder ?? -1) + 1; - await this.insertSkeletonEntry({ - journeyId, - tripId, - placeId, - assignmentId: place.assignment_id ?? null, - authorId: ownerId, - title: place.name, - entryDate, - entryTime: place.assignment_time || place.place_time || null, - locationName: place.address || place.name, - lat: place.lat || null, - lng: place.lng || null, - sortOrder: nextOrder, - now, - }); + await this.insertSkeletonEntry({ + journeyId, + tripId, + placeId, + assignmentId: place.assignment_id ?? null, + authorId: ownerId, + title: place.name, + entryDate, + entryTime: place.assignment_time || place.place_time || null, + locationName: place.address || place.name, + lat: place.lat || null, + lng: place.lng || null, + sortOrder: nextOrder, + now, + }); + } } - } + }); } // called when a trip place is updated @@ -695,137 +709,148 @@ export class JourneyDomainService { const now = this.ts(); for (const journeyId of links) { - const ownerId = await this.journeysRepo.findOwnerId(journeyId); - if (ownerId === undefined) continue; + // M2 (rule 11/24) — the existence read (`listForTripReconcile`) and the + // upsert/delete writes below are separate awaits, so two concurrent + // reconciles for the same trip (or a reconcile racing `onPlaceCreated`) + // could both read "not there yet" and both insert. Wrapped whole per + // journey: every statement inside is DB-only, and the mutex serializes + // a second caller's statements behind this one until it commits — the + // broadcast (non-DB I/O) stays outside, after commit, per rule 24. + const changed = await this.uow.transactional(async () => { + const ownerId = await this.journeysRepo.findOwnerId(journeyId); + if (ownerId === undefined) return false; - let changed = false; - // JG55 — a wider column set than JG35 (`listSourceKeysForTrip`), not a dup. - const existing = await this.entriesRepo.listForTripReconcile(journeyId, tripId); - const existingByKey = new Map(); - // Rows from before the assignment link existed, and any the backfill could not - // resolve. The place's earliest assignment claims one below, rather than the row - // being read as "no longer in the plan" and annotated out from under its author. - const unclaimed = new Map(); - for (const e of existing) { - if (e.source_place_id == null) continue; - if (e.source_assignment_id == null) { - const pool = unclaimed.get(e.source_place_id); - if (pool) pool.push(e); - else unclaimed.set(e.source_place_id, [e]); - } else { - existingByKey.set(skeletonKey(e.source_place_id, e.source_assignment_id), e); - } - } - - // Next sort_order per date for freshly inserted skeletons. - const dateMaxOrder = new Map(); - const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId); - for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m); - - // 1) Upsert a skeleton for every current day assignment. - for (const place of places) { - const entryDate = place.day_date || new Date().toISOString().split('T')[0]; - const entryTime = place.assignment_time || place.place_time || null; - const locationName = place.address || place.name; - const lat = place.lat || null; - const lng = place.lng || null; - let found = existingByKey.get(skeletonKey(place.id, place.assignment_id)); - - if (!found) { - // `places` is ordered by day, so the earliest assignment claims it. - const adopted = unclaimed.get(place.id)?.shift(); - if (adopted) { - await this.entriesRepo.claimAssignment(adopted.id, place.assignment_id ?? null); - adopted.source_assignment_id = place.assignment_id ?? null; - existingByKey.set(skeletonKey(place.id, place.assignment_id), adopted); - found = adopted; + let didChange = false; + // JG55 — a wider column set than JG35 (`listSourceKeysForTrip`), not a dup. + const existing = await this.entriesRepo.listForTripReconcile(journeyId, tripId); + const existingByKey = new Map(); + // Rows from before the assignment link existed, and any the backfill could not + // resolve. The place's earliest assignment claims one below, rather than the row + // being read as "no longer in the plan" and annotated out from under its author. + const unclaimed = new Map(); + for (const e of existing) { + if (e.source_place_id == null) continue; + if (e.source_assignment_id == null) { + const pool = unclaimed.get(e.source_place_id); + if (pool) pool.push(e); + else unclaimed.set(e.source_place_id, [e]); + } else { + existingByKey.set(skeletonKey(e.source_place_id, e.source_assignment_id), e); } } - if (!found) { - const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1; - dateMaxOrder.set(entryDate, nextOrder); - await this.insertSkeletonEntry({ - journeyId, - tripId, - placeId: place.id, - assignmentId: place.assignment_id ?? null, - authorId: ownerId, - title: place.name, - entryDate, - entryTime, - locationName, - lat, - lng, - sortOrder: nextOrder, - now, - }); - changed = true; - } else if (found.type === 'skeleton') { - // Skeletons follow the place's day/time/location snapshot. - const stale = - found.title !== place.name || - found.entry_date !== entryDate || - found.entry_time !== entryTime || - found.location_name !== locationName || - found.location_lat !== lat || - found.location_lng !== lng; - if (stale) { - await this.entriesRepo.updateSkeletonSnapshot(found.id, { + // Next sort_order per date for freshly inserted skeletons. + const dateMaxOrder = new Map(); + const maxRows = await this.entriesRepo.dateSortOrderMaxima(journeyId); + for (const row of maxRows) dateMaxOrder.set(row.entry_date, row.m); + + // 1) Upsert a skeleton for every current day assignment. + for (const place of places) { + const entryDate = place.day_date || new Date().toISOString().split('T')[0]; + const entryTime = place.assignment_time || place.place_time || null; + const locationName = place.address || place.name; + const lat = place.lat || null; + const lng = place.lng || null; + let found = existingByKey.get(skeletonKey(place.id, place.assignment_id)); + + if (!found) { + // `places` is ordered by day, so the earliest assignment claims it. + const adopted = unclaimed.get(place.id)?.shift(); + if (adopted) { + await this.entriesRepo.claimAssignment(adopted.id, place.assignment_id ?? null); + adopted.source_assignment_id = place.assignment_id ?? null; + existingByKey.set(skeletonKey(place.id, place.assignment_id), adopted); + found = adopted; + } + } + + if (!found) { + const nextOrder = (dateMaxOrder.get(entryDate) ?? -1) + 1; + dateMaxOrder.set(entryDate, nextOrder); + await this.insertSkeletonEntry({ + journeyId, + tripId, + placeId: place.id, + assignmentId: place.assignment_id ?? null, + authorId: ownerId, title: place.name, - entry_date: entryDate, - entry_time: entryTime, - location_name: locationName, - location_lat: lat, - location_lng: lng, - country_code: this.countryFor(lat, lng), - updated_at: now, + entryDate, + entryTime, + locationName, + lat, + lng, + sortOrder: nextOrder, + now, }); - changed = true; - } - } else { - // Filled entries keep the user's date/story; only location follows the place. - const stale = - found.location_name !== locationName || found.location_lat !== lat || found.location_lng !== lng; - if (stale) { - await this.entriesRepo.updateLocationOnly(found.id, { - location_name: locationName, - location_lat: lat, - location_lng: lng, - country_code: this.countryFor(lat, lng), - updated_at: now, - }); - changed = true; + didChange = true; + } else if (found.type === 'skeleton') { + // Skeletons follow the place's day/time/location snapshot. + const stale = + found.title !== place.name || + found.entry_date !== entryDate || + found.entry_time !== entryTime || + found.location_name !== locationName || + found.location_lat !== lat || + found.location_lng !== lng; + if (stale) { + await this.entriesRepo.updateSkeletonSnapshot(found.id, { + title: place.name, + entry_date: entryDate, + entry_time: entryTime, + location_name: locationName, + location_lat: lat, + location_lng: lng, + country_code: this.countryFor(lat, lng), + updated_at: now, + }); + didChange = true; + } + } else { + // Filled entries keep the user's date/story; only location follows the place. + const stale = + found.location_name !== locationName || found.location_lat !== lat || found.location_lng !== lng; + if (stale) { + await this.entriesRepo.updateLocationOnly(found.id, { + location_name: locationName, + location_lat: lat, + location_lng: lng, + country_code: this.countryFor(lat, lng), + updated_at: now, + }); + didChange = true; + } } } - } - // 2) Drop skeletons whose assignment is gone. One still waiting to be claimed is - // spared while its place is on the plan somewhere: having no link yet is not - // the same as the stop having left. - for (const e of existing) { - if (e.source_place_id == null) continue; - if (e.source_assignment_id == null && assignedPlaceIds.has(e.source_place_id)) continue; - if (e.source_assignment_id != null && assignedKeys.has(skeletonKey(e.source_place_id, e.source_assignment_id))) { - continue; - } - if (e.type === 'skeleton') { - const hasPhotos = await this.entriesRepo.existsPhotoForEntry(e.id); - if (!hasPhotos && !e.story) { - await this.entriesRepo.deleteById(e.id); - changed = true; + // 2) Drop skeletons whose assignment is gone. One still waiting to be claimed is + // spared while its place is on the plan somewhere: having no link yet is not + // the same as the stop having left. + for (const e of existing) { + if (e.source_place_id == null) continue; + if (e.source_assignment_id == null && assignedPlaceIds.has(e.source_place_id)) continue; + if (e.source_assignment_id != null && assignedKeys.has(skeletonKey(e.source_place_id, e.source_assignment_id))) { continue; } + if (e.type === 'skeleton') { + const hasPhotos = await this.entriesRepo.existsPhotoForEntry(e.id); + if (!hasPhotos && !e.story) { + await this.entriesRepo.deleteById(e.id); + didChange = true; + continue; + } + } + const note = '\n\n> _Note: the original trip place was removed from the trip plan_'; + const newStory = (e.story || '') + note; + await this.entriesRepo.detachAndAnnotate(e.id, { + type: e.type === 'skeleton' ? 'entry' : e.type, + story: newStory, + updated_at: now, + }); + didChange = true; } - const note = '\n\n> _Note: the original trip place was removed from the trip plan_'; - const newStory = (e.story || '') + note; - await this.entriesRepo.detachAndAnnotate(e.id, { - type: e.type === 'skeleton' ? 'entry' : e.type, - story: newStory, - updated_at: now, - }); - changed = true; - } + + return didChange; + }); if (changed) await this.broadcastJourneyEvent(journeyId, 'journey:trip:synced', { tripId }, sid); } @@ -1273,13 +1298,16 @@ export class JourneyDomainService { updated_at: [true, this.ts()], }); - // The legacy no-op check (`fields.length === 0`) fires exactly when NO - // allow-listed key of `data` carries a defined value — the country - // recompute and the type-promotion push are both themselves gated on - // keys already in that same set (`location_lat`/`location_lng`/`story`), - // so "any key of `data` is defined" is the identical condition. - const anyFieldProvided = Object.values(data).some((v) => v !== undefined); - if (!anyFieldProvided) return decodeEntryRow(entry); + // M3 (parity) — the legacy no-op check (`fields.length === 0`) fires + // exactly when NO allow-listed key made it into the dynamic SET list. + // `Object.values(data).some(v => v !== undefined)` is the WRONG mirror + // of that: `data` is `journeyEntryUpdateRequestSchema`'s `z.looseObject`, + // so a PATCH body carrying only non-allow-listed keys (`{"foo":1}`) has + // a defined value there and this used to write and broadcast anyway. + // `patch` (built above by `presenceSet`) always carries `updated_at` + // unconditionally, so its length is 1 exactly when nothing allow-listed + // was provided — the same condition the legacy field-count check tested. + if (Object.keys(patch).length === 1) return decodeEntryRow(entry); await this.entriesRepo.updateFields(entryId, patch); diff --git a/server/src/nest/journey/journey-share.service.ts b/server/src/nest/journey/journey-share.service.ts index 2ac5ccb5b..9db0c3e5e 100644 --- a/server/src/nest/journey/journey-share.service.ts +++ b/server/src/nest/journey/journey-share.service.ts @@ -4,6 +4,7 @@ import crypto from 'crypto'; import { JourneyDomainService } from './journey-domain.service'; import { decodeEntryRow } from './journey-entry-row'; import { SettingsService } from '../settings/settings.service'; +import { UnitOfWork } from '../database/unit-of-work'; import { Journeys } from '../../db/entities/Journeys.entity'; import type { JourneysRepository } from '../../db/repositories/Journeys.repository'; import { JourneyShareTokens } from '../../db/entities/JourneyShareTokens.entity'; @@ -11,6 +12,8 @@ import type { JourneyPublicEntryPhotoRow, JourneyShareTokensRepository, } from '../../db/repositories/JourneyShareTokens.repository'; +import { JourneyPhotos } from '../../db/entities/JourneyPhotos.entity'; +import type { JourneyPhotosRepository } from '../../db/repositories/JourneyPhotos.repository'; interface JourneySharePermissions { share_timeline?: boolean; @@ -47,6 +50,17 @@ export class JourneyShareService { // already-stable `JourneysRepository.findById` (the JG5 dup group) rather // than a second, hand-kept copy of the same statement. @InjectRepository(Journeys) private readonly journeysRepo: JourneysRepository, + // Fix-wave additions (task-5-review.md): + // - `uow` (L1): `createOrUpdateJourneyShareLink`'s existing-link read and + // its insert are separate awaits now, so two concurrent first-creates + // for the same journey could both read "no link yet" and both insert — + // the second hits `UNIQUE(journey_id)` and 500s instead of returning + // `{created:false}` like the legacy synchronous path always did. + // - `photosRepo` (L2): `getPublicJourney`'s gallery read (JS15) reuses + // `JourneyPhotosRepository.galleryRead` instead of carrying its own + // byte-identical copy of the ORDER BY builder and the gallery query. + private readonly uow: UnitOfWork, + @InjectRepository(JourneyPhotos) private readonly photosRepo: JourneyPhotosRepository, ) {} async createOrUpdateJourneyShareLink( @@ -58,45 +72,55 @@ export class JourneyShareService { // able to publish the journey or change which screens are shared. if (!(await this.journey.isOwner(journeyId, createdBy))) return null; - // JS1 — `JourneyShareTokensRepository.findFlagsByJourneyId`. - const existing = await this.shareTokensRepo.findFlagsByJourneyId(journeyId); + // L1 (rule 11/24) — the existing-link read and the insert below are + // separate awaits, so two concurrent first-creates for the same journey + // could both read "no link yet" and both try to insert: the loser hits + // `UNIQUE(journey_id)` and throws (a 500 over HTTP) instead of the + // legacy's `{created:false}` outcome. Wrapped whole, DB-only, same fix + // as M1: the mutex serializes the second caller's read behind the + // first's commit, so it finds the row the first just created and takes + // the UPDATE branch instead, exactly like the synchronous legacy path. + return this.uow.transactional(async () => { + // JS1 — `JourneyShareTokensRepository.findFlagsByJourneyId`. + const existing = await this.shareTokensRepo.findFlagsByJourneyId(journeyId); - if (existing) { - // An update only changes the flags it was actually given. Falling back to - // the create-time defaults here would silently re-publish a gallery or map - // the owner had switched off, at the unchanged token. - const share_timeline = permissions.share_timeline ?? !!existing.share_timeline; - const share_gallery = permissions.share_gallery ?? !!existing.share_gallery; - const share_map = permissions.share_map ?? !!existing.share_map; - const newest_first = permissions.newest_first ?? !!existing.newest_first; - // JS2 — the SERVICE resolves the final value of every flag (above); the - // repository writes exactly those four already-resolved booleans, a - // plain 4-column UPDATE, not a presence-sentinel one. - await this.shareTokensRepo.updateFlags(journeyId, { + if (existing) { + // An update only changes the flags it was actually given. Falling back to + // the create-time defaults here would silently re-publish a gallery or map + // the owner had switched off, at the unchanged token. + const share_timeline = permissions.share_timeline ?? !!existing.share_timeline; + const share_gallery = permissions.share_gallery ?? !!existing.share_gallery; + const share_map = permissions.share_map ?? !!existing.share_map; + const newest_first = permissions.newest_first ?? !!existing.newest_first; + // JS2 — the SERVICE resolves the final value of every flag (above); the + // repository writes exactly those four already-resolved booleans, a + // plain 4-column UPDATE, not a presence-sentinel one. + await this.shareTokensRepo.updateFlags(journeyId, { + share_timeline: share_timeline ? 1 : 0, + share_gallery: share_gallery ? 1 : 0, + share_map: share_map ? 1 : 0, + newest_first: newest_first ? 1 : 0, + }); + return { token: existing.token, created: false }; + } + + const { + share_timeline = true, + share_gallery = true, + share_map = true, + newest_first = false, + } = permissions; + + const token = crypto.randomBytes(24).toString('base64url'); + // JS3. + await this.shareTokensRepo.insertLink(journeyId, token, createdBy, { share_timeline: share_timeline ? 1 : 0, share_gallery: share_gallery ? 1 : 0, share_map: share_map ? 1 : 0, newest_first: newest_first ? 1 : 0, }); - return { token: existing.token, created: false }; - } - - const { - share_timeline = true, - share_gallery = true, - share_map = true, - newest_first = false, - } = permissions; - - const token = crypto.randomBytes(24).toString('base64url'); - // JS3. - await this.shareTokensRepo.insertLink(journeyId, token, createdBy, { - share_timeline: share_timeline ? 1 : 0, - share_gallery: share_gallery ? 1 : 0, - share_map: share_map ? 1 : 0, - newest_first: newest_first ? 1 : 0, + return { token, created: true }; }); - return { token, created: true }; } /** @@ -194,8 +218,10 @@ export class JourneyShareService { (photosByEntry[p.entry_id] ||= []).push(p); } - // JS15 (R1's second `GALLERY_CHRONOLOGICAL_ORDER` site). - const gallery = await this.shareTokensRepo.listGalleryForPublicJourney(row.journey_id); + // JS15 (R1's second `GALLERY_CHRONOLOGICAL_ORDER` site) — L2: reuses + // `JourneyPhotosRepository.galleryRead` (same `GALLERY_COLUMNS`, same + // ORDER BY) rather than a second, hand-kept copy of the same query. + const gallery = await this.photosRepo.galleryRead(row.journey_id); const enrichedEntries = entries .map(e => ({ diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 9e7f4702b..7eaf43322 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -125,6 +125,7 @@ import { createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo, createTestTripMembersRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, createTestRoadtripViasRepo, createTestRoadtripDayTracksRepo, + createTestPlaceDetailsCacheRepo, createTestReservationsRepo, createTestReservationEndpointsRepo, createTestReservationTravelersRepo, @@ -251,7 +252,7 @@ export async function createMcpTestRegistry(): Promise { // Exactly one instance, shared by maps, places and share: its stampede guard // and its on-disk set only work if all three readers see the same maps. const placePhotoCache = new PlacePhotoCacheService(dbService, makeStorageFixture('photos/google/').storage, await createTestGooglePlacePhotoMetaRepo(dbService.connection), await createTestPlacesRepo(dbService.connection)); - const mapsService = new MapsService(dbService, placePhotoCache, appSettings, usersRepo); + const mapsService = new MapsService(placePhotoCache, appSettings, usersRepo, await createTestPlaceDetailsCacheRepo(dbService.connection), await createTestPlacesRepo(dbService.connection)); const journeyDomain = new JourneyDomainService( dbService, realtimeService, new TrekPhotoRegistrationService(mcpOrm.repo(TrekPhotos), mcpOrm.repo(TripPhotos), await createTestJourneyPhotosRepo(dbService.connection), dbService), await createTestUnitOfWork(dbService.connection), // Plan 3g Task 1 — the constructor-ripple fix (R9): four journey-owned @@ -472,11 +473,16 @@ export async function createMcpTestRegistry(): Promise { // Plan 3g Task 3 — the constructor-ripple fix: `JourneyShareTokensRepository` // (JS1-JS15) + the already-shared `JourneysRepository` (JS8/JS12). await createTestJourneyShareTokensRepo(dbService.connection), await createTestJourneysRepo(dbService.connection), + // task-5-fix-brief constructor-ripple: `UnitOfWork` (L1) + `JourneyPhotosRepository` (L2). + await createTestUnitOfWork(dbService.connection), await createTestJourneyPhotosRepo(dbService.connection), ), addonsService, authService, captureBackfill), new MemoriesMcp(immichService, synologyService, addonsService, mcpOrm.repo(PhotoProviders)), new NotificationsMcp(await makeNotificationsService(dbService, realtimeService), authService), - new AirtrailMcp(new AirtrailService(dbService, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()), addonsService), - new ReservationImportMcp(new AirtrailImportService(dbService, realtimeService, reservationsService, new AirtrailClient(), new AirtrailService(dbService, new AuditService(auditLogRepo, usersRepo), new AirtrailClient())), dbService, authService, guards, addonsService), + new AirtrailMcp(new AirtrailService(usersRepo, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()), addonsService), + new ReservationImportMcp(new AirtrailImportService( + await createTestReservationsRepo(dbService.connection), await createTestReservationEndpointsRepo(dbService.connection), await createTestDaysRepo(dbService.connection), + realtimeService, reservationsService, new AirtrailClient(), new AirtrailService(usersRepo, new AuditService(auditLogRepo, usersRepo), new AirtrailClient()), + ), dbService, authService, guards, addonsService), new SettingsMcp(new SettingsService(await createTestUnitOfWork(dbService.connection), appSettings, await createTestSettingsRepo(dbService.connection)), authService), new HelpMcp(), new AddonsMcp(addonsService), new TripWarningsMcp(new PluginHooks({ providersOf: () => [], invokeHook: async () => [] } as unknown as PluginRuntimeService), dbService), diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index a1af862d6..f98320fe0 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -81,6 +81,7 @@ import { createTestDaysRepo, createTestDayAssignmentsRepo, createTestDayNotesRepo, createTestTripsRepo, createTestTripMembersRepo, createTestPlaceRatingsRepo, createTestAssignmentParticipantsRepo, createTestGooglePlacePhotoMetaRepo, createTestPlacesRepo, createTestRoadtripViasRepo, + createTestPlaceDetailsCacheRepo, createTestReservationsRepo, createTestReservationEndpointsRepo, createTestReservationTravelersRepo, @@ -253,7 +254,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< ); // After it: deleting a place cancels the nights booked at it through this one. const places = new PlacesService( - dbs, permissions, realtime, new MapsService(dbs, photoCache, appSettings, usersRepo), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection), + dbs, permissions, realtime, new MapsService(photoCache, appSettings, usersRepo, await createTestPlaceDetailsCacheRepo(dbs.connection), await createTestPlacesRepo(dbs.connection)), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection), await createTestPlacesRepo(dbs.connection), await createTestTagsRepo(dbs.connection), await createTestPlaceRatingsRepo(dbs.connection), diff --git a/server/tests/unit/nest/journey-book.service.test.ts b/server/tests/unit/nest/journey-book.service.test.ts index a26789784..e3e762f6f 100644 --- a/server/tests/unit/nest/journey-book.service.test.ts +++ b/server/tests/unit/nest/journey-book.service.test.ts @@ -81,7 +81,9 @@ beforeAll(async () => { await createTestJourneyPhotosRepo(testDb), await createTestJourneyEntryPhotosRepo(testDb), await createTestPlacesRepo(testDb), ); // Plan 3g Task 3: JourneyBooksRepository (JB1-JB7), not `dbs` any more. - books = new JourneyBookService(domain, await createTestJourneyBooksRepo(testDb)); + // task-5-fix-brief constructor-ripple (M1): `UnitOfWork`, so `saveBook`'s + // read-then-insert can be wrapped in one transaction. + books = new JourneyBookService(domain, await createTestJourneyBooksRepo(testDb), uow); }); beforeEach(() => { @@ -351,6 +353,37 @@ describe('concurrency', () => { const afterMutation = testDb.prepare('SELECT title FROM journey_books WHERE id = ?').get(row.id) as { title: string }; expect(afterMutation.title).toBe('Clobbered'); }); + + // M1 (task-5-review.md) — JB3's existing-link read and JB4's insert used + // to be two separate awaits, so two concurrent FIRST saves of a journey + // with no book yet could both read "no book" and both insert, leaving one + // editor's save orphaned behind `getBook`'s `ORDER BY id LIMIT 1` (there is + // no unique index on `journey_id`). Races two REAL `await Promise.all([...])` + // callers through the actual service on the shared connection, then + // asserts the base's outcome: exactly one row, and the loser lands on the + // JB5 CAS-update path (version 2), not a second insert. + it('journey-book-svc M1: two concurrent first saves of a journey with no book — exactly one row, ending at version 2', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + const [a, b] = await Promise.all([ + books.saveBook(journey.id, user.id, { title: 'A', document: doc('a') }), + books.saveBook(journey.id, user.id, { title: 'B', document: doc('b') }), + ]); + + expect(a).not.toBeNull(); + expect(b).not.toBeNull(); + // One of the two lands as the first insert (version 1, before the + // second's own read observes it and takes the CAS-update branch); the + // other either also lands as {record} (version 2, having taken the + // update branch against baseVersion undefined -> existing.version) or + // sees a stale conflict — either way, never a second inserted row. + const rows = testDb.prepare('SELECT COUNT(*) AS n FROM journey_books WHERE journey_id = ?').get(journey.id) as { n: number }; + expect(rows.n).toBe(1); + + const finalVersion = (testDb.prepare('SELECT version FROM journey_books WHERE journey_id = ?').get(journey.id) as { version: number }).version; + expect(finalVersion).toBe(2); + }); }); // -- Broadcasting ------------------------------------------------------------- diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index 5a2e2a2c6..19799512d 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -60,6 +60,10 @@ import { createTestJourneysRepo, createTestJourneyContributorsRepo, createTestJourneyTripsRepo, createTestJourneyEntriesRepo, createTestJourneyPhotosRepo, createTestJourneyEntryPhotosRepo, } from '../../helpers/journey-repos'; +// M5d (task-5-review.md) — full-key parity tests for Task 2's read models, +// ported from the reviewer's probe (scratchpad r3g/probe/zz-r3g-parity-probe.test.ts). +import { todayUtc } from '@trek/shared'; +import { GALLERY_CHRONOLOGICAL_ORDER } from '../../../src/nest/journey/journey-gallery-order'; let dbs: DatabaseService; let svc: JourneyDomainService; @@ -701,6 +705,36 @@ describe('updateEntry', () => { expect((await svc.updateEntry(entry!.id, user.id, { title: 'Renamed' }))!.pros_cons).toEqual(prosCons); }); + // L3 (task-5-review.md, user decision: KEEP HEAD's behaviour, deliberate + // deviation FOR THE USER) — base stored the raw non-array `tags`/non-object + // `pros_cons` value as-is and then 500ed on every later read that tried to + // JSON.parse it, leaving the entry permanently unreadable; base also 500s + // outright on a `false` bind. HEAD writes SQL NULL instead and answers 200, + // which is the narrowing that fixes that legacy defect. Pinned here so a + // future change to this branch is a deliberate decision, not an accident. + it('L3: a non-array tags / non-object pros_cons patch writes NULL and returns 200, rather than the raw value HEAD used to store (a legacy defect, not reproduced)', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, journey.id, user.id, { entry_date: '2026-03-01' }); + + const updated = await svc.updateEntry(entry.id, user.id, { + tags: 'not-an-array' as unknown as string[], + pros_cons: false as unknown as { pros: string[]; cons: string[] }, + }); + + expect(updated).not.toBeNull(); + expect(updated!.tags).toEqual([]); + expect(updated!.pros_cons).toBeNull(); + + const row = testDb.prepare('SELECT tags, pros_cons FROM journey_entries WHERE id = ?').get(entry.id) as { tags: string | null; pros_cons: string | null }; + expect(row.tags).toBeNull(); + expect(row.pros_cons).toBeNull(); + + // And the entry stays readable afterwards — no 500 on the next read, + // which is exactly the defect this deviation fixes. + expect(await svc.listEntries(journey.id, user.id)).not.toBeNull(); + }); + /* An update that changes nothing takes its own early return out of the method. */ it('decodes on the no-op update too', async () => { const { user } = createUser(testDb); @@ -808,6 +842,37 @@ describe('updateEntry', () => { expect(updated!.story).toBe('original'); // injection key dropped — no hash leaked into story expect(updated!.author_id).toBe(user.id); // mass-assignment blocked }); + + // M3 (task-5-review.md) — a patch carrying ONLY non-allow-listed keys (no + // legit field at all, unlike JOURNEY-SVC-034b above which always mixes in + // a real `title`) used to be treated as "something was provided" because + // the old no-op check read `Object.values(data).some(v => v !== undefined)` + // off the RAW request body, not off the allow-listed `patch` presenceSet + // actually builds. Base: no write, no `updated_at` bump on either row, no + // broadcast. HEAD (before this fix): stamped both `updated_at`s and + // broadcast anyway. + it('JOURNEY-SVC-M3-01: a patch with only non-allow-listed keys is a no-op — no write, no updated_at bump, no broadcast', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, journey.id, user.id, { title: 'Original', entry_date: '2026-03-01' }); + + const updateFieldsSpy = vi.spyOn(entriesRepoDirect, 'updateFields'); + const journeyUpdateSpy = vi.spyOn(journeysRepoDirect, 'updateFields'); + const broadcastSpy = vi.spyOn(RealtimeService.prototype, 'broadcastToUser').mockImplementation(() => {}); + try { + const updated = await svc.updateEntry(entry.id, user.id, { foo: 1 } as unknown as Parameters[2]); + + expect(updated).not.toBeNull(); + expect(updated!.title).toBe('Original'); + expect(updateFieldsSpy).not.toHaveBeenCalled(); + expect(journeyUpdateSpy).not.toHaveBeenCalled(); + expect(broadcastSpy).not.toHaveBeenCalled(); + } finally { + updateFieldsSpy.mockRestore(); + journeyUpdateSpy.mockRestore(); + broadcastSpy.mockRestore(); + } + }); }); describe('deleteEntry', () => { @@ -1029,6 +1094,27 @@ describe('addContributor / updateContributorRole / removeContributor', () => { expect(result).toBe(false); }); + // JG118 (task-5-review.md, user decision: pin, do NOT fix) — a PRE-EXISTING + // hole preserved on purpose (also present at legacy base): nothing here or + // in SQL stops the owner from writing `role = 'owner'` through this path — + // only the TypeScript parameter type discourages it, and that boundary is + // bypassable from a raw HTTP/MCP body. The hardening line (refuse unless + // `role === 'editor' || role === 'viewer'`) goes in the report FOR THE + // USER; this test pins today's behaviour so a future fix is a deliberate + // change, not a silent one. + it('JOURNEY-SVC-JG118-PIN: updateContributorRole still accepts role "owner" through the typed path (pre-existing, not fixed by this wave)', async () => { + const { user: owner } = createUser(testDb); + const { user: target } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + addJourneyContributor(testDb, journey.id, target.id, 'editor'); + + const result = await svc.updateContributorRole(journey.id, owner.id, target.id, 'owner' as unknown as 'editor' | 'viewer'); + + expect(result).toBe(true); + const row = testDb.prepare('SELECT role FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(journey.id, target.id) as { role: string }; + expect(row.role).toBe('owner'); + }); + it('JOURNEY-SVC-050: owner can remove contributor', async () => { const { user: owner } = createUser(testDb); const { user: contrib } = createUser(testDb); @@ -1059,6 +1145,109 @@ describe('addContributor / updateContributorRole / removeContributor', () => { }); }); +// M5c (task-5-review.md) — R3's reset-column trap on `journey_contributors`'s +// composite PK: the legacy `INSERT OR REPLACE` deletes and re-inserts on a +// conflict, so `hide_skeletons` (not in the column list `upsertContributor` +// writes on purpose) resets to its table default (0) — verified live by W2's +// probe, but until now pinned only as report text (the ledger's "upsert SQL +// pinned = the reference" claim did not hold). Mutation M10 (drop the +// explicit `hide_skeletons: 0`) stayed green with no test to catch it. +describe('R3 composite-PK writes — reset-on-conflict behaviour and rendered SQL (M5c)', () => { + it('JOURNEY-SVC-M5C-01: re-adding a contributor (upsertContributor merge branch) resets hide_skeletons to 0', async () => { + const { user: owner } = createUser(testDb); + const { user: contrib } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + + await svc.addContributor(journey.id, owner.id, contrib.id, 'editor'); + await svc.updateJourneyPreferences(journey.id, contrib.id, { hide_skeletons: true }); + expect( + (testDb.prepare('SELECT hide_skeletons FROM journey_contributors WHERE journey_id = ? AND user_id = ?').get(journey.id, contrib.id) as { hide_skeletons: number }).hide_skeletons, + ).toBe(1); + + // Re-add (same target, a possibly different role) — the ON CONFLICT + // merge branch of upsertContributor, exercised through the real service. + await svc.addContributor(journey.id, owner.id, contrib.id, 'viewer'); + + const row = testDb + .prepare('SELECT role, hide_skeletons FROM journey_contributors WHERE journey_id = ? AND user_id = ?') + .get(journey.id, contrib.id) as { role: string; hide_skeletons: number }; + expect(row.role).toBe('viewer'); + expect(row.hide_skeletons).toBe(0); + }); + + it('JOURNEY-SVC-M5C-02: upsertContributor (merge-shaped) renders an ON CONFLICT over the (journey_id, user_id) composite PK', async () => { + const { user: owner } = createUser(testDb); + const { user: contrib } = createUser(testDb); + const journey = createJourney(testDb, owner.id); + + const connection = contributorsRepoDirect.getEntityManager().getConnection(); + const spy = vi.spyOn(connection, 'execute'); + try { + await contributorsRepoDirect.upsertContributor(journey.id, contrib.id, 'editor', Date.now()); + const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql)); + expect(insertSql).toBeDefined(); + expect(insertSql).toMatch(/on conflict \(`journey_id`, ?`user_id`\)/i); + expect(insertSql).toMatch(/do update set/i); + } finally { + spy.mockRestore(); + } + }); + + it('JOURNEY-SVC-M5C-03: JourneyTripsRepository.insertIgnore (ignore-shaped) renders an ON CONFLICT ... DO NOTHING over (journey_id, trip_id)', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const trip = createTrip(testDb, user.id); + + const connection = journeyTripsRepoDirect.getEntityManager().getConnection(); + const spy = vi.spyOn(connection, 'execute'); + try { + await journeyTripsRepoDirect.insertIgnore(journey.id, trip.id, Date.now()); + const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql)); + expect(insertSql).toBeDefined(); + expect(insertSql).toMatch(/on conflict \(`journey_id`, ?`trip_id`\)/i); + expect(insertSql).toMatch(/do nothing/i); + } finally { + spy.mockRestore(); + } + }); + + it('JOURNEY-SVC-M5C-04: JourneyEntryPhotosRepository.insertIgnore (ignore-shaped) renders an ON CONFLICT ... DO NOTHING over (entry_id, journey_photo_id)', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + const entry = createJourneyEntry(testDb, journey.id, user.id); + const trekResult = testDb + .prepare('INSERT INTO trek_photos (provider, file_path, owner_id, created_at) VALUES (?, ?, ?, ?)') + .run('local', '/photos/m5c.jpg', user.id, Date.now()); + const trekId = trekResult.lastInsertRowid as number; + const jpResult = testDb + .prepare('INSERT INTO journey_photos (journey_id, photo_id, sort_order, created_at) VALUES (?, ?, ?, ?)') + .run(journey.id, trekId, 0, Date.now()); + const journeyPhotoId = jpResult.lastInsertRowid as number; + + const connection = entryPhotosRepoDirect.getEntityManager().getConnection(); + const spy = vi.spyOn(connection, 'execute'); + try { + await entryPhotosRepoDirect.insertIgnore(entry.id, journeyPhotoId, 0, Date.now()); + const insertSql = spy.mock.calls.map(([sql]) => sql).find((sql): sql is string => typeof sql === 'string' && /insert into/i.test(sql)); + expect(insertSql).toBeDefined(); + expect(insertSql).toMatch(/on conflict \(`entry_id`, ?`journey_photo_id`\)/i); + expect(insertSql).toMatch(/do nothing/i); + } finally { + spy.mockRestore(); + } + }); +}); + +// M4 (task-5-review.md) — `JourneysRepository.listRecipientUserIds`'s +// `if (ownerId !== undefined) ids.add(ownerId)` null arm had no test where +// `findOwnerId` actually returns undefined (a missing journey), so the +// branch went uncovered. +describe('JourneysRepository.listRecipientUserIds — missing journey (M4)', () => { + it('returns an empty array for a journey id that does not exist (findOwnerId undefined arm)', async () => { + expect(await journeysRepoDirect.listRecipientUserIds(999_999)).toEqual([]); + }); +}); + // -- Suggestions -------------------------------------------------------------- describe('getSuggestions', () => { @@ -1935,6 +2124,48 @@ describe('reconcileTripSkeletons', () => { const anyEntry = testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries').get() as { n: number }; expect(anyEntry.n).toBe(0); }); + + // -- M2 (task-5-review.md) — the sync engine's existence-check-then-insert + // used to be two separate awaits, so two concurrent callers could both see + // "not there yet" and both insert a skeleton for the same (place, + // assignment). Races two REAL `await Promise.all([...])` callers through + // the actual service on the shared connection, then asserts the base's + // outcome: exactly one skeleton per (place, assignment), never two. + describe('concurrency (M2) — exactly one skeleton per (place, assignment) under a race', () => { + it('two concurrent reconcileTripSkeletons(trip) calls after two new assignments produce exactly one skeleton per place, not two', async () => { + const { journey, trip } = await linkedJourneyTrip(); + const days = daysOf(trip.id); + const placeA = createPlace(testDb, trip.id, { name: 'Race Place A' }); + const placeB = createPlace(testDb, trip.id, { name: 'Race Place B' }); + createDayAssignment(testDb, days[0].id, placeA.id); + createDayAssignment(testDb, days[1].id, placeB.id); + + await Promise.all([ + svc.reconcileTripSkeletons(trip.id), + svc.reconcileTripSkeletons(trip.id), + ]); + + const countFor = (placeId: number) => + (testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries WHERE journey_id = ? AND source_place_id = ?').get(journey.id, placeId) as { n: number }).n; + expect(countFor(placeA.id)).toBe(1); + expect(countFor(placeB.id)).toBe(1); + }); + + it('onPlaceCreated racing reconcileTripSkeletons for the same new assignment produces exactly one skeleton, not two', async () => { + const { journey, trip } = await linkedJourneyTrip(); + const days = daysOf(trip.id); + const place = createPlace(testDb, trip.id, { name: 'Race Place C' }); + createDayAssignment(testDb, days[0].id, place.id); + + await Promise.all([ + svc.onPlaceCreated(trip.id, place.id), + svc.reconcileTripSkeletons(trip.id), + ]); + + const count = (testDb.prepare('SELECT COUNT(*) AS n FROM journey_entries WHERE journey_id = ? AND source_place_id = ?').get(journey.id, place.id) as { n: number }).n; + expect(count).toBe(1); + }); + }); }); // -- the same place on two days (#2329) --------------------------------------- @@ -2955,6 +3186,188 @@ describe('Plan 3g Task 1 — repository parity (full-key toEqual against the leg }); }); +// M5d (task-5-review.md) — Task 2's read models had no full-key parity +// tests: mutations dropping `lng` from JG15 (M19) and from JG19's gallery +// read (M20) stayed green. Ported from the reviewer's own probe +// (`scratchpad/r3g/probe/zz-r3g-parity-probe.test.ts`, which the review +// names as the template), turned from console.log(OK/DIFF) reports into +// real `toEqual` assertions against the legacy statement run raw on the +// SAME seeded rows. One journey seeded with every tier this plan's read +// models touch: dated + undated trips, a multi-day place, an unattached +// gallery photo, ties on `sort_order`/`taken_at`, a video (excluded from +// the "photograph per stop" read), a dismissed entry, and a second, +// unrelated journey's photo (must never leak in). +describe('Plan 3g Task 2 — repository parity (full-key toEqual against the legacy statement, M5d)', () => { + async function seedParityJourney() { + const { user: owner } = createUser(testDb); + const { user: member } = createUser(testDb); + const d = (n: number) => new Date(Date.now() - n * 86400000).toISOString().slice(0, 10); + const t1 = createTrip(testDb, owner.id, { title: 'T1', start_date: '2026-01-01', end_date: '2026-01-03' }); + const t2 = createTrip(testDb, owner.id, { title: 'T2 undated' }); + const t3 = createTrip(testDb, member.id, { title: 'T3 recent', start_date: d(10), end_date: d(5) }); + const t4 = createTrip(testDb, owner.id, { title: 'T4 recent', start_date: d(12), end_date: d(2) }); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(t3.id, owner.id); + const days = testDb.prepare('SELECT id, date FROM days WHERE trip_id = ? ORDER BY date').all(t1.id) as { id: number }[]; + const pA = createPlace(testDb, t1.id, { name: 'A' }); + const pB = createPlace(testDb, t1.id, { name: 'B', lat: 1, lng: 2 }); + createPlace(testDb, t1.id, { name: 'C unassigned' }); + const pD = createPlace(testDb, t2.id, { name: 'D' }); + testDb.prepare("UPDATE places SET route_geometry = '[[1,2],[3,4]]', route_color = '#f00' WHERE id IN (?, ?)").run(pA.id, pD.id); + createDayAssignment(testDb, days[1].id, pA.id); + createDayAssignment(testDb, days[0].id, pA.id); + createDayAssignment(testDb, days[0].id, pB.id); + createDayAssignment(testDb, days[2].id, pB.id, { order_index: 0 }); + const place4 = createPlace(testDb, t4.id, { name: 'X' }); + const d4 = createDay(testDb, t4.id, { date: d(3) }); + createDayAssignment(testDb, d4.id, place4.id); + const journey = createJourney(testDb, owner.id); + for (const tid of [t1.id, t2.id]) { + testDb.prepare('INSERT INTO journey_trips (journey_id, trip_id, added_at) VALUES (?,?,1)').run(journey.id, tid); + await svc.syncTripPlaces(journey.id, tid, owner.id); + } + const now = Date.now(); + const ins = testDb.prepare( + `INSERT INTO journey_entries (journey_id, author_id, type, title, story, entry_date, entry_time, location_name, location_lat, location_lng, sort_order, created_at, updated_at, stats_excluded, dismissed) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, + ); + const e1 = Number(ins.run(journey.id, owner.id, 'entry', 'E1', 's', '2026-01-02', '10:00', 'Paris', 48.8, 2.3, 0, now, now, 0, 0).lastInsertRowid); + const e2 = Number(ins.run(journey.id, owner.id, 'entry', 'E2', null, '2026-01-02', '', null, null, null, 0, now, now, 1, 0).lastInsertRowid); + const e3 = Number(ins.run(journey.id, owner.id, 'entry', 'E3 dismissed', null, '2026-01-01', null, 'X', null, null, 1, now, now, 0, 1).lastInsertRowid); + const e4 = Number(ins.run(journey.id, owner.id, 'entry', 'E4', null, '2025-12-31', null, 'Paris', 1, 1, 5, now, now, 0, 0).lastInsertRowid); + const tp = testDb.prepare( + `INSERT INTO trek_photos (provider, asset_id, owner_id, file_path, thumbnail_path, width, height, media_type, taken_at, lat, lng) VALUES (?,?,?,?,?,?,?,?,?,?,?)`, + ); + const gp = testDb.prepare(`INSERT INTO journey_photos (journey_id, photo_id, caption, shared, sort_order, provider, asset_id, owner_id, created_at) VALUES (?,?,?,?,?,?,?,?,?)`); + const jep = testDb.prepare(`INSERT INTO journey_entry_photos (entry_id, journey_photo_id, sort_order, created_at) VALUES (?,?,?,?)`); + // taken_at/media_type combinations: null, empty string, two ties at the + // same taken_at (one video — excluded from listFirstPhotoPerEntry), and + // a null media_type (treated as 'image'). + const specs: [string | null, string | null][] = [ + [null, 'image'], ['', 'image'], ['2026-01-02T08:00:00', 'image'], ['2026-01-02T08:00:00', 'video'], + [null, 'image'], ['', null], [null, 'image'], + ]; + const gids: number[] = []; + specs.forEach(([taken, mt], i) => { + const pid = Number( + tp.run('local', null, owner.id, `f${i}.jpg`, i % 2 ? null : `t${i}.jpg`, 100, i % 3 ? null : 50, mt ?? 'image', taken, i % 2 ? 1.5 : null, i % 2 ? 2.5 : null).lastInsertRowid, + ); + gids.push(Number(gp.run(journey.id, pid, i % 2 ? `cap${i}` : null, i % 2, i % 3, null, null, null, 1700000000000 + (i % 4) * 1000).lastInsertRowid)); + }); + jep.run(e1, gids[0], 0, now); jep.run(e1, gids[1], 0, now); jep.run(e1, gids[2], 0, now); + jep.run(e2, gids[0], 1, now); jep.run(e2, gids[3], 0, now); + jep.run(e4, gids[5], 2, now); jep.run(e4, gids[4], 2, now); + jep.run(e3, gids[6], 0, now); + // An unrelated journey's photo — must never leak into this journey's reads. + const other = createJourney(testDb, member.id); + const opid = Number(tp.run('local', null, member.id, 'o.jpg', null, null, null, 'image', null, null, null).lastInsertRowid); + gp.run(other.id, opid, null, 0, 0, null, null, null, 1); + + return { owner, member, journey }; + } + + const LEG = { + JG15: `SELECT gp.id, jep.entry_id, gp.photo_id, gp.caption, jep.sort_order, gp.shared, gp.created_at, + tp.provider, tp.asset_id, tp.owner_id, tp.file_path, tp.thumbnail_path, tp.width, tp.height, + tp.media_type, tp.duration_ms, tp.taken_at, tp.lat, tp.lng FROM journey_entry_photos jep + JOIN journey_photos gp ON gp.id = jep.journey_photo_id + JOIN trek_photos tp ON tp.id = gp.photo_id WHERE jep.entry_id IN (SELECT id FROM journey_entries WHERE journey_id = ?) ORDER BY jep.sort_order ASC`, + JG19: `SELECT gp.id, gp.journey_id, gp.photo_id, gp.caption, gp.shared, gp.sort_order, gp.created_at, + tp.provider, tp.asset_id, tp.owner_id, tp.file_path, tp.thumbnail_path, tp.width, tp.height, + tp.media_type, tp.duration_ms, tp.taken_at, tp.lat, tp.lng FROM journey_photos gp JOIN trek_photos tp ON tp.id = gp.photo_id WHERE gp.journey_id = ? ${GALLERY_CHRONOLOGICAL_ORDER}`, + STATS_ENTRIES: `SELECT id, title, location_name, location_lat, location_lng, entry_date, source_trip_id, source_place_id, stats_excluded FROM journey_entries WHERE journey_id = ? AND dismissed = 0 ORDER BY entry_date ASC, sort_order ASC, id ASC`, + STATS_TRIPS: `SELECT t.id, t.title, t.start_date AS start, t.end_date AS end FROM journey_trips jt JOIN trips t ON t.id = jt.trip_id WHERE jt.journey_id = ? ORDER BY t.start_date IS NULL, t.start_date ASC, t.id ASC`, + STATS_PLACES: `SELECT p.id, p.name, p.lat, p.lng, p.trip_id AS tripId, MIN(d.date) AS day, MIN(da.order_index) AS ord FROM journey_trips jt JOIN places p ON p.trip_id = jt.trip_id LEFT JOIN day_assignments da ON da.place_id = p.id LEFT JOIN days d ON d.id = da.day_id WHERE jt.journey_id = ? GROUP BY p.id ORDER BY day IS NULL, day ASC, ord ASC, p.id ASC`, + STATS_PLACECOUNT: `SELECT COUNT(*) AS n FROM journey_trips jt JOIN places p ON p.trip_id = jt.trip_id WHERE jt.journey_id = ?`, + STATS_PHOTOCOUNT: `SELECT COUNT(*) AS n FROM journey_photos WHERE journey_id = ?`, + STATS_EPHOTOS: `SELECT jep.entry_id AS entryId, gp.photo_id AS photoId FROM journey_entry_photos jep JOIN journey_photos gp ON gp.id = jep.journey_photo_id JOIN trek_photos tp ON tp.id = gp.photo_id WHERE gp.journey_id = ? AND (tp.media_type IS NULL OR tp.media_type = 'image') ORDER BY jep.entry_id ASC, jep.sort_order ASC, gp.sort_order ASC, gp.id ASC`, + TRACKS: `SELECT DISTINCT p.id AS place_id, p.trip_id, p.name, p.route_color, p.route_geometry FROM journey_entries je JOIN places p ON p.trip_id = je.source_trip_id WHERE je.journey_id = ? AND je.source_trip_id IS NOT NULL AND p.route_geometry IS NOT NULL ORDER BY p.trip_id, p.id`, + SUGG: `SELECT t.id, t.title, t.start_date, t.end_date, t.cover_image, (SELECT COUNT(*) FROM places p INNER JOIN day_assignments da ON da.place_id = p.id WHERE p.trip_id = t.id) as place_count FROM trips t LEFT JOIN trip_members tm ON t.id = tm.trip_id AND tm.user_id = ? WHERE (t.user_id = ? OR tm.user_id = ?) AND t.end_date IS NOT NULL AND t.end_date >= ? AND t.end_date <= date('now') AND t.id NOT IN (SELECT trip_id FROM journey_trips) ORDER BY t.end_date DESC`, + PICKER: `SELECT t.id, t.title, t.start_date, t.end_date, t.cover_image, (SELECT COUNT(*) FROM places p INNER JOIN day_assignments da ON da.place_id = p.id WHERE p.trip_id = t.id) as place_count FROM trips t LEFT JOIN trip_members tm ON t.id = tm.trip_id AND tm.user_id = ? WHERE t.user_id = ? OR tm.user_id = ? ORDER BY t.start_date DESC`, + }; + + /** L8 — JG15's flat row order can differ from legacy on `sort_order` ties across entries (join order vs. the IN subquery); the only consumer groups by entry, so the parity contract is the GROUPED shape, not the flat row order. */ + function groupedByEntry(rows: { entry_id: number; id: number }[]) { + const out: Record = {}; + for (const r of rows) (out[r.entry_id] ||= []).push(r.id); + return out; + } + + it('JourneyEntryPhotosRepository.listForJourney (JG15) matches the legacy statement, grouped by entry (L8: flat order may tie-break differently)', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.JG15).all(journey.id) as { entry_id: number; id: number }[]; + const converted = (await entryPhotosRepoDirect.listForJourney(journey.id)) as unknown as { entry_id: number; id: number }[]; + + expect(groupedByEntry(converted)).toEqual(groupedByEntry(legacy)); + // Full-key equality still holds per row (only ORDER may tie-differ) — a + // sorted-by-id comparison catches a dropped/renamed column. + const sortById = (rows: { id: number }[]) => [...rows].sort((a, b) => a.id - b.id); + expect(sortById(converted)).toEqual(sortById(legacy)); + }); + + it('JourneyPhotosRepository.galleryRead (JG19, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement exactly, including row order', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.JG19).all(journey.id); + const converted = await photosRepoDirect.galleryRead(journey.id); + expect(converted).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listStatsRows (JG64) matches the legacy statement', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.STATS_ENTRIES).all(journey.id); + expect(await entriesRepoDirect.listStatsRows(journey.id)).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listStatsTrips (JG65) matches the legacy statement, undated trips sorted last', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.STATS_TRIPS).all(journey.id); + expect(await entriesRepoDirect.listStatsTrips(journey.id)).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listStatsPlaces (JG66) matches the legacy statement, one row per place at its earliest day', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.STATS_PLACES).all(journey.id); + expect(await entriesRepoDirect.listStatsPlaces(journey.id)).toEqual(legacy); + }); + + it('JourneyEntriesRepository.countStatsPlaces (JG67) matches the legacy count', async () => { + const { journey } = await seedParityJourney(); + const legacy = (testDb.prepare(LEG.STATS_PLACECOUNT).get(journey.id) as { n: number }).n; + expect(await entriesRepoDirect.countStatsPlaces(journey.id)).toBe(legacy); + }); + + it('JourneyPhotosRepository.countForJourney (JG68) matches the legacy count', async () => { + const { journey } = await seedParityJourney(); + const legacy = (testDb.prepare(LEG.STATS_PHOTOCOUNT).get(journey.id) as { n: number }).n; + expect(await photosRepoDirect.countForJourney(journey.id)).toBe(legacy); + }); + + it('JourneyEntryPhotosRepository.listFirstPhotoPerEntry (JG69) matches the legacy statement, videos excluded', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.STATS_EPHOTOS).all(journey.id); + expect(await entryPhotosRepoDirect.listFirstPhotoPerEntry(journey.id)).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listTracksSource (JG63) matches the legacy statement', async () => { + const { journey } = await seedParityJourney(); + const legacy = testDb.prepare(LEG.TRACKS).all(journey.id); + expect(await entriesRepoDirect.listTracksSource(journey.id)).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listSuggestedTrips (JG120) matches the legacy statement', async () => { + const { owner } = await seedParityJourney(); + const since = new Date(Date.now() - 30 * 86400000).toISOString().split('T')[0]; + const legacy = testDb.prepare(LEG.SUGG).all(owner.id, owner.id, owner.id, since); + expect(await entriesRepoDirect.listSuggestedTrips(owner.id, since, todayUtc())).toEqual(legacy); + }); + + it('JourneyEntriesRepository.listUserTripsPicker (JG121) matches the legacy statement — owner and a member', async () => { + const { owner, member } = await seedParityJourney(); + const legacyOwner = testDb.prepare(LEG.PICKER).all(owner.id, owner.id, owner.id); + const legacyMember = testDb.prepare(LEG.PICKER).all(member.id, member.id, member.id); + expect(await entriesRepoDirect.listUserTripsPicker(owner.id)).toEqual(legacyOwner); + expect(await entriesRepoDirect.listUserTripsPicker(member.id)).toEqual(legacyMember); + }); +}); + describe('Plan 3g Task 1 — mutation proofs (R5/R7)', () => { it("JG119: JourneyContributorsRepository.deleteNonOwner never removes the owner row, even targeted at the owner's own id", async () => { const { user: owner } = createUser(testDb); diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index f09f4febe..de749efbc 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -81,6 +81,9 @@ beforeAll(async () => { // already-built JourneysRepository (JS8/JS12), same instance the domain // service above uses. await createTestJourneyShareTokensRepo(testDb), journeysRepo, + // task-5-fix-brief constructor-ripple: `UnitOfWork` (L1's transactional + // create/update) + `JourneyPhotosRepository` (L2's `galleryRead` reuse). + uow, await createTestJourneyPhotosRepo(testDb), ); }); @@ -249,6 +252,32 @@ describe('createOrUpdateJourneyShareLink', () => { expect(r1.token).not.toBe(r2.token); }); + + // L1 (task-5-review.md) — two concurrent FIRST creates for the same + // journey used to race the existing-link read against the insert: the + // loser hit `UNIQUE(journey_id)` and threw (a 500 over HTTP) instead of + // the base's `{created:false}`. Races two real `Promise.all` callers + // through the actual service on the shared connection. + it('JOURNEY-SHARE-L1: two concurrent first-creates for the same journey — one created, one {created:false}, no throw', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + const [a, b] = await Promise.all([ + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_timeline: true }), + svc.createOrUpdateJourneyShareLink(journey.id, user.id, { share_timeline: false }), + ]); + + const results = [a, b]; + const created = results.filter((r) => r!.created); + const notCreated = results.filter((r) => !r!.created); + expect(created).toHaveLength(1); + expect(notCreated).toHaveLength(1); + // Both callers agree on the one token that exists. + expect(a!.token).toBe(b!.token); + + const rows = testDb.prepare('SELECT COUNT(*) AS n FROM journey_share_tokens WHERE journey_id = ?').get(journey.id) as { n: number }; + expect(rows.n).toBe(1); + }); }); describe('getJourneyShareLink', () => { @@ -441,6 +470,26 @@ describe('validateShareTokenForAsset', () => { expect(result).toBeNull(); }); + + // M5a (task-5-review.md) — JOURNEY-SHARE-015 above only tries an asset id + // that does not exist at all; it stays green even with `gp.journey_id = ?` + // dropped from `findAssetForValidation` (mutation M23), because the join + // still fails to find a row. This test uses an asset id that IS real and + // gallery-linked, just to a DIFFERENT journey, so a dropped scope check + // would resolve it (leaking a cross-journey asset through a valid token) + // where JOURNEY-SHARE-015 cannot catch that. + it('JOURNEY-SHARE-M5A: denies an asset that is real but belongs to a DIFFERENT journey (findAssetForValidation journey scope)', async () => { + const { user } = createUser(testDb); + const sharedJourney = createJourney(testDb, user.id); + const otherJourney = createJourney(testDb, user.id); + const otherEntry = createJourneyEntry(testDb, otherJourney.id, user.id); + insertJourneyPhoto(otherEntry.id, { assetId: 'other-journey-asset', ownerId: user.id }); + const { token } = await svc.createOrUpdateJourneyShareLink(sharedJourney.id, user.id, {}); + + const result = await svc.validateShareTokenForAsset(token, 'other-journey-asset'); + + expect(result).toBeNull(); + }); }); describe('getPublicJourney', () => { @@ -726,9 +775,14 @@ describe('getPublicJourney', () => { describe('parity — JourneyShareTokensRepository reads match the legacy statement run raw', () => { let shareTokensRepo: JourneyShareTokensRepository; + // L2 — JS15 moved to `JourneyPhotosRepository.galleryRead`; P06 below now + // pins that method's output against the legacy statement instead of the + // (now-deleted) `JourneyShareTokensRepository.listGalleryForPublicJourney`. + let journeyPhotosRepo: Awaited>; beforeAll(async () => { shareTokensRepo = await createTestJourneyShareTokensRepo(testDb); + journeyPhotosRepo = await createTestJourneyPhotosRepo(testDb); }); it('JOURNEY-SHARE-P01: findFlagsByJourneyId (JS1) matches the legacy 5-column read', async () => { @@ -813,7 +867,7 @@ describe('parity — JourneyShareTokensRepository reads match the legacy stateme expect(await shareTokensRepo.listEntryPhotosForPublicJourney(journey.id)).toEqual(legacy); }); - it('JOURNEY-SHARE-P06: listGalleryForPublicJourney (JS15, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement — a photo linked to an entry AND an unattached gallery photo', async () => { + it('JOURNEY-SHARE-P06: JourneyPhotosRepository.galleryRead (JS15, GALLERY_CHRONOLOGICAL_ORDER) matches the legacy statement — a photo linked to an entry AND an unattached gallery photo', async () => { const { user } = createUser(testDb); const journey = createJourney(testDb, user.id); const entry = createJourneyEntry(testDb, journey.id, user.id, { type: 'entry', entry_date: '2026-01-01' }); @@ -838,7 +892,7 @@ describe('parity — JourneyShareTokensRepository reads match the legacy stateme `) .all(journey.id); - expect(await shareTokensRepo.listGalleryForPublicJourney(journey.id)).toEqual(legacy); + expect(await journeyPhotosRepo.galleryRead(journey.id)).toEqual(legacy); expect(legacy).toHaveLength(2); }); diff --git a/server/tests/unit/nest/journey-stats.service.test.ts b/server/tests/unit/nest/journey-stats.service.test.ts index c22f2fc7a..53739f26c 100644 --- a/server/tests/unit/nest/journey-stats.service.test.ts +++ b/server/tests/unit/nest/journey-stats.service.test.ts @@ -137,6 +137,20 @@ describe('journeyStats access', () => { expect(stats).not.toBeNull(); expect(stats!.journeyId).toBe(journey.id); }); + + // M4 (task-5-review.md) — `JourneyEntriesRepository.countStatsPlaces`'s + // `row?.n ?? 0` null arm had no test exercising a journey with zero linked + // trips at all (worse coverage than base). `places` must still answer 0, + // not throw or leave the field undefined. + it('places is 0 for a journey with no trips at all (countStatsPlaces)', async () => { + const { user } = createUser(testDb); + const journey = createJourney(testDb, user.id); + + const stats = await svc.journeyStats(journey.id, user.id); + + expect(stats).not.toBeNull(); + expect(stats!.places).toBe(0); + }); }); describe('journeyStats route', () => { diff --git a/server/tests/unit/nest/memories-access.service.test.ts b/server/tests/unit/nest/memories-access.service.test.ts index 11dcf49e2..5d4f88afa 100644 --- a/server/tests/unit/nest/memories-access.service.test.ts +++ b/server/tests/unit/nest/memories-access.service.test.ts @@ -429,6 +429,31 @@ describe('canAccessUserPhoto', () => { expect(await access.canAccessUserPhoto(viewer.id, owner.id, '0', 'j-asset-viewer', 'immich')).toBe(true); }); + // M5b (task-5-review.md) — mutation M24 dropped `tkp.owner_id = ?` from + // `JourneyPhotosRepository.findJourneyIdForAsset` (MA1) and every existing + // test stayed green, because none put the SAME asset_id/provider under a + // DIFFERENT trek_photos.owner_id than the `ownerUserId` the caller asks + // about. Here the asset really is in a journey the requester (the journey + // owner) could otherwise reach — but `ownerUserId` names someone who did + // NOT upload it, so MA1's owner scope must refuse to resolve it. A dropped + // scope would match on asset_id+provider alone, resolve the journey, and + // MA2's owner-or-contributor check would then grant access wrongly. + it('MEM-ACCESS-M5B: tripId "0" — same asset_id/provider exists in the journey but under a DIFFERENT owner_id (findJourneyIdForAsset owner scope)', async () => { + const { user: owner } = createUser(testDb); + const { user: realUploader } = createUser(testDb, { username: 'real-uploader-m5b' }); + const { user: otherOwner } = createUser(testDb, { username: 'other-owner-m5b' }); + const journeyId = makeJourney(owner.id); + const photoId = Number(testDb.prepare( + "INSERT INTO trek_photos (provider, asset_id, owner_id) VALUES ('immich', 'm5b-shared-asset', ?)" + ).run(realUploader.id).lastInsertRowid); + testDb.prepare('INSERT INTO journey_photos (journey_id, photo_id, created_at) VALUES (?, ?, 0)').run(journeyId, photoId); + + // requestingUserId (owner) !== ownerUserId (otherOwner), so the trivial + // shortcut does not fire; MA1 must refuse because otherOwner never + // uploaded this asset. + expect(await access.canAccessUserPhoto(owner.id, otherOwner.id, '0', 'm5b-shared-asset', 'immich')).toBe(false); + }); + it('MEM-ACCESS-007: tripId "0" refuses someone with no journey link', async () => { const { user: owner } = createUser(testDb); const { user: stranger } = createUser(testDb, { username: 'stranger' });