From 6fafcd72f26ad4bc35a633a8bc3e09ace61c1a22 Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 15:47:02 +0200 Subject: [PATCH] fix(settings): drop dead wiring and the speculative deleteForUser; strict row assertions --- .../db/repositories/Settings.repository.ts | 38 ++++++++----------- server/src/nest/settings/settings.module.ts | 16 ++++---- server/tests/helpers/test-uow.ts | 16 ++++---- server/tests/unit/mcp/tools-trips.test.ts | 17 --------- 4 files changed, 29 insertions(+), 58 deletions(-) diff --git a/server/src/db/repositories/Settings.repository.ts b/server/src/db/repositories/Settings.repository.ts index 250ab2ba5..03f292f0e 100644 --- a/server/src/db/repositories/Settings.repository.ts +++ b/server/src/db/repositories/Settings.repository.ts @@ -46,12 +46,21 @@ export class SettingsRepository extends EntityRepository { * `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. + * by a subsequent read. `Settings`'s own `uniques: [{ properties: ['user', + * 'key'] }]` (`Settings.entity.ts`, generated off the schema's inline + * `UNIQUE(user_id, key)` by the `fix(db)` commit that precedes this one) + * names that same relation field — `Settings.repository.test.ts` asserts + * both the schema-level unique index and the entity metadata. + * + * `onConflictFields` is passed explicitly here for CLARITY at the call + * site, not because `em.upsert` needs it to find the conflict target: the + * `uniques` entry above is fidelity to the schema first, but as a + * consequence it is also consumable — `em.upsert` infers the same + * `(user, key)` target from it alone when `onConflictFields` is omitted + * (`getWhereCondition` in `@mikro-orm/core/utils/upsert-utils.js` matches a + * `uniques` entry's `properties` against the payload's own property-named + * keys). `SETTINGSREPO-014` proves that inference directly, independent of + * this method's explicit option. * * Only `value` is ever updated on conflict, matching the legacy statement's * `DO UPDATE SET value = excluded.value` exactly (no other column, no @@ -60,21 +69,4 @@ export class SettingsRepository extends EntityRepository { 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/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index 1fe8a0929..7130de6da 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -8,7 +8,6 @@ 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({ @@ -17,14 +16,13 @@ import { Users } from '../../db/entities/Users.entity'; // 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])], + // No `Users` entry: `instance-api-keys.ts`'s repository-backed functions + // take a `UsersRepository` as an explicit parameter from their caller + // (nothing resolves it off an ambient MikroORM request context — an + // earlier design tried that and broke ~185 hand-built unit tests, see + // task-5-review.md §4), so nothing in this module ever needs + // `@InjectRepository(Users)`. + imports: [AppConfigModule, AuthModule, AuditModule, MikroOrmModule.forFeature([AppSettings, Settings])], controllers: [SettingsController, AdminDefaultUserSettingsController], providers: [SettingsService, SettingsMcp], exports: [SettingsService], diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index f8f19889b..e70975aee 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -128,15 +128,13 @@ export function createTestSettingsRepo(db: Database.Database): Promise { const existing = usersPerHandle.get(db); diff --git a/server/tests/unit/mcp/tools-trips.test.ts b/server/tests/unit/mcp/tools-trips.test.ts index af47e8da5..af761f32c 100644 --- a/server/tests/unit/mcp/tools-trips.test.ts +++ b/server/tests/unit/mcp/tools-trips.test.ts @@ -470,23 +470,6 @@ 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({