From 17ff6d6a8fb9f33b49e2ce56df62d9a0f93eeb65 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Mon, 13 Mar 2023 16:52:01 +0100 Subject: [PATCH] Connect: Show config file errors (#22728) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add a function that returns error that might happen during loading the file * Return validation and file loading errors from `ConfigService` * Discard file storage updates when the file was not loaded correctly * Do not show usage reporting dialog when the file was not loaded correctly * Make title of notifications a little bit smaller, so longer titles do not look weird * Get rid of sync fs functions wherever possible in file storage * Move error handling code to `createFileStorage` * Improve `getConfigError` comment * Rename `discardUpdatesWhenLoadingFileFailed` to `discardUpdatesOnLoadError` * Fix the error message in `notifyAboutConfigErrors` * Revert "Make title of notifications a little bit smaller, so longer titles do not look weird" * Make `writeSync` async too * Return `unknown` from `FileStorage.get` * Add a TODO comment about createFileStorage type * Add "the" to "Using default config instead" * Remove `toString()` --------- Co-authored-by: Rafał Cieślak --- web/packages/teleterm/src/main.ts | 58 ++++++++----- .../teleterm/src/mainProcess/types.ts | 5 +- .../src/mainProcess/windowsManager.ts | 2 +- .../src/services/config/configService.test.ts | 40 ++++++--- .../src/services/config/configService.ts | 52 +++++++++-- .../services/config/configServiceClient.ts | 8 +- .../src/services/config/fixtures/mocks.ts | 4 +- .../src/services/fileStorage/fileStorage.ts | 86 +++++++++++++------ .../services/fileStorage/fileStorageClient.ts | 18 ++-- .../services/fileStorage/fixtures/mocks.ts | 8 +- web/packages/teleterm/src/ui/initUi.test.ts | 19 ++++ web/packages/teleterm/src/ui/initUi.ts | 51 +++++++---- .../statePersistenceService.ts | 5 +- .../ui/services/usage/setUpUsageReporting.ts | 5 ++ 14 files changed, 258 insertions(+), 103 deletions(-) diff --git a/web/packages/teleterm/src/main.ts b/web/packages/teleterm/src/main.ts index e780a92ae86e0..7237a27936230 100644 --- a/web/packages/teleterm/src/main.ts +++ b/web/packages/teleterm/src/main.ts @@ -39,22 +39,15 @@ if (app.requestSingleInstanceLock()) { app.exit(1); } -function initializeApp(): void { +async function initializeApp(): Promise { let devRelaunchScheduled = false; const settings = getRuntimeSettings(); const logger = initMainLogger(settings); - const appStateFileStorage = createFileStorage({ - filePath: path.join(settings.userDataDir, 'app_state.json'), - debounceWrites: true, - }); - const configFileStorage = createFileStorage({ - filePath: path.join(settings.userDataDir, 'app_config.json'), - debounceWrites: false, - }); - const configJsonSchemaFileStorage = createFileStorage({ - filePath: path.join(settings.userDataDir, 'schema_app_config.json'), - debounceWrites: false, - }); + const { + appStateFileStorage, + configFileStorage, + configJsonSchemaFileStorage, + } = await createFileStorages(settings.userDataDir); const configService = createConfigService({ configFile: configFileStorage, @@ -97,16 +90,17 @@ function initializeApp(): void { app.on('will-quit', async event => { event.preventDefault(); + const disposeMainProcess = async () => { + try { + await mainProcess.dispose(); + } catch (e) { + logger.error('Failed to gracefully dispose of main process', e); + } + }; - appStateFileStorage.writeSync(); globalShortcut.unregisterAll(); - try { - await mainProcess.dispose(); - } catch (e) { - logger.error('Failed to gracefully dispose of main process', e); - } finally { - app.exit(); - } + await Promise.all([appStateFileStorage.write(), disposeMainProcess()]); // none of them can throw + app.exit(); }); app.on('quit', () => { @@ -204,3 +198,25 @@ function initMainLogger(settings: types.RuntimeSettings) { return new Logger('Main'); } + +function createFileStorages(userDataDir: string) { + return Promise.all([ + createFileStorage({ + filePath: path.join(userDataDir, 'app_state.json'), + debounceWrites: true, + }), + createFileStorage({ + filePath: path.join(userDataDir, 'app_config.json'), + debounceWrites: false, + discardUpdatesOnLoadError: true, + }), + createFileStorage({ + filePath: path.join(userDataDir, 'schema_app_config.json'), + debounceWrites: false, + }), + ]).then(storages => ({ + appStateFileStorage: storages[0], + configFileStorage: storages[1], + configJsonSchemaFileStorage: storages[2], + })); +} diff --git a/web/packages/teleterm/src/mainProcess/types.ts b/web/packages/teleterm/src/mainProcess/types.ts index 12e06edd89fee..e99ef726f4627 100644 --- a/web/packages/teleterm/src/mainProcess/types.ts +++ b/web/packages/teleterm/src/mainProcess/types.ts @@ -125,13 +125,14 @@ export enum TabContextMenuEventType { export enum ConfigServiceEventType { Get = 'Get', Set = 'Set', - GetStoredConfigErrors = 'GetStoredConfigErrors', + GetConfigError = 'GetConfigError', } export enum FileStorageEventType { Get = 'Get', Put = 'Put', - WriteSync = 'WriteSync', + Write = 'Write', Replace = 'Replace', GetFilePath = 'GetFilePath', + GetFileLoadingError = 'GetFileLoadingError', } diff --git a/web/packages/teleterm/src/mainProcess/windowsManager.ts b/web/packages/teleterm/src/mainProcess/windowsManager.ts index 81ce00bb106b0..02b2df3e81a4c 100644 --- a/web/packages/teleterm/src/mainProcess/windowsManager.ts +++ b/web/packages/teleterm/src/mainProcess/windowsManager.ts @@ -197,7 +197,7 @@ export class WindowsManager { } private getWindowState(): WindowState { - const windowState = this.fileStorage.get(this.storageKey); + const windowState = this.fileStorage.get(this.storageKey) as WindowState; const getDefaults = () => ({ height: 720, width: 1280, diff --git a/web/packages/teleterm/src/services/config/configService.test.ts b/web/packages/teleterm/src/services/config/configService.test.ts index 69e5fe89afc04..ab193a3ed1b0d 100644 --- a/web/packages/teleterm/src/services/config/configService.test.ts +++ b/web/packages/teleterm/src/services/config/configService.test.ts @@ -32,7 +32,7 @@ test('stored and default values are combined', () => { platform: 'darwin', }); - expect(configService.getStoredConfigErrors()).toBeUndefined(); + expect(configService.getConfigError()).toBeUndefined(); const usageReportingEnabled = configService.get('usageReporting.enabled'); expect(usageReportingEnabled.value).toBe(true); @@ -52,15 +52,18 @@ test('in case of invalid value a default one is returned', () => { platform: 'darwin', }); - expect(configService.getStoredConfigErrors()).toStrictEqual([ - { - code: 'invalid_type', - expected: 'boolean', - received: 'string', - message: 'Expected boolean, received string', - path: ['usageReporting.enabled'], - }, - ]); + expect(configService.getConfigError()).toStrictEqual({ + source: 'validation', + errors: [ + { + code: 'invalid_type', + expected: 'boolean', + received: 'string', + message: 'Expected boolean, received string', + path: ['usageReporting.enabled'], + }, + ], + }); const usageReportingEnabled = configService.get('usageReporting.enabled'); expect(usageReportingEnabled.value).toBe(false); @@ -71,6 +74,23 @@ test('in case of invalid value a default one is returned', () => { expect(terminalFontSize.metadata.isStored).toBe(false); }); +test('if config file failed to load correctly the error is returned', () => { + const configFile = createMockFileStorage(); + const error = new Error('Failed to read'); + jest.spyOn(configFile, 'getFileLoadingError').mockReturnValue(error); + + const configService = createConfigService({ + configFile, + jsonSchemaFile: createMockFileStorage(), + platform: 'darwin', + }); + + expect(configService.getConfigError()).toStrictEqual({ + source: 'file-loading', + error, + }); +}); + test('calling set updated the value in store', () => { const configFile = createMockFileStorage(); const configService = createConfigService({ diff --git a/web/packages/teleterm/src/services/config/configService.ts b/web/packages/teleterm/src/services/config/configService.ts index 7563ef3f95734..ce3d0d278df9a 100644 --- a/web/packages/teleterm/src/services/config/configService.ts +++ b/web/packages/teleterm/src/services/config/configService.ts @@ -31,6 +31,18 @@ import { const logger = new Logger('ConfigService'); +type FileLoadingError = { + source: 'file-loading'; + error: Error; +}; + +type ValidationError = { + source: 'validation'; + errors: ZodIssue[]; +}; + +type ConfigError = FileLoadingError | ValidationError; + export interface ConfigService { get( key: K @@ -38,7 +50,16 @@ export interface ConfigService { set(key: K, value: AppConfig[K]): void; - getStoredConfigErrors(): ZodIssue[] | undefined; + /** + * Returns validation errors or an error that occurred during loading the config file (this means IO and syntax errors). + * This error has to be checked during the initialization of the app. + * + * The reason we have a getter for this error instead of making `createConfigService` fail with an error + * is that in the presence of this error we want to notify about it and then continue with default values: + * - If validation errors occur, the incorrect values are replaced with the defaults. + * - In case of an error coming from loading the file, all values are replaced with the defaults. + * */ + getConfigError(): ConfigError | undefined; } export function createConfigService({ @@ -53,10 +74,11 @@ export function createConfigService({ const schema = createAppConfigSchema(platform); updateJsonSchema({ schema, configFile, jsonSchemaFile }); - const { storedConfig, configWithDefaults, errors } = validateStoredConfig( - schema, - configFile - ); + const { + storedConfig, + configWithDefaults, + errors: validationErrors, + } = validateStoredConfig(schema, configFile); return { get: key => ({ @@ -68,7 +90,21 @@ export function createConfigService({ configWithDefaults[key] = value; storedConfig[key] = value; }, - getStoredConfigErrors: () => errors, + getConfigError: () => { + const fileLoadingError = configFile.getFileLoadingError(); + if (fileLoadingError) { + return { + source: 'file-loading', + error: fileLoadingError, + }; + } + if (validationErrors) { + return { + source: 'validation', + errors: validationErrors, + }; + } + }, }; } @@ -96,8 +132,6 @@ function updateJsonSchema({ } } -//TODO (gzdunek): syntax errors of the JSON file are silently ignored, report -// them to the user too function validateStoredConfig( schema: AppConfigSchema, configFile: FileStorage @@ -108,7 +142,7 @@ function validateStoredConfig( } { const parse = (data: Partial) => schema.safeParse(data); - const storedConfig = configFile.get>(); + const storedConfig = configFile.get() as Partial; const parsed = parse(storedConfig); if (parsed.success === true) { return { diff --git a/web/packages/teleterm/src/services/config/configServiceClient.ts b/web/packages/teleterm/src/services/config/configServiceClient.ts index dd761a8c6de10..e12389c89585c 100644 --- a/web/packages/teleterm/src/services/config/configServiceClient.ts +++ b/web/packages/teleterm/src/services/config/configServiceClient.ts @@ -33,8 +33,8 @@ export function subscribeToConfigServiceEvents( return (event.returnValue = configService.get(item.path)); case ConfigServiceEventType.Set: return configService.set(item.path, item.value); - case ConfigServiceEventType.GetStoredConfigErrors: - return (event.returnValue = configService.getStoredConfigErrors()); + case ConfigServiceEventType.GetConfigError: + return (event.returnValue = configService.getConfigError()); } } ); @@ -54,10 +54,10 @@ export function createConfigServiceClient(): ConfigService { value, }); }, - getStoredConfigErrors: () => { + getConfigError: () => { return ipcRenderer.sendSync( ConfigServiceEventChannel, - ConfigServiceEventType.GetStoredConfigErrors + ConfigServiceEventType.GetConfigError ); }, }; diff --git a/web/packages/teleterm/src/services/config/fixtures/mocks.ts b/web/packages/teleterm/src/services/config/fixtures/mocks.ts index fd094e97ebaf3..d500814bdd8db 100644 --- a/web/packages/teleterm/src/services/config/fixtures/mocks.ts +++ b/web/packages/teleterm/src/services/config/fixtures/mocks.ts @@ -27,8 +27,8 @@ export function createMockConfigService( set(key, value) { values[key] = value; }, - getStoredConfigErrors() { - return []; + getConfigError() { + return undefined; }, }; } diff --git a/web/packages/teleterm/src/services/fileStorage/fileStorage.ts b/web/packages/teleterm/src/services/fileStorage/fileStorage.ts index 94455aa9af25e..03da0187b424c 100644 --- a/web/packages/teleterm/src/services/fileStorage/fileStorage.ts +++ b/web/packages/teleterm/src/services/fileStorage/fileStorage.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import fs, { existsSync, readFileSync, writeFileSync } from 'fs'; +import fs from 'fs/promises'; import { debounce } from 'shared/utils/highbar'; @@ -29,54 +29,80 @@ export interface FileStorage { /** Asynchronously replaces the entire storage state with a new value. */ replace(json: any): void; - /** Synchronously writes the storage state to disk. */ - writeSync(): void; + /** Asynchronously writes the storage state to disk. */ + write(): Promise; /** Returns value for a given key. If the key is omitted, the entire storage state is returned. */ - get(key?: string): T; + // TODO(ravicious): Add a generic type to createFileStorage rather than returning `unknown`. + // https://github.com/gravitational/teleport/pull/22728#discussion_r1129566566 + get(key?: string): unknown; /** Returns the file path used to create the storage. */ getFilePath(): string; + + /** Returns the error that could occur while reading and parsing the file. */ + getFileLoadingError(): Error | undefined; } -export function createFileStorage(opts: { +export async function createFileStorage(opts: { filePath: string; debounceWrites: boolean; -}): FileStorage { + /** Prevents state updates when the file has not been loaded correctly, so its content will not be overwritten. */ + discardUpdatesOnLoadError?: boolean; +}): Promise { if (!opts || !opts.filePath) { throw Error('missing filePath'); } const { filePath } = opts; - let state = loadState(opts.filePath); + + let state: any, error: Error | undefined; + try { + state = await loadState(filePath); + } catch (e) { + state = {}; + error = e; + logger.error(`Cannot read ${filePath} file`, e); + } + + const discardUpdates = error && opts.discardUpdatesOnLoadError; function put(key: string, json: any): void { + if (discardUpdates) { + return; + } state[key] = json; stringifyAndWrite(); } - function writeSync(): void { - const text = stringify(state); - try { - fs.writeFileSync(filePath, text); - } catch (error) { - logger.error(`Cannot update ${filePath} file`, error); + function write(): Promise { + if (discardUpdates) { + return; } - } - - function get(key?: string): T { - return key ? state[key] : (state as T); + const text = stringify(state); + writeFile(filePath, text); } function replace(json: any): void { + if (discardUpdates) { + return; + } state = json; stringifyAndWrite(); } + function get(key?: string): unknown { + return key ? state[key] : state; + } + function getFilePath(): string { return opts.filePath; } + function getFileLoadingError(): Error | undefined { + return error; + } + function stringifyAndWrite(): void { const text = stringify(state); @@ -87,23 +113,29 @@ export function createFileStorage(opts: { return { put, - writeSync, + write, get, replace, getFilePath, + getFileLoadingError, }; } -function loadState(filePath: string) { - try { - if (!existsSync(filePath)) { - writeFileSync(filePath, '{}'); - } +async function loadState(filePath: string): Promise { + const file = await readOrCreateFile(filePath); + return JSON.parse(file); +} - return JSON.parse(readFileSync(filePath, { encoding: 'utf-8' })); +async function readOrCreateFile(filePath: string): Promise { + try { + return await fs.readFile(filePath, { encoding: 'utf-8' }); } catch (error) { - logger.error(`Cannot read ${filePath} file`, error); - return {}; + const defaultValue = '{}'; + if (error?.code === 'ENOENT') { + await fs.writeFile(filePath, defaultValue); + return defaultValue; + } + throw error; } } @@ -117,6 +149,6 @@ const writeFileDebounced = debounce( ); const writeFile = (filePath: string, text: string) => - fs.promises.writeFile(filePath, text).catch(error => { + fs.writeFile(filePath, text).catch(error => { logger.error(`Cannot update ${filePath} file`, error); }); diff --git a/web/packages/teleterm/src/services/fileStorage/fileStorageClient.ts b/web/packages/teleterm/src/services/fileStorage/fileStorageClient.ts index 8be82f4b8e296..defcb951b13bf 100644 --- a/web/packages/teleterm/src/services/fileStorage/fileStorageClient.ts +++ b/web/packages/teleterm/src/services/fileStorage/fileStorageClient.ts @@ -32,12 +32,14 @@ export function subscribeToFileStorageEvents(configService: FileStorage): void { return (event.returnValue = configService.get(item.key)); case FileStorageEventType.Put: return configService.put(item.key, item.json); - case FileStorageEventType.WriteSync: - return configService.writeSync(); + case FileStorageEventType.Write: + return configService.write(); case FileStorageEventType.Replace: return configService.replace(item.json); case FileStorageEventType.GetFilePath: return configService.getFilePath(); + case FileStorageEventType.GetFileLoadingError: + return configService.getFileLoadingError(); } } ); @@ -54,10 +56,10 @@ export function createFileStorageClient(): FileStorage { key, json, }), - writeSync: () => - ipcRenderer.send( + write: () => + ipcRenderer.invoke( FileStorageEventChannel, - FileStorageEventType.WriteSync, + FileStorageEventType.Write, {} ), replace: json => @@ -70,5 +72,11 @@ export function createFileStorageClient(): FileStorage { FileStorageEventType.GetFilePath, {} ), + getFileLoadingError: () => + ipcRenderer.sendSync( + FileStorageEventChannel, + FileStorageEventType.GetFileLoadingError, + {} + ), }; } diff --git a/web/packages/teleterm/src/services/fileStorage/fixtures/mocks.ts b/web/packages/teleterm/src/services/fileStorage/fixtures/mocks.ts index 09cd7c90bd10a..a7243e06836a3 100644 --- a/web/packages/teleterm/src/services/fileStorage/fixtures/mocks.ts +++ b/web/packages/teleterm/src/services/fileStorage/fixtures/mocks.ts @@ -29,14 +29,18 @@ export function createMockFileStorage(opts?: { return key ? state[key] : (state as T); }, - writeSync() {}, + async write() {}, replace(json: any) { state = json; }, - getFilePath(): string { + getFilePath() { return opts?.filePath || ''; }, + + getFileLoadingError() { + return undefined; + }, }; } diff --git a/web/packages/teleterm/src/ui/initUi.test.ts b/web/packages/teleterm/src/ui/initUi.test.ts index 3cd2ce87a6420..4e73331fc14e0 100644 --- a/web/packages/teleterm/src/ui/initUi.test.ts +++ b/web/packages/teleterm/src/ui/initUi.test.ts @@ -109,6 +109,25 @@ describe('usage reporting dialogs', () => { }); }); +test('no dialog is shown when config file did not load properly', async () => { + const mockedAppContext = new MockAppContext(); + jest + .spyOn(mockedAppContext.mainProcessClient.configService, 'getConfigError') + .mockImplementation(() => ({ source: 'file-loading', error: new Error() })); + mockOpenRegularDialog(mockedAppContext); + + await initUi(mockedAppContext); + + expect( + mockedAppContext.modalsService.openRegularDialog + ).not.toHaveBeenCalledWith(expect.objectContaining({ kind: 'usage-data' })); + expect( + mockedAppContext.modalsService.openRegularDialog + ).not.toHaveBeenCalledWith( + expect.objectContaining({ kind: 'user-job-role' }) + ); +}); + function mockUsageReportingEnabled( mockedAppContext: AppContext, options: { enabled: boolean } diff --git a/web/packages/teleterm/src/ui/initUi.ts b/web/packages/teleterm/src/ui/initUi.ts index 40782043610c5..a327fffc284fb 100644 --- a/web/packages/teleterm/src/ui/initUi.ts +++ b/web/packages/teleterm/src/ui/initUi.ts @@ -43,34 +43,47 @@ export async function initUi(ctx: IAppContext): Promise { // "User job role" dialog is shown on the second launch (only if user agreed to reporting earlier). await setUpUsageReporting(configService, ctx.modalsService); ctx.workspacesService.restorePersistedState(); - notifyAboutStoredConfigErrors(configService, ctx.notificationsService); + notifyAboutConfigErrors(configService, ctx.notificationsService); notifyAboutDuplicatedShortcutsCombinations( ctx.keyboardShortcutsService, ctx.notificationsService ); } -function notifyAboutStoredConfigErrors( +function notifyAboutConfigErrors( configService: ConfigService, notificationsService: NotificationsService ): void { - const errors = configService.getStoredConfigErrors(); - if (errors) { - const isKeymapError = errors.some(e => - e.path[0].toString().startsWith('keymap.') - ); - notificationsService.notifyError({ - title: 'Encountered errors in config file', - list: errors.map(e => `${e.path[0].toString()}: ${e.message}`), - description: - isKeymapError && - 'A valid shortcut contains at least one modifier and a single key code, for example "Shift+Tab".\nFunction keys do not require a modifier.', - link: { - // TODO(gzdunek): point to the properer section - href: 'https://goteleport.com/docs/connect-your-client/teleport-connect/', - text: 'See the config file documentation', - }, - }); + const configError = configService.getConfigError(); + if (configError) { + switch (configError.source) { + case 'file-loading': { + notificationsService.notifyError({ + title: 'Failed to load config file', + description: `Using the default config instead.\n${configError.error}`, + }); + break; + } + case 'validation': { + const isKeymapError = configError.errors.some(e => + e.path[0].toString().startsWith('keymap.') + ); + notificationsService.notifyError({ + title: 'Encountered errors in config file', + list: configError.errors.map( + e => `${e.path[0].toString()}: ${e.message}` + ), + description: + isKeymapError && + 'A valid shortcut contains at least one modifier and a single key code, for example "Shift+Tab".\nFunction keys do not require a modifier.', + link: { + // TODO(gzdunek): point to the properer section + href: 'https://goteleport.com/docs/connect-your-client/teleport-connect/', + text: 'See the config file documentation', + }, + }); + } + } } } diff --git a/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts b/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts index 502f6429501f3..01c3dc3032c00 100644 --- a/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts +++ b/web/packages/teleterm/src/ui/services/statePersistence/statePersistenceService.ts @@ -106,7 +106,10 @@ export class StatePersistenceService { askedForUserJobRole: false, }, }; - return { ...defaultState, ...this._fileStorage.get('state') }; + return { + ...defaultState, + ...(this._fileStorage.get('state') as StatePersistenceState), + }; } private putState(state: StatePersistenceState): void { diff --git a/web/packages/teleterm/src/ui/services/usage/setUpUsageReporting.ts b/web/packages/teleterm/src/ui/services/usage/setUpUsageReporting.ts index 46f07fdd5f78c..54656f76f8f63 100644 --- a/web/packages/teleterm/src/ui/services/usage/setUpUsageReporting.ts +++ b/web/packages/teleterm/src/ui/services/usage/setUpUsageReporting.ts @@ -29,6 +29,11 @@ export async function setUpUsageReporting( return; } + if (configService.getConfigError()?.source === 'file-loading') { + // do not show the dialog, response cannot be saved to the file + return; + } + if (configService.get('usageReporting.enabled').metadata.isStored) { return; }