diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 34eec413d..0fd183f27 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -9,7 +9,8 @@ "Bash(pnpm test:*)", "Bash(grep:*)", "Bash(pnpm type-check:*)", - "Bash(pnpm lint:*)" + "Bash(pnpm lint:*)", + "Bash(pnpm --filter ./api lint)" ] }, "enableAllProjectMcpServers": false diff --git a/api/src/core/utils/misc/parse-config.ts b/api/src/core/utils/misc/parse-config.ts index 8ce1a7658..032fc94d9 100644 --- a/api/src/core/utils/misc/parse-config.ts +++ b/api/src/core/utils/misc/parse-config.ts @@ -124,7 +124,15 @@ export const parseConfig = >( throw new AppError('Invalid Parameters Passed to ParseConfig'); } - const data: Record = parseIni(fileContents); + let data: Record; + try { + data = parseIni(fileContents); + } catch (error) { + throw new AppError( + `Failed to parse config file: ${error instanceof Error ? error.message : String(error)}` + ); + } + // Remove quotes around keys const dataWithoutQuoteKeys = Object.fromEntries( Object.entries(data).map(([key, value]) => [key.replace(/^"(.+(?="$))"$/, '$1'), value]) diff --git a/api/src/unraid-api/config/api-config.module.ts b/api/src/unraid-api/config/api-config.module.ts index 6d001aa2d..e456ace46 100644 --- a/api/src/unraid-api/config/api-config.module.ts +++ b/api/src/unraid-api/config/api-config.module.ts @@ -12,6 +12,8 @@ import { ConfigPersistenceHelper } from '@app/unraid-api/config/persistence.help export { type ApiConfig }; +const logger = new Logger('ApiConfig'); + const createDefaultConfig = (): ApiConfig => ({ version: API_VERSION, extraOrigins: [], @@ -33,21 +35,54 @@ export const persistApiConfig = async (config: ApiConfig) => { }; export const loadApiConfig = async () => { - const defaultConfig = createDefaultConfig(); - const apiConfig = new ApiStateConfig( - { - name: 'api', - defaultConfig, - parse: (data) => data as ApiConfig, - }, - new ConfigPersistenceHelper() - ); - const diskConfig = await apiConfig.parseConfig(); - return { - ...defaultConfig, - ...diskConfig, - version: API_VERSION, - }; + try { + const defaultConfig = createDefaultConfig(); + const apiConfig = new ApiStateConfig( + { + name: 'api', + defaultConfig, + parse: (data) => data as ApiConfig, + }, + new ConfigPersistenceHelper() + ); + + let diskConfig: ApiConfig | undefined; + try { + diskConfig = await apiConfig.parseConfig(); + } catch (error) { + logger.error('Failed to load API config from disk, using defaults:', error); + diskConfig = undefined; + + // Try to overwrite the invalid config with defaults to fix the issue + try { + const configToWrite = { + ...defaultConfig, + version: API_VERSION, + }; + + const writeSuccess = await apiConfig.persist(configToWrite); + if (writeSuccess) { + logger.log('Successfully overwrote invalid config file with defaults.'); + } else { + logger.error( + 'Failed to overwrite invalid config file. Continuing with defaults in memory only.' + ); + } + } catch (persistError) { + logger.error('Error during config file repair:', persistError); + } + } + + return { + ...defaultConfig, + ...diskConfig, + version: API_VERSION, + }; + } catch (outerError) { + // This should never happen, but ensures the config factory never throws + logger.error('Critical error in loadApiConfig, using minimal defaults:', outerError); + return createDefaultConfig(); + } }; /** @@ -81,21 +116,29 @@ export class ApiConfigPersistence { } async onModuleInit() { - if (!(await fileExists(this.filePath))) { - this.migrateFromMyServersConfig(); + try { + if (!(await fileExists(this.filePath))) { + this.migrateFromMyServersConfig(); + } + await this.persistenceHelper.persistIfChanged(this.filePath, this.config); + this.configService.changes$.pipe(bufferTime(25)).subscribe({ + next: async (changes) => { + if (changes.some((change) => change.path.startsWith('api'))) { + this.logger.verbose(`API Config changed ${JSON.stringify(changes)}`); + try { + await this.persistenceHelper.persistIfChanged(this.filePath, this.config); + } catch (persistError) { + this.logger.error('Error persisting config changes:', persistError); + } + } + }, + error: (err) => { + this.logger.error('Error receiving config changes:', err); + }, + }); + } catch (error) { + this.logger.error('Error during API config module initialization:', error); } - await this.persistenceHelper.persistIfChanged(this.filePath, this.config); - this.configService.changes$.pipe(bufferTime(25)).subscribe({ - next: async (changes) => { - if (changes.some((change) => change.path.startsWith('api'))) { - this.logger.verbose(`API Config changed ${JSON.stringify(changes)}`); - await this.persistenceHelper.persistIfChanged(this.filePath, this.config); - } - }, - error: (err) => { - this.logger.error('Error receiving config changes:', err); - }, - }); } convertLegacyConfig( diff --git a/api/src/unraid-api/config/api-config.test.ts b/api/src/unraid-api/config/api-config.test.ts index 58f9603c2..2638856e0 100644 --- a/api/src/unraid-api/config/api-config.test.ts +++ b/api/src/unraid-api/config/api-config.test.ts @@ -2,9 +2,26 @@ import { ConfigService } from '@nestjs/config'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { ApiConfigPersistence } from '@app/unraid-api/config/api-config.module.js'; +import { fileExists } from '@app/core/utils/files/file-exists.js'; +import { ApiConfigPersistence, loadApiConfig } from '@app/unraid-api/config/api-config.module.js'; import { ConfigPersistenceHelper } from '@app/unraid-api/config/persistence.helper.js'; +// Mock the core file-exists utility used by ApiStateConfig +vi.mock('@app/core/utils/files/file-exists.js', () => ({ + fileExists: vi.fn(), +})); + +// Mock the shared file-exists utility used by ConfigPersistenceHelper +vi.mock('@unraid/shared/util/file.js', () => ({ + fileExists: vi.fn(), +})); + +// Mock fs/promises for file I/O operations +vi.mock('fs/promises', () => ({ + readFile: vi.fn(), + writeFile: vi.fn(), +})); + describe('ApiConfigPersistence', () => { let service: ApiConfigPersistence; let configService: ConfigService; @@ -135,3 +152,127 @@ describe('ApiConfigPersistence', () => { }); }); }); + +describe('loadApiConfig', () => { + let readFile: any; + let writeFile: any; + + beforeEach(async () => { + vi.clearAllMocks(); + // Reset modules to ensure fresh imports + vi.resetModules(); + + // Get mocked functions + const fsMocks = await import('fs/promises'); + readFile = fsMocks.readFile; + writeFile = fsMocks.writeFile; + }); + + it('should return default config when file does not exist', async () => { + vi.mocked(fileExists).mockResolvedValue(false); + + const result = await loadApiConfig(); + + expect(result).toEqual({ + version: expect.any(String), + extraOrigins: [], + sandbox: false, + ssoSubIds: [], + plugins: [], + }); + }); + + it('should merge disk config with defaults when file exists', async () => { + const diskConfig = { + extraOrigins: ['https://example.com'], + sandbox: true, + ssoSubIds: ['sub1', 'sub2'], + }; + + vi.mocked(fileExists).mockResolvedValue(true); + vi.mocked(readFile).mockResolvedValue(JSON.stringify(diskConfig)); + + const result = await loadApiConfig(); + + expect(result).toEqual({ + version: expect.any(String), + extraOrigins: ['https://example.com'], + sandbox: true, + ssoSubIds: ['sub1', 'sub2'], + plugins: [], + }); + }); + + it('should use default config and overwrite file when JSON parsing fails', async () => { + const { fileExists: sharedFileExists } = await import('@unraid/shared/util/file.js'); + + vi.mocked(fileExists).mockResolvedValue(true); + vi.mocked(readFile).mockResolvedValue('{ invalid json }'); + vi.mocked(sharedFileExists).mockResolvedValue(false); // For persist operation + vi.mocked(writeFile).mockResolvedValue(undefined); + + const result = await loadApiConfig(); + + // Error logging is handled by NestJS Logger, just verify the config is returned + expect(writeFile).toHaveBeenCalled(); + expect(result).toEqual({ + version: expect.any(String), + extraOrigins: [], + sandbox: false, + ssoSubIds: [], + plugins: [], + }); + }); + + it('should handle write failure gracefully when JSON parsing fails', async () => { + const { fileExists: sharedFileExists } = await import('@unraid/shared/util/file.js'); + + vi.mocked(fileExists).mockResolvedValue(true); + vi.mocked(readFile).mockResolvedValue('{ invalid json }'); + vi.mocked(sharedFileExists).mockResolvedValue(false); // For persist operation + vi.mocked(writeFile).mockRejectedValue(new Error('Permission denied')); + + const result = await loadApiConfig(); + + // Error logging is handled by NestJS Logger, just verify the config is returned + expect(writeFile).toHaveBeenCalled(); + expect(result).toEqual({ + version: expect.any(String), + extraOrigins: [], + sandbox: false, + ssoSubIds: [], + plugins: [], + }); + }); + + it('should use default config when file is empty', async () => { + vi.mocked(fileExists).mockResolvedValue(true); + vi.mocked(readFile).mockResolvedValue(''); + + const result = await loadApiConfig(); + + // No error logging expected for empty files + expect(result).toEqual({ + version: expect.any(String), + extraOrigins: [], + sandbox: false, + ssoSubIds: [], + plugins: [], + }); + }); + + it('should always override version with current API_VERSION', async () => { + const diskConfig = { + version: 'old-version', + extraOrigins: ['https://example.com'], + }; + + vi.mocked(fileExists).mockResolvedValue(true); + vi.mocked(readFile).mockResolvedValue(JSON.stringify(diskConfig)); + + const result = await loadApiConfig(); + + expect(result.version).not.toBe('old-version'); + expect(result.version).toBeTruthy(); + }); +}); diff --git a/api/src/unraid-api/config/factory/api-state.model.test.ts b/api/src/unraid-api/config/factory/api-state.model.test.ts new file mode 100644 index 000000000..09fb77725 --- /dev/null +++ b/api/src/unraid-api/config/factory/api-state.model.test.ts @@ -0,0 +1,364 @@ +import { Logger } from '@nestjs/common'; +import { readFile } from 'node:fs/promises'; +import { join } from 'path'; + +import type { Mock } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { fileExists } from '@app/core/utils/files/file-exists.js'; +import { ApiStateConfig } from '@app/unraid-api/config/factory/api-state.model.js'; +import { ConfigPersistenceHelper } from '@app/unraid-api/config/persistence.helper.js'; + +vi.mock('node:fs/promises'); +vi.mock('@app/core/utils/files/file-exists.js'); +vi.mock('@app/environment.js', () => ({ + PATHS_CONFIG_MODULES: '/test/config/path', +})); + +describe('ApiStateConfig', () => { + let mockPersistenceHelper: ConfigPersistenceHelper; + let mockLogger: Logger; + + interface TestConfig { + name: string; + value: number; + enabled: boolean; + } + + const defaultConfig: TestConfig = { + name: 'test', + value: 42, + enabled: true, + }; + + const parseFunction = (data: unknown): TestConfig => { + if (!data || typeof data !== 'object') { + throw new Error('Invalid config format'); + } + return data as TestConfig; + }; + + beforeEach(() => { + vi.clearAllMocks(); + + mockPersistenceHelper = { + persistIfChanged: vi.fn().mockResolvedValue(true), + } as any; + + mockLogger = { + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + } as any; + + vi.spyOn(Logger.prototype, 'log').mockImplementation(mockLogger.log); + vi.spyOn(Logger.prototype, 'warn').mockImplementation(mockLogger.warn); + vi.spyOn(Logger.prototype, 'error').mockImplementation(mockLogger.error); + vi.spyOn(Logger.prototype, 'debug').mockImplementation(mockLogger.debug); + }); + + describe('constructor', () => { + it('should initialize with cloned default config', () => { + const config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + + expect(config.config).toEqual(defaultConfig); + expect(config.config).not.toBe(defaultConfig); + }); + }); + + describe('token', () => { + it('should generate correct token', () => { + const config = new ApiStateConfig( + { + name: 'my-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + + expect(config.token).toBe('ApiConfig.my-config'); + }); + }); + + describe('file paths', () => { + it('should generate correct file name', () => { + const config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + + expect(config.fileName).toBe('test-config.json'); + }); + + it('should generate correct file path', () => { + const config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + + expect(config.filePath).toBe(join('/test/config/path', 'test-config.json')); + }); + }); + + describe('parseConfig', () => { + let config: ApiStateConfig; + + beforeEach(() => { + config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + }); + + it('should return undefined when file does not exist', async () => { + (fileExists as Mock).mockResolvedValue(false); + + const result = await config.parseConfig(); + + expect(result).toBeUndefined(); + expect(readFile).not.toHaveBeenCalled(); + }); + + it('should parse valid JSON config', async () => { + const validConfig = { name: 'custom', value: 100, enabled: false }; + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue(JSON.stringify(validConfig)); + + const result = await config.parseConfig(); + + expect(result).toEqual(validConfig); + expect(readFile).toHaveBeenCalledWith(config.filePath, 'utf8'); + }); + + it('should return undefined for empty file', async () => { + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue(''); + + const result = await config.parseConfig(); + + expect(result).toBeUndefined(); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('is empty')); + }); + + it('should return undefined for whitespace-only file', async () => { + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue(' \n\t '); + + const result = await config.parseConfig(); + + expect(result).toBeUndefined(); + expect(mockLogger.warn).toHaveBeenCalledWith(expect.stringContaining('is empty')); + }); + + it('should throw error for invalid JSON', async () => { + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue('{ invalid json }'); + + await expect(config.parseConfig()).rejects.toThrow(); + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to parse JSON') + ); + expect(mockLogger.debug).toHaveBeenCalledWith(expect.stringContaining('{ invalid json }')); + }); + + it('should throw error for incomplete JSON', async () => { + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue('{ "name": "test"'); + + await expect(config.parseConfig()).rejects.toThrow(); + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to parse JSON') + ); + }); + + it('should use custom file path when provided', async () => { + const customPath = '/custom/path/config.json'; + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue(JSON.stringify(defaultConfig)); + + await config.parseConfig({ filePath: customPath }); + + expect(fileExists).toHaveBeenCalledWith(customPath); + expect(readFile).toHaveBeenCalledWith(customPath, 'utf8'); + }); + }); + + describe('persist', () => { + let config: ApiStateConfig; + + beforeEach(() => { + config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + }); + + it('should persist current config when no argument provided', async () => { + const result = await config.persist(); + + expect(result).toBe(true); + expect(mockPersistenceHelper.persistIfChanged).toHaveBeenCalledWith( + config.filePath, + defaultConfig + ); + }); + + it('should persist provided config', async () => { + const customConfig = { name: 'custom', value: 999, enabled: false }; + + const result = await config.persist(customConfig); + + expect(result).toBe(true); + expect(mockPersistenceHelper.persistIfChanged).toHaveBeenCalledWith( + config.filePath, + customConfig + ); + }); + + it('should return false and log error on persistence failure', async () => { + (mockPersistenceHelper.persistIfChanged as Mock).mockResolvedValue(false); + + const result = await config.persist(); + + expect(result).toBe(false); + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Could not write config') + ); + }); + }); + + describe('load', () => { + let config: ApiStateConfig; + + beforeEach(() => { + config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + }); + + it('should load config from file when it exists', async () => { + const savedConfig = { name: 'saved', value: 200, enabled: true }; + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue(JSON.stringify(savedConfig)); + + await config.load(); + + expect(config.config).toEqual(savedConfig); + }); + + it('should create default config when file does not exist', async () => { + (fileExists as Mock).mockResolvedValue(false); + + await config.load(); + + expect(config.config).toEqual(defaultConfig); + expect(mockLogger.log).toHaveBeenCalledWith( + expect.stringContaining('Config file does not exist') + ); + expect(mockPersistenceHelper.persistIfChanged).toHaveBeenCalledWith( + config.filePath, + defaultConfig + ); + }); + + it('should not modify config when file is invalid', async () => { + (fileExists as Mock).mockResolvedValue(true); + (readFile as Mock).mockResolvedValue('invalid json'); + + await config.load(); + + expect(config.config).toEqual(defaultConfig); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.any(Error), + expect.stringContaining('is invalid') + ); + }); + + it('should not throw even when persist fails', async () => { + (fileExists as Mock).mockResolvedValue(false); + (mockPersistenceHelper.persistIfChanged as Mock).mockResolvedValue(false); + + await expect(config.load()).resolves.not.toThrow(); + + expect(config.config).toEqual(defaultConfig); + }); + }); + + describe('update', () => { + let config: ApiStateConfig; + + beforeEach(() => { + config = new ApiStateConfig( + { + name: 'test-config', + defaultConfig, + parse: parseFunction, + }, + mockPersistenceHelper + ); + }); + + it('should update config with partial values', () => { + config.update({ value: 123 }); + + expect(config.config).toEqual({ + name: 'test', + value: 123, + enabled: true, + }); + }); + + it('should return self for chaining', () => { + const result = config.update({ enabled: false }); + + expect(result).toBe(config); + }); + + it('should validate updated config through parse function', () => { + const badParseFunction = vi.fn().mockImplementation(() => { + throw new Error('Validation failed'); + }); + + const strictConfig = new ApiStateConfig( + { + name: 'strict-config', + defaultConfig, + parse: badParseFunction, + }, + mockPersistenceHelper + ); + + expect(() => strictConfig.update({ value: -1 })).toThrow('Validation failed'); + }); + }); +}); diff --git a/api/src/unraid-api/config/factory/api-state.model.ts b/api/src/unraid-api/config/factory/api-state.model.ts index 48c055de6..487f80f0d 100644 --- a/api/src/unraid-api/config/factory/api-state.model.ts +++ b/api/src/unraid-api/config/factory/api-state.model.ts @@ -56,13 +56,11 @@ export class ApiStateConfig { * @returns True if the config was written successfully, false otherwise. */ async persist(config = this.#config) { - try { - await this.persistenceHelper.persistIfChanged(this.filePath, config); - return true; - } catch (error) { - this.logger.error(error, `Could not write config to ${this.filePath}.`); - return false; + const success = await this.persistenceHelper.persistIfChanged(this.filePath, config); + if (!success) { + this.logger.error(`Could not write config to ${this.filePath}.`); } + return success; } /** @@ -76,8 +74,23 @@ export class ApiStateConfig { const { filePath = this.filePath } = opts; if (!(await fileExists(filePath))) return undefined; - const rawConfig = JSON.parse(await readFile(filePath, 'utf8')); - return this.options.parse(rawConfig); + const fileContent = await readFile(filePath, 'utf8'); + + if (!fileContent || fileContent.trim() === '') { + this.logger.warn(`Config file '${filePath}' is empty.`); + return undefined; + } + + try { + const rawConfig = JSON.parse(fileContent); + return this.options.parse(rawConfig); + } catch (error) { + this.logger.error( + `Failed to parse JSON from '${filePath}': ${error instanceof Error ? error.message : String(error)}` + ); + this.logger.debug(`File content: ${fileContent.substring(0, 100)}...`); + throw error; + } } /** diff --git a/api/src/unraid-api/config/persistence.helper.ts b/api/src/unraid-api/config/persistence.helper.ts index 449b78655..0a40c841b 100644 --- a/api/src/unraid-api/config/persistence.helper.ts +++ b/api/src/unraid-api/config/persistence.helper.ts @@ -12,24 +12,59 @@ export class ConfigPersistenceHelper { * * @param filePath - The path to the config file. * @param data - The data to persist. - * @returns `true` if the config was persisted, `false` otherwise. + * @returns `true` if the config was persisted, `false` if no changes were needed or if persistence failed. * - * @throws {Error} if the config file does not exist or is unreadable. - * @throws {Error} if the config file is not valid JSON. - * @throws {Error} if given data is not JSON (de)serializable. - * @throws {Error} if the config file is not writable. + * This method is designed to never throw errors. If the existing file is corrupted or unreadable, + * it will attempt to overwrite it with the new data. If write operations fail, it returns false + * but does not crash the application. */ async persistIfChanged(filePath: string, data: unknown): Promise { if (!(await fileExists(filePath))) { - await writeFile(filePath, JSON.stringify(data ?? {}, null, 2)); - return true; + try { + const jsonString = JSON.stringify(data ?? {}, null, 2); + await writeFile(filePath, jsonString); + return true; + } catch (error) { + // JSON serialization or write failed, but don't crash - just return false + return false; + } } - const currentData = JSON.parse(await readFile(filePath, 'utf8')); - const stagedData = JSON.parse(JSON.stringify(data)); + + let currentData: unknown; + try { + const fileContent = await readFile(filePath, 'utf8'); + currentData = JSON.parse(fileContent); + } catch (error) { + // If existing file is corrupted, treat it as if it doesn't exist + // and write the new data + try { + const jsonString = JSON.stringify(data ?? {}, null, 2); + await writeFile(filePath, jsonString); + return true; + } catch (writeError) { + // JSON serialization or write failed, but don't crash - just return false + return false; + } + } + + let stagedData: unknown; + try { + stagedData = JSON.parse(JSON.stringify(data)); + } catch (error) { + // If data can't be serialized to JSON, we can't persist it + return false; + } + if (isEqual(currentData, stagedData)) { return false; } - await writeFile(filePath, JSON.stringify(stagedData, null, 2)); - return true; + + try { + await writeFile(filePath, JSON.stringify(stagedData, null, 2)); + return true; + } catch (error) { + // Write failed, but don't crash - just return false + return false; + } } } diff --git a/api/src/unraid-api/plugin/plugin.service.ts b/api/src/unraid-api/plugin/plugin.service.ts index 32ac86695..5d5aa2dea 100644 --- a/api/src/unraid-api/plugin/plugin.service.ts +++ b/api/src/unraid-api/plugin/plugin.service.ts @@ -65,7 +65,16 @@ export class PluginService { * @returns A tuple of the plugin name and version. */ static async listPlugins(): Promise<[string, string][]> { - const { plugins = [] } = await loadApiConfig(); + let plugins: string[] = []; + try { + const config = await loadApiConfig(); + plugins = config.plugins || []; + } catch (error) { + PluginService.logger.error( + 'Failed to load API config for plugin discovery, using empty list:', + error + ); + } const pluginNames = new Set( plugins.map((plugin) => { const { name } = parsePackageArg(plugin);