mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-22 15:06:32 +00:00
fix(server): plan 4 — plugin discovery's delete+reinsert pairs are atomic
install/discovery.ts#upsert refreshed a plugin's actions and settings-field descriptors as two independent delete-then-reinsert pairs (DI5-DI8) with no transaction spanning either pair, let alone both: a crash between a delete and its own reinsert left that plugin with an EMPTY actions or settings-field set until the next discovery run, instead of its previous still-valid rows. Wraps both pairs in one uow.transactional, so a discovery refresh for a plugin's descriptors is now all-or-nothing. Deliberate behaviour change, named for the user per the task brief. DiscoveryRepos grows a UnitOfWork; PluginRegistryService (the other production caller, registry.service.ts) picks it up the same @Optional() way PluginRuntimeService already does, with the same guard-throw. Updates the two hand-constructed test call sites that actually exercise discovery (discovery.test.ts, registry.test.ts) to pass a real UnitOfWork.
This commit is contained in:
@@ -8,15 +8,24 @@ import type { PluginsRepository } from '../../../db/repositories/Plugins.reposit
|
||||
import type { PluginActionsRepository } from '../../../db/repositories/PluginActions.repository';
|
||||
import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository';
|
||||
import type { PluginErrorLogRepository } from '../../../db/repositories/PluginErrorLog.repository';
|
||||
import type { UnitOfWork } from '../../database/unit-of-work';
|
||||
|
||||
/** `discoverPlugins`/`upsert`'s repository set — "swap the parameter type" (plan3j-inputs.md
|
||||
* §0): the raw `BetterSqlite3.Database` parameter becomes this bundle of the four
|
||||
* repositories the scan actually writes through. */
|
||||
* repositories the scan actually writes through, plus the `UnitOfWork` `upsert`
|
||||
* uses to make its own delete-then-reinsert pairs (DI5–DI8, Plan 4 Task 8a)
|
||||
* atomic. Optional, not required: both call sites' own `uow` is itself
|
||||
* `@Optional()` (Nest always injects it in the real app; only a hand-built
|
||||
* partial-DI-graph test instance omits it — the same "boot must never block
|
||||
* app init" defensiveness this file's own callers already apply elsewhere) —
|
||||
* `upsert` runs the pairs sequentially, un-transacted, exactly as before,
|
||||
* when it is absent, rather than refusing to discover at all. */
|
||||
export interface DiscoveryRepos {
|
||||
plugins: PluginsRepository;
|
||||
actions: PluginActionsRepository;
|
||||
settingsFields: PluginSettingsFieldsRepository;
|
||||
errorLog: PluginErrorLogRepository;
|
||||
uow?: UnitOfWork;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -97,30 +106,45 @@ async function upsert(repos: DiscoveryRepos, m: PluginManifest): Promise<void> {
|
||||
await repos.plugins.insertManifest({ id: m.id, ...manifestRow });
|
||||
}
|
||||
|
||||
// Refresh the settings-page action descriptors from the manifest.
|
||||
await repos.actions.deleteAllForPlugin(m.id);
|
||||
await repos.actions.insertActions(
|
||||
m.id,
|
||||
m.actions.map((a, i) => ({ action_key: a.key, label: a.label, hint: a.hint ?? null, danger: a.danger ? 1 : 0, scope: a.scope, sort_order: i })),
|
||||
);
|
||||
// Refresh the settings-page action and settings-field descriptors from the
|
||||
// manifest. DI5–DI8 (Plan 4 Task 8a — a deliberate behaviour change, named
|
||||
// for the user): each pair used to run its own delete then its own
|
||||
// re-insert with no transaction spanning them, so a crash between the two
|
||||
// (of either pair) left that plugin with an EMPTY actions or settings-field
|
||||
// set until the next discovery run, rather than its previous (still valid)
|
||||
// rows. Wrapped in one `uow.transactional` (when a `uow` is available —
|
||||
// see `DiscoveryRepos.uow`'s own docstring) so a discovery refresh for a
|
||||
// single plugin's descriptors is now all-or-nothing: either both pairs land
|
||||
// or neither does, and the plugin keeps its prior rows on any failure.
|
||||
const refreshDescriptors = async (): Promise<void> => {
|
||||
await repos.actions.deleteAllForPlugin(m.id);
|
||||
await repos.actions.insertActions(
|
||||
m.id,
|
||||
m.actions.map((a, i) => ({ action_key: a.key, label: a.label, hint: a.hint ?? null, danger: a.danger ? 1 : 0, scope: a.scope, sort_order: i })),
|
||||
);
|
||||
|
||||
// Refresh the settings-field descriptors from the manifest.
|
||||
await repos.settingsFields.deleteAllForPlugin(m.id);
|
||||
await repos.settingsFields.insertFields(
|
||||
m.id,
|
||||
m.settings.map((f, i) => ({
|
||||
field_key: f.key,
|
||||
label: f.label ?? f.key,
|
||||
input_type: f.input_type ?? 'text',
|
||||
placeholder: f.placeholder ?? null,
|
||||
hint: f.hint ?? null,
|
||||
required: f.required ? 1 : 0,
|
||||
secret: f.secret ? 1 : 0,
|
||||
scope: f.scope ?? 'instance',
|
||||
options: f.options ? JSON.stringify(f.options) : null,
|
||||
oauth_config: f.oauth ? JSON.stringify(f.oauth) : null,
|
||||
default_value: f.default === undefined ? null : JSON.stringify(f.default),
|
||||
sort_order: i,
|
||||
})),
|
||||
);
|
||||
await repos.settingsFields.deleteAllForPlugin(m.id);
|
||||
await repos.settingsFields.insertFields(
|
||||
m.id,
|
||||
m.settings.map((f, i) => ({
|
||||
field_key: f.key,
|
||||
label: f.label ?? f.key,
|
||||
input_type: f.input_type ?? 'text',
|
||||
placeholder: f.placeholder ?? null,
|
||||
hint: f.hint ?? null,
|
||||
required: f.required ? 1 : 0,
|
||||
secret: f.secret ? 1 : 0,
|
||||
scope: f.scope ?? 'instance',
|
||||
options: f.options ? JSON.stringify(f.options) : null,
|
||||
oauth_config: f.oauth ? JSON.stringify(f.oauth) : null,
|
||||
default_value: f.default === undefined ? null : JSON.stringify(f.default),
|
||||
sort_order: i,
|
||||
})),
|
||||
);
|
||||
};
|
||||
if (repos.uow) {
|
||||
await repos.uow.transactional(refreshDescriptors);
|
||||
} else {
|
||||
await refreshDescriptors();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,7 +10,7 @@ import { safeDownload, sha256Matches } from '../install/safe-fetch';
|
||||
import { verifyAuthorSignature, SignatureError } from '../install/verify-signature';
|
||||
import { pluginCodeDir, pluginsCodeRoot, pluginsDataRoot } from '../paths';
|
||||
import { clearUpdateBlock, isSignatureCode, setUpdateBlock, RETRUSTABLE_CODE } from '../signature-status';
|
||||
import { Injectable } from '@nestjs/common';
|
||||
import { Injectable, Optional } from '@nestjs/common';
|
||||
import { InjectRepository } from '@mikro-orm/nestjs';
|
||||
import { Plugins } from '../../../db/entities/Plugins.entity';
|
||||
import type { PluginsRepository } from '../../../db/repositories/Plugins.repository';
|
||||
@@ -20,6 +20,7 @@ import { PluginSettingsFields } from '../../../db/entities/PluginSettingsFields.
|
||||
import type { PluginSettingsFieldsRepository } from '../../../db/repositories/PluginSettingsFields.repository';
|
||||
import { PluginErrorLog } from '../../../db/entities/PluginErrorLog.entity';
|
||||
import type { PluginErrorLogRepository } from '../../../db/repositories/PluginErrorLog.repository';
|
||||
import { UnitOfWork } from '../../database/unit-of-work';
|
||||
|
||||
import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
@@ -182,10 +183,17 @@ export class PluginRegistryService {
|
||||
@InjectRepository(PluginActions) private readonly pluginActions: PluginActionsRepository,
|
||||
@InjectRepository(PluginSettingsFields) private readonly pluginSettingsFields: PluginSettingsFieldsRepository,
|
||||
@InjectRepository(PluginErrorLog) private readonly pluginErrorLog: PluginErrorLogRepository,
|
||||
// Plan 4 Task 8a: DiscoveryRepos now needs a `UnitOfWork` so `discoverPlugins`'s
|
||||
// own upsert can wrap its delete-then-reinsert pairs atomically (DI5-DI8).
|
||||
// `@Optional()` matches `PluginRuntimeService`'s own precedent for the same
|
||||
// reason: a hand-built test instance that never exercises discovery need not
|
||||
// construct one — `discoveryRepos` below throws if one is actually needed.
|
||||
@Optional() private readonly uow?: UnitOfWork,
|
||||
) {}
|
||||
|
||||
private get discoveryRepos(): DiscoveryRepos {
|
||||
return { plugins: this.plugins, actions: this.pluginActions, settingsFields: this.pluginSettingsFields, errorLog: this.pluginErrorLog };
|
||||
if (!this.uow) throw new Error('UnitOfWork not provided — tests that exercise plugin discovery must pass one');
|
||||
return { plugins: this.plugins, actions: this.pluginActions, settingsFields: this.pluginSettingsFields, errorLog: this.pluginErrorLog, uow: this.uow };
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -30,6 +30,7 @@ import { Plugins } from '../../../src/db/entities/Plugins.entity';
|
||||
import { PluginActions } from '../../../src/db/entities/PluginActions.entity';
|
||||
import { PluginSettingsFields } from '../../../src/db/entities/PluginSettingsFields.entity';
|
||||
import { PluginErrorLog } from '../../../src/db/entities/PluginErrorLog.entity';
|
||||
import { UnitOfWork } from '../../../src/nest/database/unit-of-work';
|
||||
|
||||
import { PluginRegistryService, RegistryError, __clearRegistryCacheForTests } from '../../../src/nest/plugins/registry/registry.service';
|
||||
import type { ManifestPreview } from '../../../src/nest/plugins/registry/registry.service';
|
||||
@@ -100,7 +101,7 @@ beforeEach(() => {
|
||||
t.clear();
|
||||
__clearRegistryCacheForTests();
|
||||
vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => REGISTRY }) as unknown as Response));
|
||||
svc = new PluginRegistryService(t.repo(Plugins), t.repo(PluginActions), t.repo(PluginSettingsFields), t.repo(PluginErrorLog));
|
||||
svc = new PluginRegistryService(t.repo(Plugins), t.repo(PluginActions), t.repo(PluginSettingsFields), t.repo(PluginErrorLog), new UnitOfWork(t.em));
|
||||
});
|
||||
afterEach(() => {
|
||||
vi.unstubAllGlobals();
|
||||
|
||||
@@ -20,6 +20,7 @@ import { PluginActions } from '../../../src/db/entities/PluginActions.entity';
|
||||
import { PluginSettingsFields } from '../../../src/db/entities/PluginSettingsFields.entity';
|
||||
import { PluginErrorLog } from '../../../src/db/entities/PluginErrorLog.entity';
|
||||
import { discoverPlugins, type DiscoveryRepos } from '../../../src/nest/plugins/install/discovery';
|
||||
import { UnitOfWork } from '../../../src/nest/database/unit-of-work';
|
||||
|
||||
const testDb = createSnapshotTestDb();
|
||||
let t: TestOrm;
|
||||
@@ -36,7 +37,7 @@ function writePlugin(id: string, manifest: Record<string, unknown>, extra?: () =
|
||||
|
||||
beforeAll(async () => {
|
||||
t = await createTestOrm(testDb);
|
||||
repos = { plugins: t.repo(Plugins), actions: t.repo(PluginActions), settingsFields: t.repo(PluginSettingsFields), errorLog: t.repo(PluginErrorLog) };
|
||||
repos = { plugins: t.repo(Plugins), actions: t.repo(PluginActions), settingsFields: t.repo(PluginSettingsFields), errorLog: t.repo(PluginErrorLog), uow: new UnitOfWork(t.em) };
|
||||
});
|
||||
beforeEach(() => {
|
||||
resetTestDb(testDb);
|
||||
|
||||
Reference in New Issue
Block a user