diff --git a/.changeset/curvy-poets-dance.md b/.changeset/curvy-poets-dance.md new file mode 100644 index 0000000000..abf478a729 --- /dev/null +++ b/.changeset/curvy-poets-dance.md @@ -0,0 +1,5 @@ +--- +'@backstage/cli': patch +--- + +Keep track of filtered configuration values when running frontend in development mode. diff --git a/.changeset/large-flies-bow.md b/.changeset/large-flies-bow.md new file mode 100644 index 0000000000..e571bc9a3c --- /dev/null +++ b/.changeset/large-flies-bow.md @@ -0,0 +1,5 @@ +--- +'@backstage/config': patch +--- + +Add warning when trying to access configuration values that have been filtered out by visibility. diff --git a/.changeset/sharp-seals-explode.md b/.changeset/sharp-seals-explode.md new file mode 100644 index 0000000000..7dbcac7094 --- /dev/null +++ b/.changeset/sharp-seals-explode.md @@ -0,0 +1,5 @@ +--- +'@backstage/config-loader': patch +--- + +Add option to populate the `filteredKeys` property when processing configuration with a schema. diff --git a/packages/cli/src/commands/app/serve.ts b/packages/cli/src/commands/app/serve.ts index 85fbdd97a0..208042c719 100644 --- a/packages/cli/src/commands/app/serve.ts +++ b/packages/cli/src/commands/app/serve.ts @@ -59,6 +59,7 @@ export default async (cmd: Command) => { ...(await loadCliConfig({ args: cmd.config, fromPackage: name, + withFilteredKeys: true, })), }); diff --git a/packages/cli/src/commands/plugin/serve.ts b/packages/cli/src/commands/plugin/serve.ts index b9ba93bc62..a0200b7ce8 100644 --- a/packages/cli/src/commands/plugin/serve.ts +++ b/packages/cli/src/commands/plugin/serve.ts @@ -28,6 +28,7 @@ export default async (cmd: Command) => { ...(await loadCliConfig({ args: cmd.config, fromPackage: name, + withFilteredKeys: true, })), }); diff --git a/packages/cli/src/lib/config.ts b/packages/cli/src/lib/config.ts index db96ca7686..a93f040122 100644 --- a/packages/cli/src/lib/config.ts +++ b/packages/cli/src/lib/config.ts @@ -22,6 +22,7 @@ type Options = { args: string[]; fromPackage?: string; mockEnv?: boolean; + withFilteredKeys?: boolean; }; export async function loadCliConfig(options: Options) { @@ -57,6 +58,7 @@ export async function loadCliConfig(options: Options) { try { const frontendAppConfigs = schema.process(appConfigs, { visibility: ['frontend'], + withFilteredKeys: options.withFilteredKeys, }); const frontendConfig = ConfigReader.fromConfigs(frontendAppConfigs); diff --git a/packages/config-loader/api-report.md b/packages/config-loader/api-report.md index 37b9880448..47bc22c60f 100644 --- a/packages/config-loader/api-report.md +++ b/packages/config-loader/api-report.md @@ -59,7 +59,7 @@ export function readEnvConfig(env: { // Warnings were encountered during analysis: // -// src/lib/schema/types.d.ts:77:5 - (ae-forgotten-export) The symbol "ConfigProcessingOptions" needs to be exported by the entry point index.d.ts +// src/lib/schema/types.d.ts:83:5 - (ae-forgotten-export) The symbol "ConfigProcessingOptions" needs to be exported by the entry point index.d.ts // src/loader.d.ts:13:5 - (ae-forgotten-export) The symbol "EnvFunc" needs to be exported by the entry point index.d.ts // (No @packageDocumentation comment for this package) diff --git a/packages/config-loader/src/lib/schema/filtering.test.ts b/packages/config-loader/src/lib/schema/filtering.test.ts index 5079afa876..d4b3cd55b6 100644 --- a/packages/config-loader/src/lib/schema/filtering.test.ts +++ b/packages/config-loader/src/lib/schema/filtering.test.ts @@ -63,43 +63,115 @@ const visibility = new Map( describe('filterByVisibility', () => { test.each<[ConfigVisibility[], JsonObject]>([ - [[], {}], + [ + [], + { + data: {}, + filteredKeys: [ + 'arr[0]', + 'arr[1]', + 'arr[2]', + 'objArr[0].f', + 'objArr[0].b', + 'objArr[0].s', + 'objArr[1].f', + 'objArr[1].b', + 'objArr[1].s', + 'obj.f', + 'obj.b.s', + 'arrF[0].never', + 'arrB[0].never', + 'arrS[0].never', + 'objF.never', + 'objB.never', + 'objS.never', + ], + }, + ], [ ['frontend'], { - arr: ['f'], - objArr: [{ f: 1 }, { f: 4 }], - obj: { f: 'a' }, - arrF: [], - objF: {}, + data: { + arr: ['f'], + objArr: [{ f: 1 }, { f: 4 }], + obj: { f: 'a' }, + arrF: [], + objF: {}, + }, + filteredKeys: [ + 'arr[1]', + 'arr[2]', + 'objArr[0].b', + 'objArr[0].s', + 'objArr[1].b', + 'objArr[1].s', + 'obj.b.s', + 'arrF[0].never', + 'arrB[0].never', + 'arrS[0].never', + 'objF.never', + 'objB.never', + 'objS.never', + ], }, ], [ ['backend'], { - arr: ['b'], - objArr: [{ b: 2 }, { b: 5 }], - obj: { b: {} }, - arrF: [{ never: 'here' }], - arrB: [{ never: 'here' }], - arrS: [{ never: 'here' }], - objF: { never: 'here' }, - objB: { never: 'here' }, - objS: { never: 'here' }, + data: { + arr: ['b'], + objArr: [{ b: 2 }, { b: 5 }], + obj: { b: {} }, + arrF: [{ never: 'here' }], + arrB: [{ never: 'here' }], + arrS: [{ never: 'here' }], + objF: { never: 'here' }, + objB: { never: 'here' }, + objS: { never: 'here' }, + }, + filteredKeys: [ + 'arr[0]', + 'arr[2]', + 'objArr[0].f', + 'objArr[0].s', + 'objArr[1].f', + 'objArr[1].s', + 'obj.f', + 'obj.b.s', + ], }, ], [ ['secret'], { - arr: ['s'], - objArr: [{ s: 3 }, { s: 6 }], - obj: { b: { s: true } }, - arrS: [], - objS: {}, + data: { + arr: ['s'], + objArr: [{ s: 3 }, { s: 6 }], + obj: { b: { s: true } }, + arrS: [], + objS: {}, + }, + filteredKeys: [ + 'arr[0]', + 'arr[1]', + 'objArr[0].f', + 'objArr[0].b', + 'objArr[1].f', + 'objArr[1].b', + 'obj.f', + 'arrF[0].never', + 'arrB[0].never', + 'arrS[0].never', + 'objF.never', + 'objB.never', + 'objS.never', + ], }, ], - [['frontend', 'backend', 'secret'], data], + [['frontend', 'backend', 'secret'], { data, filteredKeys: [] }], ])('should filter correctly with %p', (filter, expected) => { - expect(filterByVisibility(data, filter, visibility)).toEqual(expected); + expect( + filterByVisibility(data, filter, visibility, undefined, true), + ).toEqual(expected); }); }); diff --git a/packages/config-loader/src/lib/schema/filtering.ts b/packages/config-loader/src/lib/schema/filtering.ts index 74d367a30a..3533be01b7 100644 --- a/packages/config-loader/src/lib/schema/filtering.ts +++ b/packages/config-loader/src/lib/schema/filtering.ts @@ -30,9 +30,17 @@ export function filterByVisibility( includeVisibilities: ConfigVisibility[], visibilityByPath: Map, transformFunc?: TransformFunc, -): JsonObject { - function transform(jsonVal: JsonValue, path: string): JsonValue | undefined { - const visibility = visibilityByPath.get(path) ?? DEFAULT_CONFIG_VISIBILITY; + withFilteredKeys?: boolean, +): { data: JsonObject; filteredKeys?: string[] } { + const filteredKeys = new Array(); + + function transform( + jsonVal: JsonValue, + visibilityPath: string, // Matches the format we get from ajv + filterPath: string, // Matches the format of the ConfigReader + ): JsonValue | undefined { + const visibility = + visibilityByPath.get(visibilityPath) ?? DEFAULT_CONFIG_VISIBILITY; const isVisible = includeVisibilities.includes(visibility); if (typeof jsonVal !== 'object') { @@ -42,6 +50,9 @@ export function filterByVisibility( } return jsonVal; } + if (withFilteredKeys) { + filteredKeys.push(filterPath); + } return undefined; } else if (jsonVal === null) { return undefined; @@ -49,7 +60,11 @@ export function filterByVisibility( const arr = new Array(); for (const [index, value] of jsonVal.entries()) { - const out = transform(value, `${path}/${index}`); + const out = transform( + value, + `${visibilityPath}/${index}`, + `${filterPath}[${index}]`, + ); if (out !== undefined) { arr.push(out); } @@ -68,7 +83,11 @@ export function filterByVisibility( if (value === undefined) { continue; } - const out = transform(value, `${path}/${key}`); + const out = transform( + value, + `${visibilityPath}/${key}`, + filterPath ? `${filterPath}.${key}` : key, + ); if (out !== undefined) { outObj[key] = out; hasOutput = true; @@ -81,5 +100,8 @@ export function filterByVisibility( return undefined; } - return (transform(data, '') as JsonObject) ?? {}; + return { + filteredKeys: withFilteredKeys ? filteredKeys : undefined, + data: (transform(data, '', '') as JsonObject) ?? {}, + }; } diff --git a/packages/config-loader/src/lib/schema/load.test.ts b/packages/config-loader/src/lib/schema/load.test.ts index 4a2b719590..73fa55b90c 100644 --- a/packages/config-loader/src/lib/schema/load.test.ts +++ b/packages/config-loader/src/lib/schema/load.test.ts @@ -68,13 +68,19 @@ describe('loadConfigSchema', () => { schema.process(configs, { visibility: ['frontend'], valueTransform: () => 'X', + withFilteredKeys: true, }), - ).toEqual([{ data: { key1: 'X' }, context: 'test' }]); + ).toEqual([ + { data: { key1: 'X' }, context: 'test', filteredKeys: ['key2'] }, + ]); expect( schema.process(configs, { valueTransform: () => 'X', + withFilteredKeys: true, }), - ).toEqual([{ data: { key1: 'X', key2: 'X' }, context: 'test' }]); + ).toEqual([ + { data: { key1: 'X', key2: 'X' }, context: 'test', filteredKeys: [] }, + ]); const serialized = schema.serialize(); diff --git a/packages/config-loader/src/lib/schema/load.ts b/packages/config-loader/src/lib/schema/load.ts index ae5823c8cc..b67bd0002f 100644 --- a/packages/config-loader/src/lib/schema/load.ts +++ b/packages/config-loader/src/lib/schema/load.ts @@ -57,7 +57,7 @@ export async function loadConfigSchema( return { process( configs: AppConfig[], - { visibility, valueTransform } = {}, + { visibility, valueTransform, withFilteredKeys } = {}, ): AppConfig[] { const result = validate(configs); if (result.errors) { @@ -73,21 +73,23 @@ export async function loadConfigSchema( if (visibility) { processedConfigs = processedConfigs.map(({ data, context }) => ({ context, - data: filterByVisibility( + ...filterByVisibility( data, visibility, result.visibilityByPath, valueTransform, + withFilteredKeys, ), })); } else if (valueTransform) { processedConfigs = processedConfigs.map(({ data, context }) => ({ context, - data: filterByVisibility( + ...filterByVisibility( data, Array.from(CONFIG_VISIBILITIES), result.visibilityByPath, valueTransform, + withFilteredKeys, ), })); } diff --git a/packages/config-loader/src/lib/schema/types.ts b/packages/config-loader/src/lib/schema/types.ts index 30e47917bc..17090ad53c 100644 --- a/packages/config-loader/src/lib/schema/types.ts +++ b/packages/config-loader/src/lib/schema/types.ts @@ -96,6 +96,13 @@ type ConfigProcessingOptions = { * will be omitted. */ valueTransform?: TransformFunc; + + /** + * Whether or not to include the `filteredKeys` property in the output `AppConfig`s. + * + * Default: `false`. + */ + withFilteredKeys?: boolean; }; /** diff --git a/packages/config/api-report.md b/packages/config/api-report.md index 50d2af0f1f..eff4236026 100644 --- a/packages/config/api-report.md +++ b/packages/config/api-report.md @@ -9,6 +9,7 @@ export type AppConfig = { context: string; data: JsonObject; + filteredKeys?: string[]; }; // Warning: (ae-missing-release-tag) "Config" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) diff --git a/packages/config/src/reader.test.ts b/packages/config/src/reader.test.ts index b1ba536797..ed0ca7febe 100644 --- a/packages/config/src/reader.test.ts +++ b/packages/config/src/reader.test.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import { withLogCollector } from '../../test-utils-core/src'; import { ConfigReader } from './reader'; const DATA = { @@ -185,6 +186,61 @@ describe('ConfigReader', () => { const config = new ConfigReader(DATA, CTX); expectInvalidValues(config); }); + + it('should warn when accessing filtered keys in development mode', () => { + const oldEnv = process.env.NODE_ENV; + (process.env as any).NODE_ENV = 'development'; + + const config = ConfigReader.fromConfigs([ + { + data: DATA, + context: CTX, + filteredKeys: ['a', 'b[0]'], + }, + ]); + + expect(withLogCollector(() => config.getOptional('a'))).toMatchObject({ + warn: [ + "Failed to read configuration value at 'a' as it is not visible. See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.", + ], + }); + expect(withLogCollector(() => config.getOptionalString('a'))).toMatchObject( + { + warn: [ + "Failed to read configuration value at 'a' as it is not visible. See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.", + ], + }, + ); + expect( + withLogCollector(() => config.getOptionalConfigArray('b')), + ).toMatchObject({ + warn: [ + "Failed to read configuration array at 'b' as it does not have any visible elements. See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.", + ], + }); + + (process.env as any).NODE_ENV = oldEnv; + }); + + it('should not warn when accessing filtered keys outside of development mode', () => { + const config = ConfigReader.fromConfigs([ + { + data: DATA, + context: CTX, + filteredKeys: ['a', 'b[0]'], + }, + ]); + + expect(withLogCollector(() => config.getOptional('a'))).toMatchObject({ + warn: [], + }); + expect( + withLogCollector(() => config.getOptionalString('a')), + ).toMatchObject({ warn: [] }); + expect( + withLogCollector(() => config.getOptionalConfigArray('b')), + ).toMatchObject({ warn: [] }); + }); }); describe('ConfigReader with fallback', () => { diff --git a/packages/config/src/reader.ts b/packages/config/src/reader.ts index 11346d4a50..d93706c12e 100644 --- a/packages/config/src/reader.ts +++ b/packages/config/src/reader.ts @@ -55,6 +55,15 @@ const errors = { }; export class ConfigReader implements Config { + /** + * A set of key paths that where removed from the config due to not being visible. + * + * This was added as a mutable private member to avoid changes to the public API. + * Its only purpose of this is to warn users of missing visibility when running + * the frontend in development mode. + */ + private filteredKeys?: string[]; + static fromConfigs(configs: AppConfig[]): ConfigReader { if (configs.length === 0) { return new ConfigReader(undefined); @@ -62,9 +71,14 @@ export class ConfigReader implements Config { // Merge together all configs into a single config with recursive fallback // readers, giving the first config object in the array the lowest priority. - return configs.reduce((previousReader, { data, context }) => { - return new ConfigReader(data, context, previousReader); - }, undefined!); + return configs.reduce( + (previousReader, { data, context, filteredKeys }) => { + const reader = new ConfigReader(data, context, previousReader); + reader.filteredKeys = filteredKeys; + return reader; + }, + undefined!, + ); } constructor( @@ -101,6 +115,18 @@ export class ConfigReader implements Config { const fallbackValue = this.fallback?.getOptional(key); if (value === undefined) { + if (process.env.NODE_ENV === 'development') { + if (fallbackValue === undefined && key) { + const fullKey = this.fullKey(key); + if (this.filteredKeys?.includes(fullKey)) { + // eslint-disable-next-line no-console + console.warn( + `Failed to read configuration value at '${fullKey}' as it is not visible. ` + + 'See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.', + ); + } + } + } return fallbackValue; } else if (fallbackValue === undefined) { return value as T; @@ -127,10 +153,9 @@ export class ConfigReader implements Config { getOptionalConfig(key: string): ConfigReader | undefined { const value = this.readValue(key); const fallbackConfig = this.fallback?.getOptionalConfig(key); - const prefix = this.fullKey(key); if (isObject(value)) { - return new ConfigReader(value, this.context, fallbackConfig, prefix); + return this.copy(value, key, fallbackConfig); } if (value !== undefined) { throw new TypeError( @@ -163,18 +188,20 @@ export class ConfigReader implements Config { }); if (!configs) { + if (process.env.NODE_ENV === 'development') { + const fullKey = this.fullKey(key); + if (this.filteredKeys?.some(k => k.startsWith(fullKey))) { + // eslint-disable-next-line no-console + console.warn( + `Failed to read configuration array at '${key}' as it does not have any visible elements. ` + + 'See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.', + ); + } + } return undefined; } - return configs.map( - (obj, index) => - new ConfigReader( - obj, - this.context, - undefined, - this.fullKey(`${key}[${index}]`), - ), - ); + return configs.map((obj, index) => this.copy(obj, `${key}[${index}]`)); } getNumber(key: string): number { @@ -261,6 +288,17 @@ export class ConfigReader implements Config { return `${this.prefix}${this.prefix ? '.' : ''}${key}`; } + private copy(data: JsonObject, key: string, fallback?: ConfigReader) { + const reader = new ConfigReader( + data, + this.context, + fallback, + this.fullKey(key), + ); + reader.filteredKeys = this.filteredKeys; + return reader; + } + private readConfigValue( key: string, validate: ( @@ -270,6 +308,17 @@ export class ConfigReader implements Config { const value = this.readValue(key); if (value === undefined) { + if (process.env.NODE_ENV === 'development') { + const fullKey = this.fullKey(key); + if (this.filteredKeys?.includes(fullKey)) { + // eslint-disable-next-line no-console + console.warn( + `Failed to read configuration value at '${fullKey}' as it is not visible. ` + + 'See https://backstage.io/docs/conf/defining#visibility for instructions on how to make it visible.', + ); + } + } + return this.fallback?.readConfigValue(key, validate); } const result = validate(value); diff --git a/packages/config/src/types.ts b/packages/config/src/types.ts index d8f12f4f6e..1135e7d554 100644 --- a/packages/config/src/types.ts +++ b/packages/config/src/types.ts @@ -22,6 +22,7 @@ export type JsonValue = JsonObject | JsonArray | JsonPrimitive; export type AppConfig = { context: string; data: JsonObject; + filteredKeys?: string[]; }; export type Config = {