From af2aac5df25e3b4a30aa9947fc55516554f64744 Mon Sep 17 00:00:00 2001 From: dcode Date: Wed, 20 Nov 2024 15:13:19 +0100 Subject: [PATCH] Fix selected dependencies going stale Always fill in selected dependencies on the backend, ensuring that these are either an empty object if an app does not have dependencies, or, if it has, match the app's dependencies, both when stored and returned. Also hardens FileStore to recover from a faulty store file. --- .../app-page/app-settings-dialog.tsx | 10 ++++---- packages/umbreld/source/modules/apps/app.ts | 19 +++++++-------- packages/umbreld/source/modules/apps/apps.ts | 11 +++++---- .../source/modules/utilities/dependencies.ts | 12 ++++++++++ .../utilities/file-store.integration.test.ts | 24 +++++++++++++++++++ .../source/modules/utilities/file-store.ts | 12 +++++++--- 6 files changed, 63 insertions(+), 25 deletions(-) create mode 100644 packages/umbreld/source/modules/utilities/dependencies.ts diff --git a/packages/ui/src/modules/app-store/app-page/app-settings-dialog.tsx b/packages/ui/src/modules/app-store/app-page/app-settings-dialog.tsx index 4dbb9a58e..801a1e4b8 100644 --- a/packages/ui/src/modules/app-store/app-page/app-settings-dialog.tsx +++ b/packages/ui/src/modules/app-store/app-page/app-settings-dialog.tsx @@ -47,9 +47,9 @@ export function AppSettingsDialog() { } function areSelectionsEqual(a?: Record, b?: Record) { - if (!a || !b || a === b) return true - const keys1 = Object.keys(a) - const keys2 = Object.keys(b) + if (a === b) return true + const keys1 = Object.keys((a ||= {})) + const keys2 = Object.keys((b ||= {})) if (keys1.length !== keys2.length) return false for (const key of keys1) { if (b[key] !== a[key]) return false @@ -71,9 +71,7 @@ function AppSettingsDialogForApp({ openDependency?: string }) { const dialogProps = useDialogOpenProps('app-settings') - const [selectedDependencies, setSelectedDependencies] = useState>( - app.selectedDependencies ?? {}, - ) + const [selectedDependencies, setSelectedDependencies] = useState(app.selectedDependencies) const [hadChanges, setHadChanges] = useState(false) const ctx = trpcReact.useContext() const setSelectedDependenciesMut = trpcReact.apps.setSelectedDependencies.useMutation({ diff --git a/packages/umbreld/source/modules/apps/app.ts b/packages/umbreld/source/modules/apps/app.ts index 791ef11cd..5e7bbc0e1 100644 --- a/packages/umbreld/source/modules/apps/app.ts +++ b/packages/umbreld/source/modules/apps/app.ts @@ -11,10 +11,9 @@ import pRetry from 'p-retry' import getDirectorySize from '../utilities/get-directory-size.js' import {pullAll} from '../utilities/docker-pull.js' import FileStore from '../utilities/file-store.js' - +import {fillSelectedDependencies} from '../utilities/dependencies.js' import type Umbreld from '../../index.js' import {validateManifest, type AppSettings} from './schema.js' - import appScript from './legacy-compat/app-script.js' async function readYaml(path: string) { @@ -365,20 +364,18 @@ export default class App { // Get the app's selected dependencies async getSelectedDependencies() { - return this.store.get('dependencies') + const [{dependencies}, selectedDependencies] = await Promise.all([ + this.readManifest(), + this.store.get('dependencies'), + ]) + return fillSelectedDependencies(dependencies, selectedDependencies) } // Set the app's selected dependencies async setSelectedDependencies(selectedDependencies: Record) { const {dependencies} = await this.readManifest() - const selections = (dependencies ?? []).reduce( - (selections, dependencyId) => { - selections[dependencyId] = selectedDependencies[dependencyId] ?? dependencyId - return selections - }, - {} as Record, - ) - const success = await this.store.set('dependencies', selections) + const filledSelectedDependencies = fillSelectedDependencies(dependencies, selectedDependencies) + const success = await this.store.set('dependencies', filledSelectedDependencies) if (success) { this.restart().catch((error) => { this.logger.error(`Failed to restart '${this.id}': ${error.message}`) diff --git a/packages/umbreld/source/modules/apps/apps.ts b/packages/umbreld/source/modules/apps/apps.ts index e06cfe0d6..5fa8a7aa8 100644 --- a/packages/umbreld/source/modules/apps/apps.ts +++ b/packages/umbreld/source/modules/apps/apps.ts @@ -9,10 +9,10 @@ import semver from 'semver' import randomToken from '../../modules/utilities/random-token.js' import type Umbreld from '../../index.js' import appEnvironment from './legacy-compat/app-environment.js' - import type {AppSettings} from './schema.js' import App, {readManifestInDirectory} from './app.js' import type {AppManifest} from './schema.js' +import {fillSelectedDependencies} from '../utilities/dependencies.js' export default class Apps { #umbreld: Umbreld @@ -210,13 +210,13 @@ export default class Apps { this.logger.log(`Installing app ${appId}`) const appTemplatePath = await this.#umbreld.appStore.getAppTemplateFilePath(appId) - let manifestVersion: AppManifest['manifestVersion'] + let manifest: AppManifest try { - manifestVersion = (await readManifestInDirectory(appTemplatePath)).manifestVersion + manifest = await readManifestInDirectory(appTemplatePath) } catch { throw new Error('App template not found') } - const manifestVersionValid = semver.valid(manifestVersion) + const manifestVersionValid = semver.valid(manifest.manifestVersion) if (!manifestVersionValid) { throw new Error('App manifest version is invalid') } @@ -235,7 +235,8 @@ export default class Apps { // Save reference to app instance const app = new App(this.#umbreld, appId) - app.store.set('dependencies', alternatives || {}) + const filledSelectedDependencies = fillSelectedDependencies(manifest.dependencies, alternatives) + await app.store.set('dependencies', filledSelectedDependencies) this.instances.push(app) // Complete the install process via the app script diff --git a/packages/umbreld/source/modules/utilities/dependencies.ts b/packages/umbreld/source/modules/utilities/dependencies.ts new file mode 100644 index 000000000..cde6a7c4d --- /dev/null +++ b/packages/umbreld/source/modules/utilities/dependencies.ts @@ -0,0 +1,12 @@ +/** + * Ensure selected dependencies are filled in when given the app's dependencies + * (where undefined = none) and a user's selection (where undefined = default). + */ +export const fillSelectedDependencies = (dependencies?: string[], selectedDependencies?: Record) => + dependencies?.reduce( + (accumulator, dependencyId) => { + accumulator[dependencyId] = selectedDependencies?.[dependencyId] ?? dependencyId + return accumulator + }, + {} as Record, + ) ?? {} diff --git a/packages/umbreld/source/modules/utilities/file-store.integration.test.ts b/packages/umbreld/source/modules/utilities/file-store.integration.test.ts index a5c72cbd5..4762de9bc 100644 --- a/packages/umbreld/source/modules/utilities/file-store.integration.test.ts +++ b/packages/umbreld/source/modules/utilities/file-store.integration.test.ts @@ -1,5 +1,6 @@ import path from 'node:path' import {describe, beforeAll, afterAll, expect, test} from 'vitest' +import fse from 'fs-extra' import temporaryDirectory from './temporary-directory.js' @@ -168,3 +169,26 @@ describe('store.getWriteLock()', () => { }) }) }) + +const createFaultyStore = async () => { + const filePath = path.join(await directory.create(), 'store.yaml') + + // Create a faulty store where the store file is empty, in turn + // deserializing as `undefined` if not explicitly handled + await fse.ensureFile(filePath) + + type LooseSchema = Record + const store = new FileStore({filePath}) + + return store +} + +describe('Filestore', () => { + test('recovers from faulty store', async () => { + const store = await createFaultyStore() + expect(await store.get()).toStrictEqual({}) + + await store.set('test', 123) + expect(await store.get('test')).toStrictEqual(123) + }) +}) diff --git a/packages/umbreld/source/modules/utilities/file-store.ts b/packages/umbreld/source/modules/utilities/file-store.ts index 50ff1d18c..89709fdf4 100644 --- a/packages/umbreld/source/modules/utilities/file-store.ts +++ b/packages/umbreld/source/modules/utilities/file-store.ts @@ -17,7 +17,12 @@ type DotProp = P extends `${infer K}.${infer R}` type StorePath = DotProp extends never ? 'The provided path does not exist in the store' : P -export default class FileStore { +type Primitive = number | string | boolean | null | undefined +type Serializable = { + [key: string]: Serializable | Serializable[] | Primitive | Primitive[] +} + +export default class FileStore { filePath: string #parser @@ -37,9 +42,10 @@ export default class FileStore { } async #read() { - const rawData = await getOrCreateFile(this.filePath, this.#parser.encode({})) + const defaultValue: Serializable = {} + const rawData = await getOrCreateFile(this.filePath, this.#parser.encode(defaultValue)) - const store = this.#parser.decode(rawData) as T + const store = (this.#parser.decode(rawData) || defaultValue) as T return store }