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,