From 6763648c452e472252d60817a7da663d629ccfe8 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 15:08:49 +0200 Subject: [PATCH] refactor(settings): user and instance settings through repositories SettingsRepository (getForUser/upsertForUser/getOne/deleteForUser) replaces the per-user settings table's raw SQL; SettingsService drops DatabaseService entirely. instance-api-keys.ts's readInstanceApiKey/writeInstanceApiKey/ resolveApiKey take an explicit AppSettingsRepository/UsersRepository instead of a raw connection. An earlier version resolved those two repositories itself from the active MikroORM request context, so none of its six callers needed to change. That worked in production and e2e (always inside a real request) but broke service construction in plain unit tests across every calling domain (addons, auth x2, maps, transit, unsplash - 185 failures on a full run), since none of them wrap a request context. Each of those six services now injects its own AppSettingsRepository/UsersRepository and passes them through explicitly, matching how every other converted repository in this migration is consumed; their own SQL is untouched (outside Plan 3a). --- .../db/repositories/Settings.repository.ts | 78 ++++++++- .../src/db/repositories/Users.repository.ts | 3 +- server/src/nest/addons/addons.module.ts | 8 +- server/src/nest/addons/addons.service.ts | 19 ++- server/src/nest/auth/auth.module.ts | 11 +- server/src/nest/auth/auth.service.ts | 11 +- server/src/nest/auth/user-profile.service.ts | 27 +++- server/src/nest/maps/maps.module.ts | 10 +- server/src/nest/maps/maps.service.ts | 11 +- server/src/nest/settings/instance-api-keys.ts | 75 ++++++--- server/src/nest/settings/settings.module.ts | 16 +- server/src/nest/settings/settings.service.ts | 32 ++-- .../nest/transit/google-transit.provider.ts | 13 +- server/src/nest/transit/transit.module.ts | 11 +- server/src/nest/unsplash/unsplash.module.ts | 11 +- server/src/nest/unsplash/unsplash.service.ts | 18 ++- server/tests/helpers/test-uow.ts | 38 +++++ .../repositories/Settings.repository.test.ts | 150 ++++++++++++++++++ server/tests/unit/mcp/tools-trips.test.ts | 17 ++ server/tests/unit/nest/admin.service.test.ts | 4 +- .../nest/admin.version-notification.test.ts | 4 +- server/tests/unit/nest/auth.service.test.ts | 3 +- .../unit/nest/google-transit.provider.test.ts | 101 +++++++----- .../unit/nest/journey-share.service.test.ts | 15 +- server/tests/unit/nest/maps.amap.test.ts | 20 ++- server/tests/unit/nest/maps.area.test.ts | 10 +- .../tests/unit/nest/maps.autocomplete.test.ts | 10 +- .../unit/nest/maps.curated-images.test.ts | 2 +- server/tests/unit/nest/maps.geocode.test.ts | 10 +- .../tests/unit/nest/maps.gers-details.test.ts | 12 +- server/tests/unit/nest/maps.identity.test.ts | 2 +- server/tests/unit/nest/maps.pois.test.ts | 10 +- .../tests/unit/nest/maps.search-merge.test.ts | 10 +- server/tests/unit/nest/maps.service.test.ts | 40 +++-- server/tests/unit/nest/oidc.service.test.ts | 3 +- .../tests/unit/nest/passkey.service.test.ts | 3 +- .../tests/unit/nest/places.directions.test.ts | 4 +- server/tests/unit/nest/places.service.test.ts | 17 +- .../tests/unit/nest/settings.service.test.ts | 44 +++-- .../nest/settings/instance-api-keys.test.ts | 109 +++++++------ server/tests/unit/nest/share.service.test.ts | 13 +- .../tests/unit/nest/transit.service.test.ts | 6 +- .../unit/nest/trip-read-model.service.test.ts | 6 +- server/tests/unit/nest/trips.service.test.ts | 6 +- .../nest/unsplash/unsplash.service.test.ts | 61 ++++--- .../unit/nest/user-profile.service.test.ts | 4 +- .../unit/services/conflictUpdate.test.ts | 6 +- 47 files changed, 813 insertions(+), 281 deletions(-) create mode 100644 server/tests/unit/db/repositories/Settings.repository.test.ts diff --git a/server/src/db/repositories/Settings.repository.ts b/server/src/db/repositories/Settings.repository.ts index c22855775..250ab2ba5 100644 --- a/server/src/db/repositories/Settings.repository.ts +++ b/server/src/db/repositories/Settings.repository.ts @@ -1,4 +1,80 @@ import type { Settings } from '../entities/Settings.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; import { EntityRepository } from '@mikro-orm/sql'; -export class SettingsRepository extends EntityRepository {} +/** A `settings` row (the per-user key/value table) as the API emits it. */ +export interface SettingRow { + id: number; + user_id: number; + key: string; + value: string | null; +} + +const _settingRowKeys: AssertRowKeys = true; + +export class SettingsRepository extends EntityRepository { + /** `SELECT key, value FROM settings WHERE user_id = ?` */ + async getForUser(userId: number): Promise { + const rows = await this.find({ user: userId }); + return rows.map((row) => toRow(row) as SettingRow); + } + + /** + * `SELECT value FROM settings WHERE user_id = ? AND key = ?` + * + * No `refresh: true`: the identity-map short-circuit (Task 0 review, I1; + * corrected by the Task 3 review — it is NOT about a `fields` restriction) + * only fires for a PRIMARY-KEY-ONLY filter (`findOne({ id })` / `findOne(pk)`): + * MikroORM answers that shape straight from the identity map with zero + * queries, invisible to a write on the same row earlier in the request. + * This filter is `(user, key)` — `Settings`'s PK is the surrogate `id`, not + * this composite — so it is never PK-only and always re-queries, the same + * guarantee a `find()` call gets. `SETTINGSREPO-012`/`013` pin this + * directly (a raw UPDATE/DELETE on the same row, then `getOne` in the same + * request, sees it) rather than assume it. + */ + async getOne(userId: number, key: string): Promise { + const row = await this.findOne({ user: userId, key }); + return row ? (toRow(row) as SettingRow) : null; + } + + /** + * `INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) + * ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value` + * + * The composite conflict target is the relation field (`user`), never its + * `persist(false)` scalar twin (`user_id`) — same rule as every other + * `create`/`upsert` call in this migration (D4's defaults rule): the + * relation is the one column MikroORM actually writes, the twin is filled + * by a subsequent read. `onConflictFields: ['user', 'key']` matches + * `Settings`'s own `uniques: [{ properties: ['user_id', 'key'] }]` + * (`Settings.entity.ts`, generated off the schema's inline + * `UNIQUE(user_id, key)` by the `fix(db)` commit that precedes this one) — + * `Settings.repository.test.ts` asserts both the schema-level unique index + * and the entity metadata before this method relies on either. + * + * Only `value` is ever updated on conflict, matching the legacy statement's + * `DO UPDATE SET value = excluded.value` exactly (no other column, no + * `updated_at` — the `settings` table has none). + */ + async upsertForUser(userId: number, key: string, value: string): Promise { + await this.upsert({ user: userId, key, value }, { onConflictFields: ['user', 'key'], onConflictAction: 'merge' }); + } + + /** + * `DELETE FROM settings WHERE user_id = ?` (`key` omitted) or + * `DELETE FROM settings WHERE user_id = ? AND key = ?` (`key` given). + * + * No site in `nest/settings` itself calls this today (the inventory's 10 + * sites have no `DELETE FROM settings` — the one existing call, + * `plugin-runtime.service.ts`'s per-plugin `key LIKE 'plugin::%'` + * sweep, is `nest/plugins`' own conversion, out of this task's scope). + * Declared for the same reason `AppSettingsRepository.deleteValue` is: the + * repository's surface should be symmetric, and a future caller (a + * user-cleanup/account-deletion flow, say) gets a real method instead of + * reaching for raw SQL. Exercised by its own repository test only. + */ + async deleteForUser(userId: number, key?: string): Promise { + return this.nativeDelete(key === undefined ? { user: userId } : { user: userId, key }); + } +} diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 51b0f7c90..9e0e67dcf 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -9,8 +9,7 @@ import { EntityRepository } from '@mikro-orm/sql'; * name outside this union fails `tsc` rather than reaching a query. * * This repository is the union's source of truth; `instance-api-keys.ts` - * will import it rather than keeping its own copy (Task 5 wires that import - * — it still declares the identical union itself today). + * imports it rather than keeping its own copy (Task 5). */ export type InstanceApiKeyName = 'maps_api_key' | 'unsplash_api_key' | 'amap_api_key'; diff --git a/server/src/nest/addons/addons.module.ts b/server/src/nest/addons/addons.module.ts index 960298dde..1f19b64b7 100644 --- a/server/src/nest/addons/addons.module.ts +++ b/server/src/nest/addons/addons.module.ts @@ -8,6 +8,7 @@ import { Addons } from '../../db/entities/Addons.entity'; import { PhotoProviders } from '../../db/entities/PhotoProviders.entity'; import { PhotoProviderFields } from '../../db/entities/PhotoProviderFields.entity'; import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Users } from '../../db/entities/Users.entity'; /** * GET /api/addons — enabled add-ons + photo providers (was an inline handler in @@ -27,10 +28,13 @@ import { AppSettings } from '../../db/entities/AppSettings.entity'; * MikroOrmModule.forFeature registers AddonsRepository/PhotoProvidersRepository/ * PhotoProviderFieldsRepository/AppSettingsRepository for AddonsService's * @InjectRepository constructor (Plan 3a Task 4) — the forFeature + - * @InjectRepository wiring pattern Task 0 set up on SettingsModule. + * @InjectRepository wiring pattern Task 0 set up on SettingsModule. `Users` was + * added by Task 5: `googleKeySource` passes `UsersRepository` explicitly to + * `instance-api-keys.ts`'s resolveApiKey now, instead of that file resolving + * one off the ambient request context. */ @Module({ - imports: [MikroOrmModule.forFeature([Addons, PhotoProviders, PhotoProviderFields, AppSettings])], + imports: [MikroOrmModule.forFeature([Addons, PhotoProviders, PhotoProviderFields, AppSettings, Users])], controllers: [AddonsController], providers: [AddonsService, AddonGuard, AddonsMcp], exports: [AddonsService, AddonGuard], diff --git a/server/src/nest/addons/addons.service.ts b/server/src/nest/addons/addons.service.ts index c97c8786f..eceefd6ee 100644 --- a/server/src/nest/addons/addons.service.ts +++ b/server/src/nest/addons/addons.service.ts @@ -15,6 +15,8 @@ import { PhotoProviderFields } from '../../db/entities/PhotoProviderFields.entit import type { PhotoProviderFieldsRepository, PhotoProviderFieldRow } from '../../db/repositories/PhotoProviderFields.repository'; import { AppSettings } from '../../db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; /** * Thin wrapper around the enabled-addons + photo-provider read that the legacy @@ -30,15 +32,17 @@ import type { AppSettingsRepository } from '../../db/repositories/AppSettings.re * per-call repository calls so admin toggles stay immediately visible. * * `DatabaseService` stays injected (Plan 3a Task 4) purely as a passthrough - * for two cross-domain functions this service still calls: `transit-provider.ts`'s - * readTransitProvider/writeTransitProvider (nest/transit, not one of this - * plan's six domains) and `instance-api-keys.ts`'s resolveApiKey (nest/settings, - * Task 5's own conversion). Neither reads/writes through `this.db` in THIS - * file — every `app_settings`/`addons`/`photo_providers`/`photo_provider_fields` + * for `transit-provider.ts`'s readTransitProvider/writeTransitProvider + * (nest/transit, not one of this plan's six domains — its own conversion is + * a later phase). It does not read/write through `this.db` in THIS file — + * every `app_settings`/`addons`/`photo_providers`/`photo_provider_fields` * site this service itself used to touch is repository-backed below — so this * is the same kind of carve-out `permissions`' two guard delegations are: `grep * DatabaseService src/nest/addons` still finds this constructor parameter and - * its import, not a raw query. + * its import, not a raw query. `instance-api-keys.ts`'s resolveApiKey + * (nest/settings, Task 5) no longer needs it: `googleKeySource` below passes + * this service's own already-injected `AppSettingsRepository` plus a + * `UsersRepository` Task 5 added here for exactly that. */ @Injectable() export class AddonsService { @@ -47,6 +51,7 @@ export class AddonsService { @InjectRepository(PhotoProviders) private readonly photoProviders: PhotoProvidersRepository, @InjectRepository(PhotoProviderFields) private readonly photoProviderFields: PhotoProviderFieldsRepository, @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly users: UsersRepository, private readonly dbs: DatabaseService, ) {} @@ -249,7 +254,7 @@ export class AddonsService { * — the #1939 shape, one layer up. */ private async googleKeySource(userId: number): Promise { - return (await resolveApiKey(this.dbs, 'maps_api_key', userId, readEnv().maps.placesApiKey)).source; + return (await resolveApiKey(this.appSettings, this.users, 'maps_api_key', userId, readEnv().maps.placesApiKey)).source; } async getTransitProvider(userId = 0) { diff --git a/server/src/nest/auth/auth.module.ts b/server/src/nest/auth/auth.module.ts index 2542227b8..bbd03f773 100644 --- a/server/src/nest/auth/auth.module.ts +++ b/server/src/nest/auth/auth.module.ts @@ -1,5 +1,8 @@ import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Users } from '../../db/entities/Users.entity'; import { TokensModule } from '../tokens/tokens.module'; import { AuthPublicController } from './auth-public.controller'; import { AuthController } from './auth.controller'; @@ -62,7 +65,13 @@ import { AllowedFileTypesModule } from '../files/allowed-file-types.module'; }), StorageModule, AllowedFileTypesModule, - EphemeralTokenModule, RateLimitModule, AuditModule, PermissionsModule, TripMembershipModule, MailerModule, AppConfigModule, TokensModule, BudgetModule], + EphemeralTokenModule, RateLimitModule, AuditModule, PermissionsModule, TripMembershipModule, MailerModule, AppConfigModule, TokensModule, BudgetModule, + // AppSettings/Users: AuthService/UserProfileService each pass their own + // AppSettingsRepository/UsersRepository to instance-api-keys.ts's + // resolveApiKey/readInstanceApiKey/writeInstanceApiKey now (Plan 3a Task + // 5) — everything else either service still reads/writes is raw SQL + // through DatabaseService (auth's own conversion is a later plan). + MikroOrmModule.forFeature([AppSettings, Users])], controllers: [AuthPublicController, AuthController, PasskeyController], providers: [AuthService, UserProfileService, RegistrationInvitesService, PasskeyService, UserCleanupService, WebauthnConfigService, AuthMcp], exports: [AuthService, RegistrationInvitesService, PasskeyService, UserCleanupService], diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index c55ef280d..d83ca2a5d 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -16,6 +16,11 @@ import { validatePassword } from '../common/passwordPolicy'; import { encryptMfaSecret, decryptMfaSecret } from '../common/crypto/mfaCrypto'; import { decrypt_api_key, maybe_encrypt_api_key, encrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { resolveApiKey } from '../settings/instance-api-keys'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; // Type-and-guard only: the app-config read reports the provider choice, it does // not construct one, so this does not pull the maps domain into auth. import { isPlacesProviderChoice } from '../maps/providers/places-provider'; @@ -134,6 +139,8 @@ export class AuthService { private readonly tokens: EphemeralTokenService, private readonly allowedFileTypes: AllowedFileTypesService, private readonly uow: UnitOfWork, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, ) {} // Cookie @@ -267,11 +274,11 @@ export class AuthService { // nor hide them from a member who does have one (#1939). Unauthenticated the // question is only about the instance, which is the first two steps of the // chain; id 0 matches no row. - const hasGoogleKey = !!(await resolveApiKey(this.db, 'maps_api_key', authenticatedUser?.id ?? 0, readEnv().maps.placesApiKey)).key; + const hasGoogleKey = !!(await resolveApiKey(this.appSettings, this.usersRepo, 'maps_api_key', authenticatedUser?.id ?? 0, readEnv().maps.placesApiKey)).key; // The same question for Amap, asked the same way. The client needs both to // tell "search is unavailable" from "search runs on OpenStreetMap", and to // know whether the provider the admin selected actually has a credential. - const hasAmapKey = !!(await resolveApiKey(this.db, 'amap_api_key', authenticatedUser?.id ?? 0, readEnv().maps.amapApiKey)).key; + const hasAmapKey = !!(await resolveApiKey(this.appSettings, this.usersRepo, 'amap_api_key', authenticatedUser?.id ?? 0, readEnv().maps.amapApiKey)).key; const placesProviderRow = this.db.get<{ value: string }>("SELECT value FROM app_settings WHERE key = 'places_provider'")?.value; const placesProvider = isPlacesProviderChoice(placesProviderRow) ? placesProviderRow : 'auto'; const oidcDisplayName = readEnv().oidc.displayName || diff --git a/server/src/nest/auth/user-profile.service.ts b/server/src/nest/auth/user-profile.service.ts index de73e347e..d775a6d98 100644 --- a/server/src/nest/auth/user-profile.service.ts +++ b/server/src/nest/auth/user-profile.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { readEnv, getAppUrl } from '../../app-config'; import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; @@ -16,6 +17,10 @@ import { } from '../settings/instance-api-keys'; import { SEARCH_TEXT_FIELD_MASK } from '../maps/maps.helpers'; import { User } from '../../types'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; /** * The account a user administers about themselves: display settings, avatar, @@ -28,8 +33,12 @@ import { User } from '../../types'; * moved verbatim — same SQL, same validation order, same masking, same error * strings and status codes. * - * DatabaseService is the only injected dependency, which is what made this the - * second-cheapest cut after tokens. + * DatabaseService was the only injected dependency at first, which is what + * made this the second-cheapest cut after tokens; `AppSettingsRepository`/ + * `UsersRepository` (Plan 3a Task 5) were added later purely so the three + * `instance-api-keys.ts` calls below pass their own repository rather than + * that file resolving one itself — every other read/write here is still + * `DatabaseService`'s raw SQL (auth is outside Plan 3a). */ @Injectable() export class UserProfileService { @@ -37,6 +46,8 @@ export class UserProfileService { private readonly db: DatabaseService, private readonly storage: StorageService, private readonly uow: UnitOfWork, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, ) {} /** @@ -74,7 +85,7 @@ export class UserProfileService { isAdmin: boolean, ): Promise { if (isAdmin && (INSTANCE_API_KEY_NAMES as readonly string[]).includes(name)) { - const instance = await readInstanceApiKey(this.db, name as InstanceApiKeyName); + const instance = await readInstanceApiKey(this.appSettings, name as InstanceApiKeyName); if (instance !== null) return instance; } return decrypt_api_key(current?.[name]) ?? ''; @@ -119,7 +130,7 @@ export class UserProfileService { private async mirrorInstanceKeys(body: Record, isAdmin: boolean): Promise { if (!isAdmin) return; for (const name of INSTANCE_API_KEY_NAMES) { - if (body[name] !== undefined) await writeInstanceApiKey(this.db, name, body[name]); + if (body[name] !== undefined) await writeInstanceApiKey(this.appSettings, name, body[name]); } } @@ -279,10 +290,10 @@ export class UserProfileService { // instance value exists — on that install it is what the resolver picks too. return { settings: { - maps_api_key: (await readInstanceApiKey(this.db, 'maps_api_key')) ?? decrypt_api_key(user.maps_api_key), + maps_api_key: (await readInstanceApiKey(this.appSettings, 'maps_api_key')) ?? decrypt_api_key(user.maps_api_key), openweather_api_key: decrypt_api_key(user.openweather_api_key), - unsplash_api_key: (await readInstanceApiKey(this.db, 'unsplash_api_key')) ?? decrypt_api_key(user.unsplash_api_key), - amap_api_key: (await readInstanceApiKey(this.db, 'amap_api_key')) ?? decrypt_api_key(user.amap_api_key), + unsplash_api_key: (await readInstanceApiKey(this.appSettings, 'unsplash_api_key')) ?? decrypt_api_key(user.unsplash_api_key), + amap_api_key: (await readInstanceApiKey(this.appSettings, 'amap_api_key')) ?? decrypt_api_key(user.amap_api_key), }, }; } @@ -356,7 +367,7 @@ export class UserProfileService { // The key a search would actually use, not the one in this admin's column: // testing a value nothing resolves to is how "the panel says the key is // fine" and "every search 403s" coexisted (#1939). - const { key: maps_api_key } = await resolveApiKey(this.db, 'maps_api_key', userId, readEnv().maps.placesApiKey); + const { key: maps_api_key } = await resolveApiKey(this.appSettings, this.usersRepo, 'maps_api_key', userId, readEnv().maps.placesApiKey); if (maps_api_key) { try { // Same Referer as maps.service googleFetch — without it, keys with an diff --git a/server/src/nest/maps/maps.module.ts b/server/src/nest/maps/maps.module.ts index d0056199b..805cc4f7e 100644 --- a/server/src/nest/maps/maps.module.ts +++ b/server/src/nest/maps/maps.module.ts @@ -1,18 +1,26 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { MapsController } from './maps.controller'; import { MapsService } from './maps.service'; import { MapsMcp } from './maps.mcp'; import { PlacePhotosModule } from '../place-photos/place-photos.module'; import { StorageModule } from '../storage/storage.module'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Users } from '../../db/entities/Users.entity'; /** * Maps / geo domain (L3 leaf module). Registered in AppModule. Exports * MapsService for the in-container consumers (BookingImportModule's Nominatim * geocoding, PlacesModule's search_place tool and list-import enrichment). * Nothing outside the container consumes this domain, so there is no bridge. + * + * MikroOrmModule.forFeature([AppSettings, Users]): MapsService passes its own + * AppSettingsRepository/UsersRepository to instance-api-keys.ts's + * resolveApiKey now (Plan 3a Task 5) — every other read here is still raw SQL + * through DatabaseService (maps is outside Plan 3a). */ @Module({ - imports: [PlacePhotosModule, StorageModule], + imports: [PlacePhotosModule, StorageModule, MikroOrmModule.forFeature([AppSettings, Users])], controllers: [MapsController], providers: [MapsService, MapsMcp], exports: [MapsService], diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 398309ecd..5d9f187c3 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -13,6 +13,11 @@ import { readEnv, getAppUrl } from '../../app-config'; import { safeFetchFollow, SsrfBlockedError } from '../../utils/ssrfGuard'; import { discardBody, exceedsDeclaredLength, readCapped, readCappedText } from '../../utils/cappedFetch'; import { resolveApiKey, type ApiKeySource } from '../settings/instance-api-keys'; +import { InjectRepository } from '@mikro-orm/nestjs'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import { isPlacesProviderChoice, type PlacesProviderChoice } from './providers/places-provider'; import { AMAP_SHORT_HOSTS, @@ -690,6 +695,8 @@ export class MapsService { constructor( private readonly database: DatabaseService, private readonly photoCache: PlacePhotoCacheService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, ) {} /** Brand id → logo bytes, or null for "asked, has none". Insertion-ordered, so the @@ -1002,7 +1009,7 @@ export class MapsService { * is returned so a provider error can say which of the three was used. */ async resolveMapsKey(userId: number): Promise<{ key: string | null; source: ApiKeySource | null }> { - return resolveApiKey(this.database, 'maps_api_key', userId, readEnv().maps.placesApiKey); + return resolveApiKey(this.appSettings, this.usersRepo, 'maps_api_key', userId, readEnv().maps.placesApiKey); } async getMapsKey(userId: number): Promise { @@ -1011,7 +1018,7 @@ export class MapsService { /** The Amap credential, resolved through the identical three-step chain. */ async resolveAmapKey(userId: number): Promise<{ key: string | null; source: ApiKeySource | null }> { - return resolveApiKey(this.database, 'amap_api_key', userId, readEnv().maps.amapApiKey); + return resolveApiKey(this.appSettings, this.usersRepo, 'amap_api_key', userId, readEnv().maps.amapApiKey); } // ── Keyed provider selection ─────────────────────────────────────────────── diff --git a/server/src/nest/settings/instance-api-keys.ts b/server/src/nest/settings/instance-api-keys.ts index 4fbdcc242..f51c99859 100644 --- a/server/src/nest/settings/instance-api-keys.ts +++ b/server/src/nest/settings/instance-api-keys.ts @@ -1,5 +1,6 @@ -import { DatabaseService } from '../database/database.service'; import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import type { UsersRepository, InstanceApiKeyName } from '../../db/repositories/Users.repository'; import type { ApiKeySource } from '@trek/shared'; /** @@ -17,8 +18,12 @@ import type { ApiKeySource } from '@trek/shared'; * Stored as an encrypted `app_settings` row under the name below, reusing * apiKeyCrypto, so the format matches what the users columns already hold and * a legacy plaintext value still reads back. + * + * `InstanceApiKeyName` itself now lives on `Users.repository.ts` (Plan 3a Task + * 0/5) — re-exported here so this module's existing external imports of the + * type (`auth/user-profile.service.ts` and others) keep working unchanged. */ -export type InstanceApiKeyName = 'maps_api_key' | 'unsplash_api_key' | 'amap_api_key'; +export type { InstanceApiKeyName }; /** Instance names whose per-user column is still honoured as a last resort. */ export const INSTANCE_API_KEY_NAMES: readonly InstanceApiKeyName[] = [ @@ -37,20 +42,42 @@ export const INSTANCE_API_KEY_NAMES: readonly InstanceApiKeyName[] = [ */ export type { ApiKeySource }; -// Full statements rather than an interpolated column: the name doubles as the -// users column AND the app_settings key, and identifiers only ever come from a -// literal allow-list. -const USER_ROW_SQL: Record = { - maps_api_key: 'SELECT maps_api_key FROM users WHERE id = ?', - unsplash_api_key: 'SELECT unsplash_api_key FROM users WHERE id = ?', - amap_api_key: 'SELECT amap_api_key FROM users WHERE id = ?', -}; +/** + * These three functions used to take the raw-connection wrapper class + * (`nest/database/database.service.ts`'s injectable) and run raw SQL through + * it. They now take the caller's own `AppSettingsRepository`/`UsersRepository` + * directly — every one of the six callers (`nest/addons`, `nest/auth` × 2, + * `nest/maps`, `nest/transit`, `nest/unsplash`) constructor-injects them via + * `@InjectRepository` and its own module's `MikroOrmModule.forFeature`, the + * same wiring pattern every other converted domain in this plan uses. + * + * An earlier version of this file resolved the repositories itself from the + * ACTIVE MikroORM request context (`RequestContext.getEntityManager()`) + * instead of taking them as parameters, so none of the six callers' modules + * needed new wiring. That worked in production and in e2e (both always run + * inside a real request context, D6) but broke everywhere a unit test + * constructs one of those six services directly with no MikroORM context at + * all — which turned out to be the norm, not the exception: a full `npm run + * test` run surfaced 185 failures across 12 files + * (`addons.service.test.ts`, `auth.service.test.ts`, + * `user-profile.service.test.ts`, `maps.service.test.ts` and its siblings, + * `google-transit.provider.test.ts`, `unsplash.service.test.ts`, + * `places.service.test.ts` transitively via maps, plus the one MCP-harness + * case this was first caught from). Explicit repository parameters, matching + * how every other converted repository in this migration is consumed, has no + * such failure mode: a repository built via `t.repo(X)` (`allowGlobalContext: + * true` in tests) works with or without a request context, exactly like + * `AppSettingsRepository`'s/`SettingsRepository`'s own tests already prove. + */ +async function readInstanceValue(appSettings: AppSettingsRepository, name: InstanceApiKeyName): Promise { + const value = await appSettings.getValue(name); + if (!value) return null; + return decrypt_api_key(value) || null; +} /** The instance-wide value in cleartext, or null when unset/cleared. */ -export async function readInstanceApiKey(db: DatabaseService, name: InstanceApiKeyName): Promise { - const row = db.get<{ value: string | null }>('SELECT value FROM app_settings WHERE key = ?', name); - if (!row?.value) return null; - return decrypt_api_key(row.value) || null; +export async function readInstanceApiKey(appSettings: AppSettingsRepository, name: InstanceApiKeyName): Promise { + return readInstanceValue(appSettings, name); } /** @@ -59,12 +86,8 @@ export async function readInstanceApiKey(db: DatabaseService, name: InstanceApiK * key", and a missing row would let the resolver fall through to whatever old * value still sits in their own users column. */ -export async function writeInstanceApiKey(db: DatabaseService, name: InstanceApiKeyName, value: unknown): Promise { - db.run( - `INSERT INTO app_settings (key, value) VALUES (?, ?) - ON CONFLICT(key) DO UPDATE SET value = excluded.value`, - name, maybe_encrypt_api_key(value) ?? '' - ); +export async function writeInstanceApiKey(appSettings: AppSettingsRepository, name: InstanceApiKeyName, value: unknown): Promise { + await appSettings.setValue(name, maybe_encrypt_api_key(value) ?? ''); } /** @@ -79,20 +102,24 @@ export async function writeInstanceApiKey(db: DatabaseService, name: InstanceApi * * `userId` 0 means "nobody is asking" (the unauthenticated app-config read): * there is no personal key to find, so the chain ends at the instance. + * + * The per-user fallback used to be a literal `USER_ROW_SQL[name]` statement + * map; it is now `UsersRepository.getApiKeyColumn(userId, name)`, the typed + * dispatch `Users.repository.ts` (Plan 3a Task 0) already provides. */ export async function resolveApiKey( - db: DatabaseService, + appSettings: AppSettingsRepository, + users: UsersRepository, name: InstanceApiKeyName, userId: number, operatorKey: string | undefined, ): Promise<{ key: string | null; source: ApiKeySource | null }> { if (operatorKey) return { key: operatorKey, source: 'operator-env' }; - const instance = await readInstanceApiKey(db, name); + const instance = await readInstanceValue(appSettings, name); if (instance) return { key: instance, source: 'instance' }; if (!userId) return { key: null, source: null }; - const row = db.get>(USER_ROW_SQL[name], userId); - const own = decrypt_api_key(row?.[name]) || null; + const own = decrypt_api_key(await users.getApiKeyColumn(userId, name)) || null; return own ? { key: own, source: 'user-row' } : { key: null, source: null }; } diff --git a/server/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index a46b75580..1fe8a0929 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -7,14 +7,24 @@ import { SettingsService } from './settings.service'; import { SettingsMcp } from './settings.mcp'; import { AppConfigModule } from '../app-config/app-config.module'; import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Settings } from '../../db/entities/Settings.entity'; +import { Users } from '../../db/entities/Users.entity'; /** Exports SettingsService for in-container consumers (admin, share, llm-parse). */ @Module({ // AuthModule for the admin gate on the defaults routes, and for the // AuthService that SettingsMcp's demo gate injects. MikroOrmModule.forFeature - // registers AppSettingsRepository for @InjectRepository(AppSettings) below — - // the forFeature + @InjectRepository wiring pattern every later domain copies. - imports: [AppConfigModule, AuthModule, AuditModule, MikroOrmModule.forFeature([AppSettings])], + // registers AppSettingsRepository/SettingsRepository for the + // @InjectRepository(...) constructor params below (Plan 3a Task 0/5) — the + // forFeature + @InjectRepository wiring pattern every later domain copies. + // `Users` is registered too even though nothing here uses + // `@InjectRepository(Users)`: `instance-api-keys.ts`'s repository-backed + // functions resolve `UsersRepository` off the ACTIVE MikroORM request + // context instead (see that file's own docstring for why — its callers + // span ~6 domains outside this module's DI graph), so this entry is here + // for parity with the brief/inventory's documented wiring, not because a + // provider in this module needs the token. + imports: [AppConfigModule, AuthModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings, Users])], controllers: [SettingsController, AdminDefaultUserSettingsController], providers: [SettingsService, SettingsMcp], exports: [SettingsService], diff --git a/server/src/nest/settings/settings.service.ts b/server/src/nest/settings/settings.service.ts index 6dd83ab70..f23f6602c 100644 --- a/server/src/nest/settings/settings.service.ts +++ b/server/src/nest/settings/settings.service.ts @@ -1,12 +1,13 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { MASKED_SETTING_VALUE, normalizeAppearance } from '@trek/shared'; import { readEnv } from '../../app-config'; import { AppSettings } from '../../db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Settings } from '../../db/entities/Settings.entity'; +import type { SettingsRepository } from '../../db/repositories/Settings.repository'; /** * Exported so a caller that hands settings to somebody else can assert its own @@ -154,11 +155,9 @@ function serializeValue(key: string, value: unknown): string { @Injectable() export class SettingsService { constructor( - private readonly db: DatabaseService, private readonly uow: UnitOfWork, - // Wiring proof (Plan 3a Task 0): the rest of this service's app_settings - // sites (S2/S3, the admin-defaults writes) stay on `this.db` until Task 5. @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Settings) private readonly settings: SettingsRepository, ) {} async getAdminUserDefaults(): Promise> { @@ -182,12 +181,6 @@ export class SettingsService { } async setAdminUserDefaults(partial: Record): Promise { - const upsert = this.db.prepare( - `INSERT INTO app_settings (key, value) VALUES (?, ?) - ON CONFLICT(key) DO UPDATE SET value = excluded.value` - ); - const del = this.db.prepare("DELETE FROM app_settings WHERE key = ?"); - await this.uow.transactional(async () => { for (const [key, value] of Object.entries(partial)) { if (!(DEFAULTABLE_USER_SETTING_KEYS as readonly string[]).includes(key)) { @@ -198,7 +191,7 @@ export class SettingsService { // null/undefined means "reset to built-in default" — delete the row if (value === null || value === undefined) { - del.run(appKey); + await this.appSettings.deleteValue(appKey); continue; } @@ -215,7 +208,7 @@ export class SettingsService { const stored = ENCRYPTED_SETTING_KEYS.has(key) ? (maybe_encrypt_api_key(String(value)) ?? String(value)) : JSON.stringify(value); - upsert.run(appKey, stored); + await this.appSettings.setValue(appKey, stored); } }); } @@ -223,7 +216,7 @@ export class SettingsService { async getUserSettings(userId: number): Promise> { const adminDefaults = await this.getAdminUserDefaults(); - const rows = this.db.all<{ key: string; value: string }>('SELECT key, value FROM settings WHERE user_id = ?', userId); + const rows = await this.settings.getForUser(userId); const userSettings: Record = {}; for (const row of rows) { if (MASKED_SETTING_KEYS.has(row.key)) { @@ -292,17 +285,10 @@ export class SettingsService { } async upsertSetting(userId: number, key: string, value: unknown) { - this.db.run(` - INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) - ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value - `, userId, key, serializeValue(key, value)); + await this.settings.upsertForUser(userId, key, serializeValue(key, value)); } async bulkUpsertSettings(userId: number, settings: Record) { - const upsert = this.db.prepare(` - INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) - ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value - `); let written = 0; await this.uow.transactional(async () => { for (const [key, value] of Object.entries(settings)) { @@ -310,7 +296,7 @@ export class SettingsService { // bulk save can never overwrite a stored secret with the mask (the // single-upsert route has the same no-op in the controller). if (value === MASKED_SETTING_VALUE) continue; - upsert.run(userId, key, serializeValue(key, value)); + await this.settings.upsertForUser(userId, key, serializeValue(key, value)); written++; } }); @@ -324,7 +310,7 @@ export class SettingsService { * resolver needs the real API key). Returns null when unset. */ async getDecryptedUserSetting(userId: number, key: string): Promise { - const row = this.db.get<{ value: string }>('SELECT value FROM settings WHERE user_id = ? AND key = ?', userId, key); + const row = await this.settings.getOne(userId, key); if (!row || row.value === '' || row.value == null) return null; if (ENCRYPTED_SETTING_KEYS.has(key)) return decrypt_api_key(row.value); try { diff --git a/server/src/nest/transit/google-transit.provider.ts b/server/src/nest/transit/google-transit.provider.ts index 5f1b9733d..eb6a5c656 100644 --- a/server/src/nest/transit/google-transit.provider.ts +++ b/server/src/nest/transit/google-transit.provider.ts @@ -1,8 +1,13 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { readEnv } from '../../app-config'; import { DatabaseService } from '../database/database.service'; import { toApiLang } from '../maps/maps.helpers'; import { resolveApiKey, type ApiKeySource } from '../settings/instance-api-keys'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import { readTransitProvider } from './transit-provider'; import { decodePolyline, @@ -234,10 +239,14 @@ function stopFrom(stop: GoogleStop | undefined, fallback: GoogleLatLng | undefin @Injectable() export class GoogleTransitProvider { - constructor(private readonly database: DatabaseService) {} + constructor( + private readonly database: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, + ) {} private async resolveKey(userId: number): Promise<{ key: string | null; source: ApiKeySource | null }> { - return resolveApiKey(this.database, 'maps_api_key', userId, readEnv().maps.placesApiKey); + return resolveApiKey(this.appSettings, this.usersRepo, 'maps_api_key', userId, readEnv().maps.placesApiKey); } /** diff --git a/server/src/nest/transit/transit.module.ts b/server/src/nest/transit/transit.module.ts index 2a482f56e..8503fa070 100644 --- a/server/src/nest/transit/transit.module.ts +++ b/server/src/nest/transit/transit.module.ts @@ -1,5 +1,6 @@ import { RateLimitModule } from '../common/rate-limit.module'; import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { TransitController } from './transit.controller'; import { TransitService } from './transit.service'; import { TransitMcp } from './transit.mcp'; @@ -8,16 +9,24 @@ import { DaysModule } from '../days/days.module'; import { ReservationsModule } from '../reservations/reservations.module'; import { AuthModule } from '../auth/auth.module'; import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Users } from '../../db/entities/Users.entity'; /** * Transit domain (#1065) — the Transitous/MOTIS proxy, with the optional * Google backend (#1699) behind it. TransitMcp carries the * decorator-registered MCP tools; DaysModule/ReservationsModule feed * create_transit_journey. Exports TransitService for in-container consumers. + * + * MikroOrmModule.forFeature([AppSettings, Users]): GoogleTransitProvider + * passes its own AppSettingsRepository/UsersRepository to + * instance-api-keys.ts's resolveApiKey now (Plan 3a Task 5) — everything else + * in this domain (transit-provider.ts included) is still raw SQL through + * DatabaseService (transit's own conversion is a later plan). */ @Module({ // DaysModule + ReservationsModule: TransitMcp's create_transit_journey injects both. - imports: [McpSharedModule, RateLimitModule, DaysModule, ReservationsModule, AuthModule], + imports: [McpSharedModule, RateLimitModule, DaysModule, ReservationsModule, AuthModule, MikroOrmModule.forFeature([AppSettings, Users])], controllers: [TransitController], providers: [TransitService, TransitMcp, GoogleTransitProvider], exports: [TransitService], diff --git a/server/src/nest/unsplash/unsplash.module.ts b/server/src/nest/unsplash/unsplash.module.ts index 0fa967257..d44b8d004 100644 --- a/server/src/nest/unsplash/unsplash.module.ts +++ b/server/src/nest/unsplash/unsplash.module.ts @@ -1,15 +1,22 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { UnsplashService } from './unsplash.service'; import { AppConfigModule } from '../app-config/app-config.module'; import { StorageModule } from '../storage/storage.module'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Users } from '../../db/entities/Users.entity'; /** Unsplash cover search and download. No controller of its own — trips and * places both reach it. Deliberately NOT @Global, matching PermissionsModule: * e2e TestingModules resolve it through each consumer's explicit import. * AppConfigModule is imported explicitly because @Global only applies to - * modules that are in the graph, which a single-domain TestingModule is not. */ + * modules that are in the graph, which a single-domain TestingModule is not. + * MikroOrmModule.forFeature registers AppSettingsRepository/UsersRepository + * for UnsplashService's @InjectRepository constructor params (Plan 3a Task + * 5): getUnsplashKey passes them straight into instance-api-keys.ts's + * resolveApiKey, which no longer reads a raw connection at all. */ @Module({ - imports: [AppConfigModule, StorageModule], + imports: [AppConfigModule, StorageModule, MikroOrmModule.forFeature([AppSettings, Users])], providers: [UnsplashService], exports: [UnsplashService], }) diff --git a/server/src/nest/unsplash/unsplash.service.ts b/server/src/nest/unsplash/unsplash.service.ts index 80da33b57..c48fe25bf 100644 --- a/server/src/nest/unsplash/unsplash.service.ts +++ b/server/src/nest/unsplash/unsplash.service.ts @@ -1,10 +1,14 @@ import { Readable } from 'node:stream'; import { v4 as uuidv4 } from 'uuid'; import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { safeFetch } from '../../utils/ssrfGuard'; import { exceedsDeclaredLength, readCapped } from '../../utils/cappedFetch'; import { resolveApiKey } from '../settings/instance-api-keys'; -import { DatabaseService } from '../database/database.service'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { StorageService } from '../storage/storage.service'; @@ -46,13 +50,15 @@ const COVER_EXT_BY_TYPE: Record = { * Unsplash search and cover download. * * Its own module rather than a method on trips or places: both call it, and the - * key resolution reads the env AND the database, so it needs the injected - * connection either way. + * key resolution reads the env and (via instance-api-keys.ts's resolveApiKey, + * Plan 3a Task 5) AppSettingsRepository/UsersRepository, so it needs those + * injected either way. */ @Injectable() export class UnsplashService { constructor( - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly usersRepo: UsersRepository, private readonly env: RuntimeEnvService, private readonly storage: StorageService, ) {} @@ -67,7 +73,9 @@ export class UnsplashService { * which case the search falls back to the unauthenticated endpoint. */ async getUnsplashKey(userId: number): Promise { - return (await resolveApiKey(this.db, 'unsplash_api_key', userId, this.env.env().integrations.unsplashAccessKey)).key; + return ( + await resolveApiKey(this.appSettings, this.usersRepo, 'unsplash_api_key', userId, this.env.env().integrations.unsplashAccessKey) + ).key; } async searchUnsplashPhotos(query: string, perPage = 9, accessKey?: string | null) { diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index 7cbc65aa8..f8f19889b 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -8,11 +8,17 @@ import { Categories } from '../../src/db/entities/Categories.entity'; import type { CategoriesRepository } from '../../src/db/repositories/Categories.repository'; import { Tags } from '../../src/db/entities/Tags.entity'; import type { TagsRepository } from '../../src/db/repositories/Tags.repository'; +import { Settings } from '../../src/db/entities/Settings.entity'; +import type { SettingsRepository } from '../../src/db/repositories/Settings.repository'; +import { Users } from '../../src/db/entities/Users.entity'; +import type { UsersRepository } from '../../src/db/repositories/Users.repository'; const perHandle = new WeakMap>(); const appSettingsPerHandle = new WeakMap>(); const categoriesPerHandle = new WeakMap>(); const tagsPerHandle = new WeakMap>(); +const settingsPerHandle = new WeakMap>(); +const usersPerHandle = new WeakMap>(); // ONE MikroORM per handle, shared by createTestUnitOfWork and // createTestAppSettingsRepo (task-2-review.md I2): each used to call // createTestOrm(db) independently, which opened a SECOND MikroORM.init over the @@ -108,6 +114,38 @@ export function createTestTagsRepo(db: Database.Database): Promise { + const existing = settingsPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(Settings) as SettingsRepository); + settingsPerHandle.set(db, pending); + return pending; +} + +/** + * The `UsersRepository` `instance-api-keys.ts`'s repository-backed functions + * resolve through the active MikroORM request context rather than a + * constructor argument (Plan 3a Task 5 — see the file's own docstring for + * why: its callers span ~6 domains outside this plan, still passing + * `DatabaseService`, and none of them are touched by this conversion). A + * hand-built test that exercises those functions still needs a real + * `UsersRepository` bound to the same ORM its `withRequestContext`-style + * wrapper forks from — this is that repository, same memoisation as the + * others in this file. + */ +export function createTestUsersRepo(db: Database.Database): Promise { + const existing = usersPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(Users) as UsersRepository); + usersPerHandle.set(db, pending); + return pending; +} + /** * The same `UnitOfWork`, as a **global** Nest module. * diff --git a/server/tests/unit/db/repositories/Settings.repository.test.ts b/server/tests/unit/db/repositories/Settings.repository.test.ts new file mode 100644 index 000000000..211ee1786 --- /dev/null +++ b/server/tests/unit/db/repositories/Settings.repository.test.ts @@ -0,0 +1,150 @@ +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 { createUser, type TestUser } from '../../../helpers/factories'; +import { Settings } from '../../../../src/db/entities/Settings.entity'; +import type { SettingsRepository } from '../../../../src/db/repositories/Settings.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let settings: SettingsRepository; +let user: TestUser; +let otherUser: TestUser; + +beforeAll(async () => { + t = await createTestOrm(testDb); + settings = t.repo(Settings) as SettingsRepository; +}); +beforeEach(() => { + resetTestDb(testDb); + t.clear(); + user = createUser(testDb).user; + otherUser = createUser(testDb).user; +}); +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +function rawRow(userId: number, key: string): unknown { + return testDb.prepare('SELECT * FROM settings WHERE user_id = ? AND key = ?').get(userId, key); +} + +function insertRaw(userId: number, key: string, value: string | null): void { + testDb.prepare('INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?)').run(userId, key, value); +} + +describe('settings table — the (user_id, key) unique index the ON CONFLICT target relies on', () => { + it( + "UNIQUE-SCHEMA-001: PRAGMA index_list('settings') reports a unique index over exactly (user_id, key), the migrated schema's inline UNIQUE(user_id, key)", + () => { + const indexes = testDb.prepare("PRAGMA index_list('settings')").all() as { name: string; unique: number }[]; + const uniqueIndexes = indexes.filter((idx) => idx.unique); + expect(uniqueIndexes.length).toBeGreaterThan(0); + const columnSets = uniqueIndexes.map((idx) => { + const cols = testDb.prepare(`PRAGMA index_info('${idx.name}')`).all() as { seqno: number; name: string }[]; + return [...cols].sort((a, b) => a.seqno - b.seqno).map((c) => c.name); + }); + expect(columnSets).toContainEqual(['user_id', 'key']); + }, + ); + + it('UNIQUE-SCHEMA-002: the Settings entity metadata declares the same (user, key) unique constraint (fix(db): generator captures inline UNIQUE constraints)', () => { + const meta = t.orm.getMetadata(Settings); + expect(meta.uniques).toContainEqual({ properties: ['user_id', 'key'] }); + }); +}); + +describe('SettingsRepository', () => { + it('SETTINGSREPO-001: getForUser reads every row for that user', async () => { + insertRaw(user.id, 'dark_mode', 'true'); + insertRaw(user.id, 'temperature_unit', 'celsius'); + insertRaw(otherUser.id, 'dark_mode', 'false'); // a different user's row — must not leak in + const rows = await settings.getForUser(user.id); + expect(rows.map((r) => r.key).sort()).toEqual(['dark_mode', 'temperature_unit']); + expect(rows.every((r) => r.user_id === user.id)).toBe(true); + }); + + it('SETTINGSREPO-002: getForUser for a user with no rows returns an empty array', async () => { + expect(await settings.getForUser(user.id)).toEqual([]); + }); + + it('SETTINGSREPO-003: getOne reads a single (user, key) row', async () => { + insertRaw(user.id, 'dark_mode', 'true'); + const row = await settings.getOne(user.id, 'dark_mode'); + expect(row).not.toBeNull(); + expect(row?.value).toBe('true'); + expect(row?.user_id).toBe(user.id); + }); + + it('SETTINGSREPO-004: getOne returns null when unset', async () => { + expect(await settings.getOne(user.id, 'does_not_exist')).toBeNull(); + }); + + it("SETTINGSREPO-005: getOne scopes strictly by user — another user's row for the same key never leaks", async () => { + insertRaw(otherUser.id, 'dark_mode', 'true'); + expect(await settings.getOne(user.id, 'dark_mode')).toBeNull(); + }); + + it('SETTINGSREPO-006: upsertForUser inserts a new row exactly as the legacy INSERT would', async () => { + await settings.upsertForUser(user.id, 'dark_mode', 'true'); + expect(rawRow(user.id, 'dark_mode')).toMatchObject({ user_id: user.id, key: 'dark_mode', value: 'true' }); + }); + + it('SETTINGSREPO-007: upsertForUser on an existing (user, key) replaces the value only — no duplicate row, matching ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value', async () => { + insertRaw(user.id, 'dark_mode', 'false'); + await settings.upsertForUser(user.id, 'dark_mode', 'true'); + expect(rawRow(user.id, 'dark_mode')).toMatchObject({ user_id: user.id, key: 'dark_mode', value: 'true' }); + expect( + testDb.prepare('SELECT COUNT(*) as c FROM settings WHERE user_id = ? AND key = ?').get(user.id, 'dark_mode'), + ).toEqual({ c: 1 }); + }); + + it("SETTINGSREPO-008: upsertForUser scopes by user — the same key for two different users stores two separate rows, not a collision (the conflict target is (user, key), not key alone)", async () => { + await settings.upsertForUser(user.id, 'dark_mode', 'true'); + await settings.upsertForUser(otherUser.id, 'dark_mode', 'false'); + expect(rawRow(user.id, 'dark_mode')).toMatchObject({ value: 'true' }); + expect(rawRow(otherUser.id, 'dark_mode')).toMatchObject({ value: 'false' }); + }); + + it('SETTINGSREPO-009: deleteForUser with a key deletes only that (user, key) row and reports the legacy DELETE-affected-row count', async () => { + insertRaw(user.id, 'dark_mode', 'true'); + insertRaw(user.id, 'temperature_unit', 'celsius'); + expect(await settings.deleteForUser(user.id, 'dark_mode')).toBe(1); + expect(rawRow(user.id, 'dark_mode')).toBeUndefined(); + expect(rawRow(user.id, 'temperature_unit')).toBeDefined(); + }); + + it('SETTINGSREPO-010: deleteForUser without a key deletes every row for that user only', async () => { + insertRaw(user.id, 'dark_mode', 'true'); + insertRaw(user.id, 'temperature_unit', 'celsius'); + insertRaw(otherUser.id, 'dark_mode', 'false'); + expect(await settings.deleteForUser(user.id)).toBe(2); + expect(await settings.getForUser(user.id)).toEqual([]); + expect(rawRow(otherUser.id, 'dark_mode')).toBeDefined(); + }); + + it('SETTINGSREPO-011: deleteForUser on a missing key is a no-op that reports 0 rows', async () => { + expect(await settings.deleteForUser(user.id, 'does_not_exist')).toBe(0); + }); + + // Identity-map regression: the short-circuit (Task 0 review, I1; corrected + // by the Task 3 review) only fires for a PRIMARY-KEY-ONLY findOne + // (`findOne({ id })`/`findOne(pk)`) — `getOne`'s filter is `(user, key)`, + // never the surrogate `id` PK, so it is not PK-only and always re-queries, + // needing no `refresh: true`. These pin that directly rather than assume it. + it('SETTINGSREPO-012: getOne sees a raw UPDATE on the same (user, key) in the same request', async () => { + insertRaw(user.id, 'dark_mode', 'old'); + expect((await settings.getOne(user.id, 'dark_mode'))?.value).toBe('old'); // populate the identity map + testDb.prepare('UPDATE settings SET value = ? WHERE user_id = ? AND key = ?').run('new', user.id, 'dark_mode'); + expect((await settings.getOne(user.id, 'dark_mode'))?.value).toBe('new'); + }); + + it('SETTINGSREPO-013: getOne sees a raw DELETE on the same (user, key) in the same request', async () => { + insertRaw(user.id, 'dark_mode', 'old'); + expect((await settings.getOne(user.id, 'dark_mode'))?.value).toBe('old'); // populate the identity map + testDb.prepare('DELETE FROM settings WHERE user_id = ? AND key = ?').run(user.id, 'dark_mode'); + expect(await settings.getOne(user.id, 'dark_mode')).toBeNull(); + }); +}); diff --git a/server/tests/unit/mcp/tools-trips.test.ts b/server/tests/unit/mcp/tools-trips.test.ts index af761f32c..af47e8da5 100644 --- a/server/tests/unit/mcp/tools-trips.test.ts +++ b/server/tests/unit/mcp/tools-trips.test.ts @@ -470,6 +470,23 @@ describe('Tool: search_cover_images', () => { }))); } + // UnsplashService.getUnsplashKey resolves through instance-api-keys.ts's + // resolveApiKey (Plan 3a Task 5), which now reads AppSettingsRepository/ + // UsersRepository off the active MikroORM request context when the caller + // has no repository of its own to pass — this in-memory MCP harness + // (mcp-test-controllers.ts) builds every controller by hand with no HTTP + // request around it, so there is no such context here (unlike buildApp()'s + // real e2e/production path, or a DI-injected repository test like + // AppSettingsRepository's own, both of which are covered elsewhere). + // These two tests are about the SEARCH RESULT handling (parsing Unsplash's + // response into photos / an error), not the key-resolution precedence + // (env > instance > user row), which unsplash.service.test.ts's own + // getUnsplashKey suite already covers directly — so an operator env key + // here short-circuits resolveApiKey before it ever needs that context, + // without losing any coverage this describe block is actually for. + beforeEach(() => { process.env.UNSPLASH_ACCESS_KEY = 'operator-test-key'; }); + afterEach(() => { delete process.env.UNSPLASH_ACCESS_KEY; }); + it('returns the photo candidates for a query', async () => { const { user } = createUser(testDb); stubUnsplash({ diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index feb91c4df..0b817188d 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -82,7 +82,7 @@ import { __clearVersionCacheForTests } from '../../../src/nest/admin/admin.helpe import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -101,7 +101,7 @@ let svc: AdminService; beforeAll(async () => { permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection)), await createTestUnitOfWork(dbs.connection)); - auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection)); + auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection)); svc = new AdminService( dbs, await createTestAddonsService(testDb, dbs), diff --git a/server/tests/unit/nest/admin.version-notification.test.ts b/server/tests/unit/nest/admin.version-notification.test.ts index b4b97be07..cd16e151e 100644 --- a/server/tests/unit/nest/admin.version-notification.test.ts +++ b/server/tests/unit/nest/admin.version-notification.test.ts @@ -60,7 +60,7 @@ import { AdminService } from '../../../src/nest/admin/admin.service'; import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -79,7 +79,7 @@ let svc: AdminService; beforeAll(async () => { permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection)), await createTestUnitOfWork(dbs.connection)); - auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection)); + auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection)); svc = new AdminService( dbs, await createTestAddonsService(testDb, dbs), diff --git a/server/tests/unit/nest/auth.service.test.ts b/server/tests/unit/nest/auth.service.test.ts index 7932e284d..5cec768f2 100644 --- a/server/tests/unit/nest/auth.service.test.ts +++ b/server/tests/unit/nest/auth.service.test.ts @@ -84,7 +84,7 @@ import { MailerService } from '../../../src/nest/notifications/mailer/mailer.ser import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; import { DEFAULT_ALLOWED_EXTENSIONS } from '../../../src/nest/files/files.constants'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; // MailerService is injected since the notifications fold — a stub instead of a // module mock. sendPasswordResetEmail is the only thing auth reaches for. @@ -109,6 +109,7 @@ beforeAll(async () => { mailerStub, new EphemeralTokenService(), new AllowedFileTypesService(new DatabaseService(testDb)), await createTestUnitOfWork(testDb), + await createTestAppSettingsRepo(testDb), await createTestUsersRepo(testDb), ); }); diff --git a/server/tests/unit/nest/google-transit.provider.test.ts b/server/tests/unit/nest/google-transit.provider.test.ts index e75649d97..42912cd4d 100644 --- a/server/tests/unit/nest/google-transit.provider.test.ts +++ b/server/tests/unit/nest/google-transit.provider.test.ts @@ -17,6 +17,8 @@ import { GoogleTransitProvider, clearGoogleTransitCache } from '../../../src/nes import { decodePolyline, encodePolyline } from '../../../src/nest/transit/transit.helpers'; import { TransitService } from '../../../src/nest/transit/transit.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; vi.mock('../../../src/app-config', async (importOriginal) => { const actual = await importOriginal(); @@ -41,7 +43,26 @@ function stubDb(settings: Record): DatabaseService { } as unknown as DatabaseService; } -const googleDb = () => stubDb({ transit_provider: 'google', maps_api_key: 'test-key' }); +const GOOGLE_SETTINGS = { transit_provider: 'google', maps_api_key: 'test-key' }; + +/** + * resolveKey (google-transit.provider.ts) now reads AppSettingsRepository/ + * UsersRepository directly (instance-api-keys.ts's resolveApiKey, Plan 3a + * Task 5) instead of a raw `this.database.get(...)` call through the stub + * above. `stubDb`'s settings object already carries exactly the instance-tier + * value every test here configures, so `appSettingsStub.getValue` wraps it + * directly; the per-user fallback tier was never exercised by this file even + * before this conversion (the old stub's `.get` matched on the bound + * parameter, and none of these cases seed a per-user row), so it always + * answers null. + */ +function makeProvider(settings: Record): GoogleTransitProvider { + const appSettingsStub = { + getValue: async (key: string) => settings[key] ?? null, + } as unknown as AppSettingsRepository; + const usersStub = { getApiKeyColumn: async () => null } as unknown as UsersRepository; + return new GoogleTransitProvider(stubDb(settings), appSettingsStub, usersStub); +} beforeEach(() => { vi.stubGlobal('fetch', fetchMock); @@ -163,33 +184,33 @@ function subwayRoute(): FixtureRoutes { describe('activation', () => { it('GTRANSIT-001: stays off while the instance is on Transitous, even with a key', async () => { - const provider = new GoogleTransitProvider(stubDb({ maps_api_key: 'test-key' })); + const provider = makeProvider({ maps_api_key: 'test-key' }); expect(await provider.isActive(1)).toBe(false); }); it('GTRANSIT-002: stays off when Google is selected but no key resolves', async () => { - const provider = new GoogleTransitProvider(stubDb({ transit_provider: 'google' })); + const provider = makeProvider({ transit_provider: 'google' }); expect(await provider.isActive(1)).toBe(false); }); it('GTRANSIT-003: is on only with both the setting and a key', async () => { - expect(await new GoogleTransitProvider(googleDb()).isActive(1)).toBe(true); + expect(await makeProvider(GOOGLE_SETTINGS).isActive(1)).toBe(true); }); it('GTRANSIT-004: an unknown provider value falls back to Transitous', async () => { - const provider = new GoogleTransitProvider(stubDb({ transit_provider: 'someday-maps', maps_api_key: 'test-key' })); + const provider = makeProvider({ transit_provider: 'someday-maps', maps_api_key: 'test-key' }); expect(await provider.isActive(1)).toBe(false); }); it('GTRANSIT-005: TransitService routes to Google only when it is active', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - const service = new TransitService(new GoogleTransitProvider(googleDb())); + const service = new TransitService(makeProvider(GOOGLE_SETTINGS)); await service.plan({ from: FROM, to: TO }); expect(fetchMock.mock.calls[0][0]).toBe('https://routes.googleapis.com/directions/v2:computeRoutes'); }); it('GTRANSIT-006: TransitService still validates the request before dispatching', async () => { - const service = new TransitService(new GoogleTransitProvider(googleDb())); + const service = new TransitService(makeProvider(GOOGLE_SETTINGS)); await expect(service.plan({ from: 'nowhere', to: TO })).rejects.toThrow('from must be "lat,lng"'); await expect(service.plan({ from: FROM, to: TO, modes: 'ROCKET' })).rejects.toThrow('unsupported transit mode'); expect(fetchMock).not.toHaveBeenCalled(); @@ -199,7 +220,7 @@ describe('activation', () => { describe('plan mapping', () => { it('GTRANSIT-007: maps a walk/subway/walk route and derives the journey clock', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); expect(itineraries).toHaveLength(1); const it0 = itineraries[0]; @@ -236,7 +257,7 @@ describe('plan mapping', () => { fetchMock.mockResolvedValue( okJson({ routes: [{ legs: [{ steps: [{ travelMode: 'WALK', staticDuration: '900s' }] }] }] }), ); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); expect(itineraries).toEqual([]); }); @@ -247,7 +268,7 @@ describe('plan mapping', () => { if (details) details.transitLine.vehicle.type = type; return route; }; - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); for (const [google, trek] of [ ['HEAVY_RAIL', 'RAIL'], ['HIGH_SPEED_TRAIN', 'HIGHSPEED_RAIL'], @@ -268,7 +289,7 @@ describe('plan mapping', () => { describe('request shape', () => { it('GTRANSIT-010: asks for transit, alternatives and the caller language', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO, time: '2026-09-10T09:00:00Z' }, 'ja', 1); + await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO, time: '2026-09-10T09:00:00Z' }, 'ja', 1); const body = lastBody(); expect(body.travelMode).toBe('TRANSIT'); @@ -282,7 +303,7 @@ describe('request shape', () => { it('GTRANSIT-011: arriveBy anchors the time at the destination', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - await new GoogleTransitProvider(googleDb()).plan( + await makeProvider(GOOGLE_SETTINGS).plan( { from: FROM, to: TO, time: '2026-09-10T09:00:00Z', arriveBy: true }, 'en', 1, ); expect(lastBody().arrivalTime).toBe('2026-09-10T09:00:00.000Z'); @@ -291,7 +312,7 @@ describe('request shape', () => { it('GTRANSIT-012: the field mask stays inside the tier the itinerary needs', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); const mask = lastHeaders()['X-Goog-FieldMask']; expect(mask).toContain('routes.legs.steps.transitDetails.stopDetails'); @@ -303,7 +324,7 @@ describe('request shape', () => { it('GTRANSIT-013: sends the expressible modes and holds the rest on the response', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); const subwayOnly = await provider.plan({ from: FROM, to: TO, modes: 'SUBWAY' }, 'en', 1); expect(lastBody().transitPreferences).toEqual({ allowedTravelModes: ['SUBWAY'] }); @@ -323,7 +344,7 @@ describe('request shape', () => { twoTransfers.routes[0].legs[0].steps.splice(2, 0, JSON.parse(JSON.stringify(rail))); fetchMock.mockResolvedValue(okJson(twoTransfers)); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); const capped = await provider.plan({ from: FROM, to: TO, maxTransfers: 0 }, 'en', 1); expect(capped.itineraries).toEqual([]); // A low cap also nudges Google itself, so the one billed call is likelier @@ -337,7 +358,7 @@ describe('request shape', () => { it('GTRANSIT-015: an identical plan is answered from cache, unbilled', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); await provider.plan({ from: FROM, to: TO }, 'en', 1); await provider.plan({ from: FROM, to: TO }, 'en', 1); expect(fetchMock).toHaveBeenCalledTimes(1); @@ -350,7 +371,7 @@ describe('request shape', () => { headers: { get: () => null }, json: async () => ({ error: { message: 'Routes API has not been used in project 1' } }), }); - await expect(new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1)).rejects.toThrow( + await expect(makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1)).rejects.toThrow( 'Routes API has not been used in project 1', ); }); @@ -358,7 +379,7 @@ describe('request shape', () => { it('GTRANSIT-017: upstream rate limiting stays a 429', async () => { fetchMock.mockResolvedValue({ ok: false, status: 429, headers: { get: () => null }, json: async () => ({}) }); await expect( - new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1), + makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1), ).rejects.toMatchObject({ status: 429 }); }); }); @@ -383,7 +404,7 @@ describe('rail mode filter', () => { const TRAIN_CHIP = 'HIGHSPEED_RAIL,LONG_DISTANCE,NIGHT_RAIL,REGIONAL_RAIL,SUBURBAN'; it('GTRANSIT-039: the train filter keeps the trains Google labels HEAVY_RAIL', async () => { - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); for (const vehicle of ['HEAVY_RAIL', 'RAIL', 'COMMUTER_TRAIN', 'HIGH_SPEED_TRAIN']) { clearGoogleTransitCache(); fetchMock.mockResolvedValue(okJson(withVehicle(vehicle))); @@ -395,7 +416,7 @@ describe('rail mode filter', () => { }); it('GTRANSIT-040: the RAIL umbrella keeps the fine-grained rail legs, and neither side leaks a subway', async () => { - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); // An MCP caller may name the umbrella; Google still answers by vehicle. for (const vehicle of ['COMMUTER_TRAIN', 'HIGH_SPEED_TRAIN', 'HEAVY_RAIL']) { clearGoogleTransitCache(); @@ -415,7 +436,7 @@ describe('rail mode filter', () => { it('GTRANSIT-041: a bus-only filter still refuses a train', async () => { fetchMock.mockResolvedValue(okJson(withVehicle('HEAVY_RAIL'))); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); const { itineraries } = await provider.plan({ from: FROM, to: TO, modes: 'BUS,COACH' }, 'en', 1); expect(itineraries).toEqual([]); }); @@ -442,7 +463,7 @@ describe('geocode', () => { it('GTRANSIT-018: maps places, marking stations STOP and everything else PLACE', async () => { fetchMock.mockResolvedValue(okJson(places)); - const { results } = await new GoogleTransitProvider(googleDb()).geocode('Nakanoshima', 'en', undefined, 1); + const { results } = await makeProvider(GOOGLE_SETTINGS).geocode('Nakanoshima', 'en', undefined, 1); expect(results).toEqual([ { name: 'Nakanoshima Station', lat: 34.6939, lng: 135.4915, type: 'STOP', area: '2 Chome Nakanoshima, Kita Ward, Osaka' }, @@ -452,7 +473,7 @@ describe('geocode', () => { it('GTRANSIT-019: biases on `near` and asks only for the cheap Text Search fields', async () => { fetchMock.mockResolvedValue(okJson(places)); - await new GoogleTransitProvider(googleDb()).geocode('Temmabashi', 'ja', '34.6875,135.5155', 1); + await makeProvider(GOOGLE_SETTINGS).geocode('Temmabashi', 'ja', '34.6875,135.5155', 1); const body = lastBody(); expect(body.languageCode).toBe('ja'); @@ -467,14 +488,14 @@ describe('geocode', () => { it('GTRANSIT-020: repeat lookups of the same station are answered from cache', async () => { fetchMock.mockResolvedValue(okJson(places)); - const service = new TransitService(new GoogleTransitProvider(googleDb())); + const service = new TransitService(makeProvider(GOOGLE_SETTINGS)); await service.geocode('Kyobashi', 'en', undefined, 1); await service.geocode('Kyobashi', 'en', undefined, 1); expect(fetchMock).toHaveBeenCalledTimes(1); }); it('GTRANSIT-021: a short query never reaches Google', async () => { - const service = new TransitService(new GoogleTransitProvider(googleDb())); + const service = new TransitService(makeProvider(GOOGLE_SETTINGS)); // The provider is still named: the answer comes from the length guard, not // from a backend that declined. expect(await service.geocode('a', 'en', undefined, 1)).toEqual({ results: [], provider: 'google' }); @@ -484,7 +505,7 @@ describe('geocode', () => { describe('guards', () => { it('GTRANSIT-022: refuses rather than calling Google when no key resolves', async () => { - const provider = new GoogleTransitProvider(stubDb({ transit_provider: 'google' })); + const provider = makeProvider({ transit_provider: 'google' }); await expect(provider.plan({ from: FROM, to: TO }, 'en', 1)).rejects.toThrow('no Google API key configured'); await expect(provider.geocode('Namba', 'en', undefined, 1)).rejects.toThrow('no Google API key configured'); expect(fetchMock).not.toHaveBeenCalled(); @@ -493,14 +514,14 @@ describe('guards', () => { it('GTRANSIT-023: an oversized response is refused before it is parsed', async () => { const json = vi.fn(); fetchMock.mockResolvedValue({ ok: true, status: 200, headers: { get: () => '9000000' }, json }); - await expect(new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1)).rejects.toThrow( + await expect(makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1)).rejects.toThrow( 'response too large', ); expect(json).not.toHaveBeenCalled(); }); it('GTRANSIT-024: a route with no steps, and one whose transit leg has no clock, are both dropped', async () => { - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); fetchMock.mockResolvedValue(okJson({ routes: [{ legs: [{ steps: [] }] }] })); expect((await provider.plan({ from: FROM, to: TO }, 'en', 1)).itineraries).toEqual([]); @@ -522,7 +543,7 @@ describe('guards', () => { } fetchMock.mockResolvedValue(okJson(unnamed)); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); // Every name is non-empty — transitStopSchema rejects a blank one, which // would drop the itinerary at the MCP boundary. for (const leg of itineraries[0].legs) { @@ -538,7 +559,7 @@ describe('guards', () => { noDuration.routes[0].legs[0].steps[2].staticDuration = 'not-a-duration'; fetchMock.mockResolvedValue(okJson(noDuration)); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); expect(itineraries[0].legs[0].duration).toBe(0); expect(itineraries[0].legs[2].duration).toBe(0); expect(itineraries[0].walkSeconds).toBe(0); @@ -547,7 +568,7 @@ describe('guards', () => { it('GTRANSIT-027: a cache entry past its TTL is re-fetched, not served stale', async () => { const now = vi.spyOn(Date, 'now').mockReturnValue(1_000_000); fetchMock.mockResolvedValue(okJson(subwayRoute())); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); await provider.plan({ from: FROM, to: TO }, 'en', 1); now.mockReturnValue(1_000_000 + 6 * 60 * 1000); @@ -558,7 +579,7 @@ describe('guards', () => { it('GTRANSIT-028: the cache evicts rather than growing without bound', async () => { fetchMock.mockResolvedValue(okJson({ places: [] })); - const provider = new GoogleTransitProvider(googleDb()); + const provider = makeProvider(GOOGLE_SETTINGS); for (let i = 0; i < 220; i++) await provider.geocode(`station-${i}`, 'en', undefined, 1); expect(fetchMock).toHaveBeenCalledTimes(220); @@ -606,7 +627,7 @@ describe('walk coalescing', () => { it('GTRANSIT-029: a walk split across navigation steps becomes one leg', async () => { fetchMock.mockResolvedValue(okJson(routeWithSlicedWalks())); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); const legs = itineraries[0].legs; expect(legs.map((l) => l.mode)).toEqual(['WALK', 'SUBWAY', 'WALK']); @@ -620,7 +641,7 @@ describe('walk coalescing', () => { it('GTRANSIT-030: the merged walk keeps the endpoints of the whole run', async () => { fetchMock.mockResolvedValue(okJson(routeWithSlicedWalks())); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); const [opening, , closing] = itineraries[0].legs; expect(opening.from.name).toBe('START'); @@ -634,7 +655,7 @@ describe('walk coalescing', () => { it('GTRANSIT-031: the merged walk draws the whole path, with no doubled seam', async () => { fetchMock.mockResolvedValue(okJson(routeWithSlicedWalks())); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); const points = decodePolyline(itineraries[0].legs[0].geometry ?? '', 5); // Three two-point steps sharing their seams — four points, not six. @@ -648,7 +669,7 @@ describe('walk coalescing', () => { delete partial.routes[0].legs[0].steps[1].polyline; fetchMock.mockResolvedValue(okJson(partial)); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); expect(decodePolyline(itineraries[0].legs[0].geometry ?? '', 5).length).toBeGreaterThan(0); expect(itineraries[0].legs[0].duration).toBe(180); }); @@ -661,7 +682,7 @@ describe('walk coalescing', () => { many.routes[0].legs[0].steps = [...crumbs, rail, ...crumbs]; fetchMock.mockResolvedValue(okJson(many)); - const { itineraries } = await new GoogleTransitProvider(googleDb()).plan({ from: FROM, to: TO }, 'en', 1); + const { itineraries } = await makeProvider(GOOGLE_SETTINGS).plan({ from: FROM, to: TO }, 'en', 1); expect(itineraries[0].legs).toHaveLength(3); }); }); @@ -674,7 +695,7 @@ describe('walk coalescing', () => { describe('provider reporting', () => { it('GTRANSIT-036: names Google when Google answered', async () => { fetchMock.mockResolvedValue(okJson(subwayRoute())); - const service = new TransitService(new GoogleTransitProvider(googleDb())); + const service = new TransitService(makeProvider(GOOGLE_SETTINGS)); const planned = await service.plan({ from: FROM, to: TO }, 'en', 1); expect(planned.provider).toBe('google'); @@ -686,7 +707,7 @@ describe('provider reporting', () => { fetchMock.mockResolvedValue(okJson({ itineraries: [] })); // Google selected, but no key resolves — the request goes to Transitous and // says so, instead of leaving an empty result to be blamed on Google. - const service = new TransitService(new GoogleTransitProvider(stubDb({ transit_provider: 'google' }))); + const service = new TransitService(makeProvider({ transit_provider: 'google' })); const planned = await service.plan({ from: '48.8583,2.3470', to: '48.8809,2.3553' }, 'en', 1); expect(planned.provider).toBe('transitous'); expect(planned.itineraries).toEqual([]); @@ -695,7 +716,7 @@ describe('provider reporting', () => { it('GTRANSIT-038: a cached answer still names its backend', async () => { fetchMock.mockResolvedValue(okJson({ itineraries: [] })); - const service = new TransitService(new GoogleTransitProvider(stubDb({}))); + const service = new TransitService(makeProvider({})); const first = await service.plan({ from: '48.1,2.1', to: '48.2,2.2' }, 'en', 1); const second = await service.plan({ from: '48.1,2.1', to: '48.2,2.2' }, 'en', 1); expect(fetchMock).toHaveBeenCalledTimes(1); diff --git a/server/tests/unit/nest/journey-share.service.test.ts b/server/tests/unit/nest/journey-share.service.test.ts index 36b95043f..fb4539391 100644 --- a/server/tests/unit/nest/journey-share.service.test.ts +++ b/server/tests/unit/nest/journey-share.service.test.ts @@ -41,24 +41,27 @@ import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.s import { JourneyShareService } from '../../../src/nest/journey/journey-share.service'; import { SettingsService } from '../../../src/nest/settings/settings.service'; import { db as dbConn } from '../../../src/db/database'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; -import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; -import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; -import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo } from '../../helpers/test-uow'; +import type { TestOrm } from '../../helpers/test-orm'; const dbs = new DatabaseService(dbConn); let svc: JourneyShareService; let t: TestOrm; +// `t`, `uow` and the settings repositories all derive from the SAME +// `sharedTestOrm(testDb)` (task-2-review.md I2) — see settings.service.test.ts's +// own comment on this pattern for why a second, independent `createTestOrm` +// call here would be unsafe once `SettingsService`'s writes go through +// `uow.transactional(...)`. beforeAll(async () => { createTables(testDb); runMigrations(testDb); const uow = await createTestUnitOfWork(testDb); - t = await createTestOrm(testDb); + t = await sharedTestOrm(testDb); svc = new JourneyShareService( dbs, new JourneyDomainService(dbs, new RealtimeService(), new TrekPhotosRepository(dbs), uow), - new SettingsService(dbs, uow, t.repo(AppSettings) as AppSettingsRepository), + new SettingsService(uow, await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb)), ); }); diff --git a/server/tests/unit/nest/maps.amap.test.ts b/server/tests/unit/nest/maps.amap.test.ts index 862886834..1bf0d3258 100644 --- a/server/tests/unit/nest/maps.amap.test.ts +++ b/server/tests/unit/nest/maps.amap.test.ts @@ -72,6 +72,24 @@ import { } from '../../../src/nest/maps/providers/amap.provider'; import { isGooglePlaceId } from '../../../src/nest/maps/maps.helpers'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// resolveMapsKey/resolveAmapKey (maps.service.ts) now read AppSettingsRepository/ +// UsersRepository directly (Plan 3a Task 5's instance-api-keys.ts conversion) +// instead of raw SQL through the mocked db module above — these two stubs wire +// the SAME mockInstanceGet/mockDbGet seams the rest of this file already +// controls into the new repository methods, so every existing keys()/ +// mockInstanceGet/mockProviderGet call below keeps its meaning unchanged. +const appSettingsStub = { + getValue: async (key: string) => (mockInstanceGet(key) as { value: string | null } | undefined)?.value ?? null, +} as unknown as AppSettingsRepository; +const usersStub = { + getApiKeyColumn: async (userId: number, name: 'maps_api_key' | 'amap_api_key') => { + const row = mockDbGet(userId) as { maps_api_key: string | null; amap_api_key: string | null } | undefined; + return row?.[name] ?? null; + }, +} as unknown as UsersRepository; const photoCacheStub = { get: vi.fn(() => null), @@ -83,7 +101,7 @@ const photoCacheStub = { serveKey: vi.fn(() => null), } as unknown as PlacePhotoCacheService; -const svc = new MapsService(new DatabaseService(db as never), photoCacheStub); +const svc = new MapsService(new DatabaseService(db as never), photoCacheStub, appSettingsStub, usersStub); /** A provider over a fixed key, which is all these cases need. */ function provider(): AmapPlacesProvider { diff --git a/server/tests/unit/nest/maps.area.test.ts b/server/tests/unit/nest/maps.area.test.ts index 80931ffbe..8551a3d7c 100644 --- a/server/tests/unit/nest/maps.area.test.ts +++ b/server/tests/unit/nest/maps.area.test.ts @@ -20,6 +20,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the fake database.get(() => undefined) already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; const BOX = { minLat: 54, minLng: 12, maxLat: 54.2, maxLng: 12.3 }; @@ -52,7 +60,7 @@ function make(enabled = true) { if (enabled) delete process.env.TREK_PLACES_ENABLED; else process.env.TREK_PLACES_ENABLED = 'false'; const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - return new MapsService(database, {} as PlacePhotoCacheService); + return new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); } beforeEach(() => { diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts index a51cc808d..0958cd2b0 100644 --- a/server/tests/unit/nest/maps.autocomplete.test.ts +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -26,6 +26,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the old `database.get(() => undefined)` fake already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; // The index switch is an environment variable now, not an admin row: it decides // whether a search leaves the instance at all, so it is pinned by the operator @@ -83,7 +91,7 @@ const osmHit = (over: Record = {}) => ({ function make(enabled = true) { trekPlaces.on = enabled; const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - return new MapsService(database, {} as PlacePhotoCacheService); + return new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); } beforeEach(() => { diff --git a/server/tests/unit/nest/maps.curated-images.test.ts b/server/tests/unit/nest/maps.curated-images.test.ts index f73e11c5d..a925734c1 100644 --- a/server/tests/unit/nest/maps.curated-images.test.ts +++ b/server/tests/unit/nest/maps.curated-images.test.ts @@ -27,7 +27,7 @@ import { db } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { MapsService } from '../../../src/nest/maps/maps.service'; -const svcOf = () => new MapsService(new DatabaseService(db as never), {} as never); +const svcOf = () => new MapsService(new DatabaseService(db as never), {} as never, {} as never, {} as never); const filePage = (over: Record = {}) => ({ pageid: 4711, diff --git a/server/tests/unit/nest/maps.geocode.test.ts b/server/tests/unit/nest/maps.geocode.test.ts index ae07737eb..1023373fa 100644 --- a/server/tests/unit/nest/maps.geocode.test.ts +++ b/server/tests/unit/nest/maps.geocode.test.ts @@ -33,6 +33,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the fake database.get(() => undefined) already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; const QUERY = 'Hotel Adlon Kempinski, Unter den Linden 77, Berlin'; @@ -65,7 +73,7 @@ function make(enabled = true) { if (enabled) delete process.env.TREK_PLACES_ENABLED; else process.env.TREK_PLACES_ENABLED = 'false'; const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - return new MapsService(database, {} as PlacePhotoCacheService); + return new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); } beforeEach(() => { diff --git a/server/tests/unit/nest/maps.gers-details.test.ts b/server/tests/unit/nest/maps.gers-details.test.ts index 628b9bd5b..b8cfbb354 100644 --- a/server/tests/unit/nest/maps.gers-details.test.ts +++ b/server/tests/unit/nest/maps.gers-details.test.ts @@ -20,6 +20,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the fake database.get(() => undefined) already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; const PLACE = { gers: 'abc-123', @@ -38,7 +46,7 @@ const PLACE = { function make(osmTags: Record | null) { const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - const svc = new MapsService(database, {} as PlacePhotoCacheService); + const svc = new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); vi.spyOn(svc, 'resolveOsmIdentity').mockResolvedValue( osmTags ? { tags: osmTags, osmUrl: 'https://www.openstreetmap.org/node/1', matchedName: "L'Osteria" } : null, ); @@ -104,7 +112,7 @@ describe('MapsService.getPlaceDetails for a gers: id', () => { // through would turn a working answer into an error for the one user whose // details request happened to land while Overpass was unreachable. const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - const svc = new MapsService(database, {} as PlacePhotoCacheService); + const svc = new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); vi.spyOn(svc, 'resolveOsmIdentity').mockRejectedValue(new Error('overpass down')); mockById.mockResolvedValue({ ...PLACE, hours: { osm: 'Mo-Su 12:00-22:00' } }); diff --git a/server/tests/unit/nest/maps.identity.test.ts b/server/tests/unit/nest/maps.identity.test.ts index ec5da4eab..8cc0fa393 100644 --- a/server/tests/unit/nest/maps.identity.test.ts +++ b/server/tests/unit/nest/maps.identity.test.ts @@ -24,7 +24,7 @@ import { DatabaseService } from '../../../src/nest/database/database.service'; import { MapsService } from '../../../src/nest/maps/maps.service'; import { toWikiLang, haversineMetres, namesOverlap } from '../../../src/nest/maps/maps.helpers'; -const svcOf = () => new MapsService(new DatabaseService(db as never), {} as never); +const svcOf = () => new MapsService(new DatabaseService(db as never), {} as never, {} as never, {} as never); // The Brandenburg Gate and the underground station named after it, 250m apart. const GATE = { lat: 52.5163, lng: 13.3777 }; diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index 80ffecb24..2097aa3e4 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -28,6 +28,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the fake database.get(() => undefined) already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; // 0.1 degrees a side, centred on the equator: cos(lat) is 1 there, so the radius // the service derives is exactly half the box's diagonal and the numbers below @@ -109,7 +117,7 @@ function make(enabled = true) { if (enabled) delete process.env.TREK_PLACES_ENABLED; else process.env.TREK_PLACES_ENABLED = 'false'; const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - return new MapsService(database, {} as PlacePhotoCacheService); + return new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); } // Overpass is the one network call this file must never make; stubbing it is diff --git a/server/tests/unit/nest/maps.search-merge.test.ts b/server/tests/unit/nest/maps.search-merge.test.ts index cba176ab8..ea42e786d 100644 --- a/server/tests/unit/nest/maps.search-merge.test.ts +++ b/server/tests/unit/nest/maps.search-merge.test.ts @@ -33,6 +33,14 @@ vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KE import { MapsService } from '../../../src/nest/maps/maps.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// keyedProvider/resolveMapsKey (maps.service.ts) go through instance-api-keys.ts +// on every call now — none of these cases configure a key, so the stubs just +// answer "unset" the way the fake database.get(() => undefined) already did. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; // The index switch is an environment variable now, not an admin row: it decides // whether a search leaves the instance at all, so it is pinned by the operator @@ -80,7 +88,7 @@ const osmAnswer = (rows: unknown[]) => ({ ok: true, json: async () => rows }); function make(enabled = true) { trekPlaces.on = enabled; const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; - return new MapsService(database, {} as PlacePhotoCacheService); + return new MapsService(database, {} as PlacePhotoCacheService, noAppSettings, noUsers); } beforeEach(() => { diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 20b0a9378..7049eb176 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -173,13 +173,31 @@ import { db } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { MapsService, withPhotoFetchSlot, readWikiIdentity } from '../../../src/nest/maps/maps.service'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; // Type-only, so the module stays mocked: this import is erased at runtime. import type { SsrfResult } from '../../../src/utils/ssrfGuard'; +// resolveMapsKey/resolveAmapKey (maps.service.ts) now read AppSettingsRepository/ +// UsersRepository directly (Plan 3a Task 5) instead of raw SQL through the +// mocked db module above — these two stubs wire the SAME mockInstanceGet/ +// mockDbGet seams the rest of this file already controls into the new +// repository methods, so every existing mockInstanceGet/mockDbGet call below +// keeps its meaning unchanged. +const appSettingsStub = { + getValue: async (key: string) => (mockInstanceGet(key) as { value: string | null } | undefined)?.value ?? null, +} as unknown as AppSettingsRepository; +const usersStub = { + getApiKeyColumn: async (userId: number, name: 'maps_api_key' | 'amap_api_key') => { + const row = mockDbGet(userId) as { maps_api_key?: string | null; amap_api_key?: string | null } | undefined; + return row?.[name] ?? null; + }, +} as unknown as UsersRepository; + // The service under test, constructed over the mocked db stub — DatabaseService // routes get/run through the stubbed prepare(), so mockDbGet/mockDbRun keep // flowing exactly as they did for the legacy module. -const svc = new MapsService(new DatabaseService(db as never), photoCacheStub); +const svc = new MapsService(new DatabaseService(db as never), photoCacheStub, appSettingsStub, usersStub); /** * Switch the TREK Places index off for one case. @@ -506,12 +524,14 @@ describe('resolveMapsKey', () => { it("MAPS-017c: never reads another user's row — the admin fallback is gone (#1939)", async () => { await svc.resolveMapsKey(1); - // Two statements, both scoped: the instance row and this caller's own row. - // The old chain ended in "WHERE role = 'admin' ... LIMIT 1", which handed a - // stranger's credential to every non-admin. - expect(preparedSql).toHaveLength(2); - expect(preparedSql.join(' ')).not.toContain("role = 'admin'"); - expect(preparedSql.some((sql) => sql.includes('WHERE id = ?'))).toBe(true); + // Two reads, both scoped: the instance row and this caller's own row. The + // old chain ended in "WHERE role = 'admin' ... LIMIT 1", which handed a + // stranger's credential to every non-admin — UsersRepository.getApiKeyColumn + // (instance-api-keys.ts's resolveApiKey, Plan 3a Task 5) is scoped to + // exactly the userId given, never a role-based lookup. + expect(mockInstanceGet).toHaveBeenCalledTimes(1); + expect(mockDbGet).toHaveBeenCalledTimes(1); + expect(mockDbGet).toHaveBeenCalledWith(1); }); }); @@ -2659,7 +2679,7 @@ function makeSettingsDb(row?: { value: string }) { } function settingsSvc(row?: { value: string }) { - return new MapsService(makeSettingsDb(row).db, photoCacheStub); + return new MapsService(makeSettingsDb(row).db, photoCacheStub, appSettingsStub, usersStub); } describe('kill-switch settings reads', () => { @@ -2683,7 +2703,7 @@ describe('kill-switch settings reads', () => { it('queries the matching app_settings key', async () => { const { db: settingsDb, get } = makeSettingsDb({ value: 'true' }); - const s = new MapsService(settingsDb, photoCacheStub); + const s = new MapsService(settingsDb, photoCacheStub, appSettingsStub, usersStub); await s.autocompleteDisabled(); expect(get).toHaveBeenCalledWith(expect.stringContaining('app_settings'), 'places_autocomplete_enabled'); await s.detailsDisabled(); @@ -3381,7 +3401,7 @@ describe('readWikiIdentity', () => { describe('brandLogo', () => { // A fresh service per case: the logo cache lives on the instance, and a hit from // one case would answer the next one's question before its fetch stub ran. - const service = (): MapsService => new MapsService(new DatabaseService(db as never), photoCacheStub); + const service = (): MapsService => new MapsService(new DatabaseService(db as never), photoCacheStub, appSettingsStub, usersStub); const claimResponse = (file: string | null) => ({ ok: true, diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index 9367ee447..e3e288b23 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -106,7 +106,7 @@ import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.se import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; import { OidcService } from '../../../src/nest/oidc/oidc.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; // MailerService is injected since the notifications fold — a stub instead of a // module mock. sendPasswordResetEmail is the only thing auth reaches for. @@ -130,6 +130,7 @@ beforeAll(async () => { mailerStub, new EphemeralTokenService(), new AllowedFileTypesService(new DatabaseService(testDb)), await createTestUnitOfWork(testDb), + await createTestAppSettingsRepo(testDb), await createTestUsersRepo(testDb), ); svc = new OidcService(new DatabaseService(testDb), auth, membership, await createTestUnitOfWork(testDb)); }); diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index 1d5e6e005..b4a84b0d7 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/server/tests/unit/nest/passkey.service.test.ts @@ -89,7 +89,7 @@ import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service' import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; // MailerService is injected since the notifications fold — a stub instead of a // module mock. sendPasswordResetEmail is the only thing auth reaches for. @@ -123,6 +123,7 @@ beforeAll(async () => { mailerStub, new EphemeralTokenService(), new AllowedFileTypesService(new DatabaseService(testDb)), await createTestUnitOfWork(testDb), + await createTestAppSettingsRepo(testDb), await createTestUsersRepo(testDb), ); svc = new PasskeyService(new DatabaseService(testDb), auth, webauthn, await createTestUnitOfWork(testDb)); }); diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index b0a8fdfdc..bf588e57f 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 } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const photoCacheStub = { removeIfUnreferenced: vi.fn() } as unknown as PlacePhotoCacheService; @@ -87,7 +87,7 @@ async function svc(searchNominatim: MapsService['searchNominatim']): Promise null), } as unknown as MapsService, new QueryHelpersService(dbs), - new UnsplashService(dbs, new RuntimeEnvService(), storageFx.storage), + new UnsplashService(await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), new RuntimeEnvService(), storageFx.storage), photoCacheStub, new JourneyDomainService(dbs, new RealtimeService(), new TrekPhotosRepository(dbs), await createTestUnitOfWork(dbs.connection)), storageFx.storage, diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 7a33bf67d..c0289c1fa 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -78,7 +78,16 @@ 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 } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; + +// The default `maps` MapsService below never has its key-resolving methods +// exercised by this file's own cases (a real caller passes its own `maps` +// when it needs that) — these are stand-ins only so the constructor call +// typechecks and never throws if a future case does reach them. +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; const GPX_FIXTURE = path.join(__dirname, '../../fixtures/test.gpx'); const KML_FIXTURE = path.join(__dirname, '../../fixtures/test.kml'); @@ -96,14 +105,16 @@ const dbs = new DatabaseService(testDb); */ const placesStorageFx = makeStorageFixture(''); -async function makePlacesService(maps: MapsService = new MapsService(dbs, photoCacheStub)): Promise { +async function makePlacesService( + maps: MapsService = new MapsService(dbs, photoCacheStub, noAppSettings, noUsers), +): Promise { return new PlacesService( dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), new RealtimeService(), maps, new QueryHelpersService(dbs), - new UnsplashService(dbs, new RuntimeEnvService(), placesStorageFx.storage), + new UnsplashService(await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), new RuntimeEnvService(), placesStorageFx.storage), photoCacheStub, new JourneyDomainService(dbs, new RealtimeService(), new TrekPhotosRepository(dbs), await createTestUnitOfWork(dbs.connection)), placesStorageFx.storage, diff --git a/server/tests/unit/nest/settings.service.test.ts b/server/tests/unit/nest/settings.service.test.ts index ca8316637..337d34a5b 100644 --- a/server/tests/unit/nest/settings.service.test.ts +++ b/server/tests/unit/nest/settings.service.test.ts @@ -45,24 +45,27 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; -import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; -import { DatabaseService } from '../../../src/nest/database/database.service'; +import type { TestOrm } from '../../helpers/test-orm'; import { SettingsService } from '../../../src/nest/settings/settings.service'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; -import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; -import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo } from '../../helpers/test-uow'; let svc: SettingsService; let t: TestOrm; +// `t`, `uow` and the two repositories all derive from the SAME `sharedTestOrm(testDb)` +// (task-2-review.md I2): a second, independent `createTestOrm(testDb)` here would give +// `svc`'s repositories a different Kysely client than the one `uow.transactional(...)` +// opens its transaction on — a repository write inside setAdminUserDefaults/ +// bulkUpsertSettings's transaction would then run outside it (or contend on a second +// mutex over the same connection), exactly the trap the shared-ORM helpers exist to avoid. beforeAll(async () => { createTables(testDb); runMigrations(testDb); - t = await createTestOrm(testDb); + t = await sharedTestOrm(testDb); svc = new SettingsService( - new DatabaseService(testDb), await createTestUnitOfWork(testDb), - t.repo(AppSettings) as AppSettingsRepository, + await createTestAppSettingsRepo(testDb), + await createTestSettingsRepo(testDb), ); }); @@ -338,18 +341,25 @@ describe('bulkUpsertSettings', () => { expect((await svc.getUserSettings(b.id) as any).shared_key).toBe('from-b'); }); + // Was a `vi.spyOn(testDb, 'prepare').mockImplementationOnce(...)` targeting + // the legacy raw `INSERT INTO settings ...` statement. Now that + // bulkUpsertSettings writes through SettingsRepository.upsertForUser + // (em.upsert, via Kysely), the FIRST `.prepare()` call inside + // `uow.transactional(...)` is Kysely's own internal machinery, not the + // application statement — intercepting it corrupted the connection's + // transaction/mutex state for the rest of the file (every test after this + // one timed out). Per the inventory's own note for this test + // ("narrow enough to become a mockRejectedValueOnce on the specific + // repository method under test"), this now mocks the repository method + // itself — no raw connection involved, so no risk of corrupting the shared + // Kysely client's state for later tests. it('SET-SVC-019 — rolls back and re-throws when DB write fails mid-transaction', async () => { const { user } = createUser(testDb); - const origPrepare = testDb.prepare.bind(testDb); - let intercepted = false; - vi.spyOn(testDb, 'prepare').mockImplementationOnce((sql: string) => { - const stmt = origPrepare(sql); - intercepted = true; - return { run: () => { throw new Error('forced DB error'); } } as any; - }); + const settingsRepo = await createTestSettingsRepo(testDb); + const spy = vi.spyOn(settingsRepo, 'upsertForUser').mockRejectedValueOnce(new Error('forced DB error')); await expect(svc.bulkUpsertSettings(user.id, { k: 'v' })).rejects.toThrow('forced DB error'); - expect(intercepted).toBe(true); - vi.restoreAllMocks(); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); }); }); diff --git a/server/tests/unit/nest/settings/instance-api-keys.test.ts b/server/tests/unit/nest/settings/instance-api-keys.test.ts index c33659729..10b5ab3c2 100644 --- a/server/tests/unit/nest/settings/instance-api-keys.test.ts +++ b/server/tests/unit/nest/settings/instance-api-keys.test.ts @@ -1,52 +1,51 @@ /** - * instance-api-keys.test.ts — INSTKEY-001 through INSTKEY-008. + * instance-api-keys.test.ts — INSTKEY-001 through INSTKEY-009. * * The shared resolver behind #1939: the Places/Unsplash credential is instance * configuration, so it comes from the env or an encrypted app_settings row, and * only then from the caller's own users row. Run against a real in-memory DB - * with real apiKeyCrypto (a fixed ENCRYPTION_KEY), because the round trip - * through the random IV is half of what these functions promise. + * with real apiKeyCrypto (ENCRYPTION_KEY comes from tests/setup.ts), because + * the round trip through the random IV is half of what these functions + * promise. + * + * Plan 3a Task 5: `readInstanceApiKey`/`writeInstanceApiKey`/`resolveApiKey` + * take a real `AppSettingsRepository`/`UsersRepository` directly now (every + * real caller — nest/addons, nest/auth × 2, nest/maps, nest/transit, + * nest/unsplash — constructor-injects its own), so these tests build the same + * repositories `t.repo(X)` hands any other converted domain's tests, no + * request-context wrapper needed. */ -const { testDb, dbMock } = vi.hoisted(() => { - const Database = require('better-sqlite3'); - const db = new Database(':memory:'); - db.exec('PRAGMA journal_mode = WAL'); - db.exec('PRAGMA foreign_keys = ON'); - return { testDb: db, dbMock: { db, closeDb: () => {}, reinitialize: () => {} } }; -}); - -vi.mock('../../../../src/db/database', () => dbMock); -vi.mock('../../../../src/config', () => ({ - JWT_SECRET: 'test-secret', - ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', -})); - -import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vitest'; -import { createTables } from '../../../../src/db/schema'; -import { runMigrations } from '../../../../src/db/migrations'; -import { createUser, createAdmin } from '../../../helpers/factories'; +import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; -import { - readInstanceApiKey, - writeInstanceApiKey, - resolveApiKey, -} from '../../../../src/nest/settings/instance-api-keys'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createUser, createAdmin } from '../../../helpers/factories'; +import { AppSettings } from '../../../../src/db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../../../src/db/repositories/AppSettings.repository'; +import { Users } from '../../../../src/db/entities/Users.entity'; +import type { UsersRepository } from '../../../../src/db/repositories/Users.repository'; +import { readInstanceApiKey, writeInstanceApiKey, resolveApiKey } from '../../../../src/nest/settings/instance-api-keys'; -const db = new DatabaseService(testDb); +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let appSettings: AppSettingsRepository; +let users: UsersRepository; -beforeAll(() => { - createTables(testDb); - runMigrations(testDb); +beforeAll(async () => { + t = await createTestOrm(testDb); + appSettings = t.repo(AppSettings) as AppSettingsRepository; + users = t.repo(Users) as UsersRepository; }); beforeEach(() => { resetTestDb(testDb); + t.clear(); delete process.env.PLACES_API_KEY; }); -afterAll(() => { +afterAll(async () => { + await t.close(); testDb.close(); }); @@ -55,39 +54,39 @@ const storedValue = (key: string) => describe('instance API keys', () => { it('INSTKEY-001: round-trips through encryption at rest', async () => { - await writeInstanceApiKey(db, 'maps_api_key', 'AIza-instance-key'); + await writeInstanceApiKey(appSettings, 'maps_api_key', 'AIza-instance-key'); expect(storedValue('maps_api_key')).toMatch(/^enc:v1:/); expect(storedValue('maps_api_key')).not.toContain('AIza-instance-key'); - expect(await readInstanceApiKey(db, 'maps_api_key')).toBe('AIza-instance-key'); + expect(await readInstanceApiKey(appSettings, 'maps_api_key')).toBe('AIza-instance-key'); }); it('INSTKEY-002: a second write of the same value replaces the row (no second one)', async () => { - await writeInstanceApiKey(db, 'maps_api_key', 'same-key'); + await writeInstanceApiKey(appSettings, 'maps_api_key', 'same-key'); const first = storedValue('maps_api_key'); - await writeInstanceApiKey(db, 'maps_api_key', 'same-key'); + await writeInstanceApiKey(appSettings, 'maps_api_key', 'same-key'); // Same plaintext, different blob — the IV is random. That is exactly why // "did this change?" is never asked of the stored value. expect(storedValue('maps_api_key')).not.toBe(first); - expect(await readInstanceApiKey(db, 'maps_api_key')).toBe('same-key'); + expect(await readInstanceApiKey(appSettings, 'maps_api_key')).toBe('same-key'); const rows = testDb.prepare("SELECT COUNT(*) AS n FROM app_settings WHERE key = 'maps_api_key'").get() as { n: number }; expect(rows.n).toBe(1); }); it('INSTKEY-003: a blank value reads back as unset but keeps the row', async () => { - await writeInstanceApiKey(db, 'unsplash_api_key', ' '); + await writeInstanceApiKey(appSettings, 'unsplash_api_key', ' '); expect(storedValue('unsplash_api_key')).toBe(''); - expect(await readInstanceApiKey(db, 'unsplash_api_key')).toBeNull(); + expect(await readInstanceApiKey(appSettings, 'unsplash_api_key')).toBeNull(); }); it('INSTKEY-004: a legacy plaintext row still reads', async () => { testDb.prepare("INSERT INTO app_settings (key, value) VALUES ('maps_api_key', 'plain-old-key')").run(); - expect(await readInstanceApiKey(db, 'maps_api_key')).toBe('plain-old-key'); + expect(await readInstanceApiKey(appSettings, 'maps_api_key')).toBe('plain-old-key'); }); it('INSTKEY-005: the operator env key wins and the database is never read', async () => { process.env.PLACES_API_KEY = 'operator-key'; - await writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); - expect(await resolveApiKey(db, 'maps_api_key', 1, process.env.PLACES_API_KEY)).toEqual({ + await writeInstanceApiKey(appSettings, 'maps_api_key', 'instance-key'); + expect(await resolveApiKey(appSettings, users, 'maps_api_key', 1, process.env.PLACES_API_KEY)).toEqual({ key: 'operator-key', source: 'operator-env', }); @@ -96,8 +95,11 @@ describe('instance API keys', () => { it('INSTKEY-006: the instance value wins over the caller own row', async () => { const { user } = createAdmin(testDb); testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('personal-key', user.id); - await writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); - expect(await resolveApiKey(db, 'maps_api_key', user.id, undefined)).toEqual({ key: 'instance-key', source: 'instance' }); + await writeInstanceApiKey(appSettings, 'maps_api_key', 'instance-key'); + expect(await resolveApiKey(appSettings, users, 'maps_api_key', user.id, undefined)).toEqual({ + key: 'instance-key', + source: 'instance', + }); }); it("INSTKEY-007: without an instance value the caller's own row answers — and nobody else's (#1939)", async () => { @@ -106,13 +108,13 @@ describe('instance API keys', () => { const { user: member } = createUser(testDb); // The admin gets theirs... - expect(await resolveApiKey(db, 'maps_api_key', admin.id, undefined)).toEqual({ + expect(await resolveApiKey(appSettings, users, 'maps_api_key', admin.id, undefined)).toEqual({ key: 'admins-own-key', source: 'user-row', }); // ...and the member gets nothing rather than the admin's, which is the whole // point: they used to get it, and Google answered them with a 403. - expect(await resolveApiKey(db, 'maps_api_key', member.id, undefined)).toEqual({ key: null, source: null }); + expect(await resolveApiKey(appSettings, users, 'maps_api_key', member.id, undefined)).toEqual({ key: null, source: null }); }); it('INSTKEY-009: userId 0 asks about the instance only', async () => { @@ -120,20 +122,23 @@ describe('instance API keys', () => { testDb.prepare('UPDATE users SET maps_api_key = ? WHERE id = ?').run('personal-key', user.id); // app-config is optional-auth: with nobody asking there is no own row, and // the answer must not be some other row that happens to be first. - expect(await resolveApiKey(db, 'maps_api_key', 0, undefined)).toEqual({ key: null, source: null }); - await writeInstanceApiKey(db, 'maps_api_key', 'instance-key'); - expect(await resolveApiKey(db, 'maps_api_key', 0, undefined)).toEqual({ key: 'instance-key', source: 'instance' }); + expect(await resolveApiKey(appSettings, users, 'maps_api_key', 0, undefined)).toEqual({ key: null, source: null }); + await writeInstanceApiKey(appSettings, 'maps_api_key', 'instance-key'); + expect(await resolveApiKey(appSettings, users, 'maps_api_key', 0, undefined)).toEqual({ + key: 'instance-key', + source: 'instance', + }); }); it('INSTKEY-008: an empty instance value does not fall through to the own row', async () => { const { user } = createAdmin(testDb); testDb.prepare('UPDATE users SET unsplash_api_key = ? WHERE id = ?').run('stale-personal', user.id); - await writeInstanceApiKey(db, 'unsplash_api_key', 'to-be-cleared'); - await writeInstanceApiKey(db, 'unsplash_api_key', ''); + await writeInstanceApiKey(appSettings, 'unsplash_api_key', 'to-be-cleared'); + await writeInstanceApiKey(appSettings, 'unsplash_api_key', ''); // The admin who cleared the field cleared their column in the same save, so // the fallback finding the old value would only happen on a row nobody // touched — here it must not resurrect a cleared instance key for them. testDb.prepare('UPDATE users SET unsplash_api_key = NULL WHERE id = ?').run(user.id); - expect(await resolveApiKey(db, 'unsplash_api_key', user.id, undefined)).toEqual({ key: null, source: null }); + expect(await resolveApiKey(appSettings, users, 'unsplash_api_key', user.id, undefined)).toEqual({ key: null, source: null }); }); }); diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index 02b293c05..954991906 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -60,21 +60,22 @@ import { ShareService, publicReservationMetadata } from '../../../src/nest/share import { SettingsService } from '../../../src/nest/settings/settings.service'; import { QueryHelpersService } from '../../../src/nest/query-helpers/query-helpers.service'; import type { User } from '../../../src/types'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; -import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; -import { AppSettings } from '../../../src/db/entities/AppSettings.entity'; -import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestSettingsRepo } from '../../helpers/test-uow'; +import type { TestOrm } from '../../helpers/test-orm'; let svc: ShareService; let t: TestOrm; +// `t`, `uow` and the settings repositories all derive from the SAME +// `sharedTestOrm(testDb)` (task-2-review.md I2) — see settings.service.test.ts's +// own comment on this pattern. beforeAll(async () => { createTables(testDb); runMigrations(testDb); - t = await createTestOrm(testDb); + t = await sharedTestOrm(testDb); svc = new ShareService( new DatabaseService(testDb), - new SettingsService(new DatabaseService(testDb), await createTestUnitOfWork(testDb), t.repo(AppSettings) as AppSettingsRepository), + new SettingsService(await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestSettingsRepo(testDb)), permissionsStub, new QueryHelpersService(new DatabaseService(testDb)), photoCacheStub, diff --git a/server/tests/unit/nest/transit.service.test.ts b/server/tests/unit/nest/transit.service.test.ts index 77c39ed2a..e227fc0ed 100644 --- a/server/tests/unit/nest/transit.service.test.ts +++ b/server/tests/unit/nest/transit.service.test.ts @@ -13,6 +13,8 @@ import { deriveTransitStats, type TransitLeg } from '../../../src/nest/transit/t import { GoogleTransitProvider } from '../../../src/nest/transit/google-transit.provider'; import { TransitService } from '../../../src/nest/transit/transit.service'; import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; @@ -26,7 +28,9 @@ const fetchMock = vi.fn(); // No `transit_provider` row means Transitous, so every case below keeps // exercising the MOTIS path — the Google branch has its own suite. const db = { get: () => undefined, run: () => undefined } as unknown as DatabaseService; -const svc = new TransitService(new GoogleTransitProvider(db)); +const noAppSettings = { getValue: async () => null } as unknown as AppSettingsRepository; +const noUsers = { getApiKeyColumn: async () => null } as unknown as UsersRepository; +const svc = new TransitService(new GoogleTransitProvider(db, noAppSettings, noUsers)); beforeEach(() => { vi.stubGlobal('fetch', fetchMock); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 8273136d0..4f5b47c19 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -78,7 +78,7 @@ import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.serv import { makeStorageFixture } from '../../helpers/storage-fixture'; 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, createTestUsersRepo } from '../../helpers/test-uow'; // Real sibling services over the same in-memory DB — the aggregation runs the // actual SQL of every domain it fans out to, so a shape change downstream shows @@ -103,8 +103,8 @@ beforeAll(async () => { 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( dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), - new MapsService(dbs(), photoCache), new QueryHelpersService(dbs()), - new UnsplashService(dbs(), new RuntimeEnvService(), makeStorageFixture('').storage), photoCache, + new MapsService(dbs(), photoCache, await createTestAppSettingsRepo(dbs().connection), await createTestUsersRepo(dbs().connection)), new QueryHelpersService(dbs()), + new UnsplashService(await createTestAppSettingsRepo(dbs().connection), await createTestUsersRepo(dbs().connection), new RuntimeEnvService(), makeStorageFixture('').storage), photoCache, new JourneyDomainService(dbs(), new RealtimeService(), new TrekPhotosRepository(dbs()), await createTestUnitOfWork(dbs().connection)), makeStorageFixture('').storage, await accommodationsOver(dbs()), await createTestUnitOfWork(dbs().connection), diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index cf2f11ee1..0d05b0d34 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -88,7 +88,7 @@ import fs from 'fs'; import path from 'path'; import { notificationsStub } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; // Real sibling services over the same in-memory DB — updateTrip's date-shift // resyncs and the summary/bundle aggregation run their actual SQL. @@ -128,9 +128,9 @@ beforeAll(async () => { dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), - new MapsService(dbs(), photoCache), + new MapsService(dbs(), photoCache, await createTestAppSettingsRepo(dbs().connection), await createTestUsersRepo(dbs().connection)), new QueryHelpersService(dbs()), - new UnsplashService(dbs(), new RuntimeEnvService(), coversFx.storage), + new UnsplashService(await createTestAppSettingsRepo(dbs().connection), await createTestUsersRepo(dbs().connection), new RuntimeEnvService(), coversFx.storage), photoCache, new JourneyDomainService(dbs(), new RealtimeService(), new TrekPhotosRepository(dbs()), await createTestUnitOfWork(dbs().connection)), makeStorageFixture('').storage, diff --git a/server/tests/unit/nest/unsplash/unsplash.service.test.ts b/server/tests/unit/nest/unsplash/unsplash.service.test.ts index 42f67e04a..d1c19ca28 100644 --- a/server/tests/unit/nest/unsplash/unsplash.service.test.ts +++ b/server/tests/unit/nest/unsplash/unsplash.service.test.ts @@ -3,40 +3,36 @@ import fs from 'fs'; import path from 'path'; // safeFetch is mocked so saveUnsplashCover never hits the network. -// db is mocked so getUnsplashKey resolves from a controllable stub, and // decrypt_api_key is a passthrough so stored values compare as plaintext. -// The instance-wide row (#1939) is read before the user's own, so it gets its -// own seam rather than eating the mockDbGet stub of every case below. -const { safeFetch, mockDbGet, mockInstanceGet } = vi.hoisted(() => ({ +// getUnsplashKey resolves through resolveApiKey (instance-api-keys.ts, Plan 3a +// Task 5), which now reads AppSettingsRepository.getValue (the instance-wide +// row, #1939's precedence winner) and UsersRepository.getApiKeyColumn (the +// caller's own row, the last resort) — mockGetValue/mockGetApiKeyColumn stand +// in for those two repository methods, each its own seam so a test can pin +// exactly which one was (or was not) reached. +const { safeFetch, mockGetValue, mockGetApiKeyColumn } = vi.hoisted(() => ({ safeFetch: vi.fn(), - mockDbGet: vi.fn((..._args: unknown[]) => undefined as unknown), - mockInstanceGet: vi.fn((..._args: unknown[]) => undefined as unknown), + mockGetValue: vi.fn((..._args: unknown[]) => Promise.resolve(undefined as string | null | undefined)), + mockGetApiKeyColumn: vi.fn((..._args: unknown[]) => Promise.resolve(undefined as string | null | undefined)), })); vi.mock('../../../../src/utils/ssrfGuard', () => ({ safeFetch })); -vi.mock('../../../../src/db/database', () => ({ - db: { - prepare: (sql: string) => ({ - get: (...args: unknown[]) => (sql.includes('app_settings') ? mockInstanceGet(...args) : mockDbGet(...args)), - all: vi.fn(() => []), - run: vi.fn(), - }), - }, -})); vi.mock('../../../../src/nest/common/crypto/apiKeyCrypto', () => ({ decrypt_api_key: (v: string | null) => v, // Unused by the read path here, but instance-api-keys imports it. maybe_encrypt_api_key: (v: string | null) => v, })); +import type { AppSettingsRepository } from '../../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../../src/db/repositories/Users.repository'; import { UnsplashService } from '../../../../src/nest/unsplash/unsplash.service'; -import { DatabaseService } from '../../../../src/nest/database/database.service'; import { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; -import { db } from '../../../../src/db/database'; import { makeStorageFixture } from '../../../helpers/storage-fixture'; -// Same four entry points, now methods. The db mock above still feeds them. +// Same four entry points, now methods. Stub repositories feed getUnsplashKey. const coverFx = makeStorageFixture('covers/'); -const svc = new UnsplashService(new DatabaseService(db), new RuntimeEnvService(), coverFx.storage); +const appSettingsStub = { getValue: mockGetValue } as unknown as AppSettingsRepository; +const usersStub = { getApiKeyColumn: mockGetApiKeyColumn } as unknown as UsersRepository; +const svc = new UnsplashService(appSettingsStub, usersStub, new RuntimeEnvService(), coverFx.storage); const searchUnsplashPhotos = svc.searchUnsplashPhotos.bind(svc); const getUnsplashKey = svc.getUnsplashKey.bind(svc); const saveUnsplashCover = svc.saveUnsplashCover.bind(svc); @@ -47,8 +43,8 @@ const ORIGINAL_UNSPLASH_ENV = process.env.UNSPLASH_ACCESS_KEY; afterEach(() => { vi.clearAllMocks(); vi.unstubAllGlobals(); - mockDbGet.mockReturnValue(undefined); - mockInstanceGet.mockReturnValue(undefined); + mockGetValue.mockResolvedValue(undefined); + mockGetApiKeyColumn.mockResolvedValue(undefined); if (ORIGINAL_UNSPLASH_ENV === undefined) delete process.env.UNSPLASH_ACCESS_KEY; else process.env.UNSPLASH_ACCESS_KEY = ORIGINAL_UNSPLASH_ENV; }); @@ -161,38 +157,41 @@ describe('unsplashService.searchUnsplashPhotos', () => { describe('unsplashService.getUnsplashKey', () => { it('UNSPLASH-012: prefers the UNSPLASH_ACCESS_KEY env var over any stored key', async () => { process.env.UNSPLASH_ACCESS_KEY = 'env-key'; - mockDbGet.mockReturnValue({ unsplash_api_key: 'user-key' }); + mockGetApiKeyColumn.mockResolvedValue('user-key'); expect(await getUnsplashKey(1)).toBe('env-key'); - expect(mockDbGet).not.toHaveBeenCalled(); + expect(mockGetValue).not.toHaveBeenCalled(); + expect(mockGetApiKeyColumn).not.toHaveBeenCalled(); }); it('UNSPLASH-013: returns the user key when set and no env var', async () => { delete process.env.UNSPLASH_ACCESS_KEY; - mockDbGet.mockReturnValueOnce({ unsplash_api_key: 'user-key' }); + mockGetApiKeyColumn.mockResolvedValueOnce('user-key'); expect(await getUnsplashKey(1)).toBe('user-key'); }); it('UNSPLASH-014: the instance-wide key wins over the user own key (#1939)', async () => { delete process.env.UNSPLASH_ACCESS_KEY; - mockInstanceGet.mockReturnValue({ value: 'instance-key' }); - mockDbGet.mockReturnValue({ unsplash_api_key: 'user-key' }); + mockGetValue.mockResolvedValue('instance-key'); + mockGetApiKeyColumn.mockResolvedValue('user-key'); expect(await getUnsplashKey(1)).toBe('instance-key'); - expect(mockDbGet).not.toHaveBeenCalled(); // the own row is not even read + expect(mockGetApiKeyColumn).not.toHaveBeenCalled(); // the own row is not even read }); it('UNSPLASH-015: returns null when neither env, instance, nor the user has a key', async () => { delete process.env.UNSPLASH_ACCESS_KEY; - mockDbGet.mockReturnValue(undefined); + mockGetValue.mockResolvedValue(undefined); + mockGetApiKeyColumn.mockResolvedValue(undefined); expect(await getUnsplashKey(1)).toBeNull(); }); it("UNSPLASH-015b: never reads another user's key — the admin fallback is gone (#1939)", async () => { delete process.env.UNSPLASH_ACCESS_KEY; - mockDbGet.mockReturnValue(undefined); + mockGetValue.mockResolvedValue(undefined); + mockGetApiKeyColumn.mockResolvedValue(undefined); expect(await getUnsplashKey(1)).toBeNull(); // Both reads are scoped: the instance row and this caller's own row. - expect(mockDbGet).toHaveBeenCalledTimes(1); - expect(mockDbGet).toHaveBeenCalledWith(1); + expect(mockGetApiKeyColumn).toHaveBeenCalledTimes(1); + expect(mockGetApiKeyColumn).toHaveBeenCalledWith(1, 'unsplash_api_key'); }); }); diff --git a/server/tests/unit/nest/user-profile.service.test.ts b/server/tests/unit/nest/user-profile.service.test.ts index 007cd6283..0bb6785ad 100644 --- a/server/tests/unit/nest/user-profile.service.test.ts +++ b/server/tests/unit/nest/user-profile.service.test.ts @@ -43,7 +43,7 @@ import path from 'node:path'; import { UserProfileService } from '../../../src/nest/auth/user-profile.service'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { DatabaseService } from '../../../src/nest/database/database.service'; -import { createTestUnitOfWork } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; import { SEARCH_TEXT_FIELD_MASK } from '../../../src/nest/maps/maps.helpers'; const avatarsFx = makeStorageFixture('avatars/'); @@ -56,6 +56,8 @@ beforeAll(async () => { new DatabaseService(testDb), avatarsFx.storage, await createTestUnitOfWork(testDb), + await createTestAppSettingsRepo(testDb), + await createTestUsersRepo(testDb), ); }); diff --git a/server/tests/unit/services/conflictUpdate.test.ts b/server/tests/unit/services/conflictUpdate.test.ts index 7dafec09c..658763f01 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 } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -72,9 +72,9 @@ beforeAll(async () => { dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), realtime, - new MapsService(dbs, photoCache), + new MapsService(dbs, photoCache, await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection)), new QueryHelpersService(dbs), - new UnsplashService(dbs, runtimeEnv, makeStorageFixture('').storage), + new UnsplashService(await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), runtimeEnv, makeStorageFixture('').storage), photoCache, new JourneyDomainService(dbs, realtime, new TrekPhotosRepository(dbs), await createTestUnitOfWork(dbs.connection)), makeStorageFixture('').storage,