Skip to content

Commit 6bf3f77

Browse files
elibosleyelibosley
authored
fix: invalid configs no longer crash API (#1491)
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error handling when loading and parsing configuration files, preventing crashes and ensuring fallback to default settings if issues occur. * Enhanced logging for configuration errors, including warnings for empty files and detailed error messages for JSON parsing failures. * Added error handling to plugin listing to avoid failures when configuration loading encounters errors. * **Chores** * Updated permissions to allow linting only for the `./api` package using a filtered command. * **Tests** * Added comprehensive tests for configuration loading, parsing, persistence, and updating, covering various file states and error scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent a79d049 commit 6bf3f77

File tree

8 files changed

+666
-52
lines changed

8 files changed

+666
-52
lines changed

‎.claude/settings.local.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99
"Bash(pnpm test:*)",
1010
"Bash(grep:*)",
1111
"Bash(pnpm type-check:*)",
12-
"Bash(pnpm lint:*)"
12+
"Bash(pnpm lint:*)",
13+
"Bash(pnpm --filter ./api lint)"
1314
]
1415
},
1516
"enableAllProjectMcpServers": false

‎api/src/core/utils/misc/parse-config.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,15 @@ export const parseConfig = <T extends Record<string, any>>(
124124
throw new AppError('Invalid Parameters Passed to ParseConfig');
125125
}
126126

