From d9b951bfcbf76416e3c99c258b0a4f3c41d8f371 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Tue, 11 Jan 2022 13:12:55 -0600 Subject: [PATCH] Update base config logging, removal, and tests --- app/base_config.ts | 72 +++++-- app/ephemeral_config.ts | 8 +- app/user_config.ts | 8 +- ts/test-node/app/base_config_test.ts | 309 ++++++++++++++++++++++----- 4 files changed, 322 insertions(+), 75 deletions(-) diff --git a/app/base_config.ts b/app/base_config.ts index d3056e2db..1a6f2506c 100644 --- a/app/base_config.ts +++ b/app/base_config.ts @@ -1,9 +1,11 @@ -// Copyright 2018-2020 Signal Messenger, LLC +// Copyright 2018-2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only import { readFileSync, writeFileSync, unlinkSync } from 'fs'; -import { get, set } from 'lodash'; +import { get } from 'lodash'; +import { set } from 'lodash/fp'; +import { strictAssert } from '../ts/util/assert'; const ENCODING = 'utf8'; @@ -18,12 +20,16 @@ export type ConfigType = { _getCachedValue: () => InternalConfigType | undefined; }; -export function start( - name: string, - targetPath: string, - options?: { allowMalformedOnStartup?: boolean } -): ConfigType { - let cachedValue: InternalConfigType | undefined; +export function start({ + name, + targetPath, + throwOnFilesystemErrors, +}: Readonly<{ + name: string; + targetPath: string; + throwOnFilesystemErrors: boolean; +}>): ConfigType { + let cachedValue: InternalConfigType = Object.create(null); let incomingJson: string | undefined; try { @@ -33,22 +39,22 @@ export function start( if (!cachedValue) { console.log( - `config/get: ${name} config value was falsy, cache is now empty object` + `config/start: ${name} config value was falsy, cache is now empty object` ); cachedValue = Object.create(null); } } catch (error) { - if (!options?.allowMalformedOnStartup && error.code !== 'ENOENT') { + if (throwOnFilesystemErrors && error.code !== 'ENOENT') { throw error; } if (incomingJson) { console.log( - `config/get: ${name} config file was malformed, starting afresh` + `config/start: ${name} config file was malformed, starting afresh` ); } else { console.log( - `config/get: Did not find ${name} config file (or it was empty), cache is now empty object` + `config/start: Did not find ${name} config file (or it was empty), cache is now empty object` ); } cachedValue = Object.create(null); @@ -59,19 +65,47 @@ export function start( } function ourSet(keyPath: string, value: unknown): void { - if (!cachedValue) { - throw new Error('ourSet: no cachedValue!'); - } + const newCachedValue = set(keyPath, value, cachedValue); - set(cachedValue, keyPath, value); console.log(`config/set: Saving ${name} config to disk`); - const outgoingJson = JSON.stringify(cachedValue, null, ' '); - writeFileSync(targetPath, outgoingJson, ENCODING); + + if (!throwOnFilesystemErrors) { + cachedValue = newCachedValue; + } + const outgoingJson = JSON.stringify(newCachedValue, null, ' '); + try { + writeFileSync(targetPath, outgoingJson, ENCODING); + console.log(`config/set: Saved ${name} config to disk`); + cachedValue = newCachedValue; + } catch (err: unknown) { + if (throwOnFilesystemErrors) { + throw err; + } else { + console.warn( + `config/set: Failed to save ${name} config to disk; only updating in-memory data` + ); + } + } } function remove(): void { console.log(`config/remove: Deleting ${name} config from disk`); - unlinkSync(targetPath); + try { + unlinkSync(targetPath); + console.log(`config/remove: Deleted ${name} config from disk`); + } catch (err: unknown) { + const errCode: unknown = get(err, 'code'); + if (throwOnFilesystemErrors) { + strictAssert(errCode === 'ENOENT', 'Expected deletion of no file'); + console.log(`config/remove: No ${name} config on disk, did nothing`); + } else { + console.warn( + `config/remove: Got ${String( + errCode + )} when removing ${name} config from disk` + ); + } + } cachedValue = Object.create(null); } diff --git a/app/ephemeral_config.ts b/app/ephemeral_config.ts index c207d2b4a..3857151b3 100644 --- a/app/ephemeral_config.ts +++ b/app/ephemeral_config.ts @@ -1,4 +1,4 @@ -// Copyright 2018-2021 Signal Messenger, LLC +// Copyright 2018-2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only import { join } from 'path'; @@ -10,8 +10,10 @@ import { start } from './base_config'; const userDataPath = app.getPath('userData'); const targetPath = join(userDataPath, 'ephemeral.json'); -export const ephemeralConfig = start('ephemeral', targetPath, { - allowMalformedOnStartup: true, +export const ephemeralConfig = start({ + name: 'ephemeral', + targetPath, + throwOnFilesystemErrors: false, }); export const get = ephemeralConfig.get.bind(ephemeralConfig); diff --git a/app/user_config.ts b/app/user_config.ts index 8b8f5124c..2e532a5be 100644 --- a/app/user_config.ts +++ b/app/user_config.ts @@ -1,4 +1,4 @@ -// Copyright 2017-2020 Signal Messenger, LLC +// Copyright 2017-2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only import { join } from 'path'; @@ -34,7 +34,11 @@ console.log(`userData: ${app.getPath('userData')}`); const userDataPath = app.getPath('userData'); const targetPath = join(userDataPath, 'config.json'); -export const userConfig = start('user', targetPath); +export const userConfig = start({ + name: 'user', + targetPath, + throwOnFilesystemErrors: true, +}); export const get = userConfig.get.bind(userConfig); export const remove = userConfig.remove.bind(userConfig); diff --git a/ts/test-node/app/base_config_test.ts b/ts/test-node/app/base_config_test.ts index 8a88ee7fc..35cb479a4 100644 --- a/ts/test-node/app/base_config_test.ts +++ b/ts/test-node/app/base_config_test.ts @@ -1,71 +1,278 @@ -// Copyright 2021 Signal Messenger, LLC +// Copyright 2021-2022 Signal Messenger, LLC // SPDX-License-Identifier: AGPL-3.0-only +import * as path from 'path'; import { tmpdir } from 'os'; -import { writeFileSync, unlinkSync } from 'fs'; +import { chmodSync, mkdirSync, unlinkSync, writeFileSync } from 'fs'; +import { pathExists, readJsonSync } from 'fs-extra'; import { v4 as generateGuid } from 'uuid'; import { assert } from 'chai'; +import type { ConfigType } from '../../../app/base_config'; import { start } from '../../../app/base_config'; describe('base_config', () => { - let targetFile: string | undefined; + let targetPath: string; - function getNewPath() { - return `${tmpdir()}/${generateGuid()}.txt`; - } + beforeEach(() => { + targetPath = path.join(tmpdir(), `${generateGuid()}.json`); + }); afterEach(() => { - if (targetFile) { - unlinkSync(targetFile); + try { + unlinkSync(targetPath); + } catch (err) { + assert.strictEqual(err.code, 'ENOENT'); } }); - it('does not throw if file is missing', () => { - const missingFile = getNewPath(); - const { _getCachedValue } = start('test', missingFile); - - assert.deepEqual(_getCachedValue(), Object.create(null)); - }); - - it('successfully loads config file', () => { - targetFile = getNewPath(); - - const config = { a: 1, b: 2 }; - writeFileSync(targetFile, JSON.stringify(config)); - const { _getCachedValue } = start('test', targetFile); - - assert.deepEqual(_getCachedValue(), config); - }); - - it('throws if file is malformed', () => { - targetFile = getNewPath(); - - writeFileSync(targetFile, '{{ malformed JSON'); - - const fileForClosure = targetFile; - assert.throws(() => start('test', fileForClosure)); - }); - - it('does not throw if file is empty', () => { - targetFile = getNewPath(); - - writeFileSync(targetFile, ''); - - const { _getCachedValue } = start('test', targetFile); - - assert.deepEqual(_getCachedValue(), Object.create(null)); - }); - - it('does not throw if file is malformed, with allowMalformedOnStartup', () => { - targetFile = getNewPath(); - - writeFileSync(targetFile, '{{ malformed JSON'); - const { _getCachedValue } = start('test', targetFile, { - allowMalformedOnStartup: true, + describe('start', () => { + it('does not throw if file is missing', () => { + const { _getCachedValue } = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + assert.deepEqual(_getCachedValue(), Object.create(null)); }); - assert.deepEqual(_getCachedValue(), Object.create(null)); + it("doesn't create the file if it is missing", async () => { + start({ name: 'test', targetPath, throwOnFilesystemErrors: true }); + assert.isFalse(await pathExists(targetPath)); + }); + + it('does not throw if file is empty', () => { + writeFileSync(targetPath, ''); + const { _getCachedValue } = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + assert.deepEqual(_getCachedValue(), Object.create(null)); + }); + + it('successfully loads config file', () => { + const config = { a: 1, b: 2 }; + writeFileSync(targetPath, JSON.stringify(config)); + const { _getCachedValue } = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + assert.deepEqual(_getCachedValue(), config); + }); + + describe('throwOnFilesystemErrors: true', () => { + it('throws if file is malformed', () => { + writeFileSync(targetPath, '{{ malformed JSON'); + assert.throws(() => { + start({ name: 'test', targetPath, throwOnFilesystemErrors: true }); + }); + }); + }); + + describe('throwOnFilesystemErrors: false', () => { + it('handles a malformed file, if told to', () => { + writeFileSync(targetPath, '{{ malformed JSON'); + const { _getCachedValue } = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: false, + }); + assert.deepEqual(_getCachedValue(), Object.create(null)); + }); + + it('handles a file that cannot be opened, if told to', function test() { + if (process.platform === 'win32') { + this.skip(); + } + + writeFileSync(targetPath, JSON.stringify({ foo: 123 })); + chmodSync(targetPath, 0); + const { _getCachedValue } = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: false, + }); + assert.deepEqual(_getCachedValue(), Object.create(null)); + }); + }); + }); + + describe('get', () => { + let config: ConfigType; + beforeEach(() => { + writeFileSync(targetPath, JSON.stringify({ foo: 123, bar: [1, 2, 3] })); + config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + }); + + it('returns undefined for missing keys', () => { + assert.isUndefined(config.get('garbage')); + }); + + it('can look up values by path', () => { + assert.strictEqual(config.get('foo'), 123); + assert.strictEqual(config.get('bar.1'), 2); + }); + }); + + describe('set', () => { + it('updates data in memory by path', () => { + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + config.set('foo', 1); + config.set('bar.baz', 2); + + assert.strictEqual(config.get('foo'), 1); + assert.deepStrictEqual(config.get('bar'), { baz: 2 }); + }); + + it('saves data to disk', () => { + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + + config.set('foo', 123); + assert.deepStrictEqual(readJsonSync(targetPath), { foo: 123 }); + + config.set('bar.baz', 2); + assert.deepStrictEqual(readJsonSync(targetPath), { + foo: 123, + bar: { baz: 2 }, + }); + + config.set('foo', undefined); + assert.deepStrictEqual(readJsonSync(targetPath), { bar: { baz: 2 } }); + }); + + describe('throwOnFilesystemErrors: true', () => { + it("doesn't update in-memory data if file write fails", () => { + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + config.set('foo', 123); + chmodSync(targetPath, 0); + + assert.throws(() => config.set('foo', 456)); + assert.strictEqual(config.get('foo'), 123); + + assert.throws(() => config.set('bar', 999)); + assert.isUndefined(config.get('bar')); + }); + }); + + describe('throwOnFilesystemErrors: false', () => { + it('updates in-memory data even if file write fails', () => { + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: false, + }); + config.set('foo', 123); + chmodSync(targetPath, 0); + + config.set('bar', 456); + + assert.strictEqual(config.get('bar'), 456); + }); + }); + }); + + describe('remove', () => { + it('deletes all data from memory', () => { + writeFileSync(targetPath, JSON.stringify({ foo: 123 })); + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + config.remove(); + + assert.isEmpty(config._getCachedValue()); + }); + + it('does nothing if the file never existed', async () => { + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + config.remove(); + + assert.isFalse(await pathExists(targetPath)); + }); + + it('removes the file on disk', async () => { + writeFileSync(targetPath, JSON.stringify({ foo: 123 })); + const config = start({ + name: 'test', + targetPath, + throwOnFilesystemErrors: true, + }); + config.remove(); + + assert.isFalse(await pathExists(targetPath)); + }); + + describe('throwOnFilesystemErrors: true', () => { + it("doesn't update the local cache if file removal fails", async function test() { + if (process.platform === 'win32') { + this.skip(); + } + + // We put the config file in a directory, then remove all permissions from that + // directory. This should prevent removal. + const directory = path.join(tmpdir(), generateGuid()); + const configFile = path.join(directory, 'test_config.json'); + mkdirSync(directory, { recursive: true }); + writeFileSync(configFile, JSON.stringify({ foo: 123 })); + const config = start({ + name: 'test', + targetPath: configFile, + throwOnFilesystemErrors: true, + }); + chmodSync(directory, 0); + + assert.throws(() => config.remove()); + + assert.deepStrictEqual(config._getCachedValue(), { foo: 123 }); + }); + }); + + describe('throwOnFilesystemErrors: false', () => { + it('updates the local cache even if file removal fails', async function test() { + if (process.platform === 'win32') { + this.skip(); + } + + // See above. + const directory = path.join(tmpdir(), generateGuid()); + const configFile = path.join(directory, 'test_config.json'); + mkdirSync(directory, { recursive: true }); + writeFileSync(configFile, JSON.stringify({ foo: 123 })); + const config = start({ + name: 'test', + targetPath: configFile, + throwOnFilesystemErrors: false, + }); + chmodSync(directory, 0); + + config.remove(); + + assert.isEmpty(config._getCachedValue()); + }); + }); }); });