From f7e76f1bd6274d49d4da78905c62c66dfa08701d Mon Sep 17 00:00:00 2001 From: Kristine Robison Date: Thu, 13 Dec 2018 15:47:36 -0800 Subject: [PATCH 1/2] fix(API): fix logger (#919) References: https://github.com/poetapp/node/pull/901#pullrequestreview-184806191 --- src/API/HealthController.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/API/HealthController.ts b/src/API/HealthController.ts index 440a2fd1..51fd6b36 100644 --- a/src/API/HealthController.ts +++ b/src/API/HealthController.ts @@ -117,8 +117,8 @@ export class HealthController { } private async getTransactionRetryInfo(): Promise { - this.logger.child({ message: 'getTransactionRetryInfo' }) - this.logger.trace('retrieving TransactionAnchorRetryInfo') + const logger = this.logger.child({ method: 'getTransactionRetryInfo' }) + logger.trace('retrieving TransactionAnchorRetryInfo') try { const transactionAnchorRetryResults = await this.collection.findOne( { @@ -132,10 +132,10 @@ export class HealthController { }, }, ) || emptyTransactionAnchorRetryInfo - this.logger.trace({ transactionAnchorRetryResults }, 'getTransactionRetryInfo results') + logger.trace({ transactionAnchorRetryResults }, 'getTransactionRetryInfo results') return transactionAnchorRetryResults.transactionAnchorRetryInfo } catch (error) { - this.logger.error({ error }, 'error retrieving TransactionAnchorRetryInfo') + logger.error({ error }, 'error retrieving TransactionAnchorRetryInfo') return [] } } From 3b989a85fc33cbc8bc8e81e782b21c05b8556f72 Mon Sep 17 00:00:00 2001 From: Walter Zalazar Date: Thu, 13 Dec 2018 21:26:54 -0300 Subject: [PATCH 2/2] chore: Refactor Configuration + Unit test (#912) --- src/Configuration.ts | 123 +--------------- ...tion.test.ts => LoadConfiguration.test.ts} | 138 +++++++++++++++++- src/LoadConfiguration.ts | 116 +++++++++++++++ src/app.ts | 3 +- tests/helpers/bitcoin.ts | 3 +- tests/helpers/database.ts | 2 +- tests/unit/index.ts | 2 +- 7 files changed, 257 insertions(+), 130 deletions(-) rename src/{Configuration.test.ts => LoadConfiguration.test.ts} (56%) create mode 100644 src/LoadConfiguration.ts diff --git a/src/Configuration.ts b/src/Configuration.ts index 7b0049d5..1eff8ece 100644 --- a/src/Configuration.ts +++ b/src/Configuration.ts @@ -1,15 +1,3 @@ -/* tslint:disable:no-relative-imports */ -import * as assert from 'assert' -import { readFileSync, existsSync } from 'fs' -import { homedir } from 'os' -import * as path from 'path' -import { keys, pipe } from 'ramda' - -import { createEnvToConfigurationKeyMap } from 'Helpers/Configuration' - -const defaultMongodbUrl = 'mongodb://localhost:27017/poet' - -// Provide default value in defaultConfiguration for any new configuration options export interface Configuration extends LoggingConfiguration, BitcoinRPCConfiguration, ExchangeConfiguration { readonly rabbitmqUrl: string readonly exchangePrefix: string @@ -85,7 +73,7 @@ export interface ExchangeConfiguration { readonly exchangeForkDetected: string } -const defaultConfiguration: Configuration = { +export const DefaultConfiguration: Configuration = { rabbitmqUrl: 'amqp://admin:adminPass@localhost', exchangePrefix: '', mongodbUser: '', @@ -93,7 +81,7 @@ const defaultConfiguration: Configuration = { mongodbHost: 'localhost', mongodbPort: 27017, mongodbDatabase: 'poet', - mongodbUrl: defaultMongodbUrl, + mongodbUrl: 'mongodb://localhost:27017/poet', ipfsUrl: 'http://localhost:5001', ipfsArchiveUrlPrefix: 'https://ipfs.io/ipfs', bitcoinUrl: '127.0.0.1', @@ -151,110 +139,3 @@ const defaultConfiguration: Configuration = { exchangePurgeStaleTransactions: 'BLOCK_WRITER::PURGE_STALE_TRANSACTIONS', exchangeForkDetected: 'FORK_DETECTED', } - -export const configurationPath = () => path.join(homedir(), '/.po.et/configuration.json') - -export const mergeConfigs = (localVars: any = {}) => { - const config = { - ...defaultConfiguration, - ...loadConfigurationFromEnv(localVars), - ...loadConfigurationFromFile(configurationPath()), - } - - // Support setting MONGODB_URL all at once or via separate variables. - // Especially needed for production since the schema is different (mongodb+srv) and - // there's currently no override for that. - if (config.mongodbUrl === defaultMongodbUrl) { - const mongoAuth = config.mongodbUser !== '' ? `${config.mongodbUser}:${config.mongodbPassword}@` : '' - config.mongodbUrl = `mongodb://${mongoAuth}${config.mongodbHost}:${config.mongodbPort}/${config.mongodbDatabase}` - } - - return config -} - -const prependPrefix = (prefix: string, configVars: any) => (acc: any, k: string) => ({ - ...acc, - [k]: `${prefix}.${configVars[k]}`, -}) - -const applyExchangePrefix = (configVars: any) => { - if (configVars.exchangePrefix === '') return configVars - - const exchangeNames = [ - 'exchangeAnchorNextHashRequest', - 'exchangeBatchReaderReadNextDirectoryRequest', - 'exchangeBatchReaderReadNextDirectorySuccess', - 'exchangeBatchWriterCreateNextBatchRequest', - 'exchangeBatchWriterCreateNextBatchSuccess', - 'exchangeNewClaim', - 'exchangeClaimIpfsHash', - 'exchangeIpfsHashTxId', - 'exchangePoetAnchorDownloaded', - 'exchangeClaimsDownloaded', - 'exchangeClaimsNotDownloaded', - 'exchangeStorageWriterStoreNextClaim', - 'exchangeGetHealth', - 'exchangePurgeStaleTransactions', - 'exchangeForkDetected', - ] - - return { - ...configVars, - ...exchangeNames.reduce(prependPrefix(configVars.exchangePrefix, configVars), {}), - } -} - -export const loadConfigurationWithDefaults = (localVars: any = {}) => - pipe( - mergeConfigs, - applyExchangePrefix, - )({ ...process.env, ...localVars }) - -function loadConfigurationFromFile(configPath: string): Configuration | {} { - if (!existsSync(configPath)) return {} - - const configuration = JSON.parse(readFileSync(configPath, 'utf8')) - - if (configuration.poetNetwork) validatePoetNetwork(configuration.poetNetwork) - if (configuration.poetVersion) validatePoetVersion(configuration.poetVersion) - - return configuration -} - -const extractValue = (value: any) => { - const coercedValue = value === 'true' ? true : value === 'false' ? false : value - - return isNaN(coercedValue) - ? coercedValue - : typeof coercedValue === 'boolean' - ? coercedValue - : parseInt(coercedValue, 10) -} - -function loadConfigurationFromEnv(env: any): Partial { - const map = createEnvToConfigurationKeyMap(keys(defaultConfiguration)) - - const configurationFromEnv = Object.entries(env) - .filter(([key, value]) => map[key]) - .reduce( - (previousValue, [key, value]: [string, any]) => ({ - ...previousValue, - [map[key]]: extractValue(value), - }), - {}, - ) - - return configurationFromEnv -} - -function validatePoetVersion(poetVersion: number) { - assert( - Number.isInteger(poetVersion) && 0 <= poetVersion && poetVersion <= 0xFFFF, - 'poetVersion must be an integer between 0 and 65535', - ) -} - -function validatePoetNetwork(poetNetwork: any) { - assert(typeof poetNetwork === 'string', 'Field poetNetwork must be a string') - assert(poetNetwork.length === 4, 'Field poetNetwork must have a length of 4') -} diff --git a/src/Configuration.test.ts b/src/LoadConfiguration.test.ts similarity index 56% rename from src/Configuration.test.ts rename to src/LoadConfiguration.test.ts index 55414122..100e4e54 100644 --- a/src/Configuration.test.ts +++ b/src/LoadConfiguration.test.ts @@ -1,11 +1,18 @@ /* tslint:disable:no-relative-imports */ import { pick } from 'ramda' -import { describe } from 'riteway' -import { loadConfigurationWithDefaults, mergeConfigs } from './Configuration' +import { describe, Try } from 'riteway' +import { + loadConfigurationWithDefaults, + mergeConfigs, + applyExchangePrefix, + extractValue, + validatePoetVersion, + validatePoetNetwork, +} from './LoadConfiguration' const defaultConfig = mergeConfigs() -describe('src/Configuration', async (assert: any) => { +describe('src/LoadConfiguration', async assert => { assert({ given: 'no arguments', should: 'return the default config', @@ -81,7 +88,7 @@ describe('src/Configuration', async (assert: any) => { } }) -describe('loadConfigurationWithDefaults', async (assert: any) => { +describe('loadConfigurationWithDefaults', async assert => { const mongodbOverrides = { API_PORT: '4321', ENABLE_ANCHORING: 'true', @@ -133,7 +140,7 @@ describe('loadConfigurationWithDefaults', async (assert: any) => { } }) -describe('src/Configuration RabbitmqExchangeMessages', async (assert: any) => { +describe('src/LoadConfiguration RabbitmqExchangeMessages', async assert => { { const defaultValues = { exchangeBatchReaderReadNextDirectoryRequest: 'BATCH_READER::READ_NEXT_DIRECTORY_REQUEST', @@ -201,3 +208,124 @@ describe('src/Configuration RabbitmqExchangeMessages', async (assert: any) => { }) } }) + +describe('src/LoadConfiguration applyExchangePrefix', async assert => { + { + const actual = { exchangePrefix: '' } + + const expected = applyExchangePrefix(actual) + + assert({ + given: 'configVars with the property exchangePrefix with empty string', + should: 'return the same object configVars', + actual, + expected, + }) + } + + { + const configVars = { + exchangePrefix: 'prefix', + exchangeAnchorNextHashRequest: 'exchangeAnchorNextHashRequest', + exchangeBatchReaderReadNextDirectoryRequest: 'exchangeBatchReaderReadNextDirectoryRequest', + exchangeBatchReaderReadNextDirectorySuccess: 'exchangeBatchReaderReadNextDirectorySuccess', + exchangeBatchWriterCreateNextBatchRequest: 'exchangeBatchWriterCreateNextBatchRequest', + exchangeBatchWriterCreateNextBatchSuccess: 'exchangeBatchWriterCreateNextBatchSuccess', + exchangeNewClaim: 'exchangeNewClaim', + exchangeClaimIpfsHash: 'exchangeClaimIpfsHash', + exchangeIpfsHashTxId: 'exchangeIpfsHashTxId', + exchangePoetAnchorDownloaded: 'exchangePoetAnchorDownloaded', + exchangeClaimsDownloaded: 'exchangeClaimsDownloaded', + exchangeClaimsNotDownloaded: 'exchangeClaimsNotDownloaded', + exchangeStorageWriterStoreNextClaim: 'exchangeStorageWriterStoreNextClaim', + exchangeGetHealth: 'exchangeGetHealth', + exchangePurgeStaleTransactions: 'exchangePurgeStaleTransactions', + exchangeForkDetected: 'exchangeForkDetected', + } + + const actual = applyExchangePrefix(configVars) + + const expected = { + exchangePrefix: 'prefix', + exchangeAnchorNextHashRequest: 'prefix.exchangeAnchorNextHashRequest', + exchangeBatchReaderReadNextDirectoryRequest: 'prefix.exchangeBatchReaderReadNextDirectoryRequest', + exchangeBatchReaderReadNextDirectorySuccess: 'prefix.exchangeBatchReaderReadNextDirectorySuccess', + exchangeBatchWriterCreateNextBatchRequest: 'prefix.exchangeBatchWriterCreateNextBatchRequest', + exchangeBatchWriterCreateNextBatchSuccess: 'prefix.exchangeBatchWriterCreateNextBatchSuccess', + exchangeNewClaim: 'prefix.exchangeNewClaim', + exchangeClaimIpfsHash: 'prefix.exchangeClaimIpfsHash', + exchangeIpfsHashTxId: 'prefix.exchangeIpfsHashTxId', + exchangePoetAnchorDownloaded: 'prefix.exchangePoetAnchorDownloaded', + exchangeClaimsDownloaded: 'prefix.exchangeClaimsDownloaded', + exchangeClaimsNotDownloaded: 'prefix.exchangeClaimsNotDownloaded', + exchangeStorageWriterStoreNextClaim: 'prefix.exchangeStorageWriterStoreNextClaim', + exchangeGetHealth: 'prefix.exchangeGetHealth', + exchangePurgeStaleTransactions: 'prefix.exchangePurgeStaleTransactions', + exchangeForkDetected: 'prefix.exchangeForkDetected', + } + + assert({ + given: 'configVars with the property exchangePrefix with a custom prefix', + should: 'return the same object configVars with the custom prefix over the name of exchanges', + actual, + expected, + }) + } +}) + +describe('src/LoadConfiguration extractValue', async assert => { + { + const actual = extractValue('false') + const expected = false + + assert({ + given: 'false like as string', + should: 'return false as boolean', + actual, + expected, + }) + } + + { + const actual = extractValue('true') + const expected = true + + assert({ + given: 'true like as string', + should: 'return true as boolean', + actual, + expected, + }) + } +}) + +describe('src/LoadConfiguration validatePoetVersion', async assert => { + { + assert({ + given: 'an invalid Poet version -1', + should: `return the message 'poetVersion must be an integer between 0 and 65535'`, + actual: Try(validatePoetVersion, -1).message, + expected: 'poetVersion must be an integer between 0 and 65535', + }) + } + + { + assert({ + given: 'an invalid Poet version 65536', + should: `return the message 'poetVersion must be an integer between 0 and 65535'`, + actual: Try(validatePoetVersion, 65536).message, + expected: 'poetVersion must be an integer between 0 and 65535', + }) + } +}) + +describe('src/LoadConfiguration validatePoetNetwork', async assert => { + { + assert({ + given: 'a string with more 4 letters as Poet Network', + should: `return the message 'Field poetNetwork must have a length of 4'`, + actual: Try(validatePoetNetwork, '12345').message, + expected: 'Field poetNetwork must have a length of 4', + }) + } +}) diff --git a/src/LoadConfiguration.ts b/src/LoadConfiguration.ts new file mode 100644 index 00000000..5de569d5 --- /dev/null +++ b/src/LoadConfiguration.ts @@ -0,0 +1,116 @@ +/* tslint:disable:no-relative-imports */ +import * as assert from 'assert' +import { readFileSync, existsSync } from 'fs' +import { homedir } from 'os' +import * as path from 'path' +import { keys, pipe } from 'ramda' + +import { DefaultConfiguration, Configuration } from 'Configuration' +import { createEnvToConfigurationKeyMap } from 'Helpers/Configuration' + +export const configurationPath = () => path.join(homedir(), '/.po.et/configuration.json') + +export const mergeConfigs = (localVars: any = {}) => { + const config = { + ...DefaultConfiguration, + ...loadConfigurationFromEnv(localVars), + ...loadConfigurationFromFile(configurationPath()), + } + + // Support setting MONGODB_URL all at once or via separate variables. + // Especially needed for production since the schema is different (mongodb+srv) and + // there's currently no override for that. + if (config.mongodbUrl === DefaultConfiguration.mongodbUrl) { + const mongoAuth = config.mongodbUser !== '' ? `${config.mongodbUser}:${config.mongodbPassword}@` : '' + config.mongodbUrl = `mongodb://${mongoAuth}${config.mongodbHost}:${config.mongodbPort}/${config.mongodbDatabase}` + } + + return config +} + +const prependPrefix = (prefix: string, configVars: any) => (acc: any, k: string) => ({ + ...acc, + [k]: `${prefix}.${configVars[k]}`, +}) + +export const applyExchangePrefix = (configVars: any) => { + if (configVars.exchangePrefix === '') return configVars + + const exchangeNames = [ + 'exchangeAnchorNextHashRequest', + 'exchangeBatchReaderReadNextDirectoryRequest', + 'exchangeBatchReaderReadNextDirectorySuccess', + 'exchangeBatchWriterCreateNextBatchRequest', + 'exchangeBatchWriterCreateNextBatchSuccess', + 'exchangeNewClaim', + 'exchangeClaimIpfsHash', + 'exchangeIpfsHashTxId', + 'exchangePoetAnchorDownloaded', + 'exchangeClaimsDownloaded', + 'exchangeClaimsNotDownloaded', + 'exchangeStorageWriterStoreNextClaim', + 'exchangeGetHealth', + 'exchangePurgeStaleTransactions', + 'exchangeForkDetected', + ] + + return { + ...configVars, + ...exchangeNames.reduce(prependPrefix(configVars.exchangePrefix, configVars), {}), + } +} + +export const loadConfigurationWithDefaults = (localVars: any = {}) => + pipe( + mergeConfigs, + applyExchangePrefix, + )({ ...process.env, ...localVars }) + +const loadConfigurationFromFile = (configPath: string): Configuration | {} => { + if (!existsSync(configPath)) return {} + + const configuration = JSON.parse(readFileSync(configPath, 'utf8')) + + if (configuration.poetNetwork) validatePoetNetwork(configuration.poetNetwork) + if (configuration.poetVersion) validatePoetVersion(configuration.poetVersion) + + return configuration +} + +export const extractValue = (value: any) => { + const coercedValue = value === 'true' ? true : value === 'false' ? false : value + + return isNaN(coercedValue) + ? coercedValue + : typeof coercedValue === 'boolean' + ? coercedValue + : parseInt(coercedValue, 10) +} + +const loadConfigurationFromEnv = (env: any): Partial => { + const map = createEnvToConfigurationKeyMap(keys(DefaultConfiguration)) + + const configurationFromEnv = Object.entries(env) + .filter(([key, value]) => map[key]) + .reduce( + (previousValue, [key, value]: [string, any]) => ({ + ...previousValue, + [map[key]]: extractValue(value), + }), + {}, + ) + + return configurationFromEnv +} + +export const validatePoetVersion = (poetVersion: number) => { + assert( + Number.isInteger(poetVersion) && 0 <= poetVersion && poetVersion <= 0xFFFF, + 'poetVersion must be an integer between 0 and 65535', + ) +} + +export const validatePoetNetwork = (poetNetwork: string) => { + assert(typeof poetNetwork === 'string', 'Field poetNetwork must be a string') + assert(poetNetwork.length === 4, 'Field poetNetwork must have a length of 4') +} diff --git a/src/app.ts b/src/app.ts index b92b6662..cbfc1250 100644 --- a/src/app.ts +++ b/src/app.ts @@ -10,8 +10,9 @@ import { BatchReader } from 'BatchReader/BatchReader' import { BatchWriter } from 'BatchWriter/BatchWriter' import { BlockchainReader } from 'BlockchainReader/BlockchainReader' import { BlockchainWriter } from 'BlockchainWriter/BlockchainWriter' -import { loadConfigurationWithDefaults, Configuration } from 'Configuration' +import { Configuration } from 'Configuration' import { Health } from 'Health/Health' +import { loadConfigurationWithDefaults } from 'LoadConfiguration' import { StorageReader } from 'StorageReader/StorageReader' import { StorageWriter } from 'StorageWriter/StorageWriter' import { View } from 'View/View' diff --git a/tests/helpers/bitcoin.ts b/tests/helpers/bitcoin.ts index 05f3bf5b..56d65ef0 100644 --- a/tests/helpers/bitcoin.ts +++ b/tests/helpers/bitcoin.ts @@ -1,8 +1,9 @@ /* tslint:disable:no-relative-imports */ import BitcoinCore = require('bitcoin-core') const Client = require('bitcoin-core') -import { LoggingConfiguration, Configuration, loadConfigurationWithDefaults } from 'Configuration' +import { LoggingConfiguration } from 'Configuration' import { createModuleLogger } from 'Helpers/Logging' +import { loadConfigurationWithDefaults } from 'LoadConfiguration' import * as Pino from 'pino' import {delay, delayInSeconds} from '../helpers/utils' diff --git a/tests/helpers/database.ts b/tests/helpers/database.ts index 419d8595..5e0d12f4 100644 --- a/tests/helpers/database.ts +++ b/tests/helpers/database.ts @@ -3,7 +3,7 @@ import { MongoClient } from 'mongodb' import { pipeP } from 'ramda' -import { loadConfigurationWithDefaults } from '../../src/Configuration' +import { loadConfigurationWithDefaults } from '../../src/LoadConfiguration' import { delay, runtimeId } from './utils' export const dbHelper = () => { diff --git a/tests/unit/index.ts b/tests/unit/index.ts index ba268214..de4c468f 100644 --- a/tests/unit/index.ts +++ b/tests/unit/index.ts @@ -8,7 +8,6 @@ import '../../src/BlockchainReader/Bitcoin.test' import '../../src/BlockchainWriter/Bitcoin.test' import '../../src/BlockchainWriter/Controller.test' import '../../src/BlockchainWriter/Exceptions.test' -import '../../src/Configuration.test' import '../../src/Exceptions.test' import '../../src/Extensions/Array.test' import '../../src/Extensions/Error.test' @@ -24,6 +23,7 @@ import '../../src/Helpers/isNotNil.test' import '../../src/Helpers/isTruthy.test' import '../../src/Helpers/to-promise.test' import '../../src/Interfaces.test' +import '../../src/LoadConfiguration.test' import '../../src/Messaging/Messages.test' import '../../src/StorageWriter/DAOClaims.test' import '../../src/StorageWriter/Exceptions.test'