From d2e0d7e3202fe0e7476aa0bea0091af955efa4c0 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 04:46:18 +0200 Subject: [PATCH] refactor(trips): trip access through TripsRepository; demo seed in a request context; dialect helpers for the trips cluster --- server/src/db/database.ts | 109 +------------- server/src/db/dialect/sql-functions.ts | 82 ++++++++++ server/src/db/orm.ts | 17 ++- .../src/db/repositories/Places.repository.ts | 142 +++++++++++++++++- .../db/repositories/TripMembers.repository.ts | 43 +++++- .../src/db/repositories/Trips.repository.ts | 32 +++- server/src/nest/database/database.service.ts | 79 +++++++--- .../src/nest/permissions/trip-access.guard.ts | 20 ++- .../src/nest/permissions/trip-owner.guard.ts | 18 ++- .../trip-card-contributions.controller.ts | 14 +- server/src/nest/trips/trips.service.ts | 13 +- server/tests/e2e/assignments.e2e.test.ts | 16 +- server/tests/e2e/budget.e2e.test.ts | 18 ++- server/tests/e2e/collab.e2e.test.ts | 19 ++- server/tests/e2e/days.e2e.test.ts | 2 +- server/tests/e2e/doc-sync.e2e.test.ts | 11 +- server/tests/e2e/feeds.e2e.test.ts | 2 +- server/tests/e2e/files.e2e.test.ts | 22 ++- server/tests/e2e/packing.e2e.test.ts | 2 +- server/tests/e2e/places.e2e.test.ts | 28 ++-- server/tests/e2e/reservations.e2e.test.ts | 31 +++- server/tests/e2e/roadtrip.e2e.test.ts | 2 +- server/tests/e2e/todo.e2e.test.ts | 2 +- server/tests/e2e/trip-invite.e2e.test.ts | 16 +- .../tests/helpers/accommodations-service.ts | 9 +- server/tests/helpers/mcp-test-controllers.ts | 10 +- server/tests/helpers/plugin-host.ts | 2 +- server/tests/helpers/test-uow.ts | 65 ++++++++ .../demo-seed-request-context.test.ts | 76 ++++++++++ .../orm-request-context-seams.test.ts | 43 ++++++ .../trip-access-primitives.test.ts | 88 +++++++++-- server/tests/unit/budget/costs.rpc.test.ts | 2 +- server/tests/unit/days/day-notes.rpc.test.ts | 2 +- .../unit/days/days-itinerary.rpc.test.ts | 2 +- .../unit/db/dialect/sql-functions.test.ts | 109 +++++++++++++- .../db/repositories/Places.repository.test.ts | 98 ++++++++++++ .../TripMembers.repository.test.ts | 84 +++++++++++ .../db/repositories/Trips.repository.test.ts | 8 + server/tests/unit/db/trip-access.test.ts | 73 +++++---- server/tests/unit/files/files.rpc.test.ts | 4 +- server/tests/unit/mcp/tools-prompts.test.ts | 15 +- .../unit/nest/assignments.service.test.ts | 11 +- .../unit/nest/budget.service.calc.test.ts | 2 +- .../tests/unit/nest/budget.service.db.test.ts | 14 +- server/tests/unit/nest/budget.service.test.ts | 39 +++-- server/tests/unit/nest/charging.test.ts | 4 +- .../unit/nest/collections.service.test.ts | 4 +- .../nest/database-service.delegation.test.ts | 69 ++++++--- .../tests/unit/nest/database-service.test.ts | 11 +- .../nest/dawarich-suggestions.service.test.ts | 12 +- .../unit/nest/dawarich-tracks.service.test.ts | 2 +- server/tests/unit/nest/day-boundaries.test.ts | 6 +- .../tests/unit/nest/day-notes.service.test.ts | 4 +- server/tests/unit/nest/days.service.test.ts | 5 +- server/tests/unit/nest/files.service.test.ts | 13 +- server/tests/unit/nest/google-route.test.ts | 4 +- .../unit/nest/journey-domain.service.test.ts | 5 +- .../unit/nest/memories-access.service.test.ts | 13 +- .../tests/unit/nest/packing.service.test.ts | 5 +- .../tests/unit/nest/places.directions.test.ts | 13 +- server/tests/unit/nest/places.service.test.ts | 11 +- .../unit/nest/reservations.service.test.ts | 6 +- .../unit/nest/roadtrip-plan.service.test.ts | 13 +- .../tests/unit/nest/roadtrip-planning.test.ts | 10 +- .../unit/nest/roadtrip-read-tools.test.ts | 10 +- server/tests/unit/nest/share.service.test.ts | 5 +- server/tests/unit/nest/todo.service.test.ts | 4 +- .../tests/unit/nest/trip-access.guard.test.ts | 25 ++- .../unit/nest/trip-members.controller.test.ts | 8 +- .../unit/nest/trip-members.service.test.ts | 13 +- .../tests/unit/nest/trip-owner.guard.test.ts | 30 +++- .../tests/unit/nest/trips.controller.test.ts | 14 +- server/tests/unit/nest/trips.service.test.ts | 14 +- .../nest/unified-memories.service.test.ts | 9 +- .../unit/nest/user-cleanup.service.test.ts | 11 +- server/tests/unit/packing/packing.rpc.test.ts | 2 +- server/tests/unit/places/places.rpc.test.ts | 2 +- .../plugins/addon-domains-errors.rpc.test.ts | 2 +- .../unit/plugins/addon-gates.rpc.test.ts | 2 +- .../plugins/day-schedule.controller.test.ts | 9 +- .../unit/plugins/day-tints.controller.test.ts | 9 +- server/tests/unit/plugins/host-rpc.test.ts | 10 ++ .../plugins/map-layers.controller.test.ts | 6 +- .../plugins/map-markers.controller.test.ts | 6 +- .../plugins/pdf-sections.controller.test.ts | 6 +- .../plugins/place-details.controller.test.ts | 9 +- .../tests/unit/plugins/plugin-guards.test.ts | 4 +- .../plugins/plugin-routes.controller.test.ts | 11 +- .../tests/unit/plugins/rpc-fallbacks.test.ts | 2 +- ...trip-card-contributions.controller.test.ts | 2 +- .../plugins/trip-warnings.controller.test.ts | 6 +- .../view-contributions.controller.test.ts | 6 +- .../unit/reservations/bookings.rpc.test.ts | 2 +- .../unit/services/conflictUpdate.test.ts | 11 +- server/tests/unit/todo/todo.rpc.test.ts | 2 +- server/tests/unit/trips/trips.rpc.test.ts | 2 +- server/tests/websocket/connection.test.ts | 41 +++++ 97 files changed, 1595 insertions(+), 406 deletions(-) create mode 100644 server/tests/integration/demo-seed-request-context.test.ts create mode 100644 server/tests/unit/db/repositories/Places.repository.test.ts create mode 100644 server/tests/unit/db/repositories/TripMembers.repository.test.ts diff --git a/server/src/db/database.ts b/server/src/db/database.ts index f24c17466..0a751b81c 100644 --- a/server/src/db/database.ts +++ b/server/src/db/database.ts @@ -1,5 +1,4 @@ import { readEnv } from '../app-config'; -import { Place, Tag } from '../types'; import { resolveDbPath } from './db-path'; import { applyDurabilityPragmas } from './durability'; import { readSchemaSnapshot } from './schema-snapshot'; @@ -129,104 +128,12 @@ async function reinitialize(): Promise { console.log('[DB] Database reinitialized successfully'); } -interface PlaceWithCategory extends Place { - category_name: string | null; - category_color: string | null; - category_icon: string | null; -} +// getPlaceWithTags/canAccessTrip/isOwner and the TripAccess/PlaceWithTags +// interfaces lived here through Plan 3c Task 0a's async sweep. Task 0b moved +// their bodies onto TripsRepository.findAccessible/isOwner and +// PlacesRepository.findWithTagsAndRatings — db/repositories/Trips.repository.ts's +// TripAccess is now the single source (db/database.ts's copy is deleted, not +// re-exported); DatabaseService re-exports both type names from the +// repositories that own them now, so no importer's path changed. -interface PlaceWithTags extends Place { - category: { id: number; name: string; color: string; icon: string } | null; - tags: Tag[]; - ratings: { user_id: number; username: string; avatar: string | null; rating: number }[]; - rating_avg: number | null; - rating_count: number; -} - -async function getPlaceWithTags(placeId: number | string): Promise { - const place = 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) as PlaceWithCategory | undefined; - - 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) as Tag[]; - - // Collaborative ratings (#1435): every voter with username/avatar for the - // who-voted tooltip; the displayed value is the average. - const ratings = db - .prepare( - ` - SELECT pr.user_id, u.username, u.avatar, pr.rating FROM place_ratings pr - JOIN users u ON pr.user_id = u.id - WHERE pr.place_id = ? ORDER BY pr.created_at - `, - ) - .all(placeId) as { user_id: number; username: string; avatar: string | null; rating: number }[]; - - return { - ...place, - category: place.category_id - ? { - id: place.category_id, - name: place.category_name!, - color: place.category_color!, - icon: place.category_icon!, - } - : null, - tags, - ratings, - rating_avg: ratings.length > 0 ? ratings.reduce((s, r) => s + r.rating, 0) / ratings.length : null, - rating_count: ratings.length, - }; -} - -interface TripAccess { - id: number; - user_id: number; - currency: string | null; -} - -async function canAccessTrip(tripId: number | string, userId: number): Promise { - return db - .prepare( - ` - SELECT t.id, t.user_id, t.currency 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) as TripAccess | undefined; -} - -async function isOwner(tripId: number | string, userId: number): Promise { - return !!db.prepare('SELECT id FROM trips WHERE id = ? AND user_id = ?').get(tripId, userId); -} - -export { - db, - closeDb, - reinitialize, - getPlaceWithTags, - canAccessTrip, - isOwner, - getRawConnection, - registerReinitializeHook, - runDemoSeed, -}; -export type { TripAccess, PlaceWithTags }; +export { db, closeDb, reinitialize, getRawConnection, registerReinitializeHook, runDemoSeed }; diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index 3cbc954a0..63c91a408 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -127,3 +127,85 @@ export function lowerParam(platform: Platform, value: string): RawQueryFragment return unsupported(platform); } +// --------------------------------------------------------------------------- +// Plan 3c Task 0b (R6) — no consumer yet: Tasks 1–8 wire these in as each +// converts the statement that needs them (`place-shadow.service.ts:192`, +// `places.service.ts:642,651-652`, the ~16 `COALESCE` sites across +// `days`/`assignments`/`trip-members`/`places`). Added here, with their own +// SQLF-0xx tests pinning the SQLite text against a raw statement, so a later +// task imports a tested helper instead of writing one inline. +// --------------------------------------------------------------------------- + +/** + * `datetime('now', '-N days')` — the SQLite clock shifted backwards by whole + * days, as TEXT (not a stored column's value — `dateAdd` above is the wrong + * shape for this: it shifts a *column*, not "now"). `days` is validated to be + * a non-negative integer and spelled directly into the fragment rather than + * bound: the legacy statement (`place-shadow.service.ts:192`) binds the + * WHOLE modifier string (`'-30 days'`) as one parameter, and MikroORM's raw + * fragments do not accept a placeholder inside the modifier argument of a + * SQLite date/time function — `datetime('now', ?)` with `'-30 days'` bound + * does work in raw SQLite, but composing that shape through a `raw()` + * fragment used as a QueryBuilder value needs the modifier already in the + * SQL text, the same way `dateAdd` above spells its own day count. + */ +export function nowMinusDays(platform: Platform, days: number): RawQueryFragment { + if (!Number.isInteger(days) || days < 0) { + throw new Error(`sql-functions: nowMinusDays needs a non-negative integer day count, got ${days}`); + } + if (platform instanceof SqlitePlatform) return raw(`datetime('now', '-${days} days')`); + return unsupported(platform); +} + +/** `TRIM()`. Pairs with `lower()` for the `LOWER(TRIM(name))` shape (`places.service.ts:642`). */ +export function trim(platform: Platform, ref: string): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw(`TRIM(${column(ref)})`); + return unsupported(platform); +} + +/** + * `COALESCE(, )` — both sides column references, for the + * `COALESCE(a, b)` SELECT-projection shape (`days.service.ts:93-94`, + * `assignments.service.ts:103-104`, `trip-members.service.ts:116-143`, …). + * + * Not composed by nesting a `RawQueryFragment` inside this one: MikroORM's + * `RawQueryFragment[Symbol.toPrimitive]` only returns a usable value for the + * `'string'` coercion hint (object-key position) — a template-literal + * `${fragment}` embed uses the `'default'` hint and throws + * (`node_modules/@mikro-orm/core/utils/RawQueryFragment.js`, verified + * directly, not assumed). So `coalesce`'s second argument is a second + * `column()`-validated identifier, spelled into the same fragment as the + * first — two columns, one statement, matching `lower`/`lowerParam`'s + * two-function split for the same reason (a filter KEY vs a bound VALUE + * are different shapes). + */ +export function coalesce(platform: Platform, ref: string, fallbackRef: string): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw(`COALESCE(${column(ref)}, ${column(fallbackRef)})`); + return unsupported(platform); +} + +/** + * `COALESCE(, ?)` — the value-side twin of `coalesce()` above, for the + * `COALESCE(col, ?)` UPDATE-SET shape (`places.service.ts:344,351,363,…`). + * `value` is bound as a genuine parameter, never interpolated. + */ +export function coalesceParam(platform: Platform, ref: string, value: string | number | null): RawQueryFragment { + if (platform instanceof SqlitePlatform) return raw(`COALESCE(${column(ref)}, ?)`, [value]); + return unsupported(platform); +} + +/** + * `ABS( - ?)`, for use as a filter KEY (the `'string'`-hint coercion + * `lower()` above relies on) so it composes with `$lte`/`$gte`: + * `{ [absDifference(platform, 'lat', targetLat)]: { $lte: tolerance } }` + * renders `WHERE ABS(lat - ?) <= ?`, matching `places.service.ts:651-652`'s + * `abs(lat - ?) <= ?` (and the sibling `lng` comparison at `:652`) exactly. + * Typed `RawQueryFragment & symbol`, same reason `lower()` is: only a + * `string | number | symbol` is a valid computed property name, and the + * brand only lives on that typing for values meant to be used as a key. + */ +export function absDifference(platform: Platform, ref: string, value: number): RawQueryFragment & symbol { + if (platform instanceof SqlitePlatform) return raw(`ABS(${column(ref)} - ?)`, [value]); + return unsupported(platform); +} + diff --git a/server/src/db/orm.ts b/server/src/db/orm.ts index f115b41f8..436b613ab 100644 --- a/server/src/db/orm.ts +++ b/server/src/db/orm.ts @@ -1,5 +1,6 @@ import mikroOrmConfig from '../mikro-orm.config'; import { registerReinitializeHook, runDemoSeed } from './database'; +import { withRequestContext } from '../nest/database/request-context'; import type { AnyEntity, EntityClass, EntityManager, EntitySchema, IDatabaseDriver, MikroORM } from '@mikro-orm/core'; import type { Migrator } from '@mikro-orm/migrations'; import { MikroORM as SqliteMikroORM } from '@mikro-orm/sqlite'; @@ -41,7 +42,21 @@ export async function runSchemaBootstrap(orm: AnyOrm): Promise { // Demo data depends on the seeded categories (hard-coded ids under an FK), so // it can only run once the seeders above have. - runDemoSeed(); + // + // Plan 3c Task 0b (R9): `runDemoSeed` itself stays raw better-sqlite3 today + // (`demo-seed.ts`'s own docstring: "the domain phase that gives demo + // seeding a repository read must wrap it in withRequestContext then") — but + // wrapping the call HERE, the one place `runSchemaBootstrap` already has + // `orm` in scope, covers BOTH of its real call sites at once + // (`bootstrap.ts`'s initial boot and `attachOrm`'s restore hook below, both + // of which only ever reach `runDemoSeed` through this function) with no + // second wrapper to keep in sync. A synchronous `fn` stays synchronous + // through `withRequestContext`/`RequestContext.create`, so this changes + // nothing about what `runDemoSeed` does today — only that a repository read + // added to it later already has a valid context, so Plan 3i's demo + // conversion never meets a second C1 (task-6-review-parity.md's boot-sweep + // finding). + withRequestContext(orm, () => runDemoSeed()); } /** diff --git a/server/src/db/repositories/Places.repository.ts b/server/src/db/repositories/Places.repository.ts index b750fedac..5b55d06a8 100644 --- a/server/src/db/repositories/Places.repository.ts +++ b/server/src/db/repositories/Places.repository.ts @@ -1,4 +1,144 @@ import type { Places } from '../entities/Places.entity'; +import type { Tags } from '../entities/Tags.entity'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class PlacesRepository extends TrekRepository {} +/** A `places` row as the API emits it — every scalar column of the entity. */ +export interface PlaceRow { + id: number; + trip_id: number; + name: string; + description: string | null; + lat: number | null; + lng: number | null; + address: string | null; + category_id: number | null; + price: number | null; + currency: string | null; + reservation_status: string | null; + reservation_notes: string | null; + reservation_datetime: string | null; + place_time: string | null; + end_time: string | null; + duration_minutes: number | null; + notes: string | null; + image_url: string | null; + google_place_id: string | null; + google_ftid: string | null; + website: string | null; + phone: string | null; + transport_mode: string | null; + created_at: string | null; + updated_at: string | null; + osm_id: string | null; + route_geometry: string | null; + route_color: string | null; + stop_type: string | null; + fill_percent: number | null; + amap_poi_id: string | null; + source: string | null; +} + +const _placeRowKeys: AssertRowKeys = true; + +/** A `tags` row, byte-identical to the legacy `SELECT t.* FROM tags t ...` shape. */ +export interface TagRow { + id: number; + user_id: number; + name: string; + color: string | null; + created_at: string | null; +} + +const _tagRowKeys: AssertRowKeys = true; + +interface PlaceWithCategoryRow extends PlaceRow { + category_name: string | null; + category_color: string | null; + category_icon: string | null; +} + +interface PlaceRatingRow { + user_id: number; + username: string; + avatar: string | null; + rating: number; +} + +/** + * `getPlaceWithTags` (`db/database.ts:146-197`, 0b's Concern for Task 4): + * the 11 `PlacesService` call sites move in Task 4, so `DatabaseService + * .getPlaceWithTags` keeps its own name/shape and delegates to this method + * NOW — no caller beyond `DatabaseService` changes in this task. + */ +export interface PlaceWithTagsRow extends PlaceRow { + category: { id: number; name: string; color: string; icon: string } | null; + tags: TagRow[]; + ratings: PlaceRatingRow[]; + rating_avg: number | null; + rating_count: number; +} + +export class PlacesRepository extends TrekRepository { + /** + * The legacy `getPlaceWithTags`'s three statements, byte-for-byte in intent + * (`mapResults: false` on every read, matching `TripsRepository + * .findAccessible`/`InviteTokensRepository.listWithCreatorAndTrip`): + * + * ```sql + * 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 = ? + * + * SELECT t.* FROM tags t JOIN place_tags pt ON t.id = pt.tag_id WHERE pt.place_id = ? + * + * SELECT pr.user_id, u.username, u.avatar, pr.rating FROM place_ratings pr + * JOIN users u ON pr.user_id = u.id WHERE pr.place_id = ? ORDER BY pr.created_at + * ``` + * + * `p.category`/`p.place_tags`/`p.place_ratings_collection` are the entity's + * own relation properties for these three joins (`Places.entity.ts`), so no + * hand-spelled join condition is needed; `pr.user` resolves to the + * `place_ratings.user_id` FK column the same way `TripsRepository + * .findAccessible`'s `t.user` resolves to `trips.user_id`. + */ + async findWithTagsAndRatings(place_id: number | string): Promise { + // Raw conditions (D4's T5 escape hatch), not `.where({ 'p.id': place_id })`: + // MikroORM's typed filter rejects a `string` against `p.id`'s branded + // `number` type — matching `TripsRepository.findAccessible`'s own + // raw-bind seam (`legacy getPlaceWithTags(placeId: number | string)` + // bound the raw value with no `Number()` conversion). `p.id` is the + // physical column name. + const place = await this.qb('p') + .leftJoin('p.category', 'c') + .select(['p.*', 'c.name as category_name', 'c.color as category_color', 'c.icon as category_icon']) + .where('p.id = ?', [place_id]) + .execute('get', false); + if (!place) return null; + + const tags = await this.qb('p') + .join('p.place_tags', 't') + .select(['t.*']) + .where('p.id = ?', [place_id]) + .execute('all', false); + + const ratings = await this.qb('p') + .join('p.place_ratings_collection', 'pr') + .join('pr.user', 'u') + .select(['pr.user', 'u.username', 'u.avatar', 'pr.rating']) + .where('p.id = ?', [place_id]) + .orderBy({ 'pr.created_at': 'asc' }) + .execute('all', false); + + const { category_name, category_color, category_icon, ...placeRest } = place; + return { + ...placeRest, + category: place.category_id + ? { id: place.category_id, name: category_name!, color: category_color!, icon: category_icon! } + : null, + tags, + ratings, + rating_avg: ratings.length > 0 ? ratings.reduce((s, r) => s + r.rating, 0) / ratings.length : null, + rating_count: ratings.length, + }; + } +} diff --git a/server/src/db/repositories/TripMembers.repository.ts b/server/src/db/repositories/TripMembers.repository.ts index 5b63033df..5b8141898 100644 --- a/server/src/db/repositories/TripMembers.repository.ts +++ b/server/src/db/repositories/TripMembers.repository.ts @@ -1,4 +1,45 @@ import type { TripMembers } from '../entities/TripMembers.entity'; +import { Trips } from '../entities/Trips.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class TripMembersRepository extends TrekRepository {} +export class TripMembersRepository extends TrekRepository { + /** + * The user ids a trip may refer to: its members plus the owner. Byte-for-byte + * the legacy `DatabaseService.rosterUserIds`'s statement in intent — + * `SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id + * FROM trips WHERE id = ?` — as two reads merged through a `Set` (which + * already dedupes, the same guarantee `UNION` gives) rather than one `UNION` + * statement: the two `SELECT`s read different tables with no FK between + * them, so a single-entity `QueryBuilder` (rooted on `TripMembers`) cannot + * express the second half without losing rows on a trip that has no + * members yet — an inner join from `trip_members` would produce zero rows + * for exactly the case (an owner-only trip) the legacy statement's second + * `SELECT` exists to cover. `trips` has no `TripMembersRepository` field to + * reach it through, so this repository asks the `EntityManager` for + * `TripsRepository` the same way `DatabaseService`/the guards resolve a + * repository outside their own entity — one `EntityManager`, two + * repositories, two independent reads. + */ + async rosterUserIds(trip_id: number | string): Promise> { + // Raw conditions (D4's T5 escape hatch), not `.where({ 'm.trip': trip_id })`: + // MikroORM's typed filter rejects a `string` against the relation's + // branded id type, and coercing to `Number(trip_id)` first would change + // the raw-bind id-shape parity `TripsRepository`'s docstring describes. + // `m.trip_id`/`t.id` are the physical column names. + const members = await this.qb('m') + .select(['m.user']) + .where('m.trip_id = ?', [trip_id]) + .execute<{ user_id: number }[]>('all', false); + + const trips = this.getEntityManager().getRepository(Trips); + const owner = await trips + .qb('t') + .select(['t.user']) + .where('t.id = ?', [trip_id]) + .execute<{ user_id: number } | undefined>('get', false); + + const ids = new Set(members.map((m) => m.user_id)); + if (owner) ids.add(owner.user_id); + return ids; + } +} diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index 1f7ae08d8..441ec45a6 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -16,12 +16,31 @@ export class TripsRepository extends TrekRepository { * SELECT t.id, t.user_id, t.currency 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) + * + * `trip_id: number | string` (Plan 3c Task 0b): the legacy free function + * bound whatever `req.params.tripId` resolved to straight into the + * statement with no `Number()`/`toRowId` conversion first — the raw-bind + * seam. `TripAccessGuard` still does `Number(tripId)` before calling this + * (unchanged), but `DatabaseService.canAccessTrip`/`isOwner` and every + * other of the 41 in-cluster callers pass a `number | string` through + * unconverted, so this method (and `isOwner` below) must accept and bind + * that raw value rather than coerce it — a coercion here would silently + * change the `'0x10'`/`'007'`-shaped id parity the boot matrix pins + * (`Number('0x10') === 16`, a real trip id, where SQLite's own text/integer + * affinity comparison of the unconverted string never matches one). */ - async findAccessible(trip_id: number, user_id: number): Promise { + async findAccessible(trip_id: number | string, user_id: number): Promise { const row = await this.qb('t') .select(['t.id', 't.user', 't.currency']) .leftJoin('t.trip_members_collection', 'm', { 'm.user': user_id }) - .where({ 't.id': trip_id }) + // Raw condition (D4's T5 escape hatch), not `.where({ 't.id': trip_id })`: + // MikroORM's typed filter rejects a `string` against `t.id`'s branded + // `number` type, and coercing to `Number(trip_id)` first would be + // exactly the parity break the class docstring above describes. + // `t.id`/`t.user_id` are the physical column names (not translated + // through the entity's `trip`/`user` property aliasing the way a typed + // filter object is), so the raw text names them directly. + .where('t.id = ?', [trip_id]) .andWhere({ $or: [{ 't.user': user_id }, { 'm.user': { $ne: null } }] }) // `mapResults: false` leaves the driver's row alone: the keys are the // column names, which is why the result type is spelled out here rather @@ -32,7 +51,12 @@ export class TripsRepository extends TrekRepository { } /** `SELECT id FROM trips WHERE id = ? AND user_id = ?` */ - async isOwner(trip_id: number, user_id: number): Promise { - return (await this.count({ id: trip_id, user: user_id })) > 0; + async isOwner(trip_id: number | string, user_id: number): Promise { + const row = await this.qb('t') + .select(['t.id']) + .where('t.id = ?', [trip_id]) + .andWhere({ user: user_id }) + .execute<{ id: number } | undefined>('get', false); + return !!row; } } diff --git a/server/src/nest/database/database.service.ts b/server/src/nest/database/database.service.ts index 88dad1fc2..91d248c78 100644 --- a/server/src/nest/database/database.service.ts +++ b/server/src/nest/database/database.service.ts @@ -1,10 +1,21 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Inject, Injectable, Optional } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; import type Database from 'better-sqlite3'; -import { canAccessTrip, getPlaceWithTags, isOwner } from '../../db/database'; -import type { PlaceWithTags, TripAccess } from '../../db/database'; +import { Trips } from '../../db/entities/Trips.entity'; +import { TripMembers } from '../../db/entities/TripMembers.entity'; +import { Places } from '../../db/entities/Places.entity'; +import type { TripAccess } from '../../db/repositories/Trips.repository'; +import type { PlaceWithTagsRow as PlaceWithTags } from '../../db/repositories/Places.repository'; import { DATABASE_CONNECTION } from './database.tokens'; -export type { PlaceWithTags, TripAccess }; +// Plan 3c Task 0b: `TripAccess`/`PlaceWithTags` used to be db/database.ts's +// own interfaces, re-exported from here (§18.1's duplicate-declaration +// surprise). Both are deleted there now — `Trips.repository.ts`'s +// `TripAccess` and `Places.repository.ts`'s `PlaceWithTagsRow` are the single +// sources, re-exported under their original names so every one of the ~40 +// importers of `{ TripAccess, PlaceWithTags }` from this module needs no +// change. +export type { TripAccess, PlaceWithTags }; /** * Injectable wrapper around TREK's existing better-sqlite3 connection. @@ -15,7 +26,26 @@ export type { PlaceWithTags, TripAccess }; */ @Injectable() export class DatabaseService { - constructor(@Inject(DATABASE_CONNECTION) private readonly conn: Database.Database) {} + constructor( + @Inject(DATABASE_CONNECTION) private readonly conn: Database.Database, + // Plan 3c Task 0b: `canAccessTrip`/`isOwner`/`rosterUserIds`/ + // `getPlaceWithTags` moved off the raw better-sqlite3 singleton onto + // TripsRepository/TripMembersRepository/PlacesRepository, which need an + // EntityManager to resolve — `@Optional()` (not just `?`, which is + // TS-only and does not tell Nest's DI the provider may be absent), + // matching CronRegistrarService's own `@Optional() orm` ruling: this + // service is provided by the `@Global()` DatabaseModule, which some e2e + // harnesses compose standalone without MikroOrmModule.forRoot in the + // container (see `orm.module.ts`'s docstring) — a hard EntityManager + // dependency here would fail every one of those at `compile()`, the same + // class of break `@InjectRepository` would cause (D5). Production + // (buildApp) always has it, since `MikroOrmModule.forRoot`'s core module + // is itself `@Global()` — the same resolution `JwtAuthGuard` relies on. + // A hand-built `DatabaseService` in a unit test that calls one of the + // four methods below must pass one explicitly (`sharedTestOrm(db)).em`), + // same as `createTestUsersRepo`'s callers do for `UsersRepository`. + @Optional() private readonly em?: EntityManager, + ) {} /** The shared better-sqlite3 connection (same singleton the legacy app uses). */ get connection(): Database.Database { @@ -38,17 +68,29 @@ export class DatabaseService { return this.conn.prepare(sql).run(...params); } - // Trip-access helpers delegate to the db/database exports (not this.conn): - // tests vi.mock that module and stub the helpers themselves, so the stubs - // must keep flowing through here. + /** + * The EntityManager the four repository-backed methods below resolve their + * repository through. Throws its OWN clear error when absent — never + * MikroORM's generic "global EntityManager" wording — so a hand-built + * instance that forgot to pass one fails loudly and distinctly, the same + * fail-closed shape `CronRegistrarService.runOnBoot` uses. + */ + private entityManager(): EntityManager { + if (!this.em) { + throw new Error( + 'DatabaseService: no EntityManager available — construct with one (Nest DI supplies it in production) to call canAccessTrip/isOwner/rosterUserIds/getPlaceWithTags', + ); + } + return this.em; + } /** Trip visible to the user (owner or member); undefined when no access. */ async canAccessTrip(tripId: number | string, userId: number): Promise { - return await canAccessTrip(tripId, userId); + return await this.entityManager().getRepository(Trips).findAccessible(tripId, userId); } async isOwner(tripId: number | string, userId: number): Promise { - return await isOwner(tripId, userId); + return await this.entityManager().getRepository(Trips).isOwner(tripId, userId); } /** @@ -59,18 +101,19 @@ export class DatabaseService { * This is canAccessTrip's question asked as a set rather than a predicate, so * it lives beside it. Domains that accept user ids in a write body intersect * against it: holding the permission says you may edit the trip, not that any - * id you name belongs to it. Runs on the connection rather than delegating to - * db/database, because it has no legacy export to stay parity with. + * id you name belongs to it. */ async rosterUserIds(tripId: number | string): Promise> { - const rows = this.all<{ user_id: number }>( - 'SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?', - tripId, tripId, - ); - return new Set(rows.map(r => r.user_id)); + return await this.entityManager().getRepository(TripMembers).rosterUserIds(tripId); } + /** + * Delegates to `PlacesRepository.findWithTagsAndRatings` (Plan 3c Task 0b). + * `PlacesService`'s 11 call sites are Task 4's — this method keeps its own + * name/shape on `DatabaseService` for now, so Task 4 is the only future + * change those 11 sites need. + */ async getPlaceWithTags(placeId: number | string): Promise { - return await getPlaceWithTags(placeId); + return await this.entityManager().getRepository(Places).findWithTagsAndRatings(placeId); } } diff --git a/server/src/nest/permissions/trip-access.guard.ts b/server/src/nest/permissions/trip-access.guard.ts index 6ac6b283a..3f1cf9991 100644 --- a/server/src/nest/permissions/trip-access.guard.ts +++ b/server/src/nest/permissions/trip-access.guard.ts @@ -1,7 +1,9 @@ import { CanActivate, ExecutionContext, HttpException, Injectable, SetMetadata } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; -import { DatabaseService, type TripAccess } from '../database/database.service'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripAccess } from '../database/database.service'; import { PermissionsService } from './permissions.service'; import type { User } from '../../types'; @@ -38,11 +40,23 @@ type TripRequest = Request & { user?: User; [TRIP_REQUEST_KEY]?: TripAccess }; * It deliberately does NOT replace the `verifyTripAccess`/`canEdit` methods on the * domain services. Of their callers, the large majority are `*.mcp.ts` tools, which * never pass through an HTTP guard; those methods stay exactly where they are. + * + * Injects `EntityManager`, not `DatabaseService`/`@InjectRepository(Trips)` + * (Plan 3c Task 0b, the `JwtAuthGuard` precedent): this guard is applied via + * `@UseGuards(…, TripAccessGuard)` on 146 handlers across 22 controller + * classes, and Nest resolves a class-referenced guard's constructor + * dependencies from the HOST CONTROLLER'S OWN module graph — + * `@InjectRepository(Trips)` would need `MikroOrmModule.forFeature([Trips])` + * added to every one of those modules (D5's blast radius). `EntityManager` + * comes from `MikroOrmModule.forRoot`'s core module, which IS `@Global()`, so + * no module needs new wiring. `this.em.getRepository(Trips)` inside + * `canActivate` is the same per-request resolution `JwtAuthGuard` uses for + * `Users`. */ @Injectable() export class TripAccessGuard implements CanActivate { constructor( - private readonly db: DatabaseService, + private readonly em: EntityManager, private readonly permissions: PermissionsService, private readonly reflector: Reflector, ) {} @@ -56,7 +70,7 @@ export class TripAccessGuard implements CanActivate { if (!user) throw new HttpException({ error: 'Unauthorized' }, 401); const tripId = Number((request.params as Record)?.tripId); - const trip = Number.isFinite(tripId) ? await this.db.canAccessTrip(tripId, user.id) : undefined; + const trip = Number.isFinite(tripId) ? await this.em.getRepository(Trips).findAccessible(tripId, user.id) : undefined; // A trip the user may not see is reported as absent, never as forbidden: a 403 // would confirm the id exists to someone who has no business knowing. if (!trip) throw new HttpException({ error: 'Trip not found' }, 404); diff --git a/server/src/nest/permissions/trip-owner.guard.ts b/server/src/nest/permissions/trip-owner.guard.ts index 90333c8bf..bf9bc7df5 100644 --- a/server/src/nest/permissions/trip-owner.guard.ts +++ b/server/src/nest/permissions/trip-owner.guard.ts @@ -1,7 +1,9 @@ import { CanActivate, ExecutionContext, HttpException, Injectable, SetMetadata } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; -import { DatabaseService, type TripAccess } from '../database/database.service'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripAccess } from '../database/database.service'; import { TRIP_REQUEST_KEY } from './trip-access.guard'; import type { User } from '../../types'; @@ -43,11 +45,21 @@ type TripRequest = Request & { user?: User; [TRIP_REQUEST_KEY]?: TripAccess }; * Absent access is a 404, never a 403, for the same reason TripAccessGuard does * it that way: a 403 would confirm the id exists to someone who has no business * knowing. A 403 only ever goes to somebody who can already see the trip. + * + * Injects `EntityManager`, not `DatabaseService` (Plan 3c Task 0b, the + * `task-0a-review-security.md` F-A1 correction): `trip-members.controller.ts` + * has no class-level `TripAccessGuard` (its own docstring explains why — + * guards run before pipes and the e2e suite pins the error ordering), so on + * its four owner-only routes `request[TRIP_REQUEST_KEY]` is never parked and + * THIS guard's own lookup at `:73` below is the sole access check standing + * between a stranger and those routes — the same `TripAccessGuard` precedent + * applies here for the same reason (`EntityManager` is `@Global()`, no + * module needs new wiring; `@InjectRepository(Trips)` would). */ @Injectable() export class TripOwnerGuard implements CanActivate { constructor( - private readonly db: DatabaseService, + private readonly em: EntityManager, private readonly reflector: Reflector, ) {} @@ -70,7 +82,7 @@ export class TripOwnerGuard implements CanActivate { let trip = request[TRIP_REQUEST_KEY]; if (!trip) { const tripId = Number((request.params as Record)?.[meta?.param ?? 'tripId']); - trip = Number.isFinite(tripId) ? (await this.db.canAccessTrip(tripId, user.id)) ?? undefined : undefined; + trip = Number.isFinite(tripId) ? (await this.em.getRepository(Trips).findAccessible(tripId, user.id)) ?? undefined : undefined; if (trip) request[TRIP_REQUEST_KEY] = trip; } if (!trip) throw new HttpException({ error: 'Trip not found' }, 404); diff --git a/server/src/nest/plugins/contributions/trip-card-contributions.controller.ts b/server/src/nest/plugins/contributions/trip-card-contributions.controller.ts index 4eacd9d3b..060957bfd 100644 --- a/server/src/nest/plugins/contributions/trip-card-contributions.controller.ts +++ b/server/src/nest/plugins/contributions/trip-card-contributions.controller.ts @@ -104,8 +104,18 @@ export class TripCardContributionsController { .filter((n) => Number.isInteger(n) && n > 0) .slice(0, MAX_TRIP_IDS); const uniqueRequested = [...new Set(requested)]; - const accessFlags = await Promise.all(uniqueRequested.map((id) => this.dbs.canAccessTrip(id, userId))); - const accessible = uniqueRequested.filter((_, i) => accessFlags[i]); + // Sequential, not `Promise.all` (Plan 3c Task 0b, task-0a-review-security.md + // F-A5): the 0a async sweep's `Promise.all` was a runtime no-op then (every + // read settled on the SAME synchronous free function), but `canAccessTrip` + // is a genuine repository read now — dispatching `uniqueRequested.length` + // of them concurrently would run up to `MAX_TRIP_IDS` queries against one + // forked EntityManager at once, which MikroORM does not support. `accessible` + // is passed positionally to `hooks.tripCards` below, so the order this + // produces (the filtered `uniqueRequested` order) is load-bearing. + const accessible: number[] = []; + for (const id of uniqueRequested) { + if (await this.dbs.canAccessTrip(id, userId)) accessible.push(id); + } if (accessible.length === 0) return { contributions: [] }; const allowed = new Set(accessible); diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index 190b34b11..0a4d37fc5 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -1,6 +1,8 @@ import { Injectable } from '@nestjs/common'; import path from 'path'; +import { EntityManager } from '@mikro-orm/core'; import { DatabaseService } from '../database/database.service'; +import { Trips } from '../../db/entities/Trips.entity'; import { MAX_TRIP_DAYS, tripSpanDays, type ActiveTrip, type TrekWsPayload, type TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -161,6 +163,13 @@ export class TripsService { private readonly unsplash: UnsplashService, private readonly storage: StorageService, private readonly uow: UnitOfWork, + // Plan 3c Task 0b (task-0a-review-security.md F-A1): `canAccessTrip`/ + // `isOwner` below resolve `TripsRepository` directly through this, + // rather than through `this.dbs.canAccessTrip`/`isOwner` — the third of + // the security review's three sites, alongside `TripAccessGuard` and + // `TripOwnerGuard`. `EntityManager` is `@Global()` (`MikroOrmModule + // .forRoot`'s core module), so no module needs new wiring. + private readonly em: EntityManager, ) {} private get db() { @@ -168,12 +177,12 @@ export class TripsService { } async canAccessTrip(tripId: string | number, userId: number) { - const access = await this.dbs.canAccessTrip(tripId, userId); + const access = await this.em.getRepository(Trips).findAccessible(tripId, userId); return access as { user_id: number } | null | undefined; } async isOwner(tripId: string | number, userId: number): Promise { - return await this.dbs.isOwner(tripId, userId); + return await this.em.getRepository(Trips).isOwner(tripId, userId); } async can(action: string, role: string, ownerId: number | null, userId: number, isMember: boolean): Promise { diff --git a/server/tests/e2e/assignments.e2e.test.ts b/server/tests/e2e/assignments.e2e.test.ts index f46222abe..2f31f5388 100644 --- a/server/tests/e2e/assignments.e2e.test.ts +++ b/server/tests/e2e/assignments.e2e.test.ts @@ -50,6 +50,10 @@ const { db } = vi.hoisted(() => { created_at TEXT DEFAULT CURRENT_TIMESTAMP);`); tmp.exec(`CREATE TABLE tags (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, color TEXT, created_at TEXT);`); tmp.exec(`CREATE TABLE place_tags (place_id INTEGER NOT NULL, tag_id INTEGER NOT NULL);`); + // TripAccessGuard now reads TripsRepository.findAccessible directly + // (Plan 3c Task 0b), a real join against trip_members. + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, currency TEXT);'); + tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); // StorageRegistryService (behind StorageModule, now in this module chain) reads // this at onModuleInit. tmp.exec('CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);'); @@ -98,6 +102,11 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => { beforeAll(async () => { seedUser(db as never, { id: 1 }); seedUser(db as never, { id: 2, username: 'peer', email: 'peer@example.test' }); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it, so trip 5's real row (owned by user 1) is + // seeded once here rather than faked per test. + db.prepare("INSERT INTO trips (id, user_id, title) VALUES (5, 1, 'Trip')").run(); db.prepare('INSERT INTO days (id, trip_id) VALUES (3, 5), (4, 5)').run(); db.prepare('INSERT INTO places (id, trip_id, name) VALUES (2, 5, ?)').run('Louvre'); app = await build(); @@ -106,7 +115,6 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => { }); beforeEach(() => { - canAccessTrip.mockReturnValue({ id: 5, user_id: 1 }); checkPermission.mockReturnValue(true); db.prepare('DELETE FROM day_assignments').run(); db.prepare('DELETE FROM assignment_participants').run(); @@ -331,7 +339,11 @@ describe('Assignments e2e (real auth guard + temp SQLite)', () => { const FOREIGN_TRIP = 9; beforeEach(() => { - canAccessTrip.mockImplementation((tripId: unknown) => (Number(tripId) === 5 ? { id: 5, user_id: 1 } : undefined)); + // Plan 3c Task 0b: real access now — trip 9 exists but is owned by + // user 2 (not the caller, user 1, and not a member), so + // TripsRepository.findAccessible genuinely refuses it, the same + // outcome `canAccessTrip.mockImplementation` used to fake. + db.prepare('INSERT OR IGNORE INTO trips (id, user_id, title) VALUES (?, 2, ?)').run(FOREIGN_TRIP, 'Their trip'); db.prepare('INSERT OR IGNORE INTO days (id, trip_id) VALUES (30, ?), (31, ?)').run(FOREIGN_TRIP, FOREIGN_TRIP); db.prepare('INSERT OR IGNORE INTO places (id, trip_id, name) VALUES (20, ?, ?)').run(FOREIGN_TRIP, 'Their hotel'); }); diff --git a/server/tests/e2e/budget.e2e.test.ts b/server/tests/e2e/budget.e2e.test.ts index 9463bc694..f3c16e782 100644 --- a/server/tests/e2e/budget.e2e.test.ts +++ b/server/tests/e2e/budget.e2e.test.ts @@ -104,10 +104,20 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => { }); it('404 when the trip is not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); - const res = await request(server).get(`/api/trips/${tripId}/budget`).set('Cookie', sessionCookie(1)); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Trip not found' }); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it. The trip row is seeded once in `beforeAll` (not + // re-seeded per test), so it is removed and restored around this one + // assertion instead. + db.prepare('DELETE FROM trips WHERE id = ?').run(tripId); + try { + const res = await request(server).get(`/api/trips/${tripId}/budget`).set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + } finally { + db.prepare("INSERT INTO trips (id, user_id, title, currency) VALUES (?, 1, 'E2E Trip', 'EUR')").run(tripId); + db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, 2)').run(tripId); + } }); it('201 on create with permission, then 200 list returns the stored row', async () => { diff --git a/server/tests/e2e/collab.e2e.test.ts b/server/tests/e2e/collab.e2e.test.ts index f109c6757..51e0e568b 100644 --- a/server/tests/e2e/collab.e2e.test.ts +++ b/server/tests/e2e/collab.e2e.test.ts @@ -26,8 +26,10 @@ const { db } = vi.hoisted(() => { avatar TEXT);`); // The note/message notifications read the trip title fire-and-forget; the table // must exist so that query doesn't throw after the test has torn down. - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT);'); - tmp.prepare("INSERT INTO trips (id, title) VALUES (5, 'Trip')").run(); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, user_id INTEGER, currency TEXT);'); + // TripAccessGuard now reads TripsRepository.findAccessible directly + // (Plan 3c Task 0b), a real join against trip_members. + tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); // CollabService's real SQL (DI-injected, no mock) — the collab tables as the // real schema + migrations shape them (website on notes, deleted on messages, // note_id on trip_files). @@ -104,7 +106,12 @@ describe('Collab e2e (real auth guard + temp SQLite)', () => { }); beforeEach(() => { - canAccessTrip.mockReturnValue({ id: 5, user_id: 1 }); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it, so trip 5's real row is (re-)seeded every test + // instead, owned by user 1, matching what the mock used to fake. + db.prepare('DELETE FROM trips WHERE id = 5').run(); + db.prepare("INSERT INTO trips (id, title, user_id) VALUES (5, 'Trip', 1)").run(); checkPermission.mockReturnValue(true); db.prepare('DELETE FROM collab_message_reactions').run(); db.prepare('DELETE FROM collab_poll_votes').run(); @@ -132,7 +139,7 @@ describe('Collab e2e (real auth guard + temp SQLite)', () => { }); it('404 when the trip is not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server).get('/api/trips/5/collab/notes').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Trip not found' }); @@ -184,7 +191,7 @@ describe('Collab e2e (real auth guard + temp SQLite)', () => { // The advisory this route was reported under: it answered anyone with a session, // for any trip id, and drove an outbound fetch from that. it('404 on link-preview for a trip the caller cannot reach', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server) .get('/api/trips/5/collab/link-preview?url=https://example.com/') .set('Cookie', sessionCookie(1)); @@ -242,7 +249,7 @@ describe('Collab e2e (real auth guard + temp SQLite)', () => { }); it('404 when the trip is not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server).get('/api/trips/5/collab/links').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); }); diff --git a/server/tests/e2e/days.e2e.test.ts b/server/tests/e2e/days.e2e.test.ts index 3576ab89d..01b941141 100644 --- a/server/tests/e2e/days.e2e.test.ts +++ b/server/tests/e2e/days.e2e.test.ts @@ -23,7 +23,7 @@ const { db } = vi.hoisted(() => { 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);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, end_date TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, end_date TEXT, currency TEXT);'); tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); // The tables DaysService really queries (real SQL, no service mock). tmp.exec(`CREATE TABLE days (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, diff --git a/server/tests/e2e/doc-sync.e2e.test.ts b/server/tests/e2e/doc-sync.e2e.test.ts index 488c725d7..1c50ab259 100644 --- a/server/tests/e2e/doc-sync.e2e.test.ts +++ b/server/tests/e2e/doc-sync.e2e.test.ts @@ -75,6 +75,8 @@ import { DocSyncModule } from '../../src/nest/doc-sync/doc-sync.module'; import { DocSyncMcp } from '../../src/nest/doc-sync/doc-sync.mcp'; import type { McpContext } from '../../src/nest-mcp'; import { DatabaseModule } from '../../src/nest/database/database.module'; +import { MikroORM } from '@mikro-orm/core'; +import { withRequestContext } from '../../src/nest/database/request-context'; import { AddonsService } from '../../src/nest/addons/addons.service'; import { DOCUMENT_PROVIDERS } from '../../src/nest/doc-sync/document-provider'; import { PaperlessDocumentProvider } from '../../src/nest/doc-sync/providers/paperless.provider'; @@ -401,9 +403,14 @@ describe('Document sync e2e (real guards + real services + temp SQLite)', () => .expect(200); expect(links.body[0]).toMatchObject({ providerId: 'paperless', providerName: 'Paperless-ngx' }); - // The assistant reads the same bindings through its own tool. + // The assistant reads the same bindings through its own tool. Called + // directly on the injected controller (not through the real /mcp + // transport), which is what forks a request context for a genuine MCP + // call (nest-mcp/registry.ts) — wrapped here for the same reason + // (Plan 3c Task 0b: `verifyTripAccess` now reaches `TripsRepository`, + // which validates one). const ctx = { userId: memberId, scopes: null, isStaticToken: false } as McpContext; - const result = await app.get(DocSyncMcp).getTripDocumentSync({ tripId }, ctx); + const result = await withRequestContext(app.get(MikroORM), () => app.get(DocSyncMcp).getTripDocumentSync({ tripId }, ctx)); const status = JSON.parse(result.content[0].text) as { links: Array> }; expect(status.links[0]).toMatchObject({ providerId: 'paperless', providerName: 'Paperless-ngx' }); } finally { diff --git a/server/tests/e2e/feeds.e2e.test.ts b/server/tests/e2e/feeds.e2e.test.ts index 3842650c5..476bdc0ce 100644 --- a/server/tests/e2e/feeds.e2e.test.ts +++ b/server/tests/e2e/feeds.e2e.test.ts @@ -30,7 +30,7 @@ const { db } = vi.hoisted(() => { email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0, feed_token TEXT);`); tmp.exec(`CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, - title TEXT, is_archived INTEGER NOT NULL DEFAULT 0, start_date TEXT, end_date TEXT, feed_token TEXT);`); + title TEXT, is_archived INTEGER NOT NULL DEFAULT 0, start_date TEXT, end_date TEXT, feed_token TEXT, currency TEXT);`); tmp.exec(`CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);`); // StorageRegistryService (behind StorageModule, now in this module chain) reads // this at onModuleInit. diff --git a/server/tests/e2e/files.e2e.test.ts b/server/tests/e2e/files.e2e.test.ts index 37484bb72..32013c326 100644 --- a/server/tests/e2e/files.e2e.test.ts +++ b/server/tests/e2e/files.e2e.test.ts @@ -25,7 +25,10 @@ const { db } = vi.hoisted(() => { // FilesService runs its real SQL against these (FILE_SELECT joins reservations // and users; the link batch reads file_links; findForeignLinkTarget probes // reservations/places/day_assignments). - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, currency TEXT);'); + // TripAccessGuard now reads TripsRepository.findAccessible directly + // (Plan 3c Task 0b), a real join against trip_members. + tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); 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, @@ -132,10 +135,19 @@ describe('Files + photos e2e (real auth guard + temp SQLite)', () => { }); it('404 when the trip is not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); - const res = await request(server).get('/api/trips/5/files').set('Cookie', sessionCookie(1)); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Trip not found' }); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it. Trip 5 is a persistent row seeded once in + // `beforeAll` (not re-seeded per test), so it is removed and restored + // around this one assertion instead. + db.prepare('DELETE FROM trips WHERE id = 5').run(); + try { + const res = await request(server).get('/api/trips/5/files').set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + } finally { + db.prepare("INSERT INTO trips (id, user_id, title) VALUES (5, 1, 'Trip')").run(); + } }); it('200 toggling a star with permission (real UPDATE + re-select)', async () => { diff --git a/server/tests/e2e/packing.e2e.test.ts b/server/tests/e2e/packing.e2e.test.ts index 899e81b8c..6d9c039d1 100644 --- a/server/tests/e2e/packing.e2e.test.ts +++ b/server/tests/e2e/packing.e2e.test.ts @@ -21,7 +21,7 @@ const { db } = vi.hoisted(() => { 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, display_name TEXT, avatar TEXT);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, currency TEXT);'); tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); // The post-migration shape of the packing tables (schema.ts + migrations.ts). tmp.exec(`CREATE TABLE packing_items ( diff --git a/server/tests/e2e/places.e2e.test.ts b/server/tests/e2e/places.e2e.test.ts index c58c7e769..e6d0bbc93 100644 --- a/server/tests/e2e/places.e2e.test.ts +++ b/server/tests/e2e/places.e2e.test.ts @@ -40,7 +40,10 @@ const { db } = vi.hoisted(() => { // The assignment=unassigned/assigned filters join these. tmp.exec('CREATE TABLE days (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER NOT NULL, day_number INTEGER, date TEXT, title TEXT);'); // The GPX export reads the trip title for and the filename. - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, user_id INTEGER, currency TEXT);'); + // TripAccessGuard now reads TripsRepository.findAccessible directly + // (Plan 3c Task 0b), a real join against trip_members. + tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); 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);`); // reclaimPlaceImage ref-counts an uploaded thumbnail across both tables. @@ -127,6 +130,13 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { beforeEach(() => { db.exec('DELETE FROM trips; DELETE FROM places; DELETE FROM place_tags; DELETE FROM place_ratings; DELETE FROM day_assignments; DELETE FROM days;'); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it, so trip 5's real row is (re-)seeded every test + // instead, owned by user 1, matching what the mock used to fake. GPX + // tests below re-seed their own data-bearing row with `INSERT OR + // REPLACE` over this default. + db.prepare("INSERT INTO trips (id, title, user_id) VALUES (5, 'Trip', 1)").run(); canAccessTrip.mockReturnValue({ id: 5, user_id: 1 }); checkPermission.mockReturnValue(true); }); @@ -196,7 +206,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { // And it fires ahead of the trip-access 404 it used to follow (documented // parity shift of the ratchet — the todo/trips precedent). - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const noTrip = await request(server).post('/api/trips/5/places').set('Cookie', sessionCookie(1)).send({}); expect(noTrip.status).toBe(400); }); @@ -306,7 +316,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { }); it('404 trip when not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server).get('/api/trips/5/places').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Trip not found' }); @@ -328,7 +338,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { }); it('the guarded read routes 404 an inaccessible trip without touching the place', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); for (const path of ['/api/trips/5/places/9', '/api/trips/5/places/9/image']) { const res = await request(server).get(path).set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); @@ -342,7 +352,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { // ── GPX export ──────────────────────────────────────────────────────────── describe('GET export.gpx (#1442)', () => { const seedTrip = () => { - db.prepare("INSERT INTO trips (id, title) VALUES (5, 'Alpine week')").run(); + db.prepare("INSERT OR REPLACE INTO trips (id, title, user_id) VALUES (5, 'Alpine week', 1)").run(); db.prepare("INSERT INTO places (id, trip_id, name, lat, lng) VALUES (1, 5, 'Trailhead', 47.1, 11.2)").run(); db.prepare("INSERT INTO places (id, trip_id, name, lat, lng, route_geometry) VALUES (2, 5, 'Ridge', 47.2, 11.3, '[[47.2,11.3],[47.25,11.35]]')").run(); db.prepare("INSERT INTO days (id, trip_id, day_number, date, title) VALUES (1, 5, 1, '2026-05-01', 'Warm up')").run(); @@ -366,7 +376,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { // ERR_INVALID_CHAR before the first body byte. Now the header folds to // ASCII and carries the real name RFC 5987-encoded. it('a non-ASCII trip title exports 200 with a filename* header instead of a 500 (#2165)', async () => { - db.prepare('INSERT INTO trips (id, title) VALUES (5, ?)').run('沖縄 4泊5日'); + db.prepare('INSERT OR REPLACE INTO trips (id, title, user_id) VALUES (5, ?, 1)').run('沖縄 4泊5日'); db.prepare('INSERT INTO places (id, trip_id, name, lat, lng) VALUES (1, 5, ?, 26.217, 127.719)').run('首里城'); const res = await request(server).get('/api/trips/5/places/export.gpx').set('Cookie', sessionCookie(1)); @@ -400,7 +410,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { }); it('404s an empty trip rather than handing over a file that imports as nothing', async () => { - db.prepare("INSERT INTO trips (id, title) VALUES (5, 'Nothing here')").run(); + db.prepare("INSERT OR REPLACE INTO trips (id, title, user_id) VALUES (5, 'Nothing here', 1)").run(); const res = await request(server).get('/api/trips/5/places/export.gpx').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Nothing to export' }); @@ -408,7 +418,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { it('404s a trip the caller cannot reach, and 401s without a cookie', async () => { seedTrip(); - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server).get('/api/trips/5/places/export.gpx').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Trip not found' }); @@ -428,7 +438,7 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { // before the pipe, so guarding create would answer 404 where the suite above // pins a 400. This is the non-regression pin for that decision. it('a bad create body still 400s ahead of the trip 404', async () => { - canAccessTrip.mockReturnValue(undefined); + db.prepare('DELETE FROM trips WHERE id = 5').run(); const res = await request(server).post('/api/trips/5/places').set('Cookie', sessionCookie(1)).send({}); expect(res.status).toBe(400); }); diff --git a/server/tests/e2e/reservations.e2e.test.ts b/server/tests/e2e/reservations.e2e.test.ts index 7ea99308c..6de828f6a 100644 --- a/server/tests/e2e/reservations.e2e.test.ts +++ b/server/tests/e2e/reservations.e2e.test.ts @@ -129,10 +129,19 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real }); it('404 when trip not accessible (reservations)', async () => { - canAccessTrip.mockReturnValue(undefined); - const res = await request(server).get(`/api/trips/${tripId}/reservations`).set('Cookie', sessionCookie(1)); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Trip not found' }); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — `canAccessTrip.mockReturnValue(...)` no + // longer intercepts it. The trip row is seeded once in `beforeAll` (not + // re-seeded per test), so it is removed and restored around this one + // assertion instead. + db.prepare('DELETE FROM trips WHERE id = ?').run(tripId); + try { + const res = await request(server).get(`/api/trips/${tripId}/reservations`).set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + } finally { + db.prepare("INSERT INTO trips (id, user_id, title) VALUES (?, 1, 'E2E Trip')").run(tripId); + } }); it('201 create reservation (real insert + booking notification), 400 without title', async () => { @@ -249,10 +258,16 @@ describe('Reservations + accommodations e2e (real auth guard + temp SQLite, real }); it('404 when trip not accessible (accommodations)', async () => { - canAccessTrip.mockReturnValue(undefined); - const res = await request(server).get(`/api/trips/${tripId}/accommodations`).set('Cookie', sessionCookie(1)); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Trip not found' }); + // See the reservations 404 case above for why this deletes/restores the + // real row instead of mocking canAccessTrip. + db.prepare('DELETE FROM trips WHERE id = ?').run(tripId); + try { + const res = await request(server).get(`/api/trips/${tripId}/accommodations`).set('Cookie', sessionCookie(1)); + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Trip not found' }); + } finally { + db.prepare("INSERT INTO trips (id, user_id, title) VALUES (?, 1, 'E2E Trip')").run(tripId); + } }); it('400 accommodation create without refs', async () => { diff --git a/server/tests/e2e/roadtrip.e2e.test.ts b/server/tests/e2e/roadtrip.e2e.test.ts index 16219c4f3..0e4ff1e0b 100644 --- a/server/tests/e2e/roadtrip.e2e.test.ts +++ b/server/tests/e2e/roadtrip.e2e.test.ts @@ -40,7 +40,7 @@ const { db } = vi.hoisted(() => { 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);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, end_date TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, end_date TEXT, currency TEXT);'); tmp.exec( 'CREATE TABLE roadtrip_preferences (trip_id INTEGER NOT NULL REFERENCES trips(id) ON DELETE CASCADE, key TEXT NOT NULL, value TEXT NOT NULL, PRIMARY KEY(trip_id, key));', ); diff --git a/server/tests/e2e/todo.e2e.test.ts b/server/tests/e2e/todo.e2e.test.ts index 7c3224585..9efa89d4b 100644 --- a/server/tests/e2e/todo.e2e.test.ts +++ b/server/tests/e2e/todo.e2e.test.ts @@ -21,7 +21,7 @@ const { db } = vi.hoisted(() => { 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);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER, title TEXT, currency TEXT);'); tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); tmp.exec(`CREATE TABLE todo_items ( id INTEGER PRIMARY KEY AUTOINCREMENT, diff --git a/server/tests/e2e/trip-invite.e2e.test.ts b/server/tests/e2e/trip-invite.e2e.test.ts index 755a50883..13e0d54b2 100644 --- a/server/tests/e2e/trip-invite.e2e.test.ts +++ b/server/tests/e2e/trip-invite.e2e.test.ts @@ -21,7 +21,10 @@ const { db } = vi.hoisted(() => { tmp.exec('PRAGMA journal_mode = WAL'); tmp.exec(`CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0);`); - tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT);'); + tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, user_id INTEGER, currency TEXT);'); + // TripAccessGuard/TripOwnerGuard now read TripsRepository.findAccessible + // directly (Plan 3c Task 0b), a real join against trip_members. + tmp.exec('CREATE TABLE trip_members (trip_id INTEGER NOT NULL, user_id INTEGER NOT NULL);'); // TripInviteService now runs its real SQL (DI-injected, no mock) — mirror of // the trip_invite_tokens DDL from migration 153. tmp.exec(`CREATE TABLE trip_invite_tokens ( @@ -81,8 +84,8 @@ describe('Trip invite-link e2e (real auth guard + temp SQLite)', () => { return nest; } - function seedTrip(id: number, title: string) { - db.prepare('INSERT INTO trips (id, title) VALUES (?, ?)').run(id, title); + function seedTrip(id: number, title: string, ownerId = 1) { + db.prepare('INSERT INTO trips (id, title, user_id) VALUES (?, ?, ?)').run(id, title, ownerId); } function seedToken(tripId: number, token: string, expiresAt: string | null = null) { db.prepare('INSERT INTO trip_invite_tokens (trip_id, token, created_by, expires_at) VALUES (?, ?, 1, ?)') @@ -159,19 +162,24 @@ describe('Trip invite-link e2e (real auth guard + temp SQLite)', () => { }); it('403 to create without share_manage', async () => { + seedTrip(5, 'Lisbon'); checkPermission.mockReturnValue(false); const res = await request(server).post('/api/trips/5/invite-link').set('Cookie', sessionCookie(1)).send({}); expect(res.status).toBe(403); }); it('403 to READ the link without share_manage (token grants membership)', async () => { + seedTrip(5, 'Lisbon'); checkPermission.mockReturnValue(false); const res = await request(server).get('/api/trips/5/invite-link').set('Cookie', sessionCookie(1)); expect(res.status).toBe(403); }); it('404 when the trip is not accessible', async () => { - canAccessTrip.mockReturnValue(undefined); + // Plan 3c Task 0b: TripAccessGuard reads TripsRepository.findAccessible + // directly now, a real query — no trip 5 row exists (nothing in this + // test seeded one, and `beforeEach` clears the table), so the guard's + // real 404 fires without a `canAccessTrip` mock to fake it. const res = await request(server).get('/api/trips/5/invite-link').set('Cookie', sessionCookie(1)); expect(res.status).toBe(404); }); diff --git a/server/tests/helpers/accommodations-service.ts b/server/tests/helpers/accommodations-service.ts index 04cae211d..c9f1fc54a 100644 --- a/server/tests/helpers/accommodations-service.ts +++ b/server/tests/helpers/accommodations-service.ts @@ -7,7 +7,7 @@ import { PermissionsService } from '../../src/nest/permissions/permissions.servi import { QueryHelpersService } from '../../src/nest/query-helpers/query-helpers.service'; import { RealtimeService } from '../../src/nest/realtime/realtime.service'; import { TrekPhotosRepository } from '../../src/nest/photos/trek-photos.repository'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from './test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from './test-uow'; /** * AccommodationsService over a test connection. @@ -16,9 +16,14 @@ import { createTestUnitOfWork, createTestAppSettingsRepo } from './test-uow'; * real AssignmentsService rather than a stub: the stop is a row these cases read * back, and it has to come out of the same connection. Five collaborators deep is * why this is a helper and not five copies across the suites. + * + * Plan 3c Task 0b: the `DatabaseService` built here needs a real + * `EntityManager` now — `AccommodationsService.stampLodging` reaches + * `getPlaceWithTags`, which is `PlacesRepository.findWithTagsAndRatings` + * (not `db/database.ts`'s deleted free function). */ export async function makeAccommodationsService(conn: Database): Promise { - return accommodationsOver(new DatabaseService(conn)); + return accommodationsOver(new DatabaseService(conn, (await sharedTestOrm(conn)).em)); } /** diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 019c8fff5..ee6ef6a3e 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -133,10 +133,15 @@ import { PasswordResetTokens } from '../../src/db/entities/PasswordResetTokens.e * src/db/database flowing through (same pattern as todo.bridge.ts). */ export async function createMcpTestRegistry(): Promise { - const dbService = new DatabaseService(db); + // Plan 3c Task 0b: `mcpOrm` built first (was built right after `dbService`) + // so its `EntityManager` can be threaded into `DatabaseService`'s + // constructor — `canAccessTrip`/`isOwner`/`rosterUserIds`/`getPlaceWithTags` + // resolve `TripsRepository`/`TripMembersRepository`/`PlacesRepository` + // through it now, not through `db/database.ts`'s deleted free functions. + const mcpOrm = await createTestOrm(db); + const dbService = new DatabaseService(db, mcpOrm.em); const generalStorage = makeStorageFixture('').storage; const appSettings = (await createTestOrm(dbService.connection)).repo(AppSettings); - const mcpOrm = await createTestOrm(dbService.connection); const auditLogRepo = mcpOrm.repo(AuditLog); const usersRepo = mcpOrm.repo(Users); const inviteTokensRepo = mcpOrm.repo(InviteTokens); @@ -206,6 +211,7 @@ export async function createMcpTestRegistry(): Promise { new UnsplashService(appSettings, usersRepo, new RuntimeEnvService(), generalStorage), generalStorage, await createTestUnitOfWork(dbService.connection), + mcpOrm.em, ); const readModelService = new TripReadModelService( dbService, membersService, daysService, accommodationsService, budgetService, diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 86016c9ee..e29ea2a98 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -119,7 +119,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< const places = new PlacesService(dbs, permissions, realtime, new MapsService(dbs, photoCache, appSettings, usersRepo), queryHelpers, unsplash, photoCache, journey, generalStorage, accommodations, await createTestUnitOfWork(dbs.connection)); // After accommodations: a hotel booking writes the stay's day stop through it. const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs), accommodations, await createTestUnitOfWork(dbs.connection)); - const trips = new TripsService(dbs, reservations, days, permissions, budget, vacay, realtime, unsplash, generalStorage, await createTestUnitOfWork(dbs.connection)); + const trips = new TripsService(dbs, reservations, days, permissions, budget, vacay, realtime, unsplash, generalStorage, await createTestUnitOfWork(dbs.connection), (await sharedTestOrm(dbs.connection)).em); const members = new TripMembersService(dbs, budget, new UserCleanupService(dbs, budget, await createTestUnitOfWork(dbs.connection), usersRepo), permissions, realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection)); const guards = new PluginGuards(dbs, permissions, addons); diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index 6c5a31072..a4e984ec1 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -24,6 +24,13 @@ import { OauthTokens } from '../../src/db/entities/OauthTokens.entity'; import type { OauthTokensRepository } from '../../src/db/repositories/OauthTokens.repository'; import { PasswordResetTokens } from '../../src/db/entities/PasswordResetTokens.entity'; import type { PasswordResetTokensRepository } from '../../src/db/repositories/PasswordResetTokens.repository'; +import { Trips } from '../../src/db/entities/Trips.entity'; +import type { TripsRepository } from '../../src/db/repositories/Trips.repository'; +import { TripMembers } from '../../src/db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../src/db/repositories/TripMembers.repository'; +import { Places } from '../../src/db/entities/Places.entity'; +import type { PlacesRepository } from '../../src/db/repositories/Places.repository'; +import { DatabaseService } from '../../src/nest/database/database.service'; const perHandle = new WeakMap>(); const appSettingsPerHandle = new WeakMap>(); @@ -277,3 +284,61 @@ export function createTestPasswordResetTokensRepo(db: Database.Database): Promis passwordResetTokensPerHandle.set(db, pending); return pending; } + +// --------------------------------------------------------------------------- +// Plan 3c Task 0b (trip-access primitives onto TripsRepository/ +// TripMembersRepository/PlacesRepository) — appended at the end per the +// task's own file-ownership rule. Same memoisation-per-handle pattern as +// every helper above. +// --------------------------------------------------------------------------- + +const tripsRepoPerHandle = new WeakMap>(); +const tripMembersRepoPerHandle = new WeakMap>(); +const placesRepoPerHandle = new WeakMap>(); + +/** The `TripsRepository` a hand-constructed `DatabaseService`/`TripAccessGuard` double needs. */ +export function createTestTripsRepo(db: Database.Database): Promise { + const existing = tripsRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(Trips)); + tripsRepoPerHandle.set(db, pending); + return pending; +} + +/** The `TripMembersRepository` `DatabaseService.rosterUserIds` needs. */ +export function createTestTripMembersRepo(db: Database.Database): Promise { + const existing = tripMembersRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(TripMembers)); + tripMembersRepoPerHandle.set(db, pending); + return pending; +} + +/** The `PlacesRepository` `DatabaseService.getPlaceWithTags` needs. */ +export function createTestPlacesRepo(db: Database.Database): Promise { + const existing = placesRepoPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(Places)); + placesRepoPerHandle.set(db, pending); + return pending; +} + +/** + * A `DatabaseService` bound to the suite's own better-sqlite3 handle, with + * its `EntityManager` wired through `sharedTestOrm` — so `canAccessTrip`/ + * `isOwner`/`rosterUserIds`/`getPlaceWithTags` (Plan 3c Task 0b, now + * repository-backed rather than delegating to `db/database.ts`'s deleted + * free functions) work on a directly-constructed instance the same way they + * do through Nest DI in production. `sharedTestOrm`'s ORM is built with + * `allowGlobalContext: true` (`test-orm.ts`), so these calls need no + * `withRequestContext` wrapper here, unlike production. + * + * A file that only needs `new DatabaseService(db)`'s other methods + * (`prepare`/`get`/`all`/`run`) does not need this helper — the + * `EntityManager` constructor argument is `@Optional()`, and only these four + * methods use it. + */ +export async function createTestDatabaseService(db: Database.Database): Promise { + const t = await sharedTestOrm(db); + return new DatabaseService(db, t.em); +} diff --git a/server/tests/integration/demo-seed-request-context.test.ts b/server/tests/integration/demo-seed-request-context.test.ts new file mode 100644 index 000000000..d38254fde --- /dev/null +++ b/server/tests/integration/demo-seed-request-context.test.ts @@ -0,0 +1,76 @@ +/** + * `runDemoSeed()` (`src/demo/demo-seed.ts`, called from `runSchemaBootstrap` + * in `db/orm.ts:44`) used to run before the first `withRequestContext` + * (`bootstrap.ts:108`) and on every restore (`db/orm.ts`'s reinitialize + * hook) — entirely raw `better-sqlite3`, so nothing needed a context yet, + * but the seed's own docstring pre-authorised the fix: "The domain phase + * that gives demo seeding a repository read must wrap it in + * `withRequestContext` then." `runSchemaBootstrap` (Plan 3c Task 0b) now + * wraps the call in `withRequestContext(orm, …)`, the one place it already + * has `orm` in scope — covering both of `runDemoSeed`'s real call sites + * (`bootstrap.ts`'s initial boot and `attachOrm`'s restore hook) with no + * second wrapper to keep in sync. + * + * This is a real `DEMO_MODE` boot proving that wrap doesn't change what the + * seed does and logs no missing-context error, so Plan 3i's demo + * conversion never meets a second C1 (task-6-review-parity.md's boot-sweep + * finding). + * + * Deliberately does NOT `vi.mock('../../src/db/database', ...)` the way + * most integration suites do: `buildDbMock`'s `runDemoSeed` is a + * deliberate no-op stub (tests/helpers/db-mock.ts) so that suite's app + * boots don't carry demo-seed side effects — which would silence the very + * thing this file exists to exercise. `NODE_ENV=test` already gives + * `db/database.ts`'s own singleton an isolated, per-worker `:memory:` copy + * of the migrated schema snapshot (`readSchemaSnapshot()`), so no mock is + * needed here at all. + * + * `seedDemoData`'s own `require('../demo/demo-seed')` (a deliberate dynamic + * require — see that file's docstring) cannot resolve under vitest's SWC + * transform (`Cannot find module '../demo/demo-seed'`), independent of this + * task and of request-context wrapping — `runDemoSeed`'s own try/catch + * already swallows that into a `[Demo] Seed error:` log line in every + * environment, wrapped or not, which is exactly why `buildDbMock` stubs the + * whole function out for every other suite rather than letting it run for + * real. This test cannot assert the demo user was actually created for that + * reason (a genuine environment gap, not a functional one — production runs + * compiled Node, not a vitest/SWC transform); what it CAN and does assert is + * the one thing Task 0b changed: no `cannotUseGlobalContext`/"global + * EntityManager" line reaches the log, and the pre-existing module-resolution + * failure is what actually gets logged instead, unchanged by the wrap. + */ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import type { INestApplication } from '@nestjs/common'; +import { buildApp } from '../../src/bootstrap'; + +describe('runDemoSeed runs inside a request context (Plan 3c Task 0b)', () => { + let app: INestApplication | undefined; + const prevDemo = process.env.DEMO_MODE; + + afterEach(async () => { + await app?.close(); + app = undefined; + if (prevDemo === undefined) delete process.env.DEMO_MODE; + else process.env.DEMO_MODE = prevDemo; + }); + + it('DEMO-SEED-001: a DEMO_MODE boot runs the seed inside a request context (no cannotUseGlobalContext)', async () => { + process.env.DEMO_MODE = 'true'; + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + app = await buildApp(); + + // runDemoSeed ran at all (proven by SOME [Demo] line reaching the log — + // in this environment always the pre-existing require() gap above, but + // ANY [Demo] line proves the function was invoked, not skipped). + const demoLines = errSpy.mock.calls.map((args) => args.map(String).join(' ')).filter((line) => line.includes('[Demo]')); + expect(demoLines.length).toBeGreaterThan(0); + + // The one thing this task could regress: a missing request context. + const suspicious = demoLines.filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + } finally { + errSpy.mockRestore(); + } + }); +}); diff --git a/server/tests/integration/orm-request-context-seams.test.ts b/server/tests/integration/orm-request-context-seams.test.ts index a878c6e90..cde387130 100644 --- a/server/tests/integration/orm-request-context-seams.test.ts +++ b/server/tests/integration/orm-request-context-seams.test.ts @@ -49,11 +49,13 @@ vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi. import { db as testDb } from '../../src/db/database'; import { MikroORM } from '@mikro-orm/core'; +import { SchedulerRegistry } from '@nestjs/schedule'; import request from 'supertest'; import type { Application } from 'express'; import { buildApp } from '../../src/bootstrap'; import { CronRegistrarService } from '../../src/nest/scheduling/cron-registrar.service'; import { PluginRuntimeService } from '../../src/nest/plugins/plugin-runtime.service'; +import { PlaceShadowRetentionJob } from '../../src/nest/place-shadow/place-shadow.job'; import { StorageService } from '../../src/nest/storage/storage.service'; import { Users } from '../../src/db/entities/Users.entity'; import { createUser } from '../helpers/factories'; @@ -130,4 +132,45 @@ describe('ORM request-context seams populated in production', () => { spy.mockRestore(); } }); + + /** + * Plan 3c Task 0b (R9, inventory §12b): `PlaceShadowRetentionJob` only + * `register()`s a nightly tick — it has no `runOnBoot` sweep, so + * `boot-sweeps-request-context.test.ts`'s BOOT-SWEEP-001 cannot see it at + * all. `CronRegistrarService.register()` already wraps every registered + * tick in `withRequestContext` unconditionally (fail-closed on a missing + * `orm`, `cron-registrar.service.ts:87-92`) — but nothing before this task + * drove THIS job's own `onApplicationBootstrap` against the real, + * production-wired registrar to prove the whole path (register → a real + * `cron` `CronJob` lands in `SchedulerRegistry` → firing its tick resolves + * with no missing-context error) actually holds, the Plan 3a `*-003` + * per-job pattern for a register-only job. + */ + it('SEAM-003: PlaceShadowRetentionJob registers against the real CronRegistrarService, and firing its tick runs inside the same request context with no missing-context error', async () => { + const registrar = app.get(CronRegistrarService); + const isEnabledSpy = vi.spyOn(registrar, 'isEnabled').mockReturnValue(true); + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + testDb.prepare(` + INSERT INTO place_shadow_picks (created_at, query, source, live_rank, live_count, picked_name, picked_lat, picked_lng) + VALUES (datetime('now', '-999 days'), 'seam-003', 'nominatim', 1, 1, 'expired pick', 0, 0) + `).run(); + + app.get(PlaceShadowRetentionJob).onApplicationBootstrap(); + const job = app.get(SchedulerRegistry).getCronJob('place-shadow-retention'); + await job.fireOnTick(); + + const suspicious = errSpy.mock.calls + .map((args) => args.map(String).join(' ')) + .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + + const remaining = testDb.prepare("SELECT COUNT(*) as n FROM place_shadow_picks WHERE query = 'seam-003'").get() as { n: number }; + expect(remaining.n).toBe(0); + } finally { + registrar.unregister('place-shadow-retention'); + isEnabledSpy.mockRestore(); + errSpy.mockRestore(); + } + }); }); diff --git a/server/tests/integration/trip-access-primitives.test.ts b/server/tests/integration/trip-access-primitives.test.ts index c4c833c5c..f0df48a85 100644 --- a/server/tests/integration/trip-access-primitives.test.ts +++ b/server/tests/integration/trip-access-primitives.test.ts @@ -1,19 +1,31 @@ /** - * Parity guards for the Plan 3c Task 0a async sweep. + * Parity guards for the Plan 3c Task 0a async sweep, extended by Task 0b. * - * `canAccessTrip`, `isOwner`, `rosterUserIds` and `getPlaceWithTags` - * (`src/db/database.ts` + their `DatabaseService` delegations) went from - * synchronous to `async` in that task, with every caller updated to `await` - * them. These four tests each drive a REAL guarded route through the real - * `buildApp()` DI graph — one per primitive — as owner / member / non-member - * / anonymous, and pin the legacy status + body so a later task (0b: moving - * the bodies onto `TripsRepository`) cannot silently change behaviour. + * `canAccessTrip`, `isOwner`, `rosterUserIds` and `getPlaceWithTags` went + * from synchronous to `async` in Task 0a (with every caller updated to + * `await` them), still delegating to `src/db/database.ts`'s free functions. + * Task 0b deleted those free functions and moved the bodies onto + * `TripsRepository.findAccessible`/`isOwner`, `TripMembersRepository + * .rosterUserIds` and `PlacesRepository.findWithTagsAndRatings` — these four + * tests still drive a REAL guarded route through the real `buildApp()` DI + * graph, one per primitive, as owner / member / non-member / anonymous, and + * still pin the legacy status + body, but now against the real repository + * path rather than a stand-in. */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; import request from 'supertest'; import type { Application } from 'express'; import type { INestApplication } from '@nestjs/common'; +// task-0a-review F4: this mock no longer intercepts any of the four +// primitives — `db/database.ts` holds none of their bodies any more, so +// `buildDbMock`'s own `canAccessTrip`/`isOwner`/`getPlaceWithTags` +// properties are dead code for this file's purposes. It stays only for what +// every other integration/e2e/WS suite in this program uses it for: an +// isolated snapshot db that `getRawConnection`/`db` swap the shared +// better-sqlite3 handle to, which the REAL `MikroORM` binds to underneath +// (`db/orm-driver.ts`) — so `TripsRepository`/`TripMembersRepository` +// /`PlacesRepository` read the SAME rows this file seeds through `testDb`. vi.mock('../../src/db/database', async () => { const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); return buildDbMock(createSnapshotTestDb()); @@ -148,6 +160,43 @@ describe('isOwner (async) — the delete_trip MCP tool', () => { expect(testDb.prepare('SELECT id FROM trips WHERE id = ?').get(trip.id)).toBeUndefined(); }); + /** + * Plan 3c Task 0b (R9): `isOwner`'s only real caller anywhere is this MCP + * tool (inventory §0d), and `TripsService.isOwner` → `DatabaseService + * .isOwner` is now `TripsRepository.isOwner` underneath — a repository + * read reached through the MCP transport's request context (`nest-mcp + * /registry.ts:286-294`: `@mikro-orm/nestjs`'s `registerRequestContext` + * middleware forks a context for every `/mcp` route, same as any other + * Nest route). PRIM-ISOWN-001 above already proves the happy path (the + * trip is actually deleted, which requires the repository read to + * succeed); this is the explicit MCP-CTX-style structural assertion + * (`mcp.test.ts`'s MCP-CTX-001/002 pattern) that no missing-context error + * was swallowed along the way. + */ + it('PRIM-ISOWN-005 — the repository read inside isOwner runs inside the /mcp request context (MCP-CTX style, no cannotUseGlobalContext)', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const sessionId = await mcpSession(owner.id); + + const res = await request(app) + .post('/mcp') + .set('Authorization', `Bearer ${generateToken(owner.id)}`) + .set('mcp-session-id', sessionId) + .set('Accept', 'application/json, text/event-stream') + .send(deleteTripBody(trip.id)); + expect(res.status).toBe(200); + + const suspicious = errSpy.mock.calls + .map((args) => args.map(String).join(' ')) + .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + } finally { + errSpy.mockRestore(); + } + }); + it('PRIM-ISOWN-002 — member (not owner): refused with the same "no access" message as a stranger, trip untouched', async () => { const { user: owner } = createUser(testDb); const { user: member } = createUser(testDb); @@ -279,19 +328,34 @@ describe('getPlaceWithTags (async) — GET /api/trips/:tripId/places/:id', () => it('PRIM-GPWT-001 — owner: 200 with the hydrated place (category/tags/ratings shape)', async () => { const { user: owner } = createUser(testDb); const trip = createTrip(testDb, owner.id); + // createPlace defaults category_id to the first seeded category when no + // override is given (tests/helpers/factories.ts) — read back whatever it + // actually assigned rather than assuming none, so this pins the real + // production shape instead of a factory default that happens to be null. const place = createPlace(testDb, trip.id, { name: 'Eiffel Tower' }); + const expectedCategory = place.category_id + ? testDb.prepare('SELECT id, name, color, icon FROM categories WHERE id = ?').get(place.category_id) + : null; const res = await request(app).get(`/api/trips/${trip.id}/places/${place.id}`).set('Cookie', authCookie(owner.id)); expect(res.status).toBe(200); - // The mocked db/database module's getPlaceWithTags stand-in (tests/helpers/db-mock.ts) - // only carries category + tags, not the ratings aggregate — matched here, not the - // production shape (§4 of the test-db.ts header explains the mock/real split). + // Plan 3c Task 0b: `getPlaceWithTags` is `PlacesRepository + // .findWithTagsAndRatings` now, reached for real (this file's `vi.mock` + // of `src/db/database` only swaps the underlying connection to an + // isolated snapshot db — see the top-of-file note — it no longer stands + // in for the primitive itself, which db/database.ts doesn't hold any + // more). The full production shape, including the ratings aggregate the + // old mocked stand-in omitted, is asserted directly rather than only a + // subset. expect(res.body.place).toMatchObject({ id: place.id, name: 'Eiffel Tower', + category: expectedCategory, tags: [], + ratings: [], + rating_avg: null, + rating_count: 0, }); - expect(res.body.place).toHaveProperty('category'); }); it('PRIM-GPWT-002 — member: 200 with the same hydrated place', async () => { diff --git a/server/tests/unit/budget/costs.rpc.test.ts b/server/tests/unit/budget/costs.rpc.test.ts index c75322b4f..f675bcadc 100644 --- a/server/tests/unit/budget/costs.rpc.test.ts +++ b/server/tests/unit/budget/costs.rpc.test.ts @@ -33,7 +33,7 @@ function build(opts: { addonOn?: boolean; canEdit?: boolean; missing?: boolean } } as unknown as BudgetService & Record>; const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const db = { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService; const guards = new PluginGuards( diff --git a/server/tests/unit/days/day-notes.rpc.test.ts b/server/tests/unit/days/day-notes.rpc.test.ts index 2e1994f64..4db4e9c3d 100644 --- a/server/tests/unit/days/day-notes.rpc.test.ts +++ b/server/tests/unit/days/day-notes.rpc.test.ts @@ -35,7 +35,7 @@ function build(canEdit = true) { } as unknown as DayNotesService & Record>; const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const db = { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService; const guards = new PluginGuards( diff --git a/server/tests/unit/days/days-itinerary.rpc.test.ts b/server/tests/unit/days/days-itinerary.rpc.test.ts index 920696b1c..dadfb134f 100644 --- a/server/tests/unit/days/days-itinerary.rpc.test.ts +++ b/server/tests/unit/days/days-itinerary.rpc.test.ts @@ -44,7 +44,7 @@ function build(opts: { canEdit?: boolean } = {}) { const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; const guards = new PluginGuards( { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService, { checkPermission: vi.fn(() => opts.canEdit ?? true) } as unknown as PermissionsService, diff --git a/server/tests/unit/db/dialect/sql-functions.test.ts b/server/tests/unit/db/dialect/sql-functions.test.ts index 536ae2ed9..2aeaebfd4 100644 --- a/server/tests/unit/db/dialect/sql-functions.test.ts +++ b/server/tests/unit/db/dialect/sql-functions.test.ts @@ -5,7 +5,20 @@ import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createUser } from '../../../helpers/factories'; import { Users } from '../../../../src/db/entities/Users.entity'; -import { columnIncrementedBy, columnRef, currentTimestamp, dateAdd, dateOf, lower, lowerParam } from '../../../../src/db/dialect/sql-functions'; +import { + absDifference, + coalesce, + coalesceParam, + columnIncrementedBy, + columnRef, + currentTimestamp, + dateAdd, + dateOf, + lower, + lowerParam, + nowMinusDays, + trim, +} from '../../../../src/db/dialect/sql-functions'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -219,4 +232,98 @@ describe('sql-functions (sqlite)', () => { const none = await t.em.findOne(Users, { [lower(platform, 'email')]: lowerParam(platform, 'nobody@example.com') }); expect(none).toBeNull(); }); + + // Plan 3c Task 0b (R6): no consumer yet — Tasks 1–8 wire these in as each + // converts the statement that needs them. Pinned here so a later task + // imports a tested helper instead of writing one inline. + + it("SQLF-016: nowMinusDays renders datetime('now', '-N days'), matching a JS-computed date within a few seconds' tolerance", async () => { + const row = testDb.prepare(`SELECT ${nowMinusDays(t.em.getPlatform(), 5).sql} as d`).get() as { d: string }; + const got = new Date(`${row.d.replace(' ', 'T')}Z`).getTime(); + const expected = Date.now() - 5 * 24 * 60 * 60 * 1000; + expect(Math.abs(got - expected)).toBeLessThan(10_000); + }); + + it('SQLF-016b: nowMinusDays(0) is "now", not one day back', async () => { + const row = testDb.prepare(`SELECT ${nowMinusDays(t.em.getPlatform(), 0).sql} as d`).get() as { d: string }; + const got = new Date(`${row.d.replace(' ', 'T')}Z`).getTime(); + expect(Math.abs(got - Date.now())).toBeLessThan(10_000); + }); + + it('SQLF-017: nowMinusDays rejects a non-integer or negative day count', () => { + expect(() => nowMinusDays(t.em.getPlatform(), 1.5)).toThrow(/non-negative integer day count/); + expect(() => nowMinusDays(t.em.getPlatform(), -1)).toThrow(/non-negative integer day count/); + expect(() => nowMinusDays(t.em.getPlatform(), Number.NaN)).toThrow(/non-negative integer day count/); + }); + + it('SQLF-018: trim strips leading/trailing whitespace', async () => { + const { user } = createUser(testDb, { username: ' Padded Name ' }); + const platform = t.em.getPlatform(); + const row = await t.em.createQueryBuilder(Users, 'u') + .select([trim(platform, 'u.username').as('n')]) + .where({ id: user.id }) + .execute('get', false); + expect((row as { n: string }).n).toBe('Padded Name'); + }); + + it('SQLF-019: coalesce(ref, fallbackRef) picks the first non-null column, else the second', async () => { + const { user: withName } = createUser(testDb, { username: 'has-display-name' }); + testDb.prepare('UPDATE users SET display_name = ? WHERE id = ?').run('Display Name', withName.id); + const { user: withoutName } = createUser(testDb, { username: 'no-display-name' }); + const platform = t.em.getPlatform(); + + const a = await t.em.createQueryBuilder(Users, 'u') + .select([coalesce(platform, 'u.display_name', 'u.username').as('name')]) + .where({ id: withName.id }) + .execute('get', false); + expect((a as { name: string }).name).toBe('Display Name'); + + const b = await t.em.createQueryBuilder(Users, 'u') + .select([coalesce(platform, 'u.display_name', 'u.username').as('name')]) + .where({ id: withoutName.id }) + .execute('get', false); + expect((b as { name: string }).name).toBe('no-display-name'); + }); + + it('SQLF-020: coalesceParam(ref, value) writes COALESCE(col, ?) with the fallback bound, not another column', async () => { + const { user } = createUser(testDb); + const platform = t.em.getPlatform(); + await t.em.createQueryBuilder(Users, 'u') + .update({ display_name: coalesceParam(platform, 'display_name', 'Fallback Name') }) + .where({ id: user.id }) + .execute('run'); + t.clear(); + expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(user.id) as { display_name: string }).display_name).toBe('Fallback Name'); + + await t.em.createQueryBuilder(Users, 'u') + .update({ display_name: coalesceParam(platform, 'display_name', 'Never Used') }) + .where({ id: user.id }) + .execute('run'); + t.clear(); + expect((testDb.prepare('SELECT display_name FROM users WHERE id = ?').get(user.id) as { display_name: string }).display_name).toBe('Fallback Name'); + }); + + it('SQLF-021: absDifference is usable as a filter key, matching ABS(col - ?) <= tolerance', async () => { + const { user: near } = createUser(testDb); + testDb.prepare('UPDATE users SET login_count = 12 WHERE id = ?').run(near.id); + const { user: far } = createUser(testDb); + testDb.prepare('UPDATE users SET login_count = 100 WHERE id = ?').run(far.id); + const platform = t.em.getPlatform(); + + const rows = await t.em.find(Users, { [absDifference(platform, 'login_count', 10)]: { $lte: 5 } }); + expect(rows.map((r) => r.id)).toEqual([near.id]); + + const none = await t.em.find(Users, { [absDifference(platform, 'login_count', 10)]: { $lte: 0 } }); + expect(none).toEqual([]); + }); + + it('SQLF-022: an unknown platform fails closed for every Plan 3c Task 0b helper', () => { + class FakePlatform extends Platform {} + const foreign = new FakePlatform(); + expect(() => nowMinusDays(foreign, 1)).toThrow(/no implementation for platform FakePlatform/); + expect(() => trim(foreign, 'u.name')).toThrow(/no implementation for platform FakePlatform/); + expect(() => coalesce(foreign, 'u.a', 'u.b')).toThrow(/no implementation for platform FakePlatform/); + expect(() => coalesceParam(foreign, 'u.a', 'x')).toThrow(/no implementation for platform FakePlatform/); + expect(() => absDifference(foreign, 'u.lat', 1)).toThrow(/no implementation for platform FakePlatform/); + }); }); diff --git a/server/tests/unit/db/repositories/Places.repository.test.ts b/server/tests/unit/db/repositories/Places.repository.test.ts new file mode 100644 index 000000000..4129b9b24 --- /dev/null +++ b/server/tests/unit/db/repositories/Places.repository.test.ts @@ -0,0 +1,98 @@ +/** + * PlacesRepository.findWithTagsAndRatings (Plan 3c Task 0b): the legacy + * `getPlaceWithTags`'s three statements (place+category, tags, ratings), + * merged the same way. Real rows on the real test DB, including the + * non-empty tags/ratings shape neither `trip-access-primitives.test.ts`'s + * PRIM-GPWT-00x (empty tags/ratings only) nor any other suite exercises. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createCategory, createPlace, createTag, createTrip, createUser } from '../../../helpers/factories'; +import { Places } from '../../../../src/db/entities/Places.entity'; +import type { PlacesRepository } from '../../../../src/db/repositories/Places.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let places: PlacesRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + places = t.repo(Places); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('PlacesRepository.findWithTagsAndRatings — parity with the legacy getPlaceWithTags', () => { + it('PLACEREPO-001: no category, no tags, no ratings — null category, empty arrays, null average', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + // category_id: null override defeats createPlace's default-first-category fallback. + testDb.prepare('DELETE FROM categories').run(); + const place = createPlace(testDb, trip.id, { name: 'Uncategorized Spot' }); + + const result = await places.findWithTagsAndRatings(place.id); + expect(result).toMatchObject({ + id: place.id, + name: 'Uncategorized Spot', + category: null, + tags: [], + ratings: [], + rating_avg: null, + rating_count: 0, + }); + }); + + it('PLACEREPO-002: a category joins in by name/color/icon, not just its id', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const category = createCategory(testDb, { name: 'Museums', color: '#123456', icon: '🏛️' }); + const place = createPlace(testDb, trip.id, { category_id: category.id }); + + const result = await places.findWithTagsAndRatings(place.id); + expect(result?.category).toEqual({ id: category.id, name: 'Museums', color: '#123456', icon: '🏛️' }); + }); + + it('PLACEREPO-003: tags attach through place_tags, each one the full tags row', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + const tagA = createTag(testDb, owner.id, { name: 'Must see' }); + const tagB = createTag(testDb, owner.id, { name: 'Rainy day' }); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tagA.id); + testDb.prepare('INSERT INTO place_tags (place_id, tag_id) VALUES (?, ?)').run(place.id, tagB.id); + + const result = await places.findWithTagsAndRatings(place.id); + expect(result?.tags).toHaveLength(2); + expect(result?.tags.map((tg) => tg.name).sort()).toEqual(['Must see', 'Rainy day']); + }); + + it('PLACEREPO-004: ratings hydrate with the voter\'s username/avatar, in created_at order, and average correctly', async () => { + const { user: owner } = createUser(testDb); + const { user: voterA } = createUser(testDb, { username: 'voter_a' }); + const { user: voterB } = createUser(testDb, { username: 'voter_b' }); + const trip = createTrip(testDb, owner.id); + const place = createPlace(testDb, trip.id); + testDb + .prepare("INSERT INTO place_ratings (place_id, user_id, rating, created_at) VALUES (?, ?, ?, datetime('now', '-2 minutes'))") + .run(place.id, voterA.id, 4); + testDb + .prepare("INSERT INTO place_ratings (place_id, user_id, rating, created_at) VALUES (?, ?, ?, datetime('now', '-1 minutes'))") + .run(place.id, voterB.id, 2); + + const result = await places.findWithTagsAndRatings(place.id); + expect(result?.ratings).toHaveLength(2); + expect(result?.ratings.map((r) => r.username)).toEqual(['voter_a', 'voter_b']); + expect(result?.rating_avg).toBe(3); + expect(result?.rating_count).toBe(2); + }); + + it('PLACEREPO-005: a missing place id resolves to null, not a throw', async () => { + expect(await places.findWithTagsAndRatings(999999)).toBeNull(); + }); + + it('PLACEREPO-006: a non-numeric-looking id resolves to null, the same raw-bind seam TripsRepository documents', async () => { + expect(await places.findWithTagsAndRatings('abc')).toBeNull(); + }); +}); diff --git a/server/tests/unit/db/repositories/TripMembers.repository.test.ts b/server/tests/unit/db/repositories/TripMembers.repository.test.ts new file mode 100644 index 000000000..e23e3b8ab --- /dev/null +++ b/server/tests/unit/db/repositories/TripMembers.repository.test.ts @@ -0,0 +1,84 @@ +/** + * TripMembersRepository.rosterUserIds (Plan 3c Task 0b): the legacy + * `DatabaseService.rosterUserIds` statement in intent — `SELECT user_id FROM + * trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = + * ?` — as two reads merged through a `Set`. Real rows, on the real test DB, + * through the same `TestOrm` harness `Trips.repository.test.ts` uses. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { addTripMember, createTrip, createUser } from '../../../helpers/factories'; +import { TripMembers } from '../../../../src/db/entities/TripMembers.entity'; +import type { TripMembersRepository } from '../../../../src/db/repositories/TripMembers.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let tripMembers: TripMembersRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + tripMembers = t.repo(TripMembers); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +const legacyRoster = (tripId: number): Set => { + const rows = testDb + .prepare('SELECT user_id FROM trip_members WHERE trip_id = ? UNION SELECT user_id FROM trips WHERE id = ?') + .all(tripId, tripId) as { user_id: number }[]; + return new Set(rows.map((r) => r.user_id)); +}; + +describe('TripMembersRepository.rosterUserIds — parity with the legacy UNION statement', () => { + it('TMEMREPO-001: an owner-only trip (no members row) still yields the owner', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + expect(await tripMembers.rosterUserIds(trip.id)).toEqual(new Set([owner.id])); + expect(await tripMembers.rosterUserIds(trip.id)).toEqual(legacyRoster(trip.id)); + }); + + it('TMEMREPO-002: owner + members are deduped into one set, in either read order', async () => { + const { user: owner } = createUser(testDb); + const { user: memberA } = createUser(testDb); + const { user: memberB } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, memberA.id); + addTripMember(testDb, trip.id, memberB.id); + + const roster = await tripMembers.rosterUserIds(trip.id); + expect(roster).toEqual(new Set([owner.id, memberA.id, memberB.id])); + expect(roster).toEqual(legacyRoster(trip.id)); + }); + + it('TMEMREPO-003: a member row for the owner (edge case) still dedupes to one id, not a Set of size 2', async () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + addTripMember(testDb, trip.id, owner.id); + const roster = await tripMembers.rosterUserIds(trip.id); + expect(roster).toEqual(new Set([owner.id])); + expect(roster.size).toBe(1); + }); + + it('TMEMREPO-004: a trip belonging to someone else never leaks its roster into another trip\'s read', async () => { + const { user: ownerA } = createUser(testDb); + const { user: ownerB } = createUser(testDb); + const { user: memberOfA } = createUser(testDb); + const tripA = createTrip(testDb, ownerA.id); + const tripB = createTrip(testDb, ownerB.id); + addTripMember(testDb, tripA.id, memberOfA.id); + + expect(await tripMembers.rosterUserIds(tripB.id)).toEqual(new Set([ownerB.id])); + }); + + it('TMEMREPO-005: a missing trip id yields an empty set, not a throw', async () => { + const missingId = 999999; + expect(await tripMembers.rosterUserIds(missingId)).toEqual(new Set()); + expect(legacyRoster(missingId)).toEqual(new Set()); + }); + + it('TMEMREPO-006: a non-numeric-looking id finds nothing, the same raw-bind seam TripsRepository documents', async () => { + expect(await tripMembers.rosterUserIds('abc')).toEqual(new Set()); + }); +}); diff --git a/server/tests/unit/db/repositories/Trips.repository.test.ts b/server/tests/unit/db/repositories/Trips.repository.test.ts index 929256094..3ca6431f2 100644 --- a/server/tests/unit/db/repositories/Trips.repository.test.ts +++ b/server/tests/unit/db/repositories/Trips.repository.test.ts @@ -63,6 +63,14 @@ describe('TripsRepository.findAccessible — parity with canAccessTrip', () => { expect(await trips.isOwner(trip.id, member.id)).toBe(false); }); + it('TRIPREPO-007: a missing trip id resolves to undefined, not a throw, for both findAccessible and isOwner', async () => { + const { user } = createUser(testDb); + const missingId = 999999; + expect(await trips.findAccessible(missingId, user.id)).toBeUndefined(); + expect(legacy(missingId, user.id)).toBeUndefined(); + expect(await trips.isOwner(missingId, user.id)).toBe(false); + }); + it('TRIPREPO-006: toObject on a trip omits the hidden relation and every unloaded collection', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/db/trip-access.test.ts b/server/tests/unit/db/trip-access.test.ts index 0ed3a76d0..f432d3b3f 100644 --- a/server/tests/unit/db/trip-access.test.ts +++ b/server/tests/unit/db/trip-access.test.ts @@ -1,56 +1,77 @@ /** - * canAccessTrip is the shared trip-access check every domain reads its trip row - * from. It must return the trip's `currency`: the budget settlement takes its base - * currency straight off this row, and when the column was missing from the SELECT - * it silently fell back to 'EUR', inflating balances on every non-EUR trip that had - * a foreign-currency expense (#1543). + * `TripsRepository.findAccessible` (formerly the free-function `canAccessTrip` + * in `db/database.ts`, retired onto the repository in Plan 3c Task 0b) is the + * shared trip-access check every domain reads its trip row from. It must + * return the trip's `currency`: the budget settlement takes its base currency + * straight off this row, and when the column was missing from the SELECT it + * silently fell back to 'EUR', inflating balances on every non-EUR trip that + * had a foreign-currency expense (#1543). */ -import { describe, it, expect, beforeEach } from 'vitest'; -import { db, canAccessTrip } from '../../../src/db/database'; -import { CAN_ACCESS_TRIP_SQL, buildDbMock, createTestDb } from '../../helpers/test-db'; +import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; +import { CAN_ACCESS_TRIP_SQL, buildDbMock, createTestDb, resetTestDb } from '../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; +import { Trips } from '../../../src/db/entities/Trips.entity'; +import type { TripsRepository } from '../../../src/db/repositories/Trips.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let trips: TripsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + trips = t.repo(Trips); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); function seedUser(username: string): number { return Number( - db.prepare("INSERT INTO users (username, email, password_hash, role) VALUES (?, ?, 'x', 'user')") + testDb.prepare("INSERT INTO users (username, email, password_hash, role) VALUES (?, ?, 'x', 'user')") .run(username, `${username}@example.test`).lastInsertRowid, ); } -describe('canAccessTrip', () => { - beforeEach(() => { - db.exec('DELETE FROM trip_members; DELETE FROM trips; DELETE FROM users;'); - }); - +describe('TripsRepository.findAccessible (formerly canAccessTrip)', () => { it('returns the trip currency for the owner (#1543)', async () => { const owner = seedUser('owner'); const tripId = Number( - db.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'RUB')") + testDb.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'RUB')") .run(owner).lastInsertRowid, ); - expect(await canAccessTrip(tripId, owner)).toMatchObject({ id: tripId, user_id: owner, currency: 'RUB' }); + expect(await trips.findAccessible(tripId, owner)).toMatchObject({ id: tripId, user_id: owner, currency: 'RUB' }); }); it('returns the trip currency for a member too', async () => { const owner = seedUser('owner2'); const member = seedUser('member2'); const tripId = Number( - db.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'JPY')") + testDb.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'JPY')") .run(owner).lastInsertRowid, ); - db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(tripId, member); + testDb.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, ?)').run(tripId, member); - expect(await canAccessTrip(tripId, member)).toMatchObject({ currency: 'JPY' }); + expect(await trips.findAccessible(tripId, member)).toMatchObject({ currency: 'JPY' }); }); it('returns undefined for a user with no access', async () => { const owner = seedUser('owner3'); const stranger = seedUser('stranger3'); const tripId = Number( - db.prepare("INSERT INTO trips (user_id, title) VALUES (?, 'Trip')").run(owner).lastInsertRowid, + testDb.prepare("INSERT INTO trips (user_id, title) VALUES (?, 'Trip')").run(owner).lastInsertRowid, ); - expect(await canAccessTrip(tripId, stranger)).toBeUndefined(); + expect(await trips.findAccessible(tripId, stranger)).toBeUndefined(); + }); + + // Plan 3c Task 0b: the raw-bind id seam (no `Number()`/`toRowId` conversion + // before the value reaches the statement) survives the repository move — + // pinned directly here rather than only inferred from the boot matrix. + it('a non-numeric-looking id finds nothing, the same as the legacy raw-bind statement', async () => { + const owner = seedUser('owner4'); + testDb.prepare("INSERT INTO trips (user_id, title) VALUES (?, 'Trip')").run(owner); + expect(await trips.findAccessible('not-a-number', owner)).toBeUndefined(); }); }); @@ -60,20 +81,20 @@ describe('canAccessTrip', () => { */ describe('the buildDbMock stand-in for canAccessTrip', () => { it('hands back the trip currency, like the real one', async () => { - const testDb = createTestDb(); + const dbmockDb = createTestDb(); try { const owner = Number( - testDb.prepare("INSERT INTO users (username, email, password_hash, role) VALUES ('m', 'm@example.test', 'x', 'user')") + dbmockDb.prepare("INSERT INTO users (username, email, password_hash, role) VALUES ('m', 'm@example.test', 'x', 'user')") .run().lastInsertRowid, ); const tripId = Number( - testDb.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'ISK')") + dbmockDb.prepare("INSERT INTO trips (user_id, title, currency) VALUES (?, 'Trip', 'ISK')") .run(owner).lastInsertRowid, ); - expect(await buildDbMock(testDb).canAccessTrip(tripId, owner)).toMatchObject({ id: tripId, currency: 'ISK' }); + expect(await buildDbMock(dbmockDb).canAccessTrip(tripId, owner)).toMatchObject({ id: tripId, currency: 'ISK' }); } finally { - testDb.close(); + dbmockDb.close(); } }); diff --git a/server/tests/unit/files/files.rpc.test.ts b/server/tests/unit/files/files.rpc.test.ts index e4b905f78..8d7b08650 100644 --- a/server/tests/unit/files/files.rpc.test.ts +++ b/server/tests/unit/files/files.rpc.test.ts @@ -46,7 +46,7 @@ function build(opts: { file?: Record | undefined; foreign?: str softDeleteFile: vi.fn(), } as unknown as FilesService & Record>; const db = { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user', email: 'real@example.test' }) })), } as unknown as DatabaseService; const permissions = { @@ -257,7 +257,7 @@ describe('FilesRpc writes', () => { const f = build(); // The demo guard resolves the uploader's email; user 9 is the demo account. const db = { - canAccessTrip: vi.fn(() => ({ id: 1, user_id: 42 })), + canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })), prepare: vi.fn(() => ({ get: (id: number) => (id === 9 ? { role: 'user', email: 'demo@trek.app' } : { role: 'user', email: 'real@example.test' }) })), } as unknown as DatabaseService; const guards = new PluginGuards( diff --git a/server/tests/unit/mcp/tools-prompts.test.ts b/server/tests/unit/mcp/tools-prompts.test.ts index 12e4ad734..06cf890d7 100644 --- a/server/tests/unit/mcp/tools-prompts.test.ts +++ b/server/tests/unit/mcp/tools-prompts.test.ts @@ -87,7 +87,8 @@ import type { TodoService } from '../../../src/nest/todo/todo.service'; import type { CollabService } from '../../../src/nest/collab/collab.service'; import { AddonsService } from '../../../src/nest/addons/addons.service'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../helpers/test-uow'; +import type { EntityManager } from '@mikro-orm/core'; // The trip-summary prompt moved to the DI-discovered TripsMcp — its cases below // exercise it through a hand-built registry over a stub TripsService whose @@ -106,7 +107,14 @@ const readModelStub = { // The three remaining prompts moved to their domains' @McpController classes: // packing-list, budget-overview and the static-token notice. Built over the same // in-memory DB so the cases below keep asserting real rows. -const promptDbs = () => new DatabaseService(testDb); +// +// Plan 3c Task 0b: `promptEm` is resolved once in the first `beforeAll` below +// (before any `promptDbs()` call) — `canAccessTrip`/`isOwner`/`rosterUserIds`/ +// `getPlaceWithTags` resolve `TripsRepository`/`TripMembersRepository`/ +// `PlacesRepository` through it now, not through `db/database.ts`'s deleted +// free functions this file's `dbMock` used to stand in for. +let promptEm: EntityManager | undefined; +const promptDbs = () => new DatabaseService(testDb, promptEm); const authStub = { isDemoUser: () => false } as unknown as AuthService; @@ -120,7 +128,8 @@ let packingMcp: PackingMcp; let budgetMcp: BudgetMcp; let tripPromptsMcp: TripPromptsMcp; beforeAll(async () => { - promptGuards = new McpToolGuardsService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService()); + promptEm = (await sharedTestOrm(testDb)).em; + promptGuards = new McpToolGuardsService(promptDbs(), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService()); tripsMcp = new TripsMcp( tripsStub, { listItems: () => [] } as unknown as TodoService, diff --git a/server/tests/unit/nest/assignments.service.test.ts b/server/tests/unit/nest/assignments.service.test.ts index 4c0d8d618..5697e423c 100644 --- a/server/tests/unit/nest/assignments.service.test.ts +++ b/server/tests/unit/nest/assignments.service.test.ts @@ -55,12 +55,21 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); let svc: AssignmentsService; 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. + 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)); + vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); svc = new AssignmentsService( dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), diff --git a/server/tests/unit/nest/budget.service.calc.test.ts b/server/tests/unit/nest/budget.service.calc.test.ts index 052c8934b..e317b7141 100644 --- a/server/tests/unit/nest/budget.service.calc.test.ts +++ b/server/tests/unit/nest/budget.service.calc.test.ts @@ -30,7 +30,7 @@ const mockDb = vi.hoisted(() => { closeDb: () => {}, reinitialize: () => {}, getPlaceWithTags: () => null, - canAccessTrip: vi.fn(() => true), + canAccessTrip: vi.fn(async () => true), isOwner: () => false, }; }); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index d60c6d67a..54b901e09 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -72,7 +72,7 @@ import { CollabService } from '../../../src/nest/collab/collab.service'; import { VacayService } from '../../../src/nest/vacay/vacay.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; @@ -80,7 +80,14 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } // TripsService before, and on the deleted services/tripService before that); // deleteGuest routes through the SAME BudgetService domain SQL // (removeUserFromBudgetItems) under test. -const dbs = () => new DatabaseService(testDb); +// +// Plan 3c Task 0b: `dbsEm` is resolved once, at the top of the `beforeAll` +// below, before any `dbs()`/`new DatabaseService(testDb)` call — +// `canAccessTrip`/`isOwner`/`rosterUserIds`/`getPlaceWithTags` resolve +// `TripsRepository`/`TripMembersRepository`/`PlacesRepository` through it +// now, not through `db/database.ts`'s deleted free functions. +let dbsEm: import('@mikro-orm/core').EntityManager | undefined; +const dbs = () => new DatabaseService(testDb, dbsEm); let budget: BudgetService; @@ -88,8 +95,9 @@ let membersSvc: TripMembersService; let createGuest: typeof membersSvc.createGuest; let deleteGuest: typeof membersSvc.deleteGuest; beforeAll(async () => { + dbsEm = (await sharedTestOrm(testDb)).em; budget = new BudgetService( - new DatabaseService(testDb), + dbs(), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new ExchangeRatesService(), new RealtimeService(), diff --git a/server/tests/unit/nest/budget.service.test.ts b/server/tests/unit/nest/budget.service.test.ts index d134e0a12..21b8968ed 100644 --- a/server/tests/unit/nest/budget.service.test.ts +++ b/server/tests/unit/nest/budget.service.test.ts @@ -48,8 +48,8 @@ import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; */ const uowStub = { transactional: (fn: () => Promise) => fn() } as unknown as UnitOfWork; -function svc() { - return new BudgetService(new DatabaseService(dbConn), permissionsStub, exchangeRatesStub, new RealtimeService(), uowStub); +function svc(db: DatabaseService = new DatabaseService(dbConn)) { + return new BudgetService(db, permissionsStub, exchangeRatesStub, new RealtimeService(), uowStub); } beforeEach(() => { @@ -59,9 +59,16 @@ beforeEach(() => { describe('BudgetService', () => { it('verifyTripAccess resolves through DatabaseService.canAccessTrip', async () => { - canAccessTrip.mockReturnValue({ id: 5, user_id: 2 }); - expect(await svc().verifyTripAccess('5', 2)).toEqual({ id: 5, user_id: 2 }); - expect(canAccessTrip).toHaveBeenCalledWith('5', 2); + // Plan 3c Task 0b: `DatabaseService.canAccessTrip` is repository-backed + // now (not `db/database.ts`'s deleted free function, which is why this + // file's `db/database` mock's `canAccessTrip` above no longer reaches + // it) — a fake `DatabaseService` in place of the real, EntityManager-less + // one `svc()` defaults to, consistent with this suite's "no database + // behind it" design (everything else here is a `prepare` stub). + const findAccessible = vi.fn(async () => ({ id: 5, user_id: 2 })); + const fakeDb = { canAccessTrip: findAccessible } as unknown as DatabaseService; + expect(await svc(fakeDb).verifyTripAccess('5', 2)).toEqual({ id: 5, user_id: 2 }); + expect(findAccessible).toHaveBeenCalledWith('5', 2); }); it('canEdit forwards the ownership flag when the user owns the trip', async () => { @@ -150,11 +157,21 @@ describe('BudgetService', () => { // Both wrappers refuse a party who is not on the trip before they freeze // anything, so the roster lookup has to answer for these to reach the write // at all. Users 1 and 2 are the two parties every case here settles between. - const rosterHas = (...userIds: number[]) => dbMock._stmt.all.mockReturnValue(userIds.map(user_id => ({ user_id }))); + // + // Plan 3c Task 0b: `DatabaseService.rosterUserIds` is `TripMembersRepository + // .rosterUserIds` now (not `this.conn`/the `dbMock._stmt.all` prepare stub, + // which no longer intercepts it) — a real `DatabaseService` (so its other + // methods, still served by the `dbMock._stmt` prepare stub, keep working) + // with `rosterUserIds` spied directly, same fix as `verifyTripAccess`'s + // test above. + const rosterHas = (...userIds: number[]) => { + const db = new DatabaseService(dbConn); + vi.spyOn(db, 'rosterUserIds').mockResolvedValue(new Set(userIds)); + return db; + }; it('createSettlement freezes the FX rate (await) before the raw insert', async () => { - const s = svc(); - rosterHas(1, 2); + const s = svc(rosterHas(1, 2)); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); await s.createSettlement('5', { from_user_id: 1, to_user_id: 2, amount: 10 }, 3); @@ -163,8 +180,7 @@ describe('BudgetService', () => { }); it('updateSettlement threads the stored currency through the freeze', async () => { - const s = svc(); - rosterHas(1, 2); + const s = svc(rosterHas(1, 2)); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); // Quirk fix: the stored-currency lookup is a targeted getSettlement, not // a full listSettlements scan. @@ -178,8 +194,7 @@ describe('BudgetService', () => { }); it('refuses a party who is not on the trip, before freezing or writing', async () => { - const s = svc(); - rosterHas(1); // user 2 is not on this trip + const s = svc(rosterHas(1)); // user 2 is not on this trip const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); diff --git a/server/tests/unit/nest/charging.test.ts b/server/tests/unit/nest/charging.test.ts index aa7b85dbe..50ec4fb2b 100644 --- a/server/tests/unit/nest/charging.test.ts +++ b/server/tests/unit/nest/charging.test.ts @@ -134,7 +134,7 @@ describe('Open charging data before the stop exists', () => { it('serves the route and the MCP tool from one contract and one access check', async () => { const charging = { read: vi.fn(), lookup: vi.fn(async () => empty('unknown')) }; - const db = { canAccessTrip: vi.fn(() => false) }; + const db = { canAccessTrip: vi.fn(async () => false) }; const tool = new ChargingMcp(charging as never, db as never, {} as never); const input = { lat: 48.1, lng: 11.5, name: 'Ladepark Nord' }; @@ -142,7 +142,7 @@ describe('Open charging data before the stop exists', () => { // TripAccessGuard refuses it on the route. expect((await tool.lookup({ tripId: 3, ...input }, { userId: 5 } as never)).isError).toBe(true); expect(charging.lookup).not.toHaveBeenCalled(); - db.canAccessTrip.mockReturnValue(true); + db.canAccessTrip.mockResolvedValue(true); await tool.lookup({ tripId: 3, ...input }, { userId: 5 } as never); await new ChargingLookupController(charging as never).lookup(input as unknown as ChargingLookupDto); diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index a3376bc97..560973064 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -55,12 +55,12 @@ import { CollectionsService } from '../../../src/nest/collections/collections.se import { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService } from '../../helpers/test-uow'; const storageFx = makeStorageFixture(''); let svc: CollectionsService; beforeAll(async () => { - svc = new CollectionsService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), notificationsStub(notifSend), storageFx.storage, await createTestUnitOfWork(testDb)); + svc = new CollectionsService(await createTestDatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), notificationsStub(notifSend), storageFx.storage, await createTestUnitOfWork(testDb)); }); // The real cache: these cases assert what removeIfUnreferenced actually does // about collection_places (#1081), so a stub would assert nothing. diff --git a/server/tests/unit/nest/database-service.delegation.test.ts b/server/tests/unit/nest/database-service.delegation.test.ts index 432da36bf..1c730201a 100644 --- a/server/tests/unit/nest/database-service.delegation.test.ts +++ b/server/tests/unit/nest/database-service.delegation.test.ts @@ -1,39 +1,60 @@ /** - * Locks in the delegation direction: DatabaseService's trip-access helpers - * must call the db/database exports (which e2e suites stub in their vi.mock - * factories), never reimplement the SQL against the injected connection. + * Locks in the delegation direction: `DatabaseService`'s trip-access helpers + * must call the repository each one owns (`TripsRepository.findAccessible`/ + * `isOwner`, `TripMembersRepository.rosterUserIds`, `PlacesRepository + * .findWithTagsAndRatings`) through the injected `EntityManager`, never + * reimplement the SQL against the raw connection. + * + * Plan 3c Task 0b: this used to lock in the OPPOSITE direction — that these + * four methods delegated to `db/database.ts`'s free functions, which e2e + * suites stubbed in their `vi.mock` factories. Those free functions (and the + * duplicate `TripAccess` interface) are deleted now; `Trips.repository.ts`'s + * `TripAccess` is the single source, and the primitive bodies live on the + * repositories. */ import { describe, it, expect, vi } from 'vitest'; - -const { canAccessTrip, isOwner, getPlaceWithTags } = vi.hoisted(() => ({ - canAccessTrip: vi.fn(() => ({ id: -1, user_id: -2, currency: 'XXX' })), - isOwner: vi.fn(() => true), - getPlaceWithTags: vi.fn(() => null), -})); - -vi.mock('../../../src/db/database', () => ({ - db: {}, - closeDb: () => {}, - reinitialize: () => {}, - canAccessTrip, - isOwner, - getPlaceWithTags, -})); - +import type { EntityManager } from '@mikro-orm/core'; +import { Trips } from '../../../src/db/entities/Trips.entity'; +import { TripMembers } from '../../../src/db/entities/TripMembers.entity'; +import { Places } from '../../../src/db/entities/Places.entity'; import { DatabaseService } from '../../../src/nest/database/database.service'; describe('DatabaseService (helper delegation)', () => { - it('routes trip-access helpers through the db/database exports', async () => { - const { db } = await import('../../../src/db/database'); - const svc = new DatabaseService(db); + it('routes trip-access helpers through the EntityManager-resolved repositories', async () => { + const findAccessible = vi.fn(async () => ({ id: -1, user_id: -2, currency: 'XXX' })); + const isOwner = vi.fn(async () => true); + const rosterUserIds = vi.fn(async () => new Set([1, 2, 3])); + const findWithTagsAndRatings = vi.fn(async () => null); + + const em = { + getRepository: vi.fn((entity: unknown) => { + if (entity === Trips) return { findAccessible, isOwner }; + if (entity === TripMembers) return { rosterUserIds }; + if (entity === Places) return { findWithTagsAndRatings }; + throw new Error(`unexpected entity: ${String(entity)}`); + }), + } as unknown as EntityManager; + + const svc = new DatabaseService({} as never, em); expect(await svc.canAccessTrip(7, 8)).toEqual({ id: -1, user_id: -2, currency: 'XXX' }); - expect(canAccessTrip).toHaveBeenCalledWith(7, 8); + expect(findAccessible).toHaveBeenCalledWith(7, 8); expect(await svc.isOwner(7, 8)).toBe(true); expect(isOwner).toHaveBeenCalledWith(7, 8); + expect(await svc.rosterUserIds(7)).toEqual(new Set([1, 2, 3])); + expect(rosterUserIds).toHaveBeenCalledWith(7); + expect(await svc.getPlaceWithTags(9)).toBeNull(); - expect(getPlaceWithTags).toHaveBeenCalledWith(9); + expect(findWithTagsAndRatings).toHaveBeenCalledWith(9); + }); + + it('throws its own clear error — never MikroORM\'s generic one — when constructed with no EntityManager', async () => { + const svc = new DatabaseService({} as never); + await expect(svc.canAccessTrip(1, 2)).rejects.toThrow(/no EntityManager available/); + await expect(svc.isOwner(1, 2)).rejects.toThrow(/no EntityManager available/); + await expect(svc.rosterUserIds(1)).rejects.toThrow(/no EntityManager available/); + await expect(svc.getPlaceWithTags(1)).rejects.toThrow(/no EntityManager available/); }); }); diff --git a/server/tests/unit/nest/database-service.test.ts b/server/tests/unit/nest/database-service.test.ts index 667b58e76..adde65735 100644 --- a/server/tests/unit/nest/database-service.test.ts +++ b/server/tests/unit/nest/database-service.test.ts @@ -2,9 +2,11 @@ * DatabaseService — the shared better-sqlite3 provider (F3). Exercises every * helper against the real connection so the typed query surface is covered. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeAll } from 'vitest'; import { db } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { sharedTestOrm } from '../../helpers/test-uow'; +import type { EntityManager } from '@mikro-orm/core'; describe('DatabaseService (typed query helpers)', () => { const svc = new DatabaseService(db); @@ -34,7 +36,12 @@ describe('DatabaseService (typed query helpers)', () => { }); describe('DatabaseService (trip-access helpers)', () => { - const svc = new DatabaseService(db); + let svc: DatabaseService; + let em: EntityManager; + beforeAll(async () => { + em = (await sharedTestOrm(db)).em; + svc = new DatabaseService(db, em); + }); it('canAccessTrip / isOwner / getPlaceWithTags delegate to the shared helpers', async () => { svc.run( diff --git a/server/tests/unit/nest/dawarich-suggestions.service.test.ts b/server/tests/unit/nest/dawarich-suggestions.service.test.ts index 605e1d146..fca7f07e0 100644 --- a/server/tests/unit/nest/dawarich-suggestions.service.test.ts +++ b/server/tests/unit/nest/dawarich-suggestions.service.test.ts @@ -67,7 +67,7 @@ import type { PlacesService } from '../../../src/nest/places/places.service'; import type { AssignmentsService } from '../../../src/nest/assignments/assignments.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import type { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestDatabaseService } from '../../helpers/test-uow'; // ── Collaborator stubs ─────────────────────────────────────────────────────── // @@ -114,9 +114,10 @@ const assignmentsStub = { dayExists: vi.fn(), createAssignment: vi.fn(), broadca const permissionsStub = { checkPermission: vi.fn() }; const journeyStub = { canEdit: vi.fn(), createEntry: vi.fn() }; -const dbs = new DatabaseService(testDb); +let dbs: DatabaseService; let svc: DawarichSuggestionsService; beforeAll(async () => { + dbs = await createTestDatabaseService(testDb); svc = new DawarichSuggestionsService( dbs, dawarichStub as unknown as DawarichService, @@ -1292,9 +1293,14 @@ describe('DawarichSuggestionsService — the Atlas hand-off', () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); const id = seedSuggestion({ userId: user.id, tripId: trip.id }); - dbMock.getPlaceWithTags.mockReturnValue(null); + // Plan 3c Task 0b: `getPlaceWithTags` is `PlacesRepository + // .findWithTagsAndRatings` now (not `db/database.ts`'s deleted free + // function `dbMock.getPlaceWithTags` used to stand in for), so the + // "re-read comes back empty" case is spied directly on `dbs`. + const getPlaceWithTagsSpy = vi.spyOn(dbs, 'getPlaceWithTags').mockResolvedValueOnce(null); const result = await svc.accept(user.id, id, { target: 'place', tripId: trip.id }, 'socket-3'); + getPlaceWithTagsSpy.mockRestore(); expect(placesStub.broadcast).toHaveBeenCalledWith( String(trip.id), diff --git a/server/tests/unit/nest/dawarich-tracks.service.test.ts b/server/tests/unit/nest/dawarich-tracks.service.test.ts index bd940481b..b953b9d77 100644 --- a/server/tests/unit/nest/dawarich-tracks.service.test.ts +++ b/server/tests/unit/nest/dawarich-tracks.service.test.ts @@ -138,7 +138,7 @@ interface Collaborators { } function harness(over: Collaborators = {}) { - const canAccessTrip = vi.fn().mockReturnValue('access' in over ? over.access : TRIP_ACCESS); + const canAccessTrip = vi.fn().mockResolvedValue('access' in over ? over.access : TRIP_ACCESS); const get = vi .fn() .mockReturnValue( diff --git a/server/tests/unit/nest/day-boundaries.test.ts b/server/tests/unit/nest/day-boundaries.test.ts index 06247e483..3f75d6539 100644 --- a/server/tests/unit/nest/day-boundaries.test.ts +++ b/server/tests/unit/nest/day-boundaries.test.ts @@ -23,7 +23,7 @@ function setup() { get: (sql: string, ...params: unknown[]) => raw.prepare(sql).get(...params), all: (sql: string, ...params: unknown[]) => raw.prepare(sql).all(...params), run: (sql: string, ...params: unknown[]) => raw.prepare(sql).run(...params), - canAccessTrip: vi.fn(() => true), + canAccessTrip: vi.fn(async () => true), }; const service = new DayBoundariesService(db as unknown as DatabaseService); const realtime = { broadcast: vi.fn() }; @@ -68,9 +68,9 @@ it('MCP checks demo, trip access and edit permission before saving', async () => s.auth.isDemoUser.mockReturnValue(true); await s.mcp.save(request, ctx); s.auth.isDemoUser.mockReturnValue(false); - s.db.canAccessTrip.mockReturnValue(false); + s.db.canAccessTrip.mockResolvedValue(false); await s.mcp.save(request, ctx); - s.db.canAccessTrip.mockReturnValue(true); + s.db.canAccessTrip.mockResolvedValue(true); s.guards.hasTripPermission.mockReturnValue(false); await s.mcp.save(request, ctx); expect(await s.service.list(10)).toEqual([]); diff --git a/server/tests/unit/nest/day-notes.service.test.ts b/server/tests/unit/nest/day-notes.service.test.ts index 1c0726369..e61e882d0 100644 --- a/server/tests/unit/nest/day-notes.service.test.ts +++ b/server/tests/unit/nest/day-notes.service.test.ts @@ -51,11 +51,11 @@ import { PermissionsService } from '../../../src/nest/permissions/permissions.se import { DayNotesService } from '../../../src/nest/day-notes/day-notes.service'; import type { DayNote } from '../../../src/types'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService } from '../../helpers/test-uow'; let svc: DayNotesService; beforeAll(async () => { - svc = new DayNotesService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService()); + svc = new DayNotesService(await createTestDatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService()); }); beforeAll(() => { diff --git a/server/tests/unit/nest/days.service.test.ts b/server/tests/unit/nest/days.service.test.ts index b64773f88..749178b5b 100644 --- a/server/tests/unit/nest/days.service.test.ts +++ b/server/tests/unit/nest/days.service.test.ts @@ -64,11 +64,12 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import { makeAccommodationsService } from '../../helpers/accommodations-service'; import type { Day } from '../../../src/types'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService } from '../../helpers/test-uow'; let svc: DaysService; beforeAll(async () => { - svc = new DaysService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), new QueryHelpersService(new DatabaseService(testDb)), await createTestUnitOfWork(testDb)); + const dbs = await createTestDatabaseService(testDb); + svc = new DaysService(dbs, new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), new QueryHelpersService(dbs), await createTestUnitOfWork(testDb)); }); let accommodations: Awaited>; beforeAll(async () => { diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index eb29b8033..96dcd6dec 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -59,6 +59,7 @@ import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, addTripMember, createPlace, createReservation, createDay, createDayAssignment, setAppSetting } from '../../helpers/factories'; import { DatabaseService, type TripAccess } from '../../../src/nest/database/database.service'; +import { sharedTestOrm } from '../../helpers/test-uow'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { FilesService } from '../../../src/nest/files/files.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; @@ -96,9 +97,19 @@ beforeAll(() => { runMigrations(testDb); }); -beforeEach(() => { +beforeEach(async () => { resetTestDb(testDb); vi.clearAllMocks(); + // Plan 3c Task 0b: `svc`'s `DatabaseService` is constructed at module load, + // before any `beforeAll`/`beforeEach` can resolve a real `EntityManager` — + // spied directly on that instance, routed to a real `DatabaseService` built + // with one. Re-applied every test (not `beforeAll`) because `afterEach` + // below `vi.restoreAllMocks()`s it away after each one. + const real = new DatabaseService(testDb, (await sharedTestOrm(testDb)).em); + vi.spyOn(svc['db'] as DatabaseService, 'canAccessTrip').mockImplementation((...a) => real.canAccessTrip(...a)); + vi.spyOn(svc['db'] as DatabaseService, 'isOwner').mockImplementation((...a) => real.isOwner(...a)); + vi.spyOn(svc['db'] as DatabaseService, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(svc['db'] as DatabaseService, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); }); afterEach(() => { diff --git a/server/tests/unit/nest/google-route.test.ts b/server/tests/unit/nest/google-route.test.ts index 66f7cae7c..c3a241714 100644 --- a/server/tests/unit/nest/google-route.test.ts +++ b/server/tests/unit/nest/google-route.test.ts @@ -12,7 +12,7 @@ const routeTestDb = new Database(':memory:'); async function setup() { const maps = { geocodeQuery: vi.fn(), reverseGeocode: vi.fn().mockResolvedValue({ name: null, address: null }) }; - const db = { canAccessTrip: vi.fn(() => ({ user_id: 7 })), get: () => ({ role: 'user' }) }; + const db = { canAccessTrip: vi.fn(async () => ({ user_id: 7 })), get: () => ({ role: 'user' }) }; const places = { create: vi.fn((_trip: string, stop: { name: string }) => ({ id: stop.name })), broadcast: vi.fn() }; const assignments = { dayExists: vi.fn(() => true), createAssignment: vi.fn((dayId: number, placeId: string) => ({ dayId, placeId })), broadcast: vi.fn(), reconcile: vi.fn() }; const permissions = { checkPermission: vi.fn(() => true) }; @@ -68,7 +68,7 @@ describe('Google route import', () => { it('checks membership, both editing permissions and the target day before writing', async () => { const { service, db, permissions, assignments, places } = await setup(); - db.canAccessTrip.mockReturnValueOnce(null as never); + db.canAccessTrip.mockResolvedValueOnce(null as never); await expect(service.import(1, 7, input)).rejects.toThrow(); permissions.checkPermission.mockReturnValueOnce(false); await expect(service.import(1, 7, input)).rejects.toThrow(); diff --git a/server/tests/unit/nest/journey-domain.service.test.ts b/server/tests/unit/nest/journey-domain.service.test.ts index 37301bbf5..42e90d1f2 100644 --- a/server/tests/unit/nest/journey-domain.service.test.ts +++ b/server/tests/unit/nest/journey-domain.service.test.ts @@ -58,14 +58,15 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { db as dbConn } from '../../../src/db/database'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, sharedTestOrm } from '../../helpers/test-uow'; -const dbs = new DatabaseService(dbConn); +let dbs: DatabaseService; let svc: JourneyDomainService; beforeAll(async () => { createTables(testDb); runMigrations(testDb); + dbs = new DatabaseService(dbConn, (await sharedTestOrm(testDb)).em); svc = new JourneyDomainService(dbs, new RealtimeService(), new TrekPhotosRepository(dbs), await createTestUnitOfWork(testDb)); }); diff --git a/server/tests/unit/nest/memories-access.service.test.ts b/server/tests/unit/nest/memories-access.service.test.ts index 19d7674a2..5cafceecb 100644 --- a/server/tests/unit/nest/memories-access.service.test.ts +++ b/server/tests/unit/nest/memories-access.service.test.ts @@ -60,6 +60,8 @@ import { createUser, createTrip } from '../../helpers/factories'; import { mapDbError, pipeAsset, type ServiceResult } from '../../../src/nest/memories/memories.helpers'; import { MemoriesAccessService } from '../../../src/nest/memories/memories-access.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { Trips } from '../../../src/db/entities/Trips.entity'; +import { sharedTestOrm } from '../../helpers/test-uow'; const access = new MemoriesAccessService(new DatabaseService(testDb)); // A typed forwarder, not a `.bind` alias: a bound alias is typed `any`, which @@ -67,9 +69,18 @@ const access = new MemoriesAccessService(new DatabaseService(testDb)); const getAlbumIdFromLink = (...a: Parameters) => access.getAlbumIdFromLink(...a); import { SsrfBlockedError } from '../../../src/utils/ssrfGuard'; -beforeAll(() => { +beforeAll(async () => { createTables(testDb); runMigrations(testDb); + // Plan 3c Task 0b: `access` (and `DatabaseService.prototype.canAccessTrip` + // it holds a reference to) is constructed at module load, before any + // `beforeAll` can resolve a real `EntityManager` — patched onto the + // PROTOTYPE instead, which the already-constructed instance's method + // lookup still resolves through (`canAccessTrip` is not an own property). + const em = (await sharedTestOrm(testDb)).em; + vi.spyOn(DatabaseService.prototype, 'canAccessTrip').mockImplementation(async (tripId, userId) => + em.getRepository(Trips).findAccessible(tripId, userId), + ); }); beforeEach(() => { diff --git a/server/tests/unit/nest/packing.service.test.ts b/server/tests/unit/nest/packing.service.test.ts index e01ba68b3..d345fdfdf 100644 --- a/server/tests/unit/nest/packing.service.test.ts +++ b/server/tests/unit/nest/packing.service.test.ts @@ -60,7 +60,6 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin, createTrip, addTripMember } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { PackingService, isInvalidBagRef } from '../../../src/nest/packing/packing.service'; // Was packing.bridge, deleted with the other three that had no consumer outside the @@ -68,7 +67,7 @@ import { PackingService, isInvalidBagRef } from '../../../src/nest/packing/packi const bridgeListItems = (tripId: string | number, viewerId?: number) => svc.listItems(tripId, viewerId); import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestDatabaseService } from '../../helpers/test-uow'; let svc: PackingService; @@ -77,7 +76,7 @@ let svc: PackingService; beforeAll(async () => { createTables(testDb); runMigrations(testDb); - svc = new PackingService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), notificationsStub(send), await createTestUnitOfWork(testDb)); + svc = new PackingService(await createTestDatabaseService(testDb), permissionsStub, new RealtimeService(), notificationsStub(send), await createTestUnitOfWork(testDb)); }); beforeEach(() => { diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index bf588e57f..0ac33eb98 100644 --- a/server/tests/unit/nest/places.directions.test.ts +++ b/server/tests/unit/nest/places.directions.test.ts @@ -54,7 +54,7 @@ import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/plac import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const photoCacheStub = { removeIfUnreferenced: vi.fn() } as unknown as PlacePhotoCacheService; @@ -107,9 +107,18 @@ const geocoder = () => vi.fn(async (query: string) => { let tripId: string; -beforeAll(() => { +beforeAll(async () => { createTables(testDb); runMigrations(testDb); + // 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. + 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)); + vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); }); beforeEach(() => { diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index c0289c1fa..2f6276e53 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -78,7 +78,7 @@ import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpe import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; @@ -126,6 +126,15 @@ async function makePlacesService( 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. + 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)); + vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); accommodations = await accommodationsOver(dbs); svc = await makePlacesService(); }); diff --git a/server/tests/unit/nest/reservations.service.test.ts b/server/tests/unit/nest/reservations.service.test.ts index c6fb61322..2c6646320 100644 --- a/server/tests/unit/nest/reservations.service.test.ts +++ b/server/tests/unit/nest/reservations.service.test.ts @@ -48,7 +48,6 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, createReservation, createBudgetItem, createPlace, createDay, createDayAccommodation, createDayAssignment, addTripMember } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { ReservationsService } from '../../../src/nest/reservations/reservations.service'; import { ReservationsReadRepository } from '../../../src/nest/reservations/reservations-read.repository'; @@ -76,11 +75,12 @@ const bridge = { import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; import { makeAccommodationsService } from '../../helpers/accommodations-service'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestDatabaseService } from '../../helpers/test-uow'; let svc: ReservationsService; beforeAll(async () => { - svc = new ReservationsService(new DatabaseService(testDb), permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send), new ReservationsReadRepository(new DatabaseService(testDb)), await makeAccommodationsService(testDb), await createTestUnitOfWork(testDb)); + const dbs = await createTestDatabaseService(testDb); + svc = new ReservationsService(dbs, permissionsStub, budget as unknown as BudgetService, new RealtimeService(), notificationsStub(notif.send), new ReservationsReadRepository(dbs), await makeAccommodationsService(testDb), await createTestUnitOfWork(testDb)); }); beforeAll(() => { createTables(testDb); runMigrations(testDb); }); diff --git a/server/tests/unit/nest/roadtrip-plan.service.test.ts b/server/tests/unit/nest/roadtrip-plan.service.test.ts index 9417884e4..487c746e9 100644 --- a/server/tests/unit/nest/roadtrip-plan.service.test.ts +++ b/server/tests/unit/nest/roadtrip-plan.service.test.ts @@ -10,6 +10,7 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { RoadtripPlanService } from '../../../src/nest/roadtrip/roadtrip-plan.service'; import { createDay, createDayAssignment, createPlace, createTrip, createUser } from '../../helpers/factories'; import { resetTestDb } from '../../helpers/test-db'; +import { sharedTestOrm } from '../../helpers/test-uow'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -39,7 +40,7 @@ function hourlyRouter() { }; } -function setup() { +async function setup() { const { user } = createUser(db); const trip = createTrip(db, user.id); const day = createDay(db, trip.id); @@ -50,7 +51,7 @@ function setup() { }); db.prepare("UPDATE day_assignments SET assignment_time = '09:00' WHERE id = ?").run(visits[0].id); const plans = new RoadtripPlanService( - new DatabaseService(db), + new DatabaseService(db, (await sharedTestOrm(db)).em), { getUserSettings: () => ({}) } as never, { read: () => ({}) } as never, hourlyRouter() as never, @@ -66,7 +67,7 @@ beforeEach(() => { describe('a visit end time on the road trip', () => { it('is read from the visit, and from the place when the visit has none', async () => { - const { user, trip, visits, plans } = setup(); + const { user, trip, visits, plans } = await setup(); db.prepare("UPDATE day_assignments SET assignment_end_time = '14:00' WHERE id = ?").run(visits[1].id); db.prepare( "UPDATE places SET end_time = '18:00' WHERE id = (SELECT place_id FROM day_assignments WHERE id = ?)", @@ -82,7 +83,7 @@ describe('a visit end time on the road trip', () => { }); it('is when the drive leaves the stop, in place of its stay', async () => { - const { user, trip, visits, plans } = setup(); + const { user, trip, visits, plans } = await setup(); db.prepare("UPDATE day_assignments SET assignment_end_time = '14:00' WHERE id = ?").run(visits[1].id); const { calculated } = await plans.calculate(trip.id, user.id); @@ -98,7 +99,7 @@ describe('a visit end time on the road trip', () => { }); it('is reported when the drive gets there after it', async () => { - const { user, trip, visits, plans } = setup(); + const { user, trip, visits, plans } = await setup(); db.prepare("UPDATE day_assignments SET assignment_end_time = '09:30' WHERE id = ?").run(visits[1].id); const { calculated } = await plans.calculate(trip.id, user.id); @@ -108,7 +109,7 @@ describe('a visit end time on the road trip', () => { }); it('leaves a stop without one to its stay', async () => { - const { user, trip, plans } = setup(); + const { user, trip, plans } = await setup(); const { calculated } = await plans.calculate(trip.id, user.id); diff --git a/server/tests/unit/nest/roadtrip-planning.test.ts b/server/tests/unit/nest/roadtrip-planning.test.ts index 741691883..510bb60a7 100644 --- a/server/tests/unit/nest/roadtrip-planning.test.ts +++ b/server/tests/unit/nest/roadtrip-planning.test.ts @@ -59,7 +59,7 @@ function setup() { fill_percent: null, })); const db = { - canAccessTrip: vi.fn(() => true), + canAccessTrip: vi.fn(async () => true), all: vi.fn((sql: string) => (sql.includes('FROM day_assignments') ? visits : days)), }; const router = { @@ -177,7 +177,7 @@ describe('browser-independent roadtrip calculation', () => { }); it('checks trip access before reading or routing', async () => { const s = setup(); - s.db.canAccessTrip.mockReturnValue(false); + s.db.canAccessTrip.mockResolvedValue(false); await expect(s.plans.calculate(20, 5)).rejects.toThrow(); expect(s.db.all).not.toHaveBeenCalled(); expect(s.router.route).not.toHaveBeenCalled(); @@ -189,11 +189,11 @@ describe('browser-independent roadtrip calculation', () => { const s = setup(); const mcp = new RoadtripPlanningMcp(s.plans, {} as never, {} as never); const reason = (res: { content: { text: string }[]; isError?: boolean }) => [res.isError, res.content[0].text]; - s.db.canAccessTrip.mockReturnValue(false); + s.db.canAccessTrip.mockResolvedValue(false); expect(reason(await mcp.context({ tripId: 20 }, ctx))).toEqual([true, 'Trip not found']); expect(reason(await mcp.calculate({ tripId: 20, includeGeometry: false }, ctx))).toEqual([true, 'Trip not found']); expect(reason(await mcp.corridor({ tripId: 20, dayNumber: 1, category: 'fuel', widthKm: 5, offset: 0 } as never, ctx))).toEqual([true, 'Trip not found']); - s.db.canAccessTrip.mockReturnValue(true); + s.db.canAccessTrip.mockResolvedValue(true); const window = await mcp.calculate({ tripId: 10, includeGeometry: false, settings: { roadtrip_day_start: '18:00', roadtrip_day_end: '08:00' } }, ctx); expect(reason(window)).toEqual([true, 'Day end must be later than day start.']); expect(s.router.route).not.toHaveBeenCalled(); @@ -376,7 +376,7 @@ describe('MCP trip preferences authorization', () => { const s = setup(); const tool = new RoadtripPreferencesMcp(s.preferences, { isDemoUser: () => false } as never, {} as never, s.db as never, { hasTripPermission: () => true } as never); expect(JSON.stringify(await tool.read({ tripId: 10 }, ctx))).toContain('100'); - s.db.canAccessTrip.mockReturnValue(false); + s.db.canAccessTrip.mockResolvedValue(false); expect((await tool.read({ tripId: 10 }, ctx)).isError).toBe(true); expect((await tool.update({ tripId: 10, settings: {} }, ctx)).isError).toBe(true); }); diff --git a/server/tests/unit/nest/roadtrip-read-tools.test.ts b/server/tests/unit/nest/roadtrip-read-tools.test.ts index f314589cb..2365cc982 100644 --- a/server/tests/unit/nest/roadtrip-read-tools.test.ts +++ b/server/tests/unit/nest/roadtrip-read-tools.test.ts @@ -9,14 +9,14 @@ import { DayBoundariesMcp } from '../../../src/nest/roadtrip/day-boundaries.mcp' const ctx = { userId: 5 } as McpContext; describe('roadtrip read tools', () => { it('checks trip access before querying charging and hazards', async () => { - const db = { canAccessTrip: vi.fn(() => false) }; + const db = { canAccessTrip: vi.fn(async () => false) }; const source = { read: vi.fn(async () => ({ sources: ['public'] })) }; const charging = new ChargingMcp(source as never, db as never, {} as never); const hazards = new RoadtripHazardsMcp(source as never, db as never, {} as never); expect((await charging.read({ tripId: 10, placeId: 2 }, ctx)).isError).toBe(true); expect((await hazards.read({ tripId: 10 }, ctx)).isError).toBe(true); expect(source.read).not.toHaveBeenCalled(); - db.canAccessTrip.mockReturnValue(true); + db.canAccessTrip.mockResolvedValue(true); expect(JSON.stringify(await charging.read({ tripId: 10, placeId: 2 }, ctx))).toContain('public'); expect(source.read).toHaveBeenCalledWith(10, 2); expect(JSON.stringify(await hazards.read({ tripId: 10 }, ctx))).toContain('public'); @@ -51,7 +51,7 @@ describe('roadtrip read tools', () => { const refuse = (error: string, status: number) => () => { throw new HttpException({ error }, status); }; const text = (res: { content: { text: string }[]; isError?: boolean }) => [res.isError, res.content[0].text]; - const db = { canAccessTrip: vi.fn(() => true) }; + const db = { canAccessTrip: vi.fn(async () => true) }; const charging = new ChargingMcp({ read: refuse('Place not found', 404) } as never, db as never, {} as never); expect(text(await charging.read({ tripId: 10, placeId: 2 }, ctx))).toEqual([true, 'Place not found']); @@ -71,11 +71,11 @@ describe('roadtrip read tools', () => { it('does not expose manual boundaries to nonmembers', async () => { const boundaries = { list: vi.fn(() => [{ day_number: 1 }]) }; - const db = { canAccessTrip: vi.fn(() => false) }; + const db = { canAccessTrip: vi.fn(async () => false) }; const tool = new DayBoundariesMcp(boundaries as never, db as never, {} as never, {} as never, {} as never, {} as never); expect((await tool.list({ tripId: 10 }, ctx)).isError).toBe(true); expect(boundaries.list).not.toHaveBeenCalled(); - db.canAccessTrip.mockReturnValue(true); + db.canAccessTrip.mockResolvedValue(true); expect(JSON.stringify(await tool.list({ tripId: 10 }, ctx))).toContain('day_number'); expect(boundaries.list).toHaveBeenCalledWith(10); }); diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index 954991906..94d76b061 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -73,11 +73,12 @@ beforeAll(async () => { createTables(testDb); runMigrations(testDb); t = await sharedTestOrm(testDb); + const dbs = new DatabaseService(testDb, t.em); svc = new ShareService( - new DatabaseService(testDb), + dbs, new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb)), permissionsStub, - new QueryHelpersService(new DatabaseService(testDb)), + new QueryHelpersService(dbs), photoCacheStub, await createTestUnitOfWork(testDb), ); diff --git a/server/tests/unit/nest/todo.service.test.ts b/server/tests/unit/nest/todo.service.test.ts index 5588fbf55..283a4a812 100644 --- a/server/tests/unit/nest/todo.service.test.ts +++ b/server/tests/unit/nest/todo.service.test.ts @@ -48,12 +48,12 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { TodoService } from '../../../src/nest/todo/todo.service'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestDatabaseService } from '../../helpers/test-uow'; let svc: TodoService; beforeAll(async () => { const uow = await createTestUnitOfWork(testDb); - svc = new TodoService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), uow), new RealtimeService(), uow); + svc = new TodoService(await createTestDatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), uow), new RealtimeService(), uow); }); beforeAll(() => { diff --git a/server/tests/unit/nest/trip-access.guard.test.ts b/server/tests/unit/nest/trip-access.guard.test.ts index ea842ab77..0f04a0e14 100644 --- a/server/tests/unit/nest/trip-access.guard.test.ts +++ b/server/tests/unit/nest/trip-access.guard.test.ts @@ -18,27 +18,36 @@ import { TripAccessGuard, } from '../../../src/nest/permissions/trip-access.guard'; import { Trip } from '../../../src/nest/permissions/trip.decorator'; -import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import type { User } from '../../../src/types'; +import type { EntityManager } from '@mikro-orm/core'; const owner = { id: 42, role: 'user' } as User; const member = { id: 7, role: 'user' } as User; /** Trip 5 belongs to user 42; user 7 is a member of it. Nothing else is reachable. */ const TRIP = { id: 5, user_id: 42 }; +/** + * Plan 3c Task 0b: the guard now injects `EntityManager` and resolves + * `TripsRepository` inside `canActivate` (the `JwtAuthGuard` precedent), + * instead of `DatabaseService.canAccessTrip` — so the double here is an + * `EntityManager` whose `getRepository(Trips)` returns a `findAccessible` + * stand-in, not a `DatabaseService` stand-in. + */ function makeGuard(options: { checkPermission?: ReturnType; action?: string } = {}) { - const canAccessTrip = vi.fn((tripId: number, userId: number) => + // async, not a bare sync return: `TripAccessGuard.canActivate` awaits this + // call, and a synchronous double would not catch a dropped `await` there + // (a missing `await` on a genuinely async repository call leaves `trip` as + // an always-truthy Promise, never falling into the 404 branch — task-0a + // mechanics review F1). + const findAccessible = vi.fn(async (tripId: number, userId: number) => tripId === 5 && (userId === 42 || userId === 7) ? TRIP : undefined, ); + const em = { getRepository: vi.fn(() => ({ findAccessible })) } as unknown as EntityManager; const checkPermission = options.checkPermission ?? vi.fn(() => true); const reflector = { getAllAndOverride: vi.fn(() => options.action) } as unknown as Reflector; - const guard = new TripAccessGuard( - { canAccessTrip } as unknown as DatabaseService, - { checkPermission } as unknown as PermissionsService, - reflector, - ); - return { guard, canAccessTrip, checkPermission, reflector }; + const guard = new TripAccessGuard(em, { checkPermission } as unknown as PermissionsService, reflector); + return { guard, canAccessTrip: findAccessible, checkPermission, reflector }; } /** The slice of ExecutionContext the guard reads. */ diff --git a/server/tests/unit/nest/trip-members.controller.test.ts b/server/tests/unit/nest/trip-members.controller.test.ts index 117c67dbe..348ca51d6 100644 --- a/server/tests/unit/nest/trip-members.controller.test.ts +++ b/server/tests/unit/nest/trip-members.controller.test.ts @@ -28,7 +28,7 @@ const tc = (s: TripMembersService) => new TripMembersController(s, audit); function svc(o: Partial = {}): TripMembersService { return { - canAccessTrip: vi.fn().mockReturnValue({ user_id: 1 }), + canAccessTrip: vi.fn().mockResolvedValue({ user_id: 1 }), can: vi.fn().mockReturnValue(true), broadcast: vi.fn(), notifyInvite: vi.fn(), @@ -51,7 +51,7 @@ beforeEach(() => vi.clearAllMocks()); describe('TripMembersController', () => { describe('members', () => { it('GET 404 without access, else owner+members+current_user_id', async () => { - expect(await thrown(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).members(user, '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrown(() => tc(svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) })).members(user, '9'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); const s = svc({ listMembers: vi.fn().mockReturnValue({ owner: { id: 1 }, members: [] }) } as Partial); expect(await tc(s).members(user, '9')).toEqual({ owner: { id: 1 }, members: [], current_user_id: 1 }); }); @@ -66,7 +66,7 @@ describe('members', () => { }); it('POST 404 without trip access', async () => { - const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); + const s = svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) }); expect(await thrown(() => tc(s).addMember(user, '9', { identifier: 'bob@x.y' }))).toEqual({ status: 404, body: { error: 'Trip not found' } }); }); @@ -80,7 +80,7 @@ describe('members', () => { }); it('DELETE 404 without trip access', async () => { - const s = svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) }); + const s = svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) }); expect(await thrown(() => tc(s).removeMember(user, '9', '2'))).toEqual({ status: 404, body: { error: 'Trip not found' } }); }); diff --git a/server/tests/unit/nest/trip-members.service.test.ts b/server/tests/unit/nest/trip-members.service.test.ts index eb37208e0..4acc825b0 100644 --- a/server/tests/unit/nest/trip-members.service.test.ts +++ b/server/tests/unit/nest/trip-members.service.test.ts @@ -65,13 +65,22 @@ import { TripMembersService } from '../../../src/nest/trip-members/trip-members. import { NotFoundError, ValidationError } from '../../../src/nest/common/domain-errors'; import type { User } from '../../../src/types'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; +import type { EntityManager } from '@mikro-orm/core'; -const dbs = () => new DatabaseService(testDb); +// Plan 3c Task 0b: `dbsEm` is resolved once, at the top of the `beforeAll` +// below, before any `dbs()` call — `canAccessTrip`/`isOwner`/`rosterUserIds`/ +// `getPlaceWithTags` resolve `TripsRepository`/`TripMembersRepository`/ +// `PlacesRepository` through it now, not through `db/database.ts`'s deleted +// free functions. Kept as a module-level variable (not threaded through +// `dbs()`'s signature) so the ~17 existing `dbs()` call sites stay unchanged. +let dbsEm: EntityManager | undefined; +const dbs = () => new DatabaseService(testDb, dbsEm); let budgetSvc: BudgetService; let roster: TripMembersService; beforeAll(async () => { + dbsEm = (await sharedTestOrm(testDb)).em; budgetSvc = new BudgetService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(dbs().connection)); roster = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(notifySend), await createTestUnitOfWork(dbs().connection)); }); diff --git a/server/tests/unit/nest/trip-owner.guard.test.ts b/server/tests/unit/nest/trip-owner.guard.test.ts index 3eadf9247..8dda70949 100644 --- a/server/tests/unit/nest/trip-owner.guard.test.ts +++ b/server/tests/unit/nest/trip-owner.guard.test.ts @@ -13,7 +13,7 @@ import { TRIP_OWNER_KEY, TripOwnerGuard, } from '../../../src/nest/permissions/trip-owner.guard'; -import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { EntityManager } from '@mikro-orm/core'; import type { User } from '../../../src/types'; const owner = { id: 42, role: 'user' } as User; @@ -22,13 +22,25 @@ const admin = { id: 99, role: 'admin' } as User; /** Trip 5 belongs to user 42; user 7 is a member. Nobody else can reach it. */ const TRIP = { id: 5, user_id: 42 }; +/** + * Plan 3c Task 0b (task-0a-review-security.md F-A1): the guard now injects + * `EntityManager` and resolves `TripsRepository` inside `canActivate` (the + * `TripAccessGuard`/`JwtAuthGuard` precedent), instead of `DatabaseService + * .canAccessTrip` — this guard is the SOLE access check on + * `trip-members.controller.ts`'s four owner-only routes (no class-level + * `TripAccessGuard` there), so it gets the same direct-repository wiring. + */ function makeGuard(meta: { message: string; param?: string } | undefined = { message: 'Only the owner can do this' }) { - const canAccessTrip = vi.fn((tripId: number, userId: number) => + // async, not a bare sync return: `TripOwnerGuard.canActivate` awaits this + // call, and a synchronous double would not catch a dropped `await` there + // (task-0a mechanics review F1). + const findAccessible = vi.fn(async (tripId: number, userId: number) => tripId === 5 && (userId === 42 || userId === 7) ? TRIP : undefined, ); + const em = { getRepository: vi.fn(() => ({ findAccessible })) } as unknown as EntityManager; const reflector = { getAllAndOverride: vi.fn(() => meta) } as unknown as Reflector; - const guard = new TripOwnerGuard({ canAccessTrip } as unknown as DatabaseService, reflector); - return { guard, canAccessTrip }; + const guard = new TripOwnerGuard(em, reflector); + return { guard, canAccessTrip: findAccessible }; } function ctx(request: Record) { @@ -77,9 +89,10 @@ describe('TripOwnerGuard', () => { // returns true for every admin and the trip actions are admin-lowerable, so routing // ownership through it would hand admins other people's trips. The guard does not // inject it at all. - const canAccessTrip = vi.fn(() => TRIP); + const findAccessible = vi.fn(async () => TRIP); + const em = { getRepository: vi.fn(() => ({ findAccessible })) } as unknown as EntityManager; const reflector = { getAllAndOverride: vi.fn(() => ({ message: 'Only the owner can transfer ownership' })) } as unknown as Reflector; - const guard = new TripOwnerGuard({ canAccessTrip } as unknown as DatabaseService, reflector); + const guard = new TripOwnerGuard(em, reflector); expect(await thrown(() => guard.canActivate(ctx({ user: admin, params: { tripId: '5' } })))).toEqual({ status: 403, body: { error: 'Only the owner can transfer ownership' }, @@ -129,9 +142,10 @@ describe('TripOwnerGuard', () => { it('OWNER-010: without metadata it still refuses a non-owner, with a generic message', async () => { // Built inline: passing undefined to makeGuard would hit its own default. - const canAccessTrip = vi.fn(() => TRIP); + const findAccessible = vi.fn(async () => TRIP); + const em = { getRepository: vi.fn(() => ({ findAccessible })) } as unknown as EntityManager; const reflector = { getAllAndOverride: vi.fn(() => undefined) } as unknown as Reflector; - const guard = new TripOwnerGuard({ canAccessTrip } as unknown as DatabaseService, reflector); + const guard = new TripOwnerGuard(em, reflector); expect(await thrown(() => guard.canActivate(ctx({ user: member, params: { tripId: '5' } })))).toEqual({ status: 403, body: { error: 'Only the trip owner can do this' }, diff --git a/server/tests/unit/nest/trips.controller.test.ts b/server/tests/unit/nest/trips.controller.test.ts index 41eff79b3..2bce70977 100644 --- a/server/tests/unit/nest/trips.controller.test.ts +++ b/server/tests/unit/nest/trips.controller.test.ts @@ -42,7 +42,7 @@ const tc = (s: TripsService, calendar?: Partial, readModel?: Pa function svc(o: Partial = {}): TripsService { return { - canAccessTrip: vi.fn().mockReturnValue({ user_id: 1 }), + canAccessTrip: vi.fn().mockResolvedValue({ user_id: 1 }), can: vi.fn().mockReturnValue(true), broadcast: vi.fn(), notifyInvite: vi.fn(), @@ -206,7 +206,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = describe('PUT /:id', () => { it('404 when no access; 403 on archive without trip_archive', async () => { - expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).update(user, '9', {}, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) })).update(user, '9', {}, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); const s = svc({ can: vi.fn().mockImplementation((a: string) => a !== 'trip_archive') }); expect(await thrownAsync(() => tc(s).update(user, '9', { is_archived: 1 }, req))).toEqual({ status: 403, body: { error: 'No permission to archive/unarchive this trip' } }); }); @@ -273,7 +273,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = describe('POST /:id/copy', () => { it('403 without trip_create, 404 without access', async () => { expect(await thrownAsync(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).copy(user, '9', {}, req))).toEqual({ status: 403, body: { error: 'No permission to create trips' } }); - expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).copy(user, '9', {}, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) })).copy(user, '9', {}, req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); }); it('copies + returns the new trip', async () => { @@ -290,7 +290,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = }); it('404s for someone with no access, so the 403 is not an existence oracle', async () => { - const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockReturnValue(null), remove: vi.fn() } as Partial); + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockResolvedValue(null), remove: vi.fn() } as Partial); expect(await thrownAsync(() => tc(s).remove(user, '9', req))).toEqual({ status: 404, body: { error: 'Trip not found' } }); expect(s.remove).not.toHaveBeenCalled(); }); @@ -298,7 +298,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = it('still lets an admin delete a trip they are not a member of', async () => { const admin = { id: 1, role: 'admin', email: 'a@example.test' } as User; const remove = vi.fn().mockReturnValue({ tripId: 9, title: 'T', isAdminDelete: true, ownerEmail: 'owner@x.y' }); - const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockReturnValue(null), remove, broadcast: vi.fn() } as Partial); + const s = svc({ getOwner: vi.fn().mockReturnValue({ user_id: 2 }), canAccessTrip: vi.fn().mockResolvedValue(null), remove, broadcast: vi.fn() } as Partial); expect(await tc(s).remove(admin, '9', req)).toEqual({ success: true }); expect(remove).toHaveBeenCalledWith('9', 1, 'admin'); }); @@ -329,7 +329,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = describe('POST /:id/cover', () => { const file = { filename: 'abc.jpg' } as Express.Multer.File; it('404 without access, 403 without permission, 404 raw trip, 400 no file, else commits + returns url', async () => { - expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) })).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); expect(await thrownAsync(() => tc(svc({ can: vi.fn().mockReturnValue(false) })).cover(user, '9', file))).toEqual({ status: 403, body: { error: 'No permission to change the cover image' } }); expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue(undefined) } as Partial)).cover(user, '9', file))).toEqual({ status: 404, body: { error: 'Trip not found' } }); expect(await thrownAsync(() => tc(svc({ getRaw: vi.fn().mockReturnValue({ cover_image: null }) } as Partial)).cover(user, '9', undefined))).toEqual({ status: 400, body: { error: 'No image uploaded' } }); @@ -386,7 +386,7 @@ describe('TripsController (parity with the legacy /api/trips route)', async () = describe('GET /:id/export.ics', () => { function makeRes() { return { setHeader: vi.fn(), send: vi.fn() } as never; } it('404 without access, else sends the calendar with headers', async () => { - expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockReturnValue(undefined) })).exportIcs(user, '9', makeRes()))).toEqual({ status: 404, body: { error: 'Trip not found' } }); + expect(await thrownAsync(() => tc(svc({ canAccessTrip: vi.fn().mockResolvedValue(undefined) })).exportIcs(user, '9', makeRes()))).toEqual({ status: 404, body: { error: 'Trip not found' } }); const res = { setHeader: vi.fn(), send: vi.fn() }; const cal = { exportICS: vi.fn().mockReturnValue({ ics: 'BEGIN:VCALENDAR', filename: 'trip.ics' }) }; await tc(svc(), cal).exportIcs(user, '9', res as never); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 382538330..8497da0f5 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -92,7 +92,16 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, s // Real sibling services over the same in-memory DB — updateTrip's date-shift // resyncs and the summary/bundle aggregation run their actual SQL. -const dbs = () => new DatabaseService(testDb); +// +// Plan 3c Task 0b: `dbsEm` is resolved once, at the top of the first +// `beforeAll` below, before any `dbs()` call — `canAccessTrip`/`isOwner`/ +// `rosterUserIds`/`getPlaceWithTags` resolve `TripsRepository`/ +// `TripMembersRepository`/`PlacesRepository` through it now, not through +// `db/database.ts`'s deleted free functions. Kept as a module-level variable +// (not threaded through `dbs()`'s signature) so the many existing `dbs()` +// call sites in this file stay unchanged. +let dbsEm: import('@mikro-orm/core').EntityManager | undefined; +const dbs = () => new DatabaseService(testDb, dbsEm); // Same collaborator set the container hands PlacesService (see places.service.test.ts). @@ -122,6 +131,7 @@ let svc: TripsService; let membersSvc: TripMembersService; let readModelSvc: TripReadModelService; beforeAll(async () => { + dbsEm = (await sharedTestOrm(testDb)).em; budgetSvc = new BudgetService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(dbs().connection)); daysSvc = new DaysService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), new QueryHelpersService(dbs()), await createTestUnitOfWork(dbs().connection)); placesSvc = new PlacesService( @@ -147,6 +157,7 @@ beforeAll(async () => { undefined as never, // unsplash — not exercised here coversFx.storage, await createTestUnitOfWork(dbs().connection), + (await sharedTestOrm(testDb)).em, ); membersSvc = new TripMembersService(dbs(), budgetSvc, new UserCleanupService(dbs(), budgetSvc, await createTestUnitOfWork(dbs().connection), await createTestUsersRepo(dbs().connection)), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), await createTestUnitOfWork(dbs().connection)); readModelSvc = new TripReadModelService( @@ -1214,6 +1225,7 @@ describe('quirk fixes', () => { undefined as never, coversFx.storage, await createTestUnitOfWork(dbs().connection), + (await sharedTestOrm(testDb)).em, ); } diff --git a/server/tests/unit/nest/unified-memories.service.test.ts b/server/tests/unit/nest/unified-memories.service.test.ts index 7acde1e21..3099754b4 100644 --- a/server/tests/unit/nest/unified-memories.service.test.ts +++ b/server/tests/unit/nest/unified-memories.service.test.ts @@ -51,7 +51,7 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import type { ImmichService } from '../../../src/nest/memories/immich.service'; import type { SynologyService } from '../../../src/nest/memories/synology.service'; import { notificationsStub } from '../../helpers/notifications'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, sharedTestOrm } from '../../helpers/test-uow'; // The album-sync paths are the providers' half and have their own suites; these // cases never reach them, so stubs keep the graph small. @@ -74,6 +74,13 @@ const removeAlbumLink = (...a: Parameters) => svc.remove beforeAll(async () => { createTables(testDb); runMigrations(testDb); + // Plan 3c Task 0b: `dbs` is constructed at module load, before any + // `beforeAll` can resolve a real `EntityManager` — `canAccessTrip` (the + // only one of the four primitives this suite's code paths reach) is spied + // directly on this instance, 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)); svc = new UnifiedMemoriesService( dbs, new TrekPhotosRepository(dbs), diff --git a/server/tests/unit/nest/user-cleanup.service.test.ts b/server/tests/unit/nest/user-cleanup.service.test.ts index f4e9a67c1..aff84b59c 100644 --- a/server/tests/unit/nest/user-cleanup.service.test.ts +++ b/server/tests/unit/nest/user-cleanup.service.test.ts @@ -49,13 +49,22 @@ import { ExchangeRatesService } from '../../../src/nest/budget/exchange-rates.se import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); let budget: BudgetService; let svc: UserCleanupService; 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. + 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)); + vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); budget = new BudgetService(dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(dbs.connection)); svc = new UserCleanupService(dbs, budget, await createTestUnitOfWork(dbs.connection), await createTestUsersRepo(dbs.connection)); }); diff --git a/server/tests/unit/packing/packing.rpc.test.ts b/server/tests/unit/packing/packing.rpc.test.ts index 3a6b21151..5ffe02eb8 100644 --- a/server/tests/unit/packing/packing.rpc.test.ts +++ b/server/tests/unit/packing/packing.rpc.test.ts @@ -51,7 +51,7 @@ function build(opts: { canEdit?: boolean; before?: Item | undefined; updated?: I Object.assign(packing, data); const guards = new PluginGuards( { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService, { checkPermission: vi.fn(() => opts.canEdit ?? true) } as unknown as PermissionsService, diff --git a/server/tests/unit/places/places.rpc.test.ts b/server/tests/unit/places/places.rpc.test.ts index e215d65d0..5fc32ce80 100644 --- a/server/tests/unit/places/places.rpc.test.ts +++ b/server/tests/unit/places/places.rpc.test.ts @@ -52,7 +52,7 @@ function build(opts: { canEdit?: boolean; journeyThrows?: boolean } = {}) { const realtime = { broadcast: vi.fn() }; const guards = new PluginGuards( { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService, { checkPermission: vi.fn(() => opts.canEdit ?? true) } as unknown as PermissionsService, diff --git a/server/tests/unit/plugins/addon-domains-errors.rpc.test.ts b/server/tests/unit/plugins/addon-domains-errors.rpc.test.ts index 1290b5ba2..5cb01ea42 100644 --- a/server/tests/unit/plugins/addon-domains-errors.rpc.test.ts +++ b/server/tests/unit/plugins/addon-domains-errors.rpc.test.ts @@ -38,7 +38,7 @@ function build(overrides: { collections?: Record; } = {}) { const db = { - canAccessTrip: vi.fn(() => ({ id: 1, user_id: 42 })), + canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }), all: () => [] })), } as unknown as DatabaseService; const guards = new PluginGuards( diff --git a/server/tests/unit/plugins/addon-gates.rpc.test.ts b/server/tests/unit/plugins/addon-gates.rpc.test.ts index b031aba62..efa6cdf61 100644 --- a/server/tests/unit/plugins/addon-gates.rpc.test.ts +++ b/server/tests/unit/plugins/addon-gates.rpc.test.ts @@ -56,7 +56,7 @@ function spyService(calls: string[], name: string) { function build(addonOn: boolean) { const calls: string[] = []; const db = { - canAccessTrip: vi.fn(() => ({ id: 1, user_id: 42 })), + canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }), all: () => [] })), } as unknown as DatabaseService; const guards = new PluginGuards( diff --git a/server/tests/unit/plugins/day-schedule.controller.test.ts b/server/tests/unit/plugins/day-schedule.controller.test.ts index 50bb19877..1c9cc9419 100644 --- a/server/tests/unit/plugins/day-schedule.controller.test.ts +++ b/server/tests/unit/plugins/day-schedule.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled, tripDays } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), tripDays: { value: [{ id: 10 }, { id: 11 }] as Array<{ id: number }> }, })); @@ -16,7 +16,6 @@ vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ all: () => tripDays.value }) }, canAccessTrip, })); -import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled })); @@ -30,12 +29,12 @@ function controller(invoke: (id: string) => unknown, providers = ['p1']) { providersOf: vi.fn(() => providers), daySchedule: vi.fn(async (id: string) => invoke(id)), } as unknown as PluginHooks; - return { c: new DayScheduleController(runtime, new DatabaseService(dbConn)), runtime }; + return { c: new DayScheduleController(runtime, { canAccessTrip, connection: { prepare: () => ({ all: () => tripDays.value }) } } as unknown as DatabaseService), runtime }; } const item = (over: Record = {}) => ({ id: 's1', dayId: 10, label: 'Charging', ...over }); describe('DayScheduleController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / no user / non-member all return [] (no plugin calls on the first)', async () => { pluginsEnabled.mockReturnValue(false); @@ -45,7 +44,7 @@ describe('DayScheduleController', () => { pluginsEnabled.mockReturnValue(true); expect((await controller(() => [item()]).c.get('1', req(undefined))).items).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [item()]).c.get('1', req(5))).items).toEqual([]); }); diff --git a/server/tests/unit/plugins/day-tints.controller.test.ts b/server/tests/unit/plugins/day-tints.controller.test.ts index 629db624b..84f2aa5ed 100644 --- a/server/tests/unit/plugins/day-tints.controller.test.ts +++ b/server/tests/unit/plugins/day-tints.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled, tripDays } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), tripDays: { value: [{ id: 10 }, { id: 11 }] as Array<{ id: number }> }, })); @@ -16,7 +16,6 @@ vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ all: () => tripDays.value }) }, canAccessTrip, })); -import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled })); @@ -30,12 +29,12 @@ function controller(invoke: (id: string) => unknown, providers = ['p1']) { providersOf: vi.fn(() => providers), dayTints: vi.fn(async (id: string) => invoke(id)), } as unknown as PluginHooks; - return { c: new DayTintsController(runtime, new DatabaseService(dbConn)), runtime }; + return { c: new DayTintsController(runtime, { canAccessTrip, connection: { prepare: () => ({ all: () => tripDays.value }) } } as unknown as DatabaseService), runtime }; } const tint = (over: Record = {}) => ({ dayId: 10, tone: 'success', ...over }); describe('DayTintsController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / no user / non-member all return [] (no plugin calls on the first)', async () => { pluginsEnabled.mockReturnValue(false); @@ -45,7 +44,7 @@ describe('DayTintsController', () => { pluginsEnabled.mockReturnValue(true); expect((await controller(() => [tint()]).c.get('1', req(undefined))).tints).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [tint()]).c.get('1', req(5))).tints).toEqual([]); }); diff --git a/server/tests/unit/plugins/host-rpc.test.ts b/server/tests/unit/plugins/host-rpc.test.ts index 366e1e2e3..eb5d6e62a 100644 --- a/server/tests/unit/plugins/host-rpc.test.ts +++ b/server/tests/unit/plugins/host-rpc.test.ts @@ -96,6 +96,16 @@ const userSettings = { } as unknown as PluginUserSettingsService; const dbs = new DatabaseService(mockDb); +// Plan 3c Task 0b: `canAccessTrip` is `TripsRepository.findAccessible` now, +// which needs a real EntityManager over the full migrated schema — this +// file's `mockDb` is a hand-trimmed `:memory:` table set (no `currency` +// column on `trips`, among others), so a real repository read isn't +// possible against it. Spied directly with the exact access rule the old +// `db/database` mock factory above encoded (trip 1 belongs to user 5, user 6 +// is a member), which is now dead code for this purpose. +vi.spyOn(dbs, 'canAccessTrip').mockImplementation(async (tripId, userId) => + Number(tripId) === 1 && (userId === 5 || userId === 6) ? { id: 1, user_id: 5, currency: null } : undefined, +); const guards = new PluginGuards(dbs, permissions, addons); const registry = createTestPluginRegistry([ new DbRpc(userSettings), diff --git a/server/tests/unit/plugins/map-layers.controller.test.ts b/server/tests/unit/plugins/map-layers.controller.test.ts index 0043877b0..13cbc6c1d 100644 --- a/server/tests/unit/plugins/map-layers.controller.test.ts +++ b/server/tests/unit/plugins/map-layers.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => undefined }) }, canAccessTrip })); @@ -35,7 +35,7 @@ const line = (n = 3, over: Record = {}) => ({ const layer = (features: unknown[], over: Record = {}) => ({ id: 'l1', features, ...over }); describe('MapLayersController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / no user / non-member all return [] (no plugin calls on the first)', async () => { pluginsEnabled.mockReturnValue(false); @@ -45,7 +45,7 @@ describe('MapLayersController', () => { pluginsEnabled.mockReturnValue(true); expect((await controller(() => [layer([line()])]).c.get('1', req(undefined))).layers).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [layer([line()])]).c.get('1', req(5))).layers).toEqual([]); }); diff --git a/server/tests/unit/plugins/map-markers.controller.test.ts b/server/tests/unit/plugins/map-markers.controller.test.ts index ba549e8ed..f574f6db9 100644 --- a/server/tests/unit/plugins/map-markers.controller.test.ts +++ b/server/tests/unit/plugins/map-markers.controller.test.ts @@ -7,7 +7,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => undefined }) }, canAccessTrip })); @@ -29,7 +29,7 @@ function controller(invoke: (id: string) => unknown, providers = ['p1']) { const mk = (over: Record = {}) => ({ id: 'm1', lat: 48.85, lng: 2.35, ...over }); describe('MapMarkersController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / no user / non-member all return [] (no plugin calls on the first)', async () => { pluginsEnabled.mockReturnValue(false); @@ -39,7 +39,7 @@ describe('MapMarkersController', () => { pluginsEnabled.mockReturnValue(true); expect((await controller(() => [mk()]).c.get('1', req(undefined))).markers).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [mk()]).c.get('1', req(5))).markers).toEqual([]); }); diff --git a/server/tests/unit/plugins/pdf-sections.controller.test.ts b/server/tests/unit/plugins/pdf-sections.controller.test.ts index 637f77076..33982205d 100644 --- a/server/tests/unit/plugins/pdf-sections.controller.test.ts +++ b/server/tests/unit/plugins/pdf-sections.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => undefined }) }, canAccessTrip })); @@ -30,7 +30,7 @@ function controller(invoke: (id: string) => unknown, providers = ['p1']) { const sec = (over: Record = {}) => ({ title: 'Weather', ...over }); describe('PdfSectionsController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / bad tripId / no user / non-member all return [] (no plugin calls on the first)', async () => { pluginsEnabled.mockReturnValue(false); @@ -41,7 +41,7 @@ describe('PdfSectionsController', () => { expect((await controller(() => [sec()]).c.get('abc', req(5))).sections).toEqual([]); expect((await controller(() => [sec()]).c.get('1', req(undefined))).sections).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [sec()]).c.get('1', req(5))).sections).toEqual([]); }); diff --git a/server/tests/unit/plugins/place-details.controller.test.ts b/server/tests/unit/plugins/place-details.controller.test.ts index cc3e1084f..6f07f51b2 100644 --- a/server/tests/unit/plugins/place-details.controller.test.ts +++ b/server/tests/unit/plugins/place-details.controller.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, placeTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), placeTrip: vi.fn((placeId: number) => (placeId === 7 ? { trip_id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); @@ -9,7 +9,6 @@ vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: (placeId: number) => placeTrip(placeId) }) }, canAccessTrip, })); -import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled })); @@ -24,11 +23,11 @@ function controller(over: Partial = {}) { placeDetails: vi.fn(async (id: string) => (id === 'p2' ? [{ label: 'Rating', value: '4.5' }] : [{ label: 'Reviews', value: '12', url: 'https://x' }])), ...over, } as unknown as PluginHooks; - return { c: new PlaceDetailsController(runtime, new DatabaseService(dbConn)), runtime }; + return { c: new PlaceDetailsController(runtime, { canAccessTrip, connection: { prepare: () => ({ get: (placeId: number) => placeTrip(placeId) }) } } as unknown as DatabaseService), runtime }; } describe('PlaceDetailsController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('returns [] when the runtime is disabled (no plugin calls)', async () => { pluginsEnabled.mockReturnValue(false); @@ -40,7 +39,7 @@ describe('PlaceDetailsController', () => { it('returns [] for an unknown place or a place the caller cannot access', async () => { const { c } = controller(); expect(await c.get('999', req(5))).toEqual({ providers: [] }); // place not found - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect(await c.get('7', req(5))).toEqual({ providers: [] }); // no access }); diff --git a/server/tests/unit/plugins/plugin-guards.test.ts b/server/tests/unit/plugins/plugin-guards.test.ts index 297a29083..118bec8a3 100644 --- a/server/tests/unit/plugins/plugin-guards.test.ts +++ b/server/tests/unit/plugins/plugin-guards.test.ts @@ -30,7 +30,7 @@ const ctx = (actingUserId: number | undefined): PluginRpcContext => ({ /** Trip 1 belongs to user 42 and only user 42 may reach it, mirroring rpc-host.test.ts. */ function build(overrides: { role?: string | undefined; allow?: boolean; addonOn?: boolean } = {}) { const db = { - canAccessTrip: vi.fn((tripId: number, userId: number) => + canAccessTrip: vi.fn(async (tripId: number, userId: number) => tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined, ), prepare: vi.fn(() => ({ @@ -128,7 +128,7 @@ describe('PluginGuards — requireTripEdit and canEditAs', () => { it('PGUARD-014 a non-owner member is flagged as shared', async () => { const db = { - canAccessTrip: vi.fn(() => ({ id: 1, user_id: 7 })), + canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 7 })), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService; const permissions = { checkPermission: vi.fn(() => true) } as unknown as PermissionsService; diff --git a/server/tests/unit/plugins/plugin-routes.controller.test.ts b/server/tests/unit/plugins/plugin-routes.controller.test.ts index ed4bae552..4700aa1bc 100644 --- a/server/tests/unit/plugins/plugin-routes.controller.test.ts +++ b/server/tests/unit/plugins/plugin-routes.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled, capabilitiesRow } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), capabilitiesRow: { value: JSON.stringify({ routeProfiles: [{ id: 'ev', label: 'EV' }] }) as string | undefined }, })); @@ -16,7 +16,6 @@ vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => (capabilitiesRow.value === undefined ? undefined : { capabilities: capabilitiesRow.value }) }) }, canAccessTrip, })); -import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled })); @@ -30,7 +29,7 @@ function controller(invoke: () => unknown, providers = ['ev-plug']) { providersOf: vi.fn(() => providers), route: vi.fn(async () => invoke()), } as unknown as PluginHooks; - return { c: new PluginRoutesController(runtime, new DatabaseService(dbConn)), runtime }; + return { c: new PluginRoutesController(runtime, { canAccessTrip, connection: { prepare: () => ({ get: () => (capabilitiesRow.value === undefined ? undefined : { capabilities: capabilitiesRow.value }) }) } } as unknown as DatabaseService), runtime }; } const wp = (n = 3) => Array.from({ length: n }, (_, i) => ({ lat: 48 + i * 0.1, lng: 2 + i * 0.1 })); const goodRoute = (n = 3, over: Record = {}) => ({ @@ -45,7 +44,7 @@ const body = (over: Record = {}) => ({ tripId: 1, dayId: 7, way describe('PluginRoutesController', () => { beforeEach(() => { pluginsEnabled.mockReturnValue(true); - canAccessTrip.mockReturnValue({ id: 1 } as never); + canAccessTrip.mockResolvedValue({ id: 1 } as never); capabilitiesRow.value = JSON.stringify({ routeProfiles: [{ id: 'ev', label: 'EV' }] }); }); @@ -57,9 +56,9 @@ describe('PluginRoutesController', () => { pluginsEnabled.mockReturnValue(true); expect((await controller(() => goodRoute()).c.route('ev-plug', 'ev', body(), req(undefined))).route).toBeNull(); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => goodRoute()).c.route('ev-plug', 'ev', body(), req(5))).route).toBeNull(); - canAccessTrip.mockReturnValue({ id: 1 } as never); + canAccessTrip.mockResolvedValue({ id: 1 } as never); expect((await controller(() => goodRoute()).c.route('ev-plug', '../etc', body(), req(5))).route).toBeNull(); }); diff --git a/server/tests/unit/plugins/rpc-fallbacks.test.ts b/server/tests/unit/plugins/rpc-fallbacks.test.ts index ae7a0a1e2..4d9a9be8a 100644 --- a/server/tests/unit/plugins/rpc-fallbacks.test.ts +++ b/server/tests/unit/plugins/rpc-fallbacks.test.ts @@ -40,7 +40,7 @@ const ALL = new Set([ function guardsFor() { const db = { - canAccessTrip: vi.fn(() => ({ id: 1, user_id: 42 })), + canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }), all: () => [] })), } as unknown as DatabaseService; return { diff --git a/server/tests/unit/plugins/trip-card-contributions.controller.test.ts b/server/tests/unit/plugins/trip-card-contributions.controller.test.ts index 9ba4c510b..20684cd24 100644 --- a/server/tests/unit/plugins/trip-card-contributions.controller.test.ts +++ b/server/tests/unit/plugins/trip-card-contributions.controller.test.ts @@ -9,7 +9,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ // trips 1 and 2 are accessible to user 5; everything else is not. - canAccessTrip: vi.fn((tripId: number, userId: number) => (userId === 5 && (tripId === 1 || tripId === 2) ? { id: tripId } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (userId === 5 && (tripId === 1 || tripId === 2) ? { id: tripId } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => undefined }) }, canAccessTrip })); diff --git a/server/tests/unit/plugins/trip-warnings.controller.test.ts b/server/tests/unit/plugins/trip-warnings.controller.test.ts index 6d2a11c9d..bdc28bb5b 100644 --- a/server/tests/unit/plugins/trip-warnings.controller.test.ts +++ b/server/tests/unit/plugins/trip-warnings.controller.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: {}, canAccessTrip })); @@ -24,14 +24,14 @@ function controller(over: Partial = {}) { } describe('TripWarningsController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('returns [] when disabled / no user / no access', async () => { pluginsEnabled.mockReturnValue(false); expect(await controller().c.get('1', req(5))).toEqual({ warnings: [] }); pluginsEnabled.mockReturnValue(true); expect(await controller().c.get('1', req(undefined))).toEqual({ warnings: [] }); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect(await controller().c.get('1', req(5))).toEqual({ warnings: [] }); }); diff --git a/server/tests/unit/plugins/view-contributions.controller.test.ts b/server/tests/unit/plugins/view-contributions.controller.test.ts index 1711999a6..79b5090be 100644 --- a/server/tests/unit/plugins/view-contributions.controller.test.ts +++ b/server/tests/unit/plugins/view-contributions.controller.test.ts @@ -8,7 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const { canAccessTrip, pluginsEnabled } = vi.hoisted(() => ({ - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 5 ? { id: 1 } : undefined)), pluginsEnabled: vi.fn(() => true), })); vi.mock('../../../src/db/database', () => ({ db: { prepare: () => ({ get: () => undefined }) }, canAccessTrip })); @@ -31,7 +31,7 @@ const col = (over: Record = {}) => ({ kind: 'column', entityId: const act = (over: Record = {}) => ({ kind: 'action', entityId: 1, id: 'a1', label: 'Go', target: { kind: 'frame', sub: '/ui' }, ...over }); describe('ViewContributionsController', () => { - beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockReturnValue({ id: 1 } as never); }); + beforeEach(() => { pluginsEnabled.mockReturnValue(true); canAccessTrip.mockResolvedValue({ id: 1 } as never); }); it('gates: disabled / unknown view / no user / non-member all return [] (no plugin calls on the first two)', async () => { pluginsEnabled.mockReturnValue(false); @@ -52,7 +52,7 @@ describe('ViewContributionsController', () => { } expect((await controller(() => [col()]).c.get('places', '1', req(undefined))).contributions).toEqual([]); - canAccessTrip.mockReturnValue(undefined as never); + canAccessTrip.mockResolvedValue(undefined as never); expect((await controller(() => [col()]).c.get('day', '1', req(5))).contributions).toEqual([]); }); diff --git a/server/tests/unit/reservations/bookings.rpc.test.ts b/server/tests/unit/reservations/bookings.rpc.test.ts index 0cbac2d25..806216f11 100644 --- a/server/tests/unit/reservations/bookings.rpc.test.ts +++ b/server/tests/unit/reservations/bookings.rpc.test.ts @@ -80,7 +80,7 @@ function build(opts: { canEdit?: boolean; cascade?: boolean; stop?: boolean; see } as unknown as AccommodationsService & Record>; const guards = new PluginGuards( { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService, { diff --git a/server/tests/unit/services/conflictUpdate.test.ts b/server/tests/unit/services/conflictUpdate.test.ts index 658763f01..49a62ff1a 100644 --- a/server/tests/unit/services/conflictUpdate.test.ts +++ b/server/tests/unit/services/conflictUpdate.test.ts @@ -54,7 +54,7 @@ import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repos import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { notificationsStub } from '../../helpers/notifications'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -67,6 +67,15 @@ const photoCache = new PlacePhotoCacheService(dbs, makeStorageFixture('photos/go let packing: PackingService; let places: PlacesService; 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. + 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)); + vi.spyOn(dbs, 'rosterUserIds').mockImplementation((...a) => real.rosterUserIds(...a)); + vi.spyOn(dbs, 'getPlaceWithTags').mockImplementation((...a) => real.getPlaceWithTags(...a)); packing = new PackingService(dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection)); places = new PlacesService( dbs, diff --git a/server/tests/unit/todo/todo.rpc.test.ts b/server/tests/unit/todo/todo.rpc.test.ts index 35e709205..d0433c119 100644 --- a/server/tests/unit/todo/todo.rpc.test.ts +++ b/server/tests/unit/todo/todo.rpc.test.ts @@ -32,7 +32,7 @@ function build(canEdit = true) { const permissions = { checkPermission: vi.fn(() => canEdit) } as unknown as PermissionsService; const guards = new PluginGuards( { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), } as unknown as DatabaseService, permissions, diff --git a/server/tests/unit/trips/trips.rpc.test.ts b/server/tests/unit/trips/trips.rpc.test.ts index b27e01ccf..052272584 100644 --- a/server/tests/unit/trips/trips.rpc.test.ts +++ b/server/tests/unit/trips/trips.rpc.test.ts @@ -61,7 +61,7 @@ export function build(opts: { allow?: (action: string) => boolean; updateThrows? } as unknown as TripMembersService & Record>; const membership = { joinTripAsMember: vi.fn(() => ({ joined: true })) } as unknown as TripMembershipService & Record>; const db = { - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), prepare: vi.fn((sql: string) => ({ get: (arg: number) => { if (sql.includes('FROM users WHERE')) return arg === 404 ? undefined : { id: arg, role: 'user' }; diff --git a/server/tests/websocket/connection.test.ts b/server/tests/websocket/connection.test.ts index 05f13898e..4d7fc26bb 100644 --- a/server/tests/websocket/connection.test.ts +++ b/server/tests/websocket/connection.test.ts @@ -238,6 +238,47 @@ describe('WS rooms', () => { } }); + /** + * Plan 3c Task 0b (R9): `handleJoin`'s `this.db.canAccessTrip(...)` call + * (`realtime.gateway.ts:173`) is now `TripsRepository.findAccessible` + * underneath `DatabaseService`, reached through the request context + * `TrekWsAdapter`'s `@SubscribeMessage` wrapper forks + * (`nest/realtime/trek-ws.adapter.ts:273`, pinned generically by + * SEAM-001/BOOT-SWEEP-001) — no ratchet existed for this SPECIFIC seam + * before this task (inventory §12b). WS-004 above already proves the happy + * path returns `{type:'joined'}`, which requires the repository read to + * have succeeded; this test makes that explicit and structural, the same + * `cannotUseGlobalContext`/"global EntityManager" console-error filter + * `boot-sweeps-request-context.test.ts` uses, so a regression that routes + * `handleJoin` around the adapter's request context fails HERE by message, + * not only by an indirect "joined never arrived" timeout. + */ + it('WS-SEAM-001 — handleJoin\'s repository read runs inside the WS request context (no cannotUseGlobalContext)', async () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const token = createEphemeralToken(user.id, 'ws')!; + + const client = await connectWs(token); + try { + await client.next(); // welcome + client.send({ type: 'join', tripId: trip.id }); + const msg = await client.next(); + expect(msg.type).toBe('joined'); + } finally { + client.close(); + } + + const suspicious = errSpy.mock.calls + .map((args) => args.map(String).join(' ')) + .filter((line) => /cannotUseGlobalContext|global EntityManager/i.test(line)); + expect(suspicious).toEqual([]); + } finally { + errSpy.mockRestore(); + } + }); + it('WS-006 — leave room receives left confirmation', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id);