From ef4991fc2b1436f7e639b5ebc30015dbfd3d30b6 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 17 Jun 2020 20:04:49 +0200 Subject: [PATCH 1/2] packages/config: added context to keep track of where config is from for error messages --- packages/config/src/reader.test.ts | 130 ++++++++++++++++++++++++----- packages/config/src/reader.ts | 46 +++++++--- packages/config/src/types.ts | 5 +- 3 files changed, 144 insertions(+), 37 deletions(-) diff --git a/packages/config/src/reader.test.ts b/packages/config/src/reader.test.ts index 27dd1b9e3b..365fddd4cf 100644 --- a/packages/config/src/reader.test.ts +++ b/packages/config/src/reader.test.ts @@ -63,37 +63,37 @@ function expectValidValues(config: ConfigReader) { function expectInvalidValues(config: ConfigReader) { expect(() => config.getBoolean('string')).toThrow( - 'Invalid type in config for key string, got string, wanted boolean', + "Invalid type in config for key 'string' in 'ctx', got string, wanted boolean", ); expect(() => config.getNumber('string')).toThrow( - 'Invalid type in config for key string, got string, wanted number', + "Invalid type in config for key 'string' in 'ctx', got string, wanted number", ); expect(() => config.getString('one')).toThrow( - 'Invalid type in config for key one, got number, wanted string', + "Invalid type in config for key 'one' in 'ctx', got number, wanted string", ); expect(() => config.getNumber('true')).toThrow( - 'Invalid type in config for key true, got boolean, wanted number', + "Invalid type in config for key 'true' in 'ctx', got boolean, wanted number", ); expect(() => config.getStringArray('null')).toThrow( - 'Invalid type in config for key null, got null, wanted string-array', + "Invalid type in config for key 'null' in 'ctx', got null, wanted string-array", ); expect(() => config.getString('emptyString')).toThrow( - 'Invalid type in config for key emptyString, got empty-string, wanted string', + "Invalid type in config for key 'emptyString' in 'ctx', got empty-string, wanted string", ); expect(() => config.getStringArray('badStrings')).toThrow( - 'Invalid type in config for key badStrings[1], got empty-string, wanted string', + "Invalid type in config for key 'badStrings[1]' in 'ctx', got empty-string, wanted string", ); expect(() => config.getStringArray('worseStrings')).toThrow( - 'Invalid type in config for key worseStrings[1], got number, wanted string', + "Invalid type in config for key 'worseStrings[1]' in 'ctx', got number, wanted string", ); expect(() => config.getStringArray('worstStrings')).toThrow( - 'Invalid type in config for key worstStrings[2], got object, wanted string', + "Invalid type in config for key 'worstStrings[2]' in 'ctx', got object, wanted string", ); expect(() => config.getConfig('one')).toThrow( - 'Invalid type in config for key one, got number, wanted object', + "Invalid type in config for key 'one' in 'ctx', got number, wanted object", ); expect(() => config.getConfigArray('one')).toThrow( - 'Invalid type in config for key one, got number, wanted object-array', + "Invalid type in config for key 'one' in 'ctx', got number, wanted object-array", ); expect(() => config.getBoolean('missing')).toThrow( "Missing required config value at 'missing'", @@ -109,9 +109,11 @@ function expectInvalidValues(config: ConfigReader) { ); } +const CTX = 'ctx'; + describe('ConfigReader', () => { it('should read empty config with valid keys', () => { - const config = new ConfigReader({}); + const config = new ConfigReader({}, CTX); expect(config.keys()).toEqual([]); expect(config.getOptionalString('x')).toBeUndefined(); expect(config.getOptionalString('x_x')).toBeUndefined(); @@ -121,11 +123,13 @@ describe('ConfigReader', () => { expect(config.getOptionalString('x0_x0')).toBeUndefined(); expect(config.getOptionalString('x_x-x_x')).toBeUndefined(); - expect(new ConfigReader(undefined).getOptionalString('x')).toBeUndefined(); + expect( + new ConfigReader(undefined, CTX).getOptionalString('x'), + ).toBeUndefined(); }); it('should throw on invalid keys', () => { - const config = new ConfigReader({}); + const config = new ConfigReader({}, CTX); expect(() => config.getString('.')).toThrow(/^Invalid config key/); expect(() => config.getString('0')).toThrow(/^Invalid config key/); @@ -143,38 +147,38 @@ describe('ConfigReader', () => { expect(() => config.getString('a._')).toThrow(/^Invalid config key/); expect(() => config.getString('a.-.a')).toThrow(/^Invalid config key/); - expect(() => new ConfigReader(undefined).getString('.')).toThrow( + expect(() => new ConfigReader(undefined, CTX).getString('.')).toThrow( /^Invalid config key/, ); }); it('should read valid values', () => { - const config = new ConfigReader(DATA); + const config = new ConfigReader(DATA, CTX); expectValidValues(config); }); it('should fail to read invalid values', () => { - const config = new ConfigReader(DATA); + const config = new ConfigReader(DATA, CTX); expectInvalidValues(config); }); }); describe('ConfigReader with fallback', () => { it('should behave as if without fallback', () => { - const config = new ConfigReader({}, new ConfigReader(DATA)); + const config = new ConfigReader({}, CTX, new ConfigReader(DATA, CTX)); expect(config.getOptionalString('x')).toBeUndefined(); expect(() => config.getString('.')).toThrow(/^Invalid config key/); expect(() => config.getString('a.')).toThrow(/^Invalid config key/); }); it('should read values from itself', () => { - const config = new ConfigReader(DATA, new ConfigReader({})); + const config = new ConfigReader(DATA, CTX, new ConfigReader({}, CTX)); expectValidValues(config); expectInvalidValues(config); }); it('should read values from a fallback', () => { - const config = new ConfigReader({}, new ConfigReader(DATA)); + const config = new ConfigReader({}, CTX, new ConfigReader(DATA, CTX)); expectValidValues(config); expectInvalidValues(config); }); @@ -182,12 +186,92 @@ describe('ConfigReader with fallback', () => { it('should read values from multiple levels of fallbacks', () => { const config = new ConfigReader( {}, - new ConfigReader({}, new ConfigReader({}, new ConfigReader(DATA))), + CTX, + new ConfigReader( + {}, + CTX, + new ConfigReader({}, CTX, new ConfigReader(DATA, CTX)), + ), ); expectValidValues(config); expectInvalidValues(config); }); + it('should show error with correct context', () => { + const config = ConfigReader.fromConfigs([ + { + data: { + c: true, + }, + context: 'x', + }, + { + data: { + b: true, + c: true, + nested1: { + a: true, + }, + badBefore: true, + badAfter: { + a: true, + }, + }, + context: 'y', + }, + { + data: { + a: true, + b: true, + c: true, + nested1: { + a: true, + b: true, + }, + badBefore: { + a: true, + }, + badAfter: true, + }, + context: 'z', + }, + ]); + + expect(() => config.getNumber('a')).toThrow( + "Invalid type in config for key 'a' in 'z', got boolean, wanted number", + ); + expect(() => config.getNumber('b')).toThrow( + "Invalid type in config for key 'b' in 'y', got boolean, wanted number", + ); + expect(() => config.getNumber('c')).toThrow( + "Invalid type in config for key 'c' in 'x', got boolean, wanted number", + ); + expect(() => config.getNumber('nested1.a')).toThrow( + "Invalid type in config for key 'nested1.a' in 'y', got boolean, wanted number", + ); + expect(() => config.getNumber('nested1.b')).toThrow( + "Invalid type in config for key 'nested1.b' in 'z', got boolean, wanted number", + ); + expect(() => config.getConfig('nested1').getNumber('a')).toThrow( + "Invalid type in config for key 'nested1.a' in 'y', got boolean, wanted number", + ); + expect(() => config.getConfig('nested1').getNumber('b')).toThrow( + "Invalid type in config for key 'nested1.b' in 'z', got boolean, wanted number", + ); + expect(() => config.getNumber('badBefore.a')).toThrow( + "Invalid type in config for key 'badBefore' in 'y', got boolean, wanted object", + ); + expect(() => config.getNumber('badBefore.b')).toThrow( + "Invalid type in config for key 'badBefore' in 'y', got boolean, wanted object", + ); + expect(() => config.getNumber('badAfter.a')).toThrow( + "Invalid type in config for key 'badAfter.a' in 'y', got boolean, wanted number", + ); + expect(() => config.getNumber('badAfter.b')).toThrow( + "Invalid type in config for key 'badAfter' in 'z', got boolean, wanted object", + ); + }); + it('should read merged objects', () => { const a = { merged: { @@ -208,7 +292,7 @@ describe('ConfigReader with fallback', () => { }, }; - const config = new ConfigReader(a, new ConfigReader(b)); + const config = new ConfigReader(a, CTX, new ConfigReader(b, CTX)); expect(config.keys()).toEqual(['merged']); expect(config.getConfig('merged').keys()).toEqual([ @@ -245,7 +329,7 @@ describe('ConfigReader with fallback', () => { 'b', ]); expect(() => config.getConfig('merged').getStringArray('x')).toThrow( - 'Invalid type in config for key merged.x, got string, wanted string-array', + "Invalid type in config for key 'merged.x' in 'ctx', got string, wanted string-array", ); // Config arrays aren't merged either diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index e992b7caac..e4c4c4d304 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -41,8 +41,8 @@ function typeOf(value: JsonValue | undefined): string { // Separate out a couple of common error messages to reduce bundle size. const errors = { - type(key: string, typeName: string, expected: string) { - return `Invalid type in config for key ${key}, got ${typeName}, wanted ${expected}`; + type(key: string, context: string, typeName: string, expected: string) { + return `Invalid type in config for key '${key}' in '${context}', got ${typeName}, wanted ${expected}`; }, missing(key: string) { return `Missing required config value at '${key}'`; @@ -57,13 +57,17 @@ export class ConfigReader implements Config { // Merge together all configs info a single config with recursive fallback // readers, giving the first config object in the array the highest priority. - return configs.reduceRight((previousReader, nextConfig) => { - return new ConfigReader(nextConfig, previousReader); - }, undefined!); + return configs.reduceRight( + (previousReader, { data, context }) => { + return new ConfigReader(data, context, previousReader); + }, + undefined!, + ); } constructor( private readonly data: JsonObject | undefined, + private readonly context: string = 'empty-config', private readonly fallback?: ConfigReader, private readonly prefix: string = '', ) {} @@ -80,14 +84,17 @@ export class ConfigReader implements Config { const prefix = this.fullKey(key); if (isObject(value)) { - return new ConfigReader(value, fallbackConfig, prefix); + return new ConfigReader(value, this.context, fallbackConfig, prefix); } if (value !== undefined) { throw new TypeError( - errors.type(this.fullKey(key), typeOf(value), 'object'), + errors.type(this.fullKey(key), this.context, typeOf(value), 'object'), ); } - return fallbackConfig ?? new ConfigReader(undefined, undefined, prefix); + return ( + fallbackConfig ?? + new ConfigReader(undefined, undefined, undefined, prefix) + ); } getConfigArray(key: string): ConfigReader[] { @@ -106,7 +113,12 @@ export class ConfigReader implements Config { return (configs ?? []).map( (obj, index) => - new ConfigReader(obj, undefined, this.fullKey(`${key}[${index}]`)), + new ConfigReader( + obj, + this.context, + undefined, + this.fullKey(`${key}[${index}]`), + ), ); } @@ -202,7 +214,12 @@ export class ConfigReader implements Config { expected, } = result; throw new TypeError( - errors.type(this.fullKey(keyName), typeOf(theValue), expected), + errors.type( + this.fullKey(keyName), + this.context, + typeOf(theValue), + expected, + ), ); } } @@ -223,11 +240,14 @@ export class ConfigReader implements Config { } let value: JsonValue | undefined = this.data; - for (const part of parts) { + for (const [index, part] of parts.entries()) { if (isObject(value)) { value = value[part]; - } else { - value = undefined; + } else if (value !== undefined) { + const badKey = this.fullKey(parts.slice(0, index).join('.')); + throw new TypeError( + errors.type(badKey, this.context, typeOf(value), 'object'), + ); } } diff --git a/packages/config/src/types.ts b/packages/config/src/types.ts index 4f8a12b5bd..9c49a45af4 100644 --- a/packages/config/src/types.ts +++ b/packages/config/src/types.ts @@ -24,7 +24,10 @@ export type JsonValue = | boolean | null; -export type AppConfig = JsonObject; +export type AppConfig = { + context: string; + data: JsonObject; +}; export type Config = { keys(): string[]; From b010319400ed10a32e09d8ce58420025433f3fc5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 18 Jun 2020 09:45:33 +0200 Subject: [PATCH 2/2] packages/config-loader,core: update config usage to include context --- packages/config-loader/src/lib/env.test.ts | 9 ++++--- packages/config-loader/src/lib/env.ts | 6 ++--- packages/config-loader/src/lib/reader.test.ts | 16 +++++++++---- packages/config-loader/src/lib/reader.ts | 10 +++++--- .../core/src/api-wrappers/createApp.test.tsx | 24 +++++++++++++------ packages/core/src/api-wrappers/createApp.tsx | 5 ++-- 6 files changed, 47 insertions(+), 23 deletions(-) diff --git a/packages/config-loader/src/lib/env.test.ts b/packages/config-loader/src/lib/env.test.ts index 6a0a115b24..9ecc80d29a 100644 --- a/packages/config-loader/src/lib/env.test.ts +++ b/packages/config-loader/src/lib/env.test.ts @@ -45,9 +45,12 @@ describe('readEnv', () => { }), ).toEqual([ { - foo: 'bar', - numbers: { a: 1, b: 2, c: false }, - very: { deep: { nested: { config: { object: {} } } } }, + data: { + foo: 'bar', + numbers: { a: 1, b: 2, c: false }, + very: { deep: { nested: { config: { object: {} } } } }, + }, + context: 'env', }, ]); }); diff --git a/packages/config-loader/src/lib/env.ts b/packages/config-loader/src/lib/env.ts index 83f86d4212..aa986931ff 100644 --- a/packages/config-loader/src/lib/env.ts +++ b/packages/config-loader/src/lib/env.ts @@ -42,7 +42,7 @@ const CONFIG_KEY_PART_PATTERN = /^[a-z][a-z0-9]*(?:[-_][a-z][a-z0-9]*)*$/i; export function readEnv(env: { [name: string]: string | undefined; }): AppConfig[] { - let config: JsonObject | undefined = undefined; + let data: JsonObject | undefined = undefined; for (const [name, value] of Object.entries(env)) { if (!value) { @@ -52,7 +52,7 @@ export function readEnv(env: { const key = name.replace(ENV_PREFIX, ''); const keyParts = key.split('_'); - let obj = (config = config ?? {}); + let obj = (data = data ?? {}); for (const [index, part] of keyParts.entries()) { if (!CONFIG_KEY_PART_PATTERN.test(part)) { throw new TypeError(`Invalid env config key '${key}'`); @@ -87,5 +87,5 @@ export function readEnv(env: { } } - return config ? [config] : []; + return data ? [{ data, context: 'env' }] : []; } diff --git a/packages/config-loader/src/lib/reader.test.ts b/packages/config-loader/src/lib/reader.test.ts index b90c53ee6b..719939ad7f 100644 --- a/packages/config-loader/src/lib/reader.test.ts +++ b/packages/config-loader/src/lib/reader.test.ts @@ -38,11 +38,14 @@ describe('readConfigFile', () => { } as ReaderContext); await expect(config).resolves.toEqual({ - app: { - title: 'Test', - x: 1, - y: [true], + data: { + app: { + title: 'Test', + x: 1, + y: [true], + }, }, + context: 'app-config.yaml', }); }); @@ -83,7 +86,10 @@ describe('readConfigFile', () => { }); await expect(config).resolves.toEqual({ - app: 'secret', + data: { + app: 'secret', + }, + context: 'app-config.yaml', }); expect(readSecret).toHaveBeenCalledWith({ file: './my-secret' }); }); diff --git a/packages/config-loader/src/lib/reader.ts b/packages/config-loader/src/lib/reader.ts index 044bf68288..4efaf1986a 100644 --- a/packages/config-loader/src/lib/reader.ts +++ b/packages/config-loader/src/lib/reader.ts @@ -15,15 +15,19 @@ */ import yaml from 'yaml'; +import { basename } from 'path'; import { isObject } from './utils'; -import { JsonValue, JsonObject } from '@backstage/config'; +import { JsonValue, JsonObject, AppConfig } from '@backstage/config'; import { ReaderContext } from './types'; /** * Reads and parses, and validates, and transforms a single config file. * The transformation rewrites any special values, like the $secret key. */ -export async function readConfigFile(filePath: string, ctx: ReaderContext) { +export async function readConfigFile( + filePath: string, + ctx: ReaderContext, +): Promise { const configYaml = await ctx.readFile(filePath); const config = yaml.parse(configYaml); @@ -76,5 +80,5 @@ export async function readConfigFile(filePath: string, ctx: ReaderContext) { if (!isObject(finalConfig)) { throw new TypeError('Expected object at config root'); } - return finalConfig; + return { data: finalConfig, context: basename(filePath) }; } diff --git a/packages/core/src/api-wrappers/createApp.test.tsx b/packages/core/src/api-wrappers/createApp.test.tsx index 30553d84a7..0a3756ad92 100644 --- a/packages/core/src/api-wrappers/createApp.test.tsx +++ b/packages/core/src/api-wrappers/createApp.test.tsx @@ -15,6 +15,7 @@ */ import { defaultConfigLoader } from './createApp'; +import { AppConfig } from '@backstage/config'; describe('defaultConfigLoader', () => { afterEach(() => { @@ -24,24 +25,33 @@ describe('defaultConfigLoader', () => { it('loads static config', async () => { Object.defineProperty(process.env, 'APP_CONFIG', { configurable: true, - value: [{ my: 'config' }, { my: 'override-config' }] as any, + value: [ + { data: { my: 'config' }, context: 'a' }, + { data: { my: 'override-config' }, context: 'b' }, + ] as AppConfig[], }); const configs = await defaultConfigLoader(); - expect(configs).toEqual([{ my: 'config' }, { my: 'override-config' }]); + expect(configs).toEqual([ + { data: { my: 'config' }, context: 'a' }, + { data: { my: 'override-config' }, context: 'b' }, + ]); }); it('loads runtime config', async () => { Object.defineProperty(process.env, 'APP_CONFIG', { configurable: true, - value: [{ my: 'override-config' }, { my: 'config' }] as any, + value: [ + { data: { my: 'override-config' }, context: 'a' }, + { data: { my: 'config' }, context: 'b' }, + ] as AppConfig[], }); const configs = await (defaultConfigLoader as any)( '{"my":"runtime-config"}', ); expect(configs).toEqual([ - { my: 'runtime-config' }, - { my: 'override-config' }, - { my: 'config' }, + { data: { my: 'runtime-config' }, context: 'env' }, + { data: { my: 'override-config' }, context: 'a' }, + { data: { my: 'config' }, context: 'b' }, ]); }); @@ -64,7 +74,7 @@ describe('defaultConfigLoader', () => { it('fails to load bad runtime config', async () => { Object.defineProperty(process.env, 'APP_CONFIG', { configurable: true, - value: [{ my: 'config' }] as any, + value: [{ data: { my: 'config' }, context: 'a' }] as AppConfig[], }); await expect((defaultConfigLoader as any)('}')).rejects.toThrow( diff --git a/packages/core/src/api-wrappers/createApp.tsx b/packages/core/src/api-wrappers/createApp.tsx index f69451249b..c13758a0b9 100644 --- a/packages/core/src/api-wrappers/createApp.tsx +++ b/packages/core/src/api-wrappers/createApp.tsx @@ -27,7 +27,7 @@ import { BrowserRouter, MemoryRouter } from 'react-router-dom'; import { ErrorPage } from '../layout/ErrorPage'; import Progress from '../components/Progress'; import { lightTheme, darkTheme } from '@backstage/theme'; -import { AppConfig } from '@backstage/config'; +import { AppConfig, JsonObject } from '@backstage/config'; const { PrivateAppImpl } = privateExports; @@ -59,7 +59,8 @@ export const defaultConfigLoader: AppConfigLoader = async ( // Avoiding this string also being replaced at runtime if (runtimeConfigJson !== '__app_injected_runtime_config__'.toUpperCase()) { try { - configs.unshift(JSON.parse(runtimeConfigJson)); + const data = JSON.parse(runtimeConfigJson) as JsonObject; + configs.unshift({ data, context: 'env' }); } catch (error) { throw new Error(`Failed to load runtime configuration, ${error}`); }