Skip to content

Commit

Permalink
Fix selected dependencies going stale
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dcodeIO authored Nov 20, 2024
1 parent 16a9a8b commit af2aac5
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ export function AppSettingsDialog() {
}

function areSelectionsEqual(a?: Record<string, string>, b?: Record<string, string>) {
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
Expand All @@ -71,9 +71,7 @@ function AppSettingsDialogForApp({
openDependency?: string
}) {
const dialogProps = useDialogOpenProps('app-settings')
const [selectedDependencies, setSelectedDependencies] = useState<Record<string, string>>(
app.selectedDependencies ?? {},
)
const [selectedDependencies, setSelectedDependencies] = useState(app.selectedDependencies)
const [hadChanges, setHadChanges] = useState(false)
const ctx = trpcReact.useContext()
const setSelectedDependenciesMut = trpcReact.apps.setSelectedDependencies.useMutation({
Expand Down
19 changes: 8 additions & 11 deletions packages/umbreld/source/modules/apps/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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<string, string>) {
const {dependencies} = await this.readManifest()
const selections = (dependencies ?? []).reduce(
(selections, dependencyId) => {
selections[dependencyId] = selectedDependencies[dependencyId] ?? dependencyId
return selections
},
{} as Record<string, string>,
)
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}`)
Expand Down
11 changes: 6 additions & 5 deletions packages/umbreld/source/modules/apps/apps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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')
}
Expand All @@ -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
Expand Down
12 changes: 12 additions & 0 deletions packages/umbreld/source/modules/utilities/dependencies.ts
Original file line number Diff line number Diff line change
@@ -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<string, string>) =>
dependencies?.reduce(
(accumulator, dependencyId) => {
accumulator[dependencyId] = selectedDependencies?.[dependencyId] ?? dependencyId
return accumulator
},
{} as Record<string, string>,
) ?? {}
Original file line number Diff line number Diff line change
@@ -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'

Expand Down Expand Up @@ -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<string, any>
const store = new FileStore<LooseSchema>({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)
})
})
12 changes: 9 additions & 3 deletions packages/umbreld/source/modules/utilities/file-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ type DotProp<T, P extends string> = P extends `${infer K}.${infer R}`

type StorePath<T, P extends string> = DotProp<T, P> extends never ? 'The provided path does not exist in the store' : P

export default class FileStore<T> {
type Primitive = number | string | boolean | null | undefined
type Serializable = {
[key: string]: Serializable | Serializable[] | Primitive | Primitive[]
}

export default class FileStore<T extends Serializable> {
filePath: string

#parser
Expand All @@ -37,9 +42,10 @@ export default class FileStore<T> {
}

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
}
Expand Down

0 comments on commit af2aac5

Please sign in to comment.