127-
const data: Record<string, any> = parseIni(fileContents);
127+
let data: Record<string, any>;
128+
try {
129+
data = parseIni(fileContents);
130+
} catch (error) {
131+
throw new AppError(
132+
`Failed to parse config file: ${error instanceof Error ? error.message : String(error)}`
133+
);
134+
}
135+
128136
// Remove quotes around keys
129137
const dataWithoutQuoteKeys = Object.fromEntries(
130138
Object.entries(data).map(([key, value]) => [key.replace(/^"(.+(?="$))"$/, '$1'), value])

‎api/src/unraid-api/config/api-config.module.ts‎

Lines changed: 72 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import { ConfigPersistenceHelper } from '@app/unraid-api/config/persistence.help
1212

1313
export { type ApiConfig };
1414

15+
const logger = new Logger('ApiConfig');
16+
1517
const createDefaultConfig = (): ApiConfig => ({
1618
version: API_VERSION,
1719
extraOrigins: [],
@@ -33,21 +35,54 @@ export const persistApiConfig = async (config: ApiConfig) => {
3335
};
3436

3537
export const loadApiConfig = async () => {
36-
const defaultConfig = createDefaultConfig();
37-
const apiConfig = new ApiStateConfig<ApiConfig>(
38-
{
39-
name: 'api',
40-
defaultConfig,
41-
parse: (data) => data as ApiConfig,
42-
},
43-
new ConfigPersistenceHelper()
44-
);
45-
const diskConfig = await apiConfig.parseConfig();
46-
return {
47-
...defaultConfig,
48-
...diskConfig,
49-
version: API_VERSION,
50-
};
38+
try {
39+
const defaultConfig = createDefaultConfig();
40+
const apiConfig = new ApiStateConfig<ApiConfig>(
41+
{
42+
name: 'api',
43+
defaultConfig,
44+
parse: (data) => data as ApiConfig,
45+
},
46+
new ConfigPersistenceHelper()
47+
);
48+
49+
let diskConfig: ApiConfig | undefined;
50+
try {
51+
diskConfig = await apiConfig.parseConfig();
52+
} catch (error) {
53+
logger.error('Failed to load API config from disk, using defaults:', error);
54+
diskConfig = undefined;
55+
56+
// Try to overwrite the invalid config with defaults to fix the issue
57+
try {
58+
const configToWrite = {
59+
...defaultConfig,
60+
version: API_VERSION,
61+
};
62+
63+
const writeSuccess = await apiConfig.persist(configToWrite);
64+
if (writeSuccess) {
65+
logger.log('Successfully overwrote invalid config file with defaults.');
66+
} else {
67+
logger.error(
68+
'Failed to overwrite invalid config file. Continuing with defaults in memory only.'
69+
);
70+
}
71+
} catch (persistError) {
72+
logger.error('Error during config file repair:', persistError);
73+
}
74+
}
75+
76+
return {
77+
...defaultConfig,
78+
...diskConfig,
79+
version: API_VERSION,
80+
};
81+
} catch (outerError) {
82+
// This should never happen, but ensures the config factory never throws
83+
logger.error('Critical error in loadApiConfig, using minimal defaults:', outerError);
84+
return createDefaultConfig();
85+
}
5186
};
5287

5388
/**
@@ -81,21 +116,29 @@ export class ApiConfigPersistence {
81116
}
82117

83118
async onModuleInit() {
84-
if (!(await fileExists(this.filePath))) {
85-
this.migrateFromMyServersConfig();
119+
try {
120+
if (!(await fileExists(this.filePath))) {
121+
this.migrateFromMyServersConfig();
122+
}
123+
await this.persistenceHelper.persistIfChanged(this.filePath, this.config);
124+
this.configService.changes$.pipe(bufferTime(25)).subscribe({
125+
next: async (changes) => {
126+
if (changes.some((change) => change.path.startsWith('api'))) {
127+
this.logger.verbose(`API Config changed ${JSON.stringify(changes)}`);
128+
try {
129+
await this.persistenceHelper.persistIfChanged(this.filePath, this.config);
130+
} catch (persistError) {
131+
this.logger.error('Error persisting config changes:', persistError);
132+
}
133+
}
134+
},
135+
error: (err) => {
136+
this.logger.error('Error receiving config changes:', err);
137+
},
138+
});
139+
} catch (error) {
140+
this.logger.error('Error during API config module initialization:', error);
86141
}
87-
await this.persistenceHelper.persistIfChanged(this.filePath, this.config);
88-
this.configService.changes$.pipe(bufferTime(25)).subscribe({
89-
next: async (changes) => {
90-
if (changes.some((change) => change.path.startsWith('api'))) {
91-
this.logger.verbose(`API Config changed ${JSON.stringify(changes)}`);
92-
await this.persistenceHelper.persistIfChanged(this.filePath, this.config);
93-
}
94-
},
95-
error: (err) => {
96-
this.logger.error('Error receiving config changes:', err);
97-
},
98-
});
99142
}
100143

101144
convertLegacyConfig(

‎api/src/unraid-api/config/api-config.test.ts‎

Lines changed: 142 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,26 @@ import { ConfigService } from '@nestjs/config';
22

33
import { beforeEach, describe, expect, it, vi } from 'vitest';
44

5-
import { ApiConfigPersistence } from '@app/unraid-api/config/api-config.module.js';
5+
import { fileExists } from '@app/core/utils/files/file-exists.js';
6+
import { ApiConfigPersistence, loadApiConfig } from '@app/unraid-api/config/api-config.module.js';
67
import { ConfigPersistenceHelper } from '@app/unraid-api/config/persistence.helper.js';
78

9+
// Mock the core file-exists utility used by ApiStateConfig
10+
vi.mock('@app/core/utils/files/file-exists.js', () => ({
11+
fileExists: vi.fn(),
12+
}));
13+
14+
// Mock the shared file-exists utility used by ConfigPersistenceHelper
15+
vi.mock('@unraid/shared/util/file.js', () => ({
16+
fileExists: vi.fn(),
17+
}));
18+
19+
// Mock fs/promises for file I/O operations
20+
vi.mock('fs/promises', () => ({
21+
readFile: vi.fn(),
22+
writeFile: vi.fn(),
23+
}));
24+
825
describe('ApiConfigPersistence', () => {
926
let service: ApiConfigPersistence;
1027
let configService: ConfigService;
@@ -135,3 +152,127 @@ describe('ApiConfigPersistence', () => {
135152
});
136153
});
137154
});
155+
156+
describe('loadApiConfig', () => {
157+
let readFile: any;
158+
let writeFile: any;
159+
160+
beforeEach(async () => {
161+
vi.clearAllMocks();
162+
// Reset modules to ensure fresh imports
163+
vi.resetModules();
164+
165+
// Get mocked functions
166+
const fsMocks = await import('fs/promises');
167+
readFile = fsMocks.readFile;
168+
writeFile = fsMocks.writeFile;
169+
});
170+
171+
it('should return default config when file does not exist', async () => {
172+
vi.mocked(fileExists).mockResolvedValue(false);
173+
174+
const result = await loadApiConfig();
175+
176+
expect(result).toEqual({
177+
version: expect.any(String),
178+
extraOrigins: [],
179+
sandbox: false,
180+
ssoSubIds: [],
181+
plugins: [],
182+
});
183+
});
184+
185+
it('should merge disk config with defaults when file exists', async () => {
186+
const diskConfig = {
187+
extraOrigins: ['https://example.com'],
188+
sandbox: true,
189+
ssoSubIds: ['sub1', 'sub2'],
190+
};
191+
192+
vi.mocked(fileExists).mockResolvedValue(true);
193+
vi.mocked(readFile).mockResolvedValue(JSON.stringify(diskConfig));
194+
195+
const result = await loadApiConfig();
196+
197+
expect(result).toEqual({
198+
version: expect.any(String),
199+
extraOrigins: ['https://example.com'],
200+
sandbox: true,
201+
ssoSubIds: ['sub1', 'sub2'],
202+
plugins: [],
203+
});
204+
});
205+
206+
it('should use default config and overwrite file when JSON parsing fails', async () => {
207+
const { fileExists: sharedFileExists } = await import('@unraid/shared/util/file.js');
208+
209+
vi.mocked(fileExists).mockResolvedValue(true);
210+
vi.mocked(readFile).mockResolvedValue('{ invalid json }');
211+
vi.mocked(sharedFileExists).mockResolvedValue(false); // For persist operation
212+
vi.mocked(writeFile).mockResolvedValue(undefined);
213+
214+
const result = await loadApiConfig();
215+
216+
// Error logging is handled by NestJS Logger, just verify the config is returned
217+
expect(writeFile).toHaveBeenCalled();
218+
expect(result).toEqual({
219+
version: expect.any(String),
220+
extraOrigins: [],
221+
sandbox: false,
222+
ssoSubIds: [],
223+
plugins: [],
224+
});
225+
});
226+
227+
it('should handle write failure gracefully when JSON parsing fails', async () => {
228+
const { fileExists: sharedFileExists } = await import('@unraid/shared/util/file.js');
229+
230+
vi.mocked(fileExists).mockResolvedValue(true);
231+
vi.mocked(readFile).mockResolvedValue('{ invalid json }');
232+
vi.mocked(sharedFileExists).mockResolvedValue(false); // For persist operation
233+
vi.mocked(writeFile).mockRejectedValue(new Error('Permission denied'));
234+
235+
const result = await loadApiConfig();
236+
237+
// Error logging is handled by NestJS Logger, just verify the config is returned
238+
expect(writeFile).toHaveBeenCalled();
239+
expect(result).toEqual({
240+
version: expect.any(String),
241+
extraOrigins: [],
242+
sandbox: false,
243+
ssoSubIds: [],
244+
plugins: [],
245+
});
246+
});
247+
248+
it('should use default config when file is empty', async () => {
249+
vi.mocked(fileExists).mockResolvedValue(true);
250+
vi.mocked(readFile).mockResolvedValue('');
251+
252+
const result = await loadApiConfig();
253+
254+
// No error logging expected for empty files
255+
expect(result).toEqual({
256+
version: expect.any(String),
257+
extraOrigins: [],
258+
sandbox: false,
259+
ssoSubIds: [],
260+
plugins: [],
261+
});
262+
});
263+
264+
it('should always override version with current API_VERSION', async () => {
265+
const diskConfig = {
266+
version: 'old-version',
267+
extraOrigins: ['https://example.com'],
268+
};
269+
270+
vi.mocked(fileExists).mockResolvedValue(true);
271+
vi.mocked(readFile).mockResolvedValue(JSON.stringify(diskConfig));
272+
273+
const result = await loadApiConfig();
274+
275+
expect(result.version).not.toBe('old-version');
276+
expect(result.version).toBeTruthy();
277+
});
278+
});

0 commit comments

Comments
 (0